diff options
| -rw-r--r-- | compiler/rustc_lint/src/context.rs | 5 | ||||
| -rw-r--r-- | compiler/rustc_lint_defs/src/builtin.rs | 30 | ||||
| -rw-r--r-- | compiler/rustc_lint_defs/src/lib.rs | 1 | ||||
| -rw-r--r-- | compiler/rustc_parse/src/parser/expr.rs | 55 | ||||
| -rw-r--r-- | src/test/ui/parser/lifetime_starts_expressions.rs | 32 | ||||
| -rw-r--r-- | src/test/ui/parser/lifetime_starts_expressions.stderr | 50 |
6 files changed, 152 insertions, 21 deletions
diff --git a/compiler/rustc_lint/src/context.rs b/compiler/rustc_lint/src/context.rs index ad8a41a56cc..a20ceacdd3a 100644 --- a/compiler/rustc_lint/src/context.rs +++ b/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)); diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 7195c41eae9..b9c2f180e38 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/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" +} diff --git a/compiler/rustc_lint_defs/src/lib.rs b/compiler/rustc_lint_defs/src/lib.rs index 6c38b8f5bc0..b8f5345ffb8 100644 --- a/compiler/rustc_lint_defs/src/lib.rs +++ b/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 diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index b774c76103f..824f2316e58 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/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 }; diff --git a/src/test/ui/parser/lifetime_starts_expressions.rs b/src/test/ui/parser/lifetime_starts_expressions.rs index e0098793e1f..903b4de6ef4 100644 --- a/src/test/ui/parser/lifetime_starts_expressions.rs +++ b/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; + }; + }; } diff --git a/src/test/ui/parser/lifetime_starts_expressions.stderr b/src/test/ui/parser/lifetime_starts_expressions.stderr index 7275841ebb8..3af2c6b9e57 100644 --- a/src/test/ui/parser/lifetime_starts_expressions.stderr +++ b/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 |