Skip to content

Suggest collapsing nested or patterns if the MSRV allows it #12745

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

Merged
merged 2 commits into from
May 2, 2024

Conversation

y21
Copy link
Member

@y21 y21 commented May 2, 2024

Nested or patterns have been stable since 1.53, so we should be able to suggest Some(1 | 2) if the MSRV isn't set below that.

This change adds an msrv check and also moves it to matches/mod.rs, because it's also needed by redundant_guards.

changelog: [collapsible_match]: suggest collapsing nested or patterns if the MSRV allows it

@rustbot
Copy link
Collaborator

rustbot commented May 2, 2024

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 2, 2024
Comment on lines -28 to -33
Arm {
pat: Pat {
kind: PatKind::Wild, ..
},
..
},
Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't look right. It seems like the intention was to catch if matches!, but because of the way it was written it would still emit a warning on a manually written match that happens to have the same pattern structure but does something in the _ => ... arm that's semantically different. Changed it to look for the matches! macro and added a test case for a false positive that this fixes.

@y21
Copy link
Member Author

y21 commented May 2, 2024

Also one thing I've noticed, these two lints redundant_guards and collapsible_match have a lot in common but unfortunately collapsible_match looks for far fewer patterns. I think it'd be neat if they could catch the same patterns by sharing the code and that collapsible_match would catch

if let Some(v) = Some(42) {
  if v == 42 {}
}

and suggest if let Some(42), just like how redundant_guards does it. I'll try to look into that.

@llogiq
Copy link
Contributor

llogiq commented May 2, 2024

Agreed, unifying those lints will improve functionality and performance. I'll look forward to a follow-up PR. This one already looks merge-worthy, so thank you for improving clippy!

@bors r+

@bors
Copy link
Contributor

bors commented May 2, 2024

📌 Commit 790fb93 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 2, 2024

⌛ Testing commit 790fb93 with merge 20b085d...

@bors
Copy link
Contributor

bors commented May 2, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 20b085d to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants