Allow labeled loops as value expressions for `break` #87026

FabianWolff · 2021-07-10T14:46:23Z

Fixes #86948. This is currently allowed:

return 'label: loop { break 'label 42; };
break ('label: loop { break 'label 42; });
break 1 + 'label: loop { break 'label 42; };
break 'outer 'inner: loop { break 'inner 42; };

But not this:

break 'label: loop { break 'label 42; };

I have fixed this, so that the above now parses as an unlabeled break with a labeled loop as its value expression.

rust-highfive · 2021-07-10T14:46:26Z

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

estebank

I'm r+ with the code changes, but I want confirmation from @rust-lang/lang that this is something we want to support going forward (it technically changes the grammar).

compiler/rustc_parse/src/parser/expr.rs

src/test/ui/parser/break-with-labeled-loop.rs

nikomatsakis · 2021-07-11T15:15:19Z

Nominated for @rust-lang/lang. I believe what is being proposed here is a change to the grammar. I believe the grammar would be something like this:

BreakExpr :=
  'break' 
  'break' lifetime Expr
  'break' Expr

Expr :=
  lifetime: 'loop' 
  ...

In other words, you can write break 'foo: loop { .. }. I would sort of expect this to work but I guess it didn't, presumably because of a parser bug. (I've not read the code closely.)

Does this sound correct?

FabianWolff · 2021-07-11T16:25:55Z

In other words, you can write break 'foo: loop { .. }. I would sort of expect this to work but I guess it didn't, presumably because of a parser bug. (I've not read the code closely.)

Does this sound correct?

Yes, that sounds correct, thanks for having a look! I believe that this was a simple oversight in the parser, which currently accepts a label/lifetime for break greedily, while it should actually check whether the label might be part of the expression, which is what I have implemented here.

scottmcm · 2021-07-11T19:05:45Z

Given that this works with return already, I'd personally consider this a bug, and that it was probably expected to work this way the whole time.

(It seems like it's an easy mistake to make, since return try { ... } didn't use to work either, #76274.)

davidtwco · 2021-07-15T10:37:05Z

r? @estebank

joshtriplett · 2021-07-15T15:36:19Z

This seems a little confusing to me, and I think I would have expected parentheses as a disambiguator.

cramertj · 2021-07-15T18:11:21Z

+1 to @joshtriplett's comment. I think this ambiguous to both human and computer parsers. That is, it's very unclear to me whether the 'label in break 'label loop { ... } acts as a break target or as a loop label. return does not have this problem, because return does not take a target label.

estebank · 2021-07-15T18:24:25Z

This PR could easily be changed to properly parse this while emitting an error suggesting surrounding with parentheses.

scottmcm · 2021-07-15T18:45:25Z

I think this ambiguous to both human and computer parsers.

I don't think it's ambiguous to computers, because of break 'foo (no colon) vs break 'foo:.

return does not have this problem, because return does not take a target label.

But I do think this is a good point.

This seems like one of those places where it'd be great to have the grammar WG.

Do we have conventions for where we force parens, vs just encouraging them with a lint of some sort? It feels a bit off, in terms of BNF structure, to need a different production to go after break vs after return...

joshtriplett · 2021-07-20T18:09:31Z

We discussed this in the @rust-lang/lang meeting, and most people expressed that they'd prefer to require parentheses here (and the remaining people felt that we should at least recommend parens).

As @estebank suggested, we'd like to see the parser improved to handle this, but to emit an error message rather than accepting it.

Note that either variation should require parentheses: break 'foo (loop { ... }); or break ('foo: loop { ... });.

FabianWolff · 2021-07-21T16:28:19Z

Note that either variation should require parentheses: break 'foo (loop { ... }); or break ('foo: loop { ... });.

But that's a breaking change, because the following is allowed today:

fn main() {
    let x = 'lbl: loop {
        break 'lbl loop { break 42; };
    };
    println!("{}", x);
}

So should I emit a warning for break 'foo loop { ... }; and a hard error for break 'foo: loop { ... };?

joshtriplett · 2021-07-21T16:30:10Z

@FabianWolff Yes, the variant that's currently accepted should get a warning, and the variant that isn't should get a hard error.

FabianWolff · 2021-07-25T13:22:30Z

@FabianWolff Yes, the variant that's currently accepted should get a warning, and the variant that isn't should get a hard error.

This is now implemented, so this PR is ready for re-review @estebank.

@rustbot label: +S-waiting-on-review -S-waiting-on-author

estebank · 2021-07-29T14:26:23Z

compiler/rustc_parse/src/parser/expr.rs

+                        .struct_span_warn(
+                            lo.to(expr.span),
+                            "this labeled break expression is easily confused with an \
+                                           unlabeled break with a labeled value expression",
+                        )


@rust-lang/lang, are we ok with an unconditional warning here?

@FabianWolff we would ideally make this a lint, or a warning only on certain editions... Would you mind turning this into an error so that we can do a crater run to get an idea of what impact the unconditional warning will have in the ecosystem?

Besides that, I'm ok with the code as is (other than some wonky formatting above).

I have now turned the warning into a warn-by-default lint. This is therefore ready for re-review @estebank.

… expressions

compiler/rustc_lint/src/context.rs

estebank · 2021-08-03T07:26:45Z

r=me after addressing the last remaining nitpick

estebank · 2021-08-04T07:07:53Z

@bors r+

bors · 2021-08-04T07:07:54Z

📌 Commit 7c81132 has been approved by estebank

bors · 2021-08-04T07:17:28Z

⌛ Testing commit 7c81132 with merge 49ca3d9...

bors · 2021-08-04T09:58:10Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 49ca3d9 to master...

warning: for loop over a single element --> tests/test_expr.rs:333:5 | 333 | / for stmt in [ 334 | | // Parentheses required. See rust-lang/rust#87026. 335 | | quote! { 336 | | break 'label: loop { break 'label 42; }; ... | 339 | | syn::parse2::<Stmt>(stmt).unwrap_err(); 340 | | } | |_____^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_element_loop = note: `-W clippy::single-element-loop` implied by `-W clippy::all` = help: to override `-W clippy::all` add `#[allow(clippy::single_element_loop)]` help: try | 333 ~ { 334 + let stmt = { 335 + let mut _s = $crate::__private::TokenStream::new(); 336 + $crate::quote_each_token!{_s $($tt)*} 337 + _s 338 + }; 339 + syn::parse2::<Stmt>(stmt).unwrap_err(); 340 + } |

rust-highfive assigned davidtwco Jul 10, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2021

estebank reviewed Jul 10, 2021

View reviewed changes

compiler/rustc_parse/src/parser/expr.rs Outdated Show resolved Hide resolved

src/test/ui/parser/break-with-labeled-loop.rs Outdated Show resolved Hide resolved

FabianWolff force-pushed the issue-86948 branch from 785f46d to fa1738d Compare July 10, 2021 19:52

nikomatsakis added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jul 11, 2021

rust-highfive assigned estebank and unassigned davidtwco Jul 15, 2021

estebank added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2021

estebank approved these changes Jul 15, 2021

View reviewed changes

joshtriplett added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 20, 2021

FabianWolff force-pushed the issue-86948 branch from fa1738d to a3d95cf Compare July 25, 2021 13:20

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 25, 2021

estebank reviewed Jul 29, 2021

View reviewed changes

Require parentheses to avoid confusions around labeled break and loop…

470cbc0

… expressions

FabianWolff force-pushed the issue-86948 branch from a3d95cf to 470cbc0 Compare July 31, 2021 17:11

estebank reviewed Aug 3, 2021

View reviewed changes

compiler/rustc_lint/src/context.rs Outdated Show resolved Hide resolved

Use a multipart suggestion for the parentheses

7c81132

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 4, 2021

bors merged commit 49ca3d9 into rust-lang:master Aug 4, 2021

rustbot added this to the 1.56.0 milestone Aug 4, 2021

This was referenced Aug 4, 2021

Deprecate crate_type and crate_name nested inside #![cfg_attr] #83744

Merged

Lint against named asm labels #87324

Merged

dtolnay mentioned this pull request Nov 6, 2023

Improve parsing of labeled loop as value expression for break dtolnay/syn#1531

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow labeled loops as value expressions for `break` #87026

Allow labeled loops as value expressions for `break` #87026

FabianWolff commented Jul 10, 2021

rust-highfive commented Jul 10, 2021

estebank left a comment

nikomatsakis commented Jul 11, 2021

FabianWolff commented Jul 11, 2021

scottmcm commented Jul 11, 2021

davidtwco commented Jul 15, 2021

joshtriplett commented Jul 15, 2021

cramertj commented Jul 15, 2021 •

edited

Loading

estebank commented Jul 15, 2021

scottmcm commented Jul 15, 2021

joshtriplett commented Jul 20, 2021

FabianWolff commented Jul 21, 2021

joshtriplett commented Jul 21, 2021

FabianWolff commented Jul 25, 2021

estebank Jul 29, 2021

FabianWolff Jul 31, 2021

estebank commented Aug 3, 2021

estebank commented Aug 4, 2021

bors commented Aug 4, 2021

bors commented Aug 4, 2021

bors commented Aug 4, 2021

Allow labeled loops as value expressions for break #87026

Allow labeled loops as value expressions for break #87026

Conversation

FabianWolff commented Jul 10, 2021

rust-highfive commented Jul 10, 2021

estebank left a comment

Choose a reason for hiding this comment

nikomatsakis commented Jul 11, 2021

FabianWolff commented Jul 11, 2021

scottmcm commented Jul 11, 2021

davidtwco commented Jul 15, 2021

joshtriplett commented Jul 15, 2021

cramertj commented Jul 15, 2021 • edited Loading

estebank commented Jul 15, 2021

scottmcm commented Jul 15, 2021

joshtriplett commented Jul 20, 2021

FabianWolff commented Jul 21, 2021

joshtriplett commented Jul 21, 2021

FabianWolff commented Jul 25, 2021

estebank Jul 29, 2021

Choose a reason for hiding this comment

FabianWolff Jul 31, 2021

Choose a reason for hiding this comment

estebank commented Aug 3, 2021

estebank commented Aug 4, 2021

bors commented Aug 4, 2021

bors commented Aug 4, 2021

bors commented Aug 4, 2021

Allow labeled loops as value expressions for `break` #87026

Allow labeled loops as value expressions for `break` #87026

cramertj commented Jul 15, 2021 •

edited

Loading