diff --git a/src/librustc/cfg/construct.rs b/src/librustc/cfg/construct.rs index bc3da13ccf8..fdb350ebaac 100644 --- a/src/librustc/cfg/construct.rs +++ b/src/librustc/cfg/construct.rs @@ -220,15 +220,24 @@ fn expr(&mut self, expr: &hir::Expr, pred: CFGIndex) -> CFGIndex { // Note that `break` and `continue` statements // may cause additional edges. - // Is the condition considered part of the loop? let loopback = self.add_dummy_node(&[pred]); // 1 - let cond_exit = self.expr(&cond, loopback); // 2 - let expr_exit = self.add_ast_node(expr.id, &[cond_exit]); // 3 + + // Create expr_exit without pred (cond_exit) + let expr_exit = self.add_ast_node(expr.id, &[]); // 3 + + // The LoopScope needs to be on the loop_scopes stack while evaluating the + // condition and the body of the loop (both can break out of the loop) self.loop_scopes.push(LoopScope { loop_id: expr.id, continue_index: loopback, break_index: expr_exit }); + + let cond_exit = self.expr(&cond, loopback); // 2 + + // Add pred (cond_exit) to expr_exit + self.add_contained_edge(cond_exit, expr_exit); + let body_exit = self.block(&body, cond_exit); // 4 self.add_contained_edge(body_exit, loopback); // 5 self.loop_scopes.pop(); @@ -580,11 +589,17 @@ fn add_returning_edge(&mut self, fn find_scope(&self, expr: &hir::Expr, label: hir::Label) -> LoopScope { - for l in &self.loop_scopes { - if l.loop_id == label.loop_id { - return *l; + + match label.loop_id.into() { + Ok(loop_id) => { + for l in &self.loop_scopes { + if l.loop_id == loop_id { + return *l; + } + } + span_bug!(expr.span, "no loop scope for id {}", loop_id); } + Err(err) => span_bug!(expr.span, "loop scope error: {}", err) } - span_bug!(expr.span, "no loop scope for id {}", label.loop_id); } } diff --git a/src/librustc/hir/intravisit.rs b/src/librustc/hir/intravisit.rs index 03b59aaf319..fd6796ccc0b 100644 --- a/src/librustc/hir/intravisit.rs +++ b/src/librustc/hir/intravisit.rs @@ -1008,14 +1008,18 @@ pub fn walk_expr<'v, V: Visitor<'v>>(visitor: &mut V, expression: &'v Expr) { } ExprBreak(label, ref opt_expr) => { label.ident.map(|ident| { - visitor.visit_def_mention(Def::Label(label.loop_id)); + if let Ok(loop_id) = label.loop_id.into() { + visitor.visit_def_mention(Def::Label(loop_id)); + } visitor.visit_name(ident.span, ident.node.name); }); walk_list!(visitor, visit_expr, opt_expr); } ExprAgain(label) => { label.ident.map(|ident| { - visitor.visit_def_mention(Def::Label(label.loop_id)); + if let Ok(loop_id) = label.loop_id.into() { + visitor.visit_def_mention(Def::Label(loop_id)); + } visitor.visit_name(ident.span, ident.node.name); }); } diff --git a/src/librustc/hir/lowering.rs b/src/librustc/hir/lowering.rs index d6f99327d4f..55226fa7605 100644 --- a/src/librustc/hir/lowering.rs +++ b/src/librustc/hir/lowering.rs @@ -81,6 +81,7 @@ pub struct LoweringContext<'a> { bodies: FxHashMap, loop_scopes: Vec, + is_in_loop_condition: bool, type_def_lifetime_params: DefIdMap, } @@ -116,6 +117,7 @@ pub fn lower_crate(sess: &Session, impl_items: BTreeMap::new(), bodies: FxHashMap(), loop_scopes: Vec::new(), + is_in_loop_condition: false, type_def_lifetime_params: DefIdMap(), }.lower_crate(krate) } @@ -251,21 +253,49 @@ fn allow_internal_unstable(&self, reason: &'static str, mut span: Span) -> Span fn with_loop_scope(&mut self, loop_id: NodeId, f: F) -> T where F: FnOnce(&mut LoweringContext) -> T { + // We're no longer in the base loop's condition; we're in another loop. + let was_in_loop_condition = self.is_in_loop_condition; + self.is_in_loop_condition = false; + let len = self.loop_scopes.len(); self.loop_scopes.push(loop_id); + let result = f(self); assert_eq!(len + 1, self.loop_scopes.len(), "Loop scopes should be added and removed in stack order"); + self.loop_scopes.pop().unwrap(); + + self.is_in_loop_condition = was_in_loop_condition; + + result + } + + fn with_loop_condition_scope(&mut self, f: F) -> T + where F: FnOnce(&mut LoweringContext) -> T + { + let was_in_loop_condition = self.is_in_loop_condition; + self.is_in_loop_condition = true; + + let result = f(self); + + self.is_in_loop_condition = was_in_loop_condition; + result } fn with_new_loop_scopes(&mut self, f: F) -> T where F: FnOnce(&mut LoweringContext) -> T { + let was_in_loop_condition = self.is_in_loop_condition; + self.is_in_loop_condition = false; + let loop_scopes = mem::replace(&mut self.loop_scopes, Vec::new()); let result = f(self); mem::replace(&mut self.loop_scopes, loop_scopes); + + self.is_in_loop_condition = was_in_loop_condition; + result } @@ -300,17 +330,16 @@ fn lower_label(&mut self, label: Option<(NodeId, Spanned)>) -> hir::Label match label { Some((id, label_ident)) => hir::Label { ident: Some(label_ident), - loop_id: match self.expect_full_def(id) { - Def::Label(loop_id) => loop_id, - _ => DUMMY_NODE_ID + loop_id: if let Def::Label(loop_id) = self.expect_full_def(id) { + hir::LoopIdResult::Ok(loop_id) + } else { + hir::LoopIdResult::Err(hir::LoopIdError::UnresolvedLabel) } }, None => hir::Label { ident: None, - loop_id: match self.loop_scopes.last() { - Some(innermost_loop_id) => *innermost_loop_id, - _ => DUMMY_NODE_ID - } + loop_id: self.loop_scopes.last().map(|innermost_loop_id| Ok(*innermost_loop_id)) + .unwrap_or(Err(hir::LoopIdError::OutsideLoopScope)).into() } } } @@ -1597,7 +1626,7 @@ fn lower_expr(&mut self, e: &Expr) -> hir::Expr { ExprKind::While(ref cond, ref body, opt_ident) => { self.with_loop_scope(e.id, |this| hir::ExprWhile( - P(this.lower_expr(cond)), + this.with_loop_condition_scope(|this| P(this.lower_expr(cond))), this.lower_block(body), this.lower_opt_sp_ident(opt_ident))) } @@ -1699,13 +1728,29 @@ fn make_struct(this: &mut LoweringContext, hir::ExprPath(self.lower_qpath(e.id, qself, path, ParamMode::Optional)) } ExprKind::Break(opt_ident, ref opt_expr) => { + let label_result = if self.is_in_loop_condition && opt_ident.is_none() { + hir::Label { + ident: opt_ident, + loop_id: Err(hir::LoopIdError::UnlabeledCfInWhileCondition).into(), + } + } else { + self.lower_label(opt_ident.map(|ident| (e.id, ident))) + }; hir::ExprBreak( - self.lower_label(opt_ident.map(|ident| (e.id, ident))), - opt_expr.as_ref().map(|x| P(self.lower_expr(x)))) + label_result, + opt_expr.as_ref().map(|x| P(self.lower_expr(x)))) } ExprKind::Continue(opt_ident) => hir::ExprAgain( - self.lower_label(opt_ident.map(|ident| (e.id, ident)))), + if self.is_in_loop_condition && opt_ident.is_none() { + hir::Label { + ident: opt_ident, + loop_id: Err( + hir::LoopIdError::UnlabeledCfInWhileCondition).into(), + } + } else { + self.lower_label(opt_ident.map( | ident| (e.id, ident))) + }), ExprKind::Ret(ref e) => hir::ExprRet(e.as_ref().map(|x| P(self.lower_expr(x)))), ExprKind::InlineAsm(ref asm) => { let hir_asm = hir::InlineAsm { @@ -1846,10 +1891,12 @@ fn make_struct(this: &mut LoweringContext, // } // } + // Note that the block AND the condition are evaluated in the loop scope. + // This is done to allow `break` from inside the condition of the loop. let (body, break_expr, sub_expr) = self.with_loop_scope(e.id, |this| ( this.lower_block(body), this.expr_break(e.span, ThinVec::new()), - P(this.lower_expr(sub_expr)), + this.with_loop_condition_scope(|this| P(this.lower_expr(sub_expr))), )); // ` => ` diff --git a/src/librustc/hir/mod.rs b/src/librustc/hir/mod.rs index fc5282d1af2..4e4187cbf89 100644 --- a/src/librustc/hir/mod.rs +++ b/src/librustc/hir/mod.rs @@ -1030,11 +1030,56 @@ pub enum LoopSource { ForLoop, } +#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug, Copy)] +pub enum LoopIdError { + OutsideLoopScope, + UnlabeledCfInWhileCondition, + UnresolvedLabel, +} + +impl fmt::Display for LoopIdError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + fmt::Display::fmt(match *self { + LoopIdError::OutsideLoopScope => "not inside loop scope", + LoopIdError::UnlabeledCfInWhileCondition => + "unlabeled control flow (break or continue) in while condition", + LoopIdError::UnresolvedLabel => "label not found", + }, f) + } +} + +// FIXME(cramertj) this should use `Result` once master compiles w/ a vesion of Rust where +// `Result` implements `Encodable`/`Decodable` +#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug, Copy)] +pub enum LoopIdResult { + Ok(NodeId), + Err(LoopIdError), +} +impl Into> for LoopIdResult { + fn into(self) -> Result { + match self { + LoopIdResult::Ok(ok) => Ok(ok), + LoopIdResult::Err(err) => Err(err), + } + } +} +impl From> for LoopIdResult { + fn from(res: Result) -> Self { + match res { + Ok(ok) => LoopIdResult::Ok(ok), + Err(err) => LoopIdResult::Err(err), + } + } +} #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug, Copy)] pub struct Label { + // This is `Some(_)` iff there is an explicit user-specified `label pub ident: Option>, - pub loop_id: NodeId + + // These errors are caught and then reported during the diagnostics pass in + // librustc_passes/loops.rs + pub loop_id: LoopIdResult, } #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug, Copy)] diff --git a/src/librustc/middle/liveness.rs b/src/librustc/middle/liveness.rs index 60d2a213ece..b7f7c49d7b0 100644 --- a/src/librustc/middle/liveness.rs +++ b/src/librustc/middle/liveness.rs @@ -1003,7 +1003,10 @@ fn propagate_through_expr(&mut self, expr: &Expr, succ: LiveNode) hir::ExprBreak(label, ref opt_expr) => { // Find which label this break jumps to - let sc = label.loop_id; + let sc = match label.loop_id.into() { + Ok(loop_id) => loop_id, + Err(err) => span_bug!(expr.span, "loop scope error: {}", err), + }; // Now that we know the label we're going to, // look it up in the break loop nodes table @@ -1016,7 +1019,11 @@ fn propagate_through_expr(&mut self, expr: &Expr, succ: LiveNode) hir::ExprAgain(label) => { // Find which label this expr continues to - let sc = label.loop_id; + let sc = match label.loop_id.into() { + Ok(loop_id) => loop_id, + Err(err) => span_bug!(expr.span, "loop scope error: {}", err), + }; + // Now that we know the label we're going to, // look it up in the continue loop nodes table @@ -1280,12 +1287,13 @@ fn propagate_through_loop(&mut self, debug!("propagate_through_loop: using id for loop body {} {}", expr.id, self.ir.tcx.hir.node_to_pretty_string(body.id)); - let cond_ln = match kind { - LoopLoop => ln, - WhileLoop(ref cond) => self.propagate_through_expr(&cond, ln), - }; - let body_ln = self.with_loop_nodes(expr.id, succ, ln, |this| { - this.propagate_through_block(body, cond_ln) + let (cond_ln, body_ln) = self.with_loop_nodes(expr.id, succ, ln, |this| { + let cond_ln = match kind { + LoopLoop => ln, + WhileLoop(ref cond) => this.propagate_through_expr(&cond, ln), + }; + let body_ln = this.propagate_through_block(body, cond_ln); + (cond_ln, body_ln) }); // repeat until fixed point is reached: diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index 695564fa85b..282361fc13e 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -389,9 +389,9 @@ pub fn find_loop_scope(&mut self, -> &mut LoopScope<'tcx> { // find the loop-scope with the correct id self.loop_scopes.iter_mut() - .rev() - .filter(|loop_scope| loop_scope.extent == label) - .next() + .rev() + .filter(|loop_scope| loop_scope.extent == label) + .next() .unwrap_or_else(|| span_bug!(span, "no enclosing loop scope found?")) } diff --git a/src/librustc_mir/hair/cx/expr.rs b/src/librustc_mir/hair/cx/expr.rs index 80ba93f0bbf..3a9ac143f78 100644 --- a/src/librustc_mir/hair/cx/expr.rs +++ b/src/librustc_mir/hair/cx/expr.rs @@ -605,14 +605,21 @@ fn make_mirror_unadjusted<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, } hir::ExprRet(ref v) => ExprKind::Return { value: v.to_ref() }, hir::ExprBreak(label, ref value) => { - ExprKind::Break { - label: cx.tcx.region_maps.node_extent(label.loop_id), - value: value.to_ref(), + match label.loop_id.into() { + Ok(loop_id) => ExprKind::Break { + label: cx.tcx.region_maps.node_extent(loop_id), + value: value.to_ref(), + }, + Err(err) => bug!("invalid loop id for break: {}", err) } + } hir::ExprAgain(label) => { - ExprKind::Continue { - label: cx.tcx.region_maps.node_extent(label.loop_id), + match label.loop_id.into() { + Ok(loop_id) => ExprKind::Continue { + label: cx.tcx.region_maps.node_extent(loop_id), + }, + Err(err) => bug!("invalid loop id for continue: {}", err) } } hir::ExprMatch(ref discr, ref arms, _) => { diff --git a/src/librustc_passes/diagnostics.rs b/src/librustc_passes/diagnostics.rs index ef871959176..c576b35cb17 100644 --- a/src/librustc_passes/diagnostics.rs +++ b/src/librustc_passes/diagnostics.rs @@ -241,6 +241,22 @@ pub fn foo() {} } ``` "##, + +E0583: r##" +`break` or `continue` must include a label when used in the condition of a +`while` loop. + +Example of erroneous code: + +```compile_fail +while break {} +``` + +To fix this, add a label specifying which loop is being broken out of: +``` +`foo: while break `foo {} +``` +"## } register_diagnostics! { diff --git a/src/librustc_passes/loops.rs b/src/librustc_passes/loops.rs index 5f9acc0430a..003142f3498 100644 --- a/src/librustc_passes/loops.rs +++ b/src/librustc_passes/loops.rs @@ -87,11 +87,21 @@ fn visit_expr(&mut self, e: &'hir hir::Expr) { self.with_context(Closure, |v| v.visit_nested_body(b)); } hir::ExprBreak(label, ref opt_expr) => { + let loop_id = match label.loop_id.into() { + Ok(loop_id) => loop_id, + Err(hir::LoopIdError::OutsideLoopScope) => ast::DUMMY_NODE_ID, + Err(hir::LoopIdError::UnlabeledCfInWhileCondition) => { + self.emit_unlabled_cf_in_while_condition(e.span, "break"); + ast::DUMMY_NODE_ID + }, + Err(hir::LoopIdError::UnresolvedLabel) => ast::DUMMY_NODE_ID, + }; + if opt_expr.is_some() { - let loop_kind = if label.loop_id == ast::DUMMY_NODE_ID { + let loop_kind = if loop_id == ast::DUMMY_NODE_ID { None } else { - Some(match self.hir_map.expect_expr(label.loop_id).node { + Some(match self.hir_map.expect_expr(loop_id).node { hir::ExprWhile(..) => LoopKind::WhileLoop, hir::ExprLoop(_, _, source) => LoopKind::Loop(source), ref r => span_bug!(e.span, @@ -110,9 +120,15 @@ fn visit_expr(&mut self, e: &'hir hir::Expr) { } } } + self.require_loop("break", e.span); } - hir::ExprAgain(_) => self.require_loop("continue", e.span), + hir::ExprAgain(label) => { + if let Err(hir::LoopIdError::UnlabeledCfInWhileCondition) = label.loop_id.into() { + self.emit_unlabled_cf_in_while_condition(e.span, "continue"); + } + self.require_loop("continue", e.span) + }, _ => intravisit::walk_expr(self, e), } } @@ -143,4 +159,12 @@ fn require_loop(&self, name: &str, span: Span) { } } } + + fn emit_unlabled_cf_in_while_condition(&mut self, span: Span, cf_type: &str) { + struct_span_err!(self.sess, span, E0583, + "`break` or `continue` with no label in the condition of a `while` loop") + .span_label(span, + &format!("unlabeled `{}` in the condition of a `while` loop", cf_type)) + .emit(); + } } diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 676ff98e602..c254833be64 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -2770,18 +2770,24 @@ fn lookup_typo_candidate(&mut self, } } - fn resolve_labeled_block(&mut self, label: Option, id: NodeId, block: &Block) { + fn with_resolved_label(&mut self, label: Option, id: NodeId, f: F) + where F: FnOnce(&mut Resolver) + { if let Some(label) = label { let def = Def::Label(id); self.with_label_rib(|this| { this.label_ribs.last_mut().unwrap().bindings.insert(label.node, def); - this.visit_block(block); + f(this); }); } else { - self.visit_block(block); + f(self); } } + fn resolve_labeled_block(&mut self, label: Option, id: NodeId, block: &Block) { + self.with_resolved_label(label, id, |this| this.visit_block(block)); + } + fn resolve_expr(&mut self, expr: &Expr, parent: Option<&ExprKind>) { // First, record candidate traits for this expression if it could // result in the invocation of a method call. @@ -2835,18 +2841,18 @@ fn resolve_expr(&mut self, expr: &Expr, parent: Option<&ExprKind>) { ExprKind::Loop(ref block, label) => self.resolve_labeled_block(label, expr.id, &block), ExprKind::While(ref subexpression, ref block, label) => { - self.visit_expr(subexpression); - self.resolve_labeled_block(label, expr.id, &block); + self.with_resolved_label(label, expr.id, |this| { + this.visit_expr(subexpression); + this.visit_block(block); + }); } ExprKind::WhileLet(ref pattern, ref subexpression, ref block, label) => { - self.visit_expr(subexpression); - self.ribs[ValueNS].push(Rib::new(NormalRibKind)); - self.resolve_pattern(pattern, PatternSource::WhileLet, &mut FxHashMap()); - - self.resolve_labeled_block(label, expr.id, block); - - self.ribs[ValueNS].pop(); + self.with_resolved_label(label, expr.id, |this| { + this.visit_expr(subexpression); + this.resolve_pattern(pattern, PatternSource::WhileLet, &mut FxHashMap()); + this.visit_block(block); + }); } ExprKind::ForLoop(ref pattern, ref subexpression, ref block, label) => { diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index e9c9a63d79b..f2c256ed7b1 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -425,8 +425,9 @@ pub struct EnclosingLoops<'gcx, 'tcx> { } impl<'gcx, 'tcx> EnclosingLoops<'gcx, 'tcx> { - fn find_loop(&mut self, id: ast::NodeId) -> Option<&mut LoopCtxt<'gcx, 'tcx>> { - if let Some(ix) = self.by_id.get(&id).cloned() { + fn find_loop(&mut self, id: hir::LoopIdResult) -> Option<&mut LoopCtxt<'gcx, 'tcx>> { + let id_res: Result<_,_> = id.into(); + if let Some(ix) = id_res.ok().and_then(|id| self.by_id.get(&id).cloned()) { Some(&mut self.stack[ix]) } else { None @@ -3592,10 +3593,9 @@ fn check_expr_kind(&self, tcx.mk_nil() } hir::ExprBreak(label, ref expr_opt) => { - let loop_id = label.loop_id; let coerce_to = { let mut enclosing_loops = self.enclosing_loops.borrow_mut(); - enclosing_loops.find_loop(loop_id).map(|ctxt| ctxt.coerce_to) + enclosing_loops.find_loop(label.loop_id).map(|ctxt| ctxt.coerce_to) }; if let Some(coerce_to) = coerce_to { let e_ty; @@ -3610,8 +3610,9 @@ fn check_expr_kind(&self, e_ty = tcx.mk_nil(); cause = self.misc(expr.span); } + let mut enclosing_loops = self.enclosing_loops.borrow_mut(); - let ctxt = enclosing_loops.find_loop(loop_id).unwrap(); + let ctxt = enclosing_loops.find_loop(label.loop_id).unwrap(); let result = if let Some(ref e) = *expr_opt { // Special-case the first element, as it has no "previous expressions". diff --git a/src/test/run-pass/loop-break-value.rs b/src/test/run-pass/loop-break-value.rs index 10ff8dbf18e..4906a8e71d7 100644 --- a/src/test/run-pass/loop-break-value.rs +++ b/src/test/run-pass/loop-break-value.rs @@ -122,4 +122,20 @@ pub fn main() { panic!(); }; assert_eq!(nested_break_value, "hello"); + + let break_from_while_cond = loop { + 'inner_loop: while break 'inner_loop { + panic!(); + } + break 123; + }; + assert_eq!(break_from_while_cond, 123); + + let break_from_while_to_outer = 'outer_loop: loop { + while break 'outer_loop 567 { + panic!("from_inner"); + } + panic!("from outer"); + }; + assert_eq!(break_from_while_to_outer, 567); }