Require parentheses to avoid confusions around labeled break and loop expressions

Author: FabianWolff

compiler/rustc_lint/src/context.rs

@@ -750,6 +750,11 @@ pub trait LintContext: Sized {
                         db.note(&format!("to ignore the value produced by the macro, add a semicolon after the invocation of `{name}`"));
                     }
                 }
+                BuiltinLintDiagnostics::BreakWithLabelAndLoop(span) => {
+                    if let Ok(code) = sess.source_map().span_to_snippet(span) {
+                        db.span_suggestion(span, "wrap this expression in parentheses", format!("({})", &code), Applicability::MachineApplicable);
+                    }
+                }
             }
             // Rewrap `db`, and pass control to the user.
             decorate(LintDiagnosticBuilder::new(db));

compiler/rustc_lint_defs/src/builtin.rs

@@ -2975,6 +2975,7 @@ declare_lint_pass! {
         RUST_2021_PRELUDE_COLLISIONS,
         RUST_2021_PREFIXES_INCOMPATIBLE_SYNTAX,
         UNSUPPORTED_CALLING_CONVENTIONS,
+        BREAK_WITH_LABEL_AND_LOOP,
     ]
 }
 
@@ -3348,3 +3349,32 @@ declare_lint! {
         reference: "issue #00000 <https://github.com/rust-lang/rust/issues/00000>",
     };
 }
+
+declare_lint! {
+    /// The `break_with_label_and_loop` lint detects labeled `break` expressions with
+    /// an unlabeled loop as their value expression.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// 'label: loop {
+    ///     break 'label loop { break 42; };
+    /// };
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// In Rust, loops can have a label, and `break` expressions can refer to that label to
+    /// break out of specific loops (and not necessarily the innermost one). `break` expressions
+    /// can also carry a value expression, which can be another loop. A labeled `break` with an
+    /// unlabeled loop as its value expression is easy to confuse with an unlabeled break with
+    /// a labeled loop and is thus discouraged (but allowed for compatibility); use parentheses
+    /// around the loop expression to silence this warning. Unlabeled `break` expressions with
+    /// labeled loops yield a hard error, which can also be silenced by wrapping the expression
+    /// in parentheses.
+    pub BREAK_WITH_LABEL_AND_LOOP,
+    Warn,
+    "`break` expression with label and unlabeled loop as value expression"
+}

compiler/rustc_lint_defs/src/lib.rs

@@ -304,6 +304,7 @@ pub enum BuiltinLintDiagnostics {
     OrPatternsBackCompat(Span, String),
     ReservedPrefix(Span),
     TrailingMacro(bool, Ident),
+    BreakWithLabelAndLoop(Span),
 }
 
 /// Lints that are buffered up early on in the `Session` before the

compiler/rustc_parse/src/parser/expr.rs

@@ -15,6 +15,8 @@ use rustc_ast::{AnonConst, BinOp, BinOpKind, FnDecl, FnRetTy, MacCall, Param, Ty
 use rustc_ast::{Arm, Async, BlockCheckMode, Expr, ExprKind, Label, Movability, RangeLimits};
 use rustc_ast_pretty::pprust;
 use rustc_errors::{Applicability, DiagnosticBuilder, PResult};
+use rustc_session::lint::builtin::BREAK_WITH_LABEL_AND_LOOP;
+use rustc_session::lint::BuiltinLintDiagnostics;
 use rustc_span::edition::LATEST_STABLE_EDITION;
 use rustc_span::source_map::{self, Span, Spanned};
 use rustc_span::symbol::{kw, sym, Ident, Symbol};
@@ -1375,14 +1377,59 @@ impl<'a> Parser<'a> {
         self.maybe_recover_from_bad_qpath(expr, true)
     }
 
-    /// Parse `"('label ":")? break expr?`.
+    /// Parse `"break" (('label (:? expr)?) | expr?)` with `"break"` token already eaten.
+    /// If the label is followed immediately by a `:` token, the label and `:` are
+    /// parsed as part of the expression (i.e. a labeled loop). The language team has
+    /// decided in #87026 to require parentheses as a visual aid to avoid confusion if
+    /// the break expression of an unlabeled break is a labeled loop (as in
+    /// `break 'lbl: loop {}`); a labeled break with an unlabeled loop as its value
+    /// expression only gets a warning for compatibility reasons; and a labeled break
+    /// with a labeled loop does not even get a warning because there is no ambiguity.
     fn parse_break_expr(&mut self, attrs: AttrVec) -> PResult<'a, P<Expr>> {
         let lo = self.prev_token.span;
-        let label = self.eat_label();
-        let kind = if self.token != token::OpenDelim(token::Brace)
+        let mut label = self.eat_label();
+        let kind = if label.is_some() && self.token == token::Colon {
+            // The value expression can be a labeled loop, see issue #86948, e.g.:
+            // `loop { break 'label: loop { break 'label 42; }; }`
+            let lexpr = self.parse_labeled_expr(label.take().unwrap(), AttrVec::new(), true)?;
+            self.struct_span_err(
+                lexpr.span,
+                "parentheses are required around this expression to avoid confusion with a labeled break expression",
+            )
+            .multipart_suggestion(
+                "wrap the expression in parentheses",
+                vec![
+                    (lexpr.span.shrink_to_lo(), "(".to_string()),
+                    (lexpr.span.shrink_to_hi(), ")".to_string()),
+                ],
+                Applicability::MachineApplicable,
+            )
+            .emit();
+            Some(lexpr)
+        } else if self.token != token::OpenDelim(token::Brace)
             || !self.restrictions.contains(Restrictions::NO_STRUCT_LITERAL)
         {
-            self.parse_expr_opt()?
+            let expr = self.parse_expr_opt()?;
+            if let Some(ref expr) = expr {
+                if label.is_some()
+                    && matches!(
+                        expr.kind,
+                        ExprKind::While(_, _, None)
+                            | ExprKind::ForLoop(_, _, _, None)
+                            | ExprKind::Loop(_, None)
+                            | ExprKind::Block(_, None)
+                    )
+                {
+                    self.sess.buffer_lint_with_diagnostic(
+                        BREAK_WITH_LABEL_AND_LOOP,
+                        lo.to(expr.span),
+                        ast::CRATE_NODE_ID,
+                        "this labeled break expression is easy to confuse with an unlabeled break with a labeled value expression",
+                        BuiltinLintDiagnostics::BreakWithLabelAndLoop(expr.span),
+                    );
+                }
+            }
+            expr
         } else {
             None
         };

src/test/ui/parser/lifetime_starts_expressions.rs

@@ -1,13 +1,39 @@
+#![allow(unused, dead_code)]
+
 fn foo() -> u32 {
     return 'label: loop { break 'label 42; };
 }
 
 fn bar() -> u32 {
     loop { break 'label: loop { break 'label 42; }; }
-    //~^ ERROR expected identifier, found keyword `loop`
-    //~| ERROR expected type, found keyword `loop`
+    //~^ ERROR: parentheses are required around this expression to avoid confusion
+    //~| HELP: wrap the expression in parentheses
+}
+
+fn baz() -> u32 {
+    'label: loop {
+        break 'label
+        //~^ WARNING: this labeled break expression is easy to confuse with an unlabeled break
+            loop { break 42; };
+            //~^ HELP: wrap this expression in parentheses
+    };
+
+    'label2: loop {
+        break 'label2 'inner: loop { break 42; };
+        // no warnings or errors here
+    }
 }
 
 pub fn main() {
-    foo();
+    // Regression test for issue #86948, as resolved in #87026:
+    let a = 'first_loop: loop {
+        break 'first_loop 1;
+    };
+    let b = loop {
+        break 'inner_loop: loop {
+        //~^ ERROR: parentheses are required around this expression to avoid confusion
+        //~| HELP: wrap the expression in parentheses
+            break 'inner_loop 1;
+        };
+    };
 }

src/test/ui/parser/lifetime_starts_expressions.stderr

@@ -1,23 +1,45 @@
-error: expected identifier, found keyword `loop`
-  --> $DIR/lifetime_starts_expressions.rs:6:26
+error: parentheses are required around this expression to avoid confusion with a labeled break expression
+  --> $DIR/lifetime_starts_expressions.rs:8:18
    |
 LL |     loop { break 'label: loop { break 'label 42; }; }
-   |                          ^^^^ expected identifier, found keyword
+   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-help: you can escape reserved keywords to use them as identifiers
+help: wrap the expression in parentheses
    |
-LL |     loop { break 'label: r#loop { break 'label 42; }; }
-   |                          ^^^^^^
+LL |     loop { break ('label: loop { break 'label 42; }); }
+   |                  ^                                 ^
 
-error: expected type, found keyword `loop`
-  --> $DIR/lifetime_starts_expressions.rs:6:26
+error: parentheses are required around this expression to avoid confusion with a labeled break expression
+  --> $DIR/lifetime_starts_expressions.rs:33:15
    |
-LL |     loop { break 'label: loop { break 'label 42; }; }
-   |                        - ^^^^ expected type
-   |                        |
-   |                        help: maybe write a path separator here: `::`
+LL |           break 'inner_loop: loop {
+   |  _______________^
+LL | |
+LL | |
+LL | |             break 'inner_loop 1;
+LL | |         };
+   | |_________^
+   |
+help: wrap the expression in parentheses
+   |
+LL |         break ('inner_loop: loop {
+LL |
+LL |
+LL |             break 'inner_loop 1;
+LL |         });
+   |
+
+warning: this labeled break expression is easy to confuse with an unlabeled break with a labeled value expression
+  --> $DIR/lifetime_starts_expressions.rs:15:9
+   |
+LL | /         break 'label
+LL | |
+LL | |             loop { break 42; };
+   | |_____________-----------------^
+   |               |
+   |               help: wrap this expression in parentheses: `(loop { break 42; })`
    |
-   = note: `#![feature(type_ascription)]` lets you annotate an expression with a type: `<expr>: <type>`
+   = note: `#[warn(break_with_label_and_loop)]` on by default
 
-error: aborting due to 2 previous errors
+error: aborting due to 2 previous errors; 1 warning emitted