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

new lint that detects useless match expression #8471

Merged
merged 6 commits into from
Mar 13, 2022

Conversation

J-ZhengLi
Copy link
Member

@J-ZhengLi J-ZhengLi commented Feb 25, 2022

fixes #7040

changelog: Add new lint [needless_match] under complexity lint group

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 25, 2022
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 2, 2022
modify `manual_map_option` uitest because one test case has confliction.
re-design test cases as some of them are not worth the effort to check.
@J-ZhengLi J-ZhengLi marked this pull request as ready for review March 8, 2022 10:26
@J-ZhengLi
Copy link
Member Author

Finally got some time to get back to this PR!

I could use some suggestions for my code, such as any missing cases, improvement for readability etc, thanks~ @rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 8, 2022
Copy link
Contributor

@llogiq llogiq 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 good! 👍 The naming should be more consistent, and we should reconsider the category, and I have a suggestion for further test cases, but otherwise this is merge-worthy.

/// }
/// ```
#[clippy::version = "1.61.0"]
pub NOP_MATCH,
Copy link
Contributor

Choose a reason for hiding this comment

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

NOP is uncommon vocabulary. I'd suggest naming it needless_match or match_identity (as we already have the quite similar map_identity).

/// ```
#[clippy::version = "1.61.0"]
pub NOP_MATCH,
correctness,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this as a correctness lint. At best, it's suspicious, or complexity.

D,
}

fn useless_match(x: i32) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a test case with a x => x arm?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point, I didn't check for that in my code. :D

Also, that reminds me, should I be checking ref x => *x as well? Or is that too much?

and change its lint group to "complexity"
@llogiq
Copy link
Contributor

llogiq commented Mar 10, 2022

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 10, 2022

📌 Commit ec91164 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Mar 10, 2022

⌛ Testing commit ec91164 with merge 7ba55de...

bors added a commit that referenced this pull request Mar 10, 2022
new lint that detects useless match expression

fixes #7040

changelog:
- Register new `correctness` lint called `nop_match`
- add basic test cases
- add logic to check for unnecessary match
- add logic to check for unnecessary if-let expressions, remove some test cases because checking them will take a lot extra effort

TODO:
- ~~detects unnecessary match~~
- ~~detects unnecessary if-let expression that similar to match~~
- modify `question_mark` and `manual_map` to solve possible confliction
@bors
Copy link
Contributor

bors commented Mar 10, 2022

💔 Test failed - checks-action_test

@J-ZhengLi
Copy link
Member Author

Thank you!

@bors r+

Thank you so much for the approval! But before I change the PR description which possiblly causing this being merged, may I request one last review for the next commit I'm about to push, I have taken your suggestion of adding a x => x check along with an extra. 😃

@llogiq
Copy link
Contributor

llogiq commented Mar 13, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Mar 13, 2022

📌 Commit 086b045 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Mar 13, 2022

⌛ Testing commit 086b045 with merge 75b616e...

@bors
Copy link
Contributor

bors commented Mar 13, 2022

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

@bors bors merged commit 75b616e into rust-lang:master Mar 13, 2022
@J-ZhengLi J-ZhengLi deleted the master-issue7040 branch March 16, 2022 01:27
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.

Remove unnecessary match
6 participants