-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
[unused_braces
] Lint multiline blocks as long as not in arms
#102432
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
r? @lcnr |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of modifying the ui tests, can you add #![allow(unused_braces)]
to the tests instead
apart from that these changes look good to me, not sure if changing a style lint like this warrants a more formal decision, so I am just going to get another compiler member to also sign this off 🤷
r? compiler |
f81fc72
to
0d0a96b
Compare
This comment has been minimized.
This comment has been minimized.
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
Saving the comments before rebasing
|
76cc9cc
to
4653e5c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I just want to emphasize that the concern raised was not exclusive to |
Afaik craters only look at compilation failures, so this lint could be sent as deny before running it and see how many crates now fail to compile. To be accurate the number of failure should not take into account crates that would have triggered this lint before its scope expansion. |
f79f70c
to
0e5a741
Compare
Solution 2. (silencing the warning on |
☔ The latest upstream changes (presumably #106914) made this pull request unmergeable. Please resolve the merge conflicts. |
e5d339c
to
9f203f2
Compare
☔ The latest upstream changes (presumably #105582) made this pull request unmergeable. Please resolve the merge conflicts. |
Currently the lint faces a severe limitation: since it only catches single-line block, running rustfmt beforehand will remove all occurences of it, because it breaks them into multiline blocks. We do not check match `Arm` for two reasons: - In case it does not use commas to separate arms, removing the block would result in a compilation error Example: ``` match expr { pat => {()} _ => println!("foo") } ``` - Do not lint multiline match arms used for formatting reasons ``` match expr { pat => { somewhat_long_expression } // ... } ``` Delete `unused-braces-lint` test The modified lint correctly provide a span in its suggestion. ```shell error: unnecessary braces around block return value --> /rust/src/test/rustdoc-ui/unused-braces-lint.rs:9:5 | LL | / { LL | | { | |________^ LL | use std; LL | } | __________^ LL | | } | |_____^ | note: the lint level is defined here --> /rust/src/test/rustdoc-ui/unused-braces-lint.rs:6:9 | LL | #![deny(unused_braces)] | ^^^^^^^^^^^^^ help: remove these braces | LL ~ { LL | use std; LL ~ } | ``` It is unclear to which extend rust-lang#70814 is still an issue, as the inital MCVE does not trigger the lint on stable either,[rust playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b6ff31a449c0b73a08daac8ee43b1fa6) Fix code with expanded `unused_braces` lint Also allow `unused_braces` on tests Mute `unused_braces` on `match_ast!`
9f203f2
to
e641393
Compare
@calebcartwright gentle ping about this |
☔ The latest upstream changes (presumably #101841) made this pull request unmergeable. Please resolve the merge conflicts. |
T-style discussed this yesterday in our meeting. Our recommendation is that this change should not be enforced within macro args. Is it possible to modify your implementation to continue silently accepting those cases? It should be possible with the visitor structure of the lint. Also, I would be keen on reverting the usages of |
@rustbot author Waiting on changes to be made + a rebase, use |
Given this, do we still need the RA change? |
It shouldn't be necessary anymore |
@kraktus FYI: when a PR is ready for review, send a message containing |
FWIW it's still there 🙂. |
Hey, thanks for the ping, I don’t plan to work on this PR in the foreseeable future. |
In that case, closing this PR. Thanks for the contribution. |
The commit message include removing braces in arm matches but I could not find a way to make a suggestion for it, since it some cases you would need to replace the block by a comma
Example:
?r @lcnr since we discussed briefly about it on zulip. Happy to have your feedback on this