-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add manual_filter
lint for Option
#9451
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
Conversation
r? @dswij (rust-highfive has picked a reviewer for you, use r? to override) |
5b29c09
to
85cf046
Compare
Share much of its implementation with `manual_map` and should greatly benefit from its previous feedback.
There's no requirement to go to nursery first. I'll try to run this on the most popular crates to see if there's lots of FPs. If not, then I think complexity is fine. |
I ran this on 900 most popular crates. Here's the result. At first glance, it seems to work as expected. I'll try taking a look further when time hopefully allows |
☔ The latest upstream changes (presumably #9525) made this pull request unmergeable. Please resolve the merge conflicts. |
b827440
to
7bb45f8
Compare
☔ The latest upstream changes (presumably #9484) made this pull request unmergeable. Please resolve the merge conflicts. |
7bb45f8
to
a994546
Compare
☔ The latest upstream changes (presumably #7962) made this pull request unmergeable. Please resolve the merge conflicts. |
7e0eebf
to
830fdf2
Compare
Move common functions to `manual_utils.rs`, better arm matching, use clippy utils `contains_unsafe_block`
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.
LGTM, and running on this on 900 most popular crates seems fine.
There's quite a bit of build error, but I think it shouldn't be related to the lint itself.
Sounds good! Build failures are on crate where the lint does not trigger indeed |
Thanks for this! @bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Share much of its implementation with
manual_map
and should greatly benefit from its previous feedback.I'm sure it's possible to even more refactor both and would gladly take input on that as well as any clippy idiomatic usage, since this is my first lint addition.
I've added the lint to the complexity section for now, I don't know if every new lint needs to go in nursery first.
The matching could be expanded to more than
Some(<value>)
to lint on arbitrary struct matching inside theSome
but I've left it like it was formanual_map
for now.needless_match::pat_same_as_expr
provides a more generic match example.close #8822
changelog: Add lint [
manual_filter
] forOption