Skip to content

Conversation

@kraktus
Copy link
Contributor

@kraktus kraktus commented Sep 9, 2022

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 the Some but I've left it like it was for manual_map for now. needless_match::pat_same_as_expr provides a more generic match example.

close #8822

changelog: Add lint [manual_filter] for Option

@rust-highfive
Copy link

r? @dswij

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 9, 2022
@kraktus kraktus force-pushed the manual_filter2 branch 2 times, most recently from 5b29c09 to 85cf046 Compare September 9, 2022 20:52
Share much of its implementation with `manual_map` and should greatly benefit from its previous feedback.
@dswij
Copy link
Member

dswij commented Sep 16, 2022

I don't know if every new lint needs to go in nursery first.

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.

@dswij
Copy link
Member

dswij commented Sep 16, 2022

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

@bors
Copy link
Contributor

bors commented Sep 23, 2022

☔ The latest upstream changes (presumably #9525) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Oct 1, 2022

☔ The latest upstream changes (presumably #9484) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Oct 2, 2022

☔ The latest upstream changes (presumably #7962) made this pull request unmergeable. Please resolve the merge conflicts.

@kraktus kraktus force-pushed the manual_filter2 branch 2 times, most recently from 7e0eebf to 830fdf2 Compare October 3, 2022 12:11
Move common functions to `manual_utils.rs`, better arm matching, use clippy utils `contains_unsafe_block`
Copy link
Member

@dswij dswij left a 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.

@kraktus
Copy link
Contributor Author

kraktus commented Oct 5, 2022

Sounds good! Build failures are on crate where the lint does not trigger indeed

@dswij
Copy link
Member

dswij commented Oct 8, 2022

Thanks for this! @bors r+

@bors
Copy link
Contributor

bors commented Oct 8, 2022

📌 Commit 830fdf2 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 8, 2022

⌛ Testing commit 830fdf2 with merge 292e313...

@bors
Copy link
Contributor

bors commented Oct 8, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing 292e313 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.

Suggest use of Option::filter()

4 participants