Auto merge of #42265 - Zoxc:for-sugar, r=eddyb

Change for-loop desugar to not borrow the iterator during the loop This is enables the use of suspend points inside for-loops in movable generators. This is illegal in the current desugaring as `iter` is borrowed across the body.
rust-lang · Jun 4, 2017 · 0b17b4c · 0b17b4c
2 parents 1b5a923 + cfdbff7
commit 0b17b4c
Show file tree

Hide file tree

Showing 9 changed files with 138 additions and 83 deletions.
diff --git a/src/libcore/iter/mod.rs b/src/libcore/iter/mod.rs
@@ -191,10 +191,11 @@
 //! {
 //!     let result = match IntoIterator::into_iter(values) {
 //!         mut iter => loop {
-//!             match iter.next() {
-//!                 Some(x) => { println!("{}", x); },
+//!             let x = match iter.next() {
+//!                 Some(val) => val,
 //!                 None => break,
-//!             }
+//!             };
+//!             let () = { println!("{}", x); };
 //!         },
 //!     };
 //!     result

diff --git a/src/librustc/hir/lowering.rs b/src/librustc/hir/lowering.rs
@@ -891,6 +891,7 @@ impl<'a> LoweringContext<'a> {
             init: l.init.as_ref().map(|e| P(self.lower_expr(e))),
             span: l.span,
             attrs: l.attrs.clone(),
+            source: hir::LocalSource::Normal,
         })
     }
 
@@ -2167,10 +2168,11 @@ impl<'a> LoweringContext<'a> {
                 //     let result = match ::std::iter::IntoIterator::into_iter(<head>) {
                 //       mut iter => {
                 //         [opt_ident]: loop {
-                //           match ::std::iter::Iterator::next(&mut iter) {
-                //             ::std::option::Option::Some(<pat>) => <body>,
+                //           let <pat> = match ::std::iter::Iterator::next(&mut iter) {
+                //             ::std::option::Option::Some(val) => val,
                 //             ::std::option::Option::None => break
-                //           }
+                //           };
+                //           SemiExpr(<body>);
                 //         }
                 //       }
                 //     };
@@ -2182,15 +2184,13 @@ impl<'a> LoweringContext<'a> {
 
                 let iter = self.str_to_ident("iter");
 
-                // `::std::option::Option::Some(<pat>) => <body>`
+                // `::std::option::Option::Some(val) => val`
                 let pat_arm = {
-                    let body_block = self.with_loop_scope(e.id,
-                                                          |this| this.lower_block(body, false));
-                    let body_expr = P(self.expr_block(body_block, ThinVec::new()));
-                    let pat = self.lower_pat(pat);
-                    let some_pat = self.pat_some(e.span, pat);
-
-                    self.arm(hir_vec![some_pat], body_expr)
+                    let val_ident = self.str_to_ident("val");
+                    let val_pat = self.pat_ident(e.span, val_ident);
+                    let val_expr = P(self.expr_ident(e.span, val_ident, val_pat.id));
+                    let some_pat = self.pat_some(e.span, val_pat);
+                    self.arm(hir_vec![some_pat], val_expr)
                 };
 
                 // `::std::option::Option::None => break`
@@ -2221,8 +2221,20 @@ impl<'a> LoweringContext<'a> {
                                 ThinVec::new()))
                 };
 
+                let pat = self.lower_pat(pat);
+                let pat_let = self.stmt_let_pat(e.span,
+                    match_expr,
+                    pat,
+                    hir::LocalSource::ForLoopDesugar);
+
+                let body_block = self.with_loop_scope(e.id,
+                                                        |this| this.lower_block(body, false));
+                let body_expr = P(self.expr_block(body_block, ThinVec::new()));
+                let body_stmt = respan(e.span, hir::StmtExpr(body_expr, self.next_id()));
+
+                let loop_block = P(self.block_all(e.span, hir_vec![pat_let, body_stmt], None));
+
                 // `[opt_ident]: loop { ... }`
-                let loop_block = P(self.block_expr(match_expr));
                 let loop_expr = hir::ExprLoop(loop_block, self.lower_opt_sp_ident(opt_ident),
                                               hir::LoopSource::ForLoop);
                 let loop_expr = P(hir::Expr {
@@ -2585,24 +2597,34 @@ impl<'a> LoweringContext<'a> {
         }
     }
 
-    fn stmt_let(&mut self, sp: Span, mutbl: bool, ident: Name, ex: P<hir::Expr>)
-                -> (hir::Stmt, NodeId) {
-        let pat = if mutbl {
-            self.pat_ident_binding_mode(sp, ident, hir::BindByValue(hir::MutMutable))
-        } else {
-            self.pat_ident(sp, ident)
-        };
-        let pat_id = pat.id;
+    fn stmt_let_pat(&mut self,
+                    sp: Span,
+                    ex: P<hir::Expr>,
+                    pat: P<hir::Pat>,
+                    source: hir::LocalSource)
+                    -> hir::Stmt {
         let local = P(hir::Local {
             pat: pat,
             ty: None,
             init: Some(ex),
             id: self.next_id(),
             span: sp,
             attrs: ThinVec::new(),
+            source,
         });
         let decl = respan(sp, hir::DeclLocal(local));
-        (respan(sp, hir::StmtDecl(P(decl), self.next_id())), pat_id)
+        respan(sp, hir::StmtDecl(P(decl), self.next_id()))
+    }
+
+    fn stmt_let(&mut self, sp: Span, mutbl: bool, ident: Name, ex: P<hir::Expr>)
+                -> (hir::Stmt, NodeId) {
+        let pat = if mutbl {
+            self.pat_ident_binding_mode(sp, ident, hir::BindByValue(hir::MutMutable))
+        } else {
+            self.pat_ident(sp, ident)
+        };
+        let pat_id = pat.id;
+        (self.stmt_let_pat(sp, ex, pat, hir::LocalSource::Normal), pat_id)
     }
 
     fn block_expr(&mut self, expr: P<hir::Expr>) -> hir::Block {

diff --git a/src/librustc/hir/mod.rs b/src/librustc/hir/mod.rs
@@ -872,6 +872,7 @@ pub struct Local {
     pub id: NodeId,
     pub span: Span,
     pub attrs: ThinVec<Attribute>,
+    pub source: LocalSource,
 }
 
 pub type Decl = Spanned<Decl_>;
@@ -1080,6 +1081,15 @@ pub enum QPath {
     TypeRelative(P<Ty>, P<PathSegment>)
 }
 
+/// Hints at the original code for a let statement
+#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug, Copy)]
+pub enum LocalSource {
+    /// A `match _ { .. }`
+    Normal,
+    /// A desugared `for _ in _ { .. }` loop
+    ForLoopDesugar,
+}
+
 /// Hints at the original code for a `match _ { .. }`
 #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug, Copy)]
 pub enum MatchSource {

diff --git a/src/librustc/ich/impls_hir.rs b/src/librustc/ich/impls_hir.rs
@@ -490,7 +490,8 @@ impl_stable_hash_for!(struct hir::Local {
     init,
     id,
     span,
-    attrs
+    attrs,
+    source
 });
 
 impl_stable_hash_for_spanned!(hir::Decl_);
@@ -640,6 +641,11 @@ impl_stable_hash_for!(enum hir::Expr_ {
     ExprRepeat(val, times)
 });
 
+impl_stable_hash_for!(enum hir::LocalSource {
+    Normal,
+    ForLoopDesugar
+});
+
 impl_stable_hash_for!(enum hir::LoopSource {
     Loop,
     WhileLet,

diff --git a/src/librustc_const_eval/check_match.rs b/src/librustc_const_eval/check_match.rs
@@ -91,7 +91,10 @@ impl<'a, 'tcx> Visitor<'tcx> for MatchVisitor<'a, 'tcx> {
     fn visit_local(&mut self, loc: &'tcx hir::Local) {
         intravisit::walk_local(self, loc);
 
-        self.check_irrefutable(&loc.pat, false);
+        self.check_irrefutable(&loc.pat, match loc.source {
+            hir::LocalSource::Normal => "local binding",
+            hir::LocalSource::ForLoopDesugar => "`for` loop binding",
+        });
 
         // Check legality of move bindings and `@` patterns.
         self.check_patterns(false, slice::ref_slice(&loc.pat));
@@ -101,7 +104,7 @@ impl<'a, 'tcx> Visitor<'tcx> for MatchVisitor<'a, 'tcx> {
         intravisit::walk_body(self, body);
 
         for arg in &body.arguments {
-            self.check_irrefutable(&arg.pat, true);
+            self.check_irrefutable(&arg.pat, "function argument");
             self.check_patterns(false, slice::ref_slice(&arg.pat));
         }
     }
@@ -210,7 +213,7 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> {
                 .map(|pat| vec![pat.0])
                 .collect();
             let scrut_ty = self.tables.node_id_to_type(scrut.id);
-            check_exhaustive(cx, scrut_ty, scrut.span, &matrix, source);
+            check_exhaustive(cx, scrut_ty, scrut.span, &matrix);
         })
     }
 
@@ -223,13 +226,7 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> {
         }
     }
 
-    fn check_irrefutable(&self, pat: &Pat, is_fn_arg: bool) {
-        let origin = if is_fn_arg {
-            "function argument"
-        } else {
-            "local binding"
-        };
-
+    fn check_irrefutable(&self, pat: &Pat, origin: &str) {
         let module = self.tcx.hir.get_module_parent(pat.id);
         MatchCheckCtxt::create_and_enter(self.tcx, module, |ref mut cx| {
             let mut patcx = PatternContext::new(self.tcx, self.tables);
@@ -395,8 +392,7 @@ fn check_arms<'a, 'tcx>(cx: &mut MatchCheckCtxt<'a, 'tcx>,
 fn check_exhaustive<'a, 'tcx>(cx: &mut MatchCheckCtxt<'a, 'tcx>,
                               scrut_ty: Ty<'tcx>,
                               sp: Span,
-                              matrix: &Matrix<'a, 'tcx>,
-                              source: hir::MatchSource) {
+                              matrix: &Matrix<'a, 'tcx>) {
     let wild_pattern = Pattern {
         ty: scrut_ty,
         span: DUMMY_SP,
@@ -409,52 +405,32 @@ fn check_exhaustive<'a, 'tcx>(cx: &mut MatchCheckCtxt<'a, 'tcx>,
             } else {
                 pats.iter().map(|w| w.single_pattern()).collect()
             };
-            match source {
-                hir::MatchSource::ForLoopDesugar => {
-                    // `witnesses[0]` has the form `Some(<head>)`, peel off the `Some`
-                    let witness = match *witnesses[0].kind {
-                        PatternKind::Variant { ref subpatterns, .. } => match &subpatterns[..] {
-                            &[ref pat] => &pat.pattern,
-                            _ => bug!(),
-                        },
-                        _ => bug!(),
-                    };
-                    let pattern_string = witness.to_string();
-                    struct_span_err!(cx.tcx.sess, sp, E0297,
-                        "refutable pattern in `for` loop binding: \
-                                `{}` not covered",
-                                pattern_string)
-                        .span_label(sp, format!("pattern `{}` not covered", pattern_string))
-                        .emit();
+
+            const LIMIT: usize = 3;
+            let joined_patterns = match witnesses.len() {
+                0 => bug!(),
+                1 => format!("`{}`", witnesses[0]),
+                2...LIMIT => {
+                    let (tail, head) = witnesses.split_last().unwrap();
+                    let head: Vec<_> = head.iter().map(|w| w.to_string()).collect();
+                    format!("`{}` and `{}`", head.join("`, `"), tail)
                 },
                 _ => {
-                    const LIMIT: usize = 3;
-                    let joined_patterns = match witnesses.len() {
-                        0 => bug!(),
-                        1 => format!("`{}`", witnesses[0]),
-                        2...LIMIT => {
-                            let (tail, head) = witnesses.split_last().unwrap();
-                            let head: Vec<_> = head.iter().map(|w| w.to_string()).collect();
-                            format!("`{}` and `{}`", head.join("`, `"), tail)
-                        },
-                        _ => {
-                            let (head, tail) = witnesses.split_at(LIMIT);
-                            let head: Vec<_> = head.iter().map(|w| w.to_string()).collect();
-                            format!("`{}` and {} more", head.join("`, `"), tail.len())
-                        }
-                    };
-
-                    let label_text = match witnesses.len() {
-                        1 => format!("pattern {} not covered", joined_patterns),
-                        _ => format!("patterns {} not covered", joined_patterns)
-                    };
-                    create_e0004(cx.tcx.sess, sp,
-                                 format!("non-exhaustive patterns: {} not covered",
-                                         joined_patterns))
-                        .span_label(sp, label_text)
-                        .emit();
-                },
-            }
+                    let (head, tail) = witnesses.split_at(LIMIT);
+                    let head: Vec<_> = head.iter().map(|w| w.to_string()).collect();
+                    format!("`{}` and {} more", head.join("`, `"), tail.len())
+                }
+            };
+
+            let label_text = match witnesses.len() {
+                1 => format!("pattern {} not covered", joined_patterns),
+                _ => format!("patterns {} not covered", joined_patterns)
+            };
+            create_e0004(cx.tcx.sess, sp,
+                            format!("non-exhaustive patterns: {} not covered",
+                                    joined_patterns))
+                .span_label(sp, label_text)
+                .emit();
         }
         NotUseful => {
             // This is good, wildcard pattern isn't reachable

diff --git a/src/librustc_const_eval/diagnostics.rs b/src/librustc_const_eval/diagnostics.rs
@@ -452,12 +452,14 @@ enum Method { GET, POST }
 
 
 E0297: r##"
+#### Note: this error code is no longer emitted by the compiler.
+
 Patterns used to bind names must be irrefutable. That is, they must guarantee
 that a name will be extracted in all cases. Instead of pattern matching the
 loop variable, consider using a `match` or `if let` inside the loop body. For
 instance:
 
-```compile_fail,E0297
+```compile_fail,E0005
 let xs : Vec<Option<i32>> = vec![Some(1), None];
 
 // This fails because `None` is not covered.

diff --git a/src/test/compile-fail/E0297.rs b/src/test/compile-fail/E0297.rs
@@ -12,6 +12,6 @@ fn main() {
     let xs : Vec<Option<i32>> = vec![Some(1), None];
 
     for Some(x) in xs {}
-    //~^ ERROR E0297
+    //~^ ERROR E0005
     //~| NOTE pattern `None` not covered
 }
diff --git a/src/test/compile-fail/for-loop-has-unit-body.rs b/src/test/compile-fail/for-loop-has-unit-body.rs
@@ -0,0 +1,17 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+fn main() {
+    for x in 0..3 {
+        x //~ ERROR mismatched types
+        //~| NOTE expected ()
+        //~| NOTE expected type `()`
+    }
+}
diff --git a/src/test/run-pass/for-loop-has-unit-body.rs b/src/test/run-pass/for-loop-has-unit-body.rs
@@ -0,0 +1,21 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+fn main() {
+    // Check that the tail statement in the body unifies with something
+    for _ in 0..3 {
+        unsafe { std::mem::uninitialized() }
+    }
+
+    // Check that the tail statement in the body can be unit
+    for _ in 0..3 {
+        ()
+    }
+}