Properly implement labeled breaks in while conditions

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);
     }
 }

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);
             });
         }

src/librustc/hir/lowering.rs

@@ -81,6 +81,7 @@ pub struct LoweringContext<'a> {
     bodies: FxHashMap<hir::BodyId, hir::Body>,
 
     loop_scopes: Vec<NodeId>,
+    is_in_loop_condition: bool,
 
     type_def_lifetime_params: DefIdMap<usize>,
 }
@@ -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<T, F>(&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<T, F>(&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<T, F>(&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<Ident>)>) -> 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))),
                     ));
 
                     // `<pat> => <body>`

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<Result<NodeId, LoopIdError>> for LoopIdResult {
+    fn into(self) -> Result<NodeId, LoopIdError> {
+        match self {
+            LoopIdResult::Ok(ok) => Ok(ok),
+            LoopIdResult::Err(err) => Err(err),
+        }
+    }
+}
+impl From<Result<NodeId, LoopIdError>> for LoopIdResult {
+    fn from(res: Result<NodeId, LoopIdError>) -> 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<Spanned<Ident>>,
-    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)]

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:

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?"))
     }
 

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, _) => {

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! {

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();
+    }
 }

src/librustc_resolve/lib.rs

@@ -2770,18 +2770,24 @@ fn lookup_typo_candidate<FilterFn>(&mut self,
         }
     }
 
-    fn resolve_labeled_block(&mut self, label: Option<SpannedIdent>, id: NodeId, block: &Block) {
+    fn with_resolved_label<F>(&mut self, label: Option<SpannedIdent>, 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<SpannedIdent>, 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) => {

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".

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);
 }