From cfdbff7c125cbdd5a2b75e526717413718d16111 Mon Sep 17 00:00:00 2001 From: =?utf8?q?John=20K=C3=A5re=20Alsaker?= Date: Sat, 27 May 2017 20:20:17 +0200 Subject: [PATCH] Change for-loop desugar to not borrow the iterator during the loop --- src/libcore/iter/mod.rs | 7 +- src/librustc/hir/lowering.rs | 64 +++++++++----- src/librustc/hir/mod.rs | 10 +++ src/librustc/ich/impls_hir.rs | 8 +- src/librustc_const_eval/check_match.rs | 88 +++++++------------ src/librustc_const_eval/diagnostics.rs | 4 +- src/test/compile-fail/E0297.rs | 2 +- .../compile-fail/for-loop-has-unit-body.rs | 17 ++++ src/test/run-pass/for-loop-has-unit-body.rs | 21 +++++ 9 files changed, 138 insertions(+), 83 deletions(-) create mode 100644 src/test/compile-fail/for-loop-has-unit-body.rs create mode 100644 src/test/run-pass/for-loop-has-unit-body.rs diff --git a/src/libcore/iter/mod.rs b/src/libcore/iter/mod.rs index 5eefa59e7ea..37cc6f98e73 100644 --- 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 index 91cfbc38aa0..35b8f01d5fd 100644 --- a/src/librustc/hir/lowering.rs +++ b/src/librustc/hir/lowering.rs @@ -891,6 +891,7 @@ fn lower_local(&mut self, l: &Local) -> P { 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 @@ fn lower_expr(&mut self, e: &Expr) -> hir::Expr { // let result = match ::std::iter::IntoIterator::into_iter() { // mut iter => { // [opt_ident]: loop { - // match ::std::iter::Iterator::next(&mut iter) { - // ::std::option::Option::Some() => , + // let = match ::std::iter::Iterator::next(&mut iter) { + // ::std::option::Option::Some(val) => val, // ::std::option::Option::None => break - // } + // }; + // SemiExpr(); // } // } // }; @@ -2182,15 +2184,13 @@ fn lower_expr(&mut self, e: &Expr) -> hir::Expr { let iter = self.str_to_ident("iter"); - // `::std::option::Option::Some() => ` + // `::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 @@ fn lower_expr(&mut self, e: &Expr) -> hir::Expr { 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,14 +2597,12 @@ fn expr(&mut self, span: Span, node: hir::Expr_, attrs: ThinVec) -> h } } - fn stmt_let(&mut self, sp: Span, mutbl: bool, ident: Name, ex: P) - -> (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, + pat: P, + source: hir::LocalSource) + -> hir::Stmt { let local = P(hir::Local { pat: pat, ty: None, @@ -2600,9 +2610,21 @@ fn stmt_let(&mut self, sp: Span, mutbl: bool, ident: Name, ex: P) 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::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::Block { diff --git a/src/librustc/hir/mod.rs b/src/librustc/hir/mod.rs index 500e95a8a77..3f2977cc503 100644 --- 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, + pub source: LocalSource, } pub type Decl = Spanned; @@ -1080,6 +1081,15 @@ pub enum QPath { TypeRelative(P, P) } +/// 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 index dbe91e2725d..c582cac67e2 100644 --- a/src/librustc/ich/impls_hir.rs +++ b/src/librustc/ich/impls_hir.rs @@ -490,7 +490,8 @@ fn hash_stable(&self, init, id, span, - attrs + attrs, + source }); impl_stable_hash_for_spanned!(hir::Decl_); @@ -640,6 +641,11 @@ fn hash_stable(&self, 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 index b35b0865991..e78ab0b234e 100644 --- a/src/librustc_const_eval/check_match.rs +++ b/src/librustc_const_eval/check_match.rs @@ -92,7 +92,10 @@ fn visit_expr(&mut self, ex: &'tcx hir::Expr) { 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)); @@ -102,7 +105,7 @@ fn visit_body(&mut self, body: &'tcx hir::Body) { 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)); } } @@ -211,7 +214,7 @@ fn check_match( .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); }) } @@ -224,13 +227,7 @@ fn conservative_is_uninhabited(&self, scrutinee_ty: Ty<'tcx>) -> bool { } } - 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); @@ -396,8 +393,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, @@ -410,52 +406,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()`, 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 index 04fc3e68c8c..4fc7ef8035e 100644 --- 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> = 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 index 5792ba06eb0..436e4c1f9d2 100644 --- a/src/test/compile-fail/E0297.rs +++ b/src/test/compile-fail/E0297.rs @@ -12,6 +12,6 @@ fn main() { let xs : Vec> = 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 new file mode 100644 index 00000000000..8c61fc602e0 --- /dev/null +++ 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 or the MIT license +// , 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 new file mode 100644 index 00000000000..4036fc84800 --- /dev/null +++ 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 or the MIT license +// , 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 { + () + } +} -- 2.44.0