Add new lint unneeded_struct_pattern
#13465
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Centri3 (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
the
S-waiting-on-review
SpriteOvO
force-pushed
the
unneeded_struct_pattern
branch
5 times, most recently
from
d1a94c7 to
4733775
Compare
| "struct pattern is not needed for a unit variant", | ||
| "remove the struct pattern", | ||
| String::new(), | ||
| Applicability::Unspecified, |
There was a problem hiding this comment.
Suppose it can be MachineApplicable
SpriteOvO
force-pushed
the
unneeded_struct_pattern
branch
2 times, most recently
from
3b45390 to
36b1025
Compare
|
☔ The latest upstream changes (presumably #13334) made this pull request unmergeable. Please resolve the merge conflicts. |
SpriteOvO
force-pushed
the
unneeded_struct_pattern
branch
2 times, most recently
from
49f5231 to
d5ae91d
Compare
|
Rebased. r? @alex-semenyuk |
|
Failed to set assignee to
|
|
☔ The latest upstream changes (presumably #13376) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
I have a couple cases for you to consider, both whether we should lint them (imo yes) and whether it works on them.
Patterns in if let & let ... else
Patterns in function parameters: fn a(A::Empty {}: A) {}
Patterns in for: A::Empty {}
Fundamentally these are all likely the same, but they shouldn't be forgotten :) There may be some weird edge cases with them. (Though I don't think your changes will ever do so)
There was a problem hiding this comment.
Done, added test cases for them.
| let enum_did = cx.tcx.parent(did); | ||
| let variant = cx.tcx.adt_def(enum_did).variant_with_id(did); | ||
|
|
||
| let has_fields_brackets = !(variant.ctor.is_some() && variant.fields.is_empty()); |
There was a problem hiding this comment.
I'd prefer if the negation is done later, in the if. This roundabout way did confuse me at first (considering the name).
There was a problem hiding this comment.
Done. After re-reading this code I wrote 2 months later, I am also getting confused. 😅 Seems I had misnamed the variable.
| /// ``` | ||
| #[clippy::version = "1.83.0"] | ||
| pub UNNEEDED_STRUCT_PATTERN, | ||
| nursery, |
There was a problem hiding this comment.
I have changed it to style.
|
Hey @SpriteOvO, been almost a month and I'm pinging from triage. Has there been any progress? Is there anything you're stuck on or have any questions? |
|
@Centri3 Oh, sorry, I totally missed this in my inbox. I'll finish it in the next days. |
SpriteOvO
force-pushed
the
unneeded_struct_pattern
branch
3 times, most recently
from
1e6f710 to
79bed8a
Compare
tests/ui/unneeded_struct_pattern.rs
Outdated
| _ => 0, | ||
| }; | ||
|
|
||
| if let Custom::HasFields { field: value } = Custom::Init {} // Should be ignored |
There was a problem hiding this comment.
This is some very misleading formatting 😅 maybe add a statement within the block to make it clear these brackets are a block and not part of the value?
There was a problem hiding this comment.
Makes sense, I have added a noop(); call in the block.
tests/ui/unneeded_struct_pattern.rs
Outdated
| let Custom::HasBracketsNoFields {} = Custom::Init else { | ||
| panic!() | ||
| }; | ||
| // Should be ignored |
There was a problem hiding this comment.
nit: Note that these could be replaced with error annotations but both are used interchangeably in clippy atm
SpriteOvO
force-pushed
the
unneeded_struct_pattern
branch
from
79bed8a to
3760e75
Compare
SpriteOvO
force-pushed
the
unneeded_struct_pattern
branch
from
3760e75 to
52abb8f
Compare
Closes #13400.
changelog: [
unneeded_struct_pattern]: Add new lint