Skip to content

Change for-loop desugar to not borrow the iterator during the loop #42265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bors merged 1 commit into from
Jun 4, 2017
Merged

Change for-loop desugar to not borrow the iterator during the loop #42265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/libcore/iter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
64 changes: 43 additions & 21 deletions src/librustc/hir/lowering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
}

Expand Down Expand Up @@ -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) {
Copy link
Member

@eddyb eddyb May 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes semantics, although I can't think of any situation in which this would fail.
FWIW when I asked about while let Some(x) = iter.next() - that would correspond to the old desugaring, not your new one, wouldn't it?

// ::std::option::Option::Some(val) => val,
// ::std::option::Option::None => break
// }
// };
// SemiExpr(<body>);
Copy link
Contributor

@mcarton mcarton Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is wrong, the statement used is a StmtExpr.

// }
// }
// };
Expand All @@ -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`
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
10 changes: 10 additions & 0 deletions src/librustc/hir/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_>;
Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 7 additions & 1 deletion src/librustc/ich/impls_hir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,8 @@ impl_stable_hash_for!(struct hir::Local {
init,
id,
span,
attrs
attrs,
source
});

impl_stable_hash_for_spanned!(hir::Decl_);
Expand Down Expand Up @@ -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,
Expand Down
88 changes: 32 additions & 56 deletions src/librustc_const_eval/check_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,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));
Expand All @@ -102,7 +105,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));
}
}
Expand Down Expand Up @@ -211,7 +214,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);
})
}

Expand All @@ -224,13 +227,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);
Expand Down Expand Up @@ -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,
Expand All @@ -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(<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
Expand Down
4 changes: 3 additions & 1 deletion src/librustc_const_eval/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,12 +452,14 @@ enum Method { GET, POST }


E0297: r##"
#### Note: this error code is no longer emitted by the compiler.
Copy link
Member

@eddyb eddyb May 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rust-lang/compiler This is an annoying (lack of a?) convention - is there any consensus?

Copy link
Contributor

@nikomatsakis nikomatsakis May 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be what we have done in the past. I see some instances of ## and some with ####.

Personally, I long to get rid of --explain XXX and just have a "long error mode" in which the long explanations are emitted 'inline'. That would basically make this point irrelevant.

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.
Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/E0297.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
17 changes: 17 additions & 0 deletions src/test/compile-fail/for-loop-has-unit-body.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 `()`
}
}
21 changes: 21 additions & 0 deletions src/test/run-pass/for-loop-has-unit-body.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
()
}
}
Copy link
Member

@eddyb eddyb Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline, here and in the other test.