Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest adding {} for 'label: non_block_expr #97759

Merged
merged 4 commits into from
Jun 6, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 62 additions & 2 deletions compiler/rustc_parse/src/parser/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ use rustc_ast::tokenstream::Spacing;
use rustc_ast::util::classify;
use rustc_ast::util::literal::LitError;
use rustc_ast::util::parser::{prec_let_scrutinee_needs_par, AssocOp, Fixity};
use rustc_ast::visit::Visitor;
use rustc_ast::StmtKind;
use rustc_ast::{self as ast, AttrStyle, AttrVec, CaptureBy, ExprField, Lit, UnOp, DUMMY_NODE_ID};
use rustc_ast::{AnonConst, BinOp, BinOpKind, FnDecl, FnRetTy, MacCall, Param, Ty, TyKind};
use rustc_ast::{Arm, Async, BlockCheckMode, Expr, ExprKind, Label, Movability, RangeLimits};
use rustc_ast_pretty::pprust;
use rustc_data_structures::thin_vec::ThinVec;
use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed, PResult};
use rustc_session::lint::builtin::BREAK_WITH_LABEL_AND_LOOP;
use rustc_session::lint::BuiltinLintDiagnostics;
Expand Down Expand Up @@ -1548,9 +1551,66 @@ impl<'a> Parser<'a> {
Ok(self.mk_expr_err(lo))
} else {
let msg = "expected `while`, `for`, `loop` or `{` after a label";
self.struct_span_err(self.token.span, msg).span_label(self.token.span, msg).emit();

let mut err = self.struct_span_err(self.token.span, msg);
err.span_label(self.token.span, msg);

// Continue as an expression in an effort to recover on `'label: non_block_expr`.
self.parse_expr()
let expr = self.parse_expr().map(|expr| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you actually use ? here, move the body of the closure to below, and remove the ? on line 1614?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other if branches also return Results, so I don't think it makes sense to do that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, oops, I thought there was only two if branches. I should've clicked the button to reveal the lines above 😓

let span = expr.span;

let found_labeled_breaks = {
struct FindLabeledBreaksVisitor(bool);

impl<'ast> Visitor<'ast> for FindLabeledBreaksVisitor {
fn visit_expr_post(&mut self, ex: &'ast Expr) {
if let ExprKind::Break(Some(_label), _) = ex.kind {
self.0 = true;
}
}
}

let mut vis = FindLabeledBreaksVisitor(false);
vis.visit_expr(&expr);
vis.0
};

// Suggestion involves adding a (as of time of writing this, unstable) labeled block.
//
// If there are no breaks that may use this label, suggest removing the label and
// recover to the unmodified expression.
if !found_labeled_breaks {
let msg = "consider removing the label";
err.span_suggestion_verbose(
lo.until(span),
msg,
"",
Applicability::MachineApplicable,
);

return expr;
}

let sugg_msg = "consider enclosing expression in a block";
let suggestions = vec![
(span.shrink_to_lo(), "{".to_owned()),
(span.shrink_to_hi(), "}".to_owned()),
WaffleLapkin marked this conversation as resolved.
Show resolved Hide resolved
];

err.multipart_suggestion_verbose(
sugg_msg,
suggestions,
Applicability::MachineApplicable,
);

// Replace `'label: non_block_expr` with `'label: {non_block_expr}` in order to supress future errors about `break 'label`.
WaffleLapkin marked this conversation as resolved.
Show resolved Hide resolved
let stmt = self.mk_stmt(span, StmtKind::Expr(expr));
let blk = self.mk_block(vec![stmt], BlockCheckMode::Default, span);
self.mk_expr(span, ExprKind::Block(blk, label), ThinVec::new())
});

err.emit();
expr
}?;

if !ate_colon && consume_colon {
Expand Down
6 changes: 6 additions & 0 deletions src/test/ui/parser/labeled-no-colon-expr.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ error: expected `while`, `for`, `loop` or `{` after a label
|
LL | 'l4 0;
| ^ expected `while`, `for`, `loop` or `{` after a label
|
help: consider removing the label
|
LL - 'l4 0;
LL + 0;
|

error: labeled expression must be followed by `:`
--> $DIR/labeled-no-colon-expr.rs:8:9
Expand Down
27 changes: 27 additions & 0 deletions src/test/ui/parser/recover-labeled-non-block-expr.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// run-rustfix
#![feature(label_break_value)]
fn main() {
let _ = 1 + 1; //~ ERROR expected `while`, `for`, `loop` or `{` after a label

match () { () => {}, }; //~ ERROR expected `while`, `for`, `loop` or `{` after a label
'label: {match () { () => break 'label, }}; //~ ERROR expected `while`, `for`, `loop` or `{` after a label
#[allow(unused_labels)]
'label: {match () { () => 'lp: loop { break 'lp 0 }, }}; //~ ERROR expected `while`, `for`, `loop` or `{` after a label

let x = 1;
let _i = 'label: {match x { //~ ERROR expected `while`, `for`, `loop` or `{` after a label
0 => 42,
1 if false => break 'label 17,
1 => {
if true {
break 'label 13
} else {
break 'label 0;
}
}
_ => 1,
}};

let other = 3;
let _val = 'label: {(1, if other == 3 { break 'label (2, 3) } else { other })}; //~ ERROR expected `while`, `for`, `loop` or `{` after a label
}
26 changes: 24 additions & 2 deletions src/test/ui/parser/recover-labeled-non-block-expr.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,27 @@
// run-rustfix
#![feature(label_break_value)]
fn main() {
'label: 1 + 1; //~ ERROR expected `while`, `for`, `loop` or `{` after a label
let _ = 'label: 1 + 1; //~ ERROR expected `while`, `for`, `loop` or `{` after a label

let _recovery_witness: () = 0; //~ ERROR mismatched types
'label: match () { () => {}, }; //~ ERROR expected `while`, `for`, `loop` or `{` after a label
'label: match () { () => break 'label, }; //~ ERROR expected `while`, `for`, `loop` or `{` after a label
#[allow(unused_labels)]
'label: match () { () => 'lp: loop { break 'lp 0 }, }; //~ ERROR expected `while`, `for`, `loop` or `{` after a label

let x = 1;
let _i = 'label: match x { //~ ERROR expected `while`, `for`, `loop` or `{` after a label
0 => 42,
1 if false => break 'label 17,
1 => {
if true {
break 'label 13
} else {
break 'label 0;
}
}
_ => 1,
};

let other = 3;
let _val = 'label: (1, if other == 3 { break 'label (2, 3) } else { other }); //~ ERROR expected `while`, `for`, `loop` or `{` after a label
}
80 changes: 69 additions & 11 deletions src/test/ui/parser/recover-labeled-non-block-expr.stderr
Original file line number Diff line number Diff line change
@@ -1,17 +1,75 @@
error: expected `while`, `for`, `loop` or `{` after a label
--> $DIR/recover-labeled-non-block-expr.rs:2:13
--> $DIR/recover-labeled-non-block-expr.rs:4:21
|
LL | 'label: 1 + 1;
| ^ expected `while`, `for`, `loop` or `{` after a label
LL | let _ = 'label: 1 + 1;
| ^ expected `while`, `for`, `loop` or `{` after a label
|
help: consider removing the label
|
LL - let _ = 'label: 1 + 1;
LL + let _ = 1 + 1;
|

error: expected `while`, `for`, `loop` or `{` after a label
--> $DIR/recover-labeled-non-block-expr.rs:6:13
|
LL | 'label: match () { () => {}, };
| ^^^^^ expected `while`, `for`, `loop` or `{` after a label
|
help: consider removing the label
|
LL - 'label: match () { () => {}, };
LL + match () { () => {}, };
|

error[E0308]: mismatched types
--> $DIR/recover-labeled-non-block-expr.rs:4:33
error: expected `while`, `for`, `loop` or `{` after a label
--> $DIR/recover-labeled-non-block-expr.rs:7:13
|
LL | 'label: match () { () => break 'label, };
| ^^^^^ expected `while`, `for`, `loop` or `{` after a label
|
help: consider enclosing expression in a block
|
LL | 'label: {match () { () => break 'label, }};
| + +

error: expected `while`, `for`, `loop` or `{` after a label
--> $DIR/recover-labeled-non-block-expr.rs:9:13
|
LL | 'label: match () { () => 'lp: loop { break 'lp 0 }, };
| ^^^^^ expected `while`, `for`, `loop` or `{` after a label
|
help: consider enclosing expression in a block
|
LL | 'label: {match () { () => 'lp: loop { break 'lp 0 }, }};
| + +

error: expected `while`, `for`, `loop` or `{` after a label
--> $DIR/recover-labeled-non-block-expr.rs:12:22
|
LL | let _i = 'label: match x {
| ^^^^^ expected `while`, `for`, `loop` or `{` after a label
|
help: consider enclosing expression in a block
|
LL ~ let _i = 'label: {match x {
LL | 0 => 42,
LL | 1 if false => break 'label 17,
LL | 1 => {
LL | if true {
LL | break 'label 13
...
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can someone explain where the second part of the suggestion (}) went?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestions only render up to 5 lines i think

Copy link
Member Author

@WaffleLapkin WaffleLapkin Jun 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that it'd show multiple parts :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bug in the rendering code. Your suggestion is still MachineApplicable so it will apply just fine, it's just the rendered code will be cut off.

I don't know if there's much you can do to fix it in this PR specifically, but maybe you could file a bug, or look into fixing the rendering code to omit lines in the middle?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll look into it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a "bug", but rather a non-implementation of the logic for multiple "windows" into the code, like span labels have.


error: expected `while`, `for`, `loop` or `{` after a label
--> $DIR/recover-labeled-non-block-expr.rs:26:24
|
LL | let _val = 'label: (1, if other == 3 { break 'label (2, 3) } else { other });
| ^ expected `while`, `for`, `loop` or `{` after a label
|
help: consider enclosing expression in a block
|
LL | let _recovery_witness: () = 0;
| -- ^ expected `()`, found integer
| |
| expected due to this
LL | let _val = 'label: {(1, if other == 3 { break 'label (2, 3) } else { other })};
| + +

error: aborting due to 2 previous errors
error: aborting due to 6 previous errors

For more information about this error, try `rustc --explain E0308`.