Skip to content

Suggest removing value from break when invalid #47829

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 3, 2018
Merged

Suggest removing value from break when invalid #47829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions src/librustc_passes/loops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,11 @@ impl<'a, 'hir> Visitor<'hir> for CheckLoopVisitor<'a, 'hir> {
kind.name())
.span_label(e.span,
"can only break with a value inside `loop`")
.span_suggestion(e.span,
&format!("instead, use `break` on its own \
without a value inside this `{}` loop",
kind.name()),
"break".to_string())
.emit();
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/test/ui/loop-break-value-no-repeat.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ error[E0571]: `break` with value from a `for` loop
|
22 | break 22 //~ ERROR `break` with value from a `for` loop
| ^^^^^^^^ can only break with a value inside `loop`
help: instead, use `break` on its own without a value inside this `for` loop
|
22 | break //~ ERROR `break` with value from a `for` loop
Copy link
Member

Choose a reason for hiding this comment

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

This second "by example" version of the error doesn't seem like it's providing the user with additional information. Would it be possible to change this PR to just add the additional help message, without the break-only example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find that when suggesting removal of text, the inline suggestion style lends itself to confusion more easily. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I find this particular line is confusing-- I don't think it adds new information, the ERROR pattern on it is misleading since there would be no error in this case (since it's the corrected code):

22 |         break //~ ERROR `break` with value from a `for` loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that's just a comment, as far as rustc is concerned. If I change the test file to have that comment one line below, it would look like this:

error[E0571]: `break` with value from a `for` loop
  --> $DIR/loop-break-value-no-repeat.rs:22:9
   |
22 |         break 22
   |         ^^^^^^^^ can only break with a value inside `loop`
help: instead, use `break` on its own without a value inside this `for` loop
   |
22 |         break
   |         ^^^^^

instead of

error[E0571]: `break` with value from a `for` loop
  --> $DIR/loop-break-value-no-repeat.rs:22:9
   |
22 |         break 22
   |         ^^^^^^^^
   |         |
   |         can only break with a value inside `loop`
   |         help: use `break` without a value: `break`

Copy link
Member

@cramertj cramertj Jan 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, okay. I personally still prefer something like this:

error[E0571]: `break` with value from a `for` loop
  --> $DIR/loop-break-value-no-repeat.rs:22:9
   |
22 |         break 22
   |         ^^^^^^^^
   |         |
   |         can only break with a value inside `loop`
   |         help: use `break` without a value inside this `for` loop

But I'll defer to your judgement.

| ^^^^^

error: aborting due to previous error