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 removing value from break when invalid #47829

Merged
merged 1 commit into from
Feb 3, 2018

Conversation

estebank
Copy link
Contributor

When attempting to use break with a value in a type of loop where it'd be invalid (any non-loop), suggest using break on its own.

Close #34359.

@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 29, 2018
@@ -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

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.

@cramertj
Copy link
Member

r=me with nit fixed.

@estebank
Copy link
Contributor Author

@bors r=cramertj rollup

We can change this to inline afterwards. I'm thinking that the current output style could be reserved for -Zteach and use your preferred inline output for normal output, but until we expand/stabilize/publicize -Zteach I prefer being as explicit as possible to minimize confusion for newcomers.

@bors
Copy link
Contributor

bors commented Jan 31, 2018

📌 Commit b7437c5 has been approved by cramertj

@estebank estebank 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 Jan 31, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Feb 2, 2018
…uggest removing value from `break` when invalid When attempting to use `break` with a value in a type of loop where it'd be invalid (any non-`loop`), suggest using `break` on its own. Close rust-lang#34359.
bors added a commit that referenced this pull request Feb 3, 2018
Rollup of 10 pull requests

- Successful merges: #46156, #47829, #47842, #47898, #47914, #47916, #47919, #47942, #47951, #47973
- Failed merges: #47753
@bors bors merged commit b7437c5 into rust-lang:master Feb 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants