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

Conversation

WaffleLapkin
Copy link
Member

Adds suggestions like this:

help: consider enclosing expression in a block
  |
3 |     'l {0};
  |        + +

inspired by #48594 (comment)

r? @compiler-errors

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 5, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2022
Comment on lines 29 to 37
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.

@WaffleLapkin
Copy link
Member Author

@rustbot label +A-diagnostics +A-suggestion-diagnostics

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` labels Jun 5, 2022
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

This looks basically read to go. Can you fix a few more nits? Thanks!

// 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 😓

compiler/rustc_parse/src/parser/expr.rs Outdated Show resolved Hide resolved
@compiler-errors
Copy link
Member

Okay this looks good! Thanks for the contribution.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 5, 2022

📌 Commit 4f85a73 has been approved by compiler-errors

@bors 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 Jun 5, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 6, 2022
…compiler-errors

Suggest adding `{}` for `'label: non_block_expr`

Adds suggestions like this:
```text
help: consider enclosing expression in a block
  |
3 |     'l {0};
  |        + +
```

inspired by rust-lang#48594 (comment)

r? `@compiler-errors`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 6, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#90905 (Add empty impl blocks if they have documentation)
 - rust-lang#97683 (Fail gracefully when encountering an HRTB in APIT. )
 - rust-lang#97721 (Do `suggest_await_before_try` with infer variables in self, and clean up binders)
 - rust-lang#97752 (typo: `-Zcodegen-backend=llvm -Cpasses=list` should work now)
 - rust-lang#97759 (Suggest adding `{}` for `'label: non_block_expr`)
 - rust-lang#97764 (use strict provenance APIs)
 - rust-lang#97765 (Restore a test that was intended to test `as` cast to ptr)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 554674b into rust-lang:master Jun 6, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 6, 2022
@WaffleLapkin WaffleLapkin deleted the recover_label_expr branch June 6, 2022 15:32
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 15, 2022
…that_are_quite_far_away_from_each_other, r=estebank

Hide irrelevant lines in suggestions to allow for suggestions that are far from each other to be shown

This is an attempt to fix suggestions one part of which is 6 lines or more far from the first. I've noticed "the problem" (of not showing some parts of the suggestion) here: rust-lang#97759 (comment).

I'm not sure about the implementation (this big closure is just bad and makes already complicated code even more so), but I want to at least discuss the result.

Here is an example of how this changes the output:

Before:
```text
help: consider enclosing expression in a block
  |
3 ~     'l: { match () { () => break 'l,
4 |
5 |
6 |
7 |
8 |
...
```

After:
```text
help: consider enclosing expression in a block
  |
3 ~     'l: { match () { () => break 'l,
4 |
...
31|
32~ } };
  |
```

r? `@estebank`
`@rustbot` label +A-diagnostics +A-suggestion-diagnostics
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 17, 2022
…that_are_quite_far_away_from_each_other, r=estebank

Hide irrelevant lines in suggestions to allow for suggestions that are far from each other to be shown

This is an attempt to fix suggestions one part of which is 6 lines or more far from the first. I've noticed "the problem" (of not showing some parts of the suggestion) here: rust-lang#97759 (comment).

I'm not sure about the implementation (this big closure is just bad and makes already complicated code even more so), but I want to at least discuss the result.

Here is an example of how this changes the output:

Before:
```text
help: consider enclosing expression in a block
  |
3 ~     'l: { match () { () => break 'l,
4 |
5 |
6 |
7 |
8 |
...
```

After:
```text
help: consider enclosing expression in a block
  |
3 ~     'l: { match () { () => break 'l,
4 |
...
31|
32~ } };
  |
```

r? `@estebank`
`@rustbot` label +A-diagnostics +A-suggestion-diagnostics
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 30, 2022
…quite_far_away_from_each_other, r=estebank

Hide irrelevant lines in suggestions to allow for suggestions that are far from each other to be shown

This is an attempt to fix suggestions one part of which is 6 lines or more far from the first. I've noticed "the problem" (of not showing some parts of the suggestion) here: rust-lang/rust#97759 (comment).

I'm not sure about the implementation (this big closure is just bad and makes already complicated code even more so), but I want to at least discuss the result.

Here is an example of how this changes the output:

Before:
```text
help: consider enclosing expression in a block
  |
3 ~     'l: { match () { () => break 'l,
4 |
5 |
6 |
7 |
8 |
...
```

After:
```text
help: consider enclosing expression in a block
  |
3 ~     'l: { match () { () => break 'l,
4 |
...
31|
32~ } };
  |
```

r? `@estebank`
`@rustbot` label +A-diagnostics +A-suggestion-diagnostics
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants