Don't lint 'unused_parens on if (break _) { .. }`
#54885
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Travis is still pretty grumpy. |
|
Sorry about that, I failed to commit the changed .stderr – will do that once I get to a PC with a stable connection. |
|
Travis is now happy. |
|
This will likely introduce a merge conflict with #54820 just a heads up! I don't mind on the order these go in; I think both will be pretty easy to rebase from. |
src/librustc_lint/unused.rs
Outdated
| err.emit(); | ||
| if struct_lit_needs_parens && | ||
| (parser::contains_exterior_struct_lit(&inner) || | ||
| if let ast::ExprKind::Break(..) = inner.node { |
There was a problem hiding this comment.
You can probably simplify this addition down to ... || inner.node == ast::ExprKind::Break
There was a problem hiding this comment.
That would be surprising to say the least, because ExprKind::Break is defined as a tuple struct of two values (note the two dots). I didn't know you could compare with incomplete value definitions.
There was a problem hiding this comment.
Sorry yes you may still need the two dots! What I was trying to point out is that you can just check if inner.node is a ExprKind::Break as the whole condition and get ride of the ... { true } else { false }.
There was a problem hiding this comment.
How would that work? The if let needs some values in the then/else branches.
There was a problem hiding this comment.
Ah yes I see what you mean. I was under the impression we could get a bool from inner.node == ast::ExprKind::Break(..). What I was mainly hoping to simplify in all this was getting rid of the additional ... { true } else { false } as it just looks a little odd to read.
A way I see doing that is keeping things how they were (with necessary) and going for something like adding those additional four lines to check for a Break.
let necessary = struct_lit_needs_parens && parser::contains_exterior_struct_lit(&inner);
if !necessary {
+ // We don't want to lint when there is a `break` in `inner`
+ if let ast::ExprKind::Break(..) = inner.node {
+ return;
+ }
...
}
Either way - just an initial reaction I had to seeing the new additions! I'm fine keeping it how it is.
|
☔ The latest upstream changes (presumably #55095) made this pull request unmergeable. Please resolve the merge conflicts. |
src/librustc_lint/unused.rs
Outdated
| err.emit(); | ||
| if struct_lit_needs_parens && | ||
| (parser::contains_exterior_struct_lit(&inner) || | ||
| if let ast::ExprKind::Break(..) = inner.node { |
There was a problem hiding this comment.
Nit: I'd prefer to see this if let captured into a temporary value, or perhaps with a helper function. This is a pretty complex conditional! Maybe even break it into a if
There was a problem hiding this comment.
That'd be a pity; I just found a way to use a return instead of a boolean that would shorten the code considerably. But I can (and should) add a comment why we don't lint on break here.
There was a problem hiding this comment.
I don't follow what would be a pity...? I was just suggesting reformatting things
There was a problem hiding this comment.
Sorry, I've been busy lately. I'll revisit and rebase this later this week.
src/librustc_lint/unused.rs
Outdated
| @@ -270,47 +270,52 @@ impl UnusedParens { | |||
| msg: &str, | |||
| struct_lit_needs_parens: bool) { | |||
There was a problem hiding this comment.
Nit: this name no longer seems very accurate; let's call it something like is_restricted_expr: bool perhaps? And I think a comment on check_unused_parens_core would be good. Something like:
Invoked on expressions that appear in places where people traditionally put parentheses out of habit, but where they are not needed in Rust. For example, if (a == b) { .. }. Issues a lint if value is a parenthesized node, unless those parentheses are deemed "necessary".
Parentheses are considered necessary if (a) is_restricted_expr is true, which indicates that this is a context (such as an if) where only a subset of expressions are permitted and (b) the parenthesized expression falls outside of that subset. So e.g. if (Foo { a: 22 }) { .. } would not get a warning; similarly while (break) { .. } would not.
nikomatsakis
added
S-waiting-on-author
|
Rebased on current master, this time I could piggy-back on the recent changes to the lint. |
|
@bors r+ rollup |
|
📌 Commit 1a37575 has been approved by |
bors
added
S-waiting-on-bors
Don't lint 'unused_parens` on `if (break _) { .. }`
This fixes rust-lang#54704
Rollup of 12 pull requests Successful merges: - #54885 (Don't lint 'unused_parens` on `if (break _) { .. }`) - #55205 (Improve a few cases of collecting to an FxHash(Map/Set)) - #55450 (msp430: remove the whole Atomic* API) - #55459 (Add UI test for #49296) - #55472 (Use opt.take() instead of mem::replace(opt, None)) - #55473 (Take advantage of impl Iterator in (transitive/elaborate)_bounds) - #55474 (Fix validation false positive) - #55476 (Change a flat_map with 0/1-element vecs to a filter_map) - #55487 (Adjust Ids of path segments in visibility modifiers) - #55493 (Doc fixes) - #55494 (borrowck=migrate must look at parents of closures) - #55496 (Update clippy) Failed merges: r? @ghost
This fixes #54704