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

Detect .map_or(false, f) in manual_is_variant_and lint #13509

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

samueltardieu
Copy link
Contributor

.map_or(false, f) on a Option or a Result was not detected by manual_is_variant_and.

This change is small (first commit), but induces a lot of (rightful) fixes (second commit) in Clippy sources.

changelog: [manual_is_variant_and]: add detection of .map_or(false, f) pattern

@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2024

r? @Manishearth

rustbot has assigned @Manishearth.
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 Oct 6, 2024
@y21
Copy link
Member

y21 commented Oct 6, 2024

FWIW, this is also being implemented in #11796, though as a new lint and not part of manual_is_variant_and since the PR predates it

@samueltardieu
Copy link
Contributor Author

FWIW, this is also being implemented in #11796, though as a new lint and not part of manual_is_variant_and since the PR predates it

Oh. So what do we do? Although I think this check rather belongs to manual_is_variant_of and should be machine applicable, I don't mind dropping it if @Jacherr prefer to finalize their PR instead.

@samueltardieu
Copy link
Contributor Author

Moreover, @Jacherr PR could be retargeted to catch instances of .is_some_and(|x| x == V) (or .is_ok_and(|x| x == V)) and propose rewriting them as == Some(V) or == Ok(V). It could be applied in some cases after the current PR, and will catch more cases of code already using .is_some_and() and .is_ok_and().

@Jacherr
Copy link
Contributor

Jacherr commented Oct 6, 2024

The existing PR is pretty far through the review process and as far as I know just needs a few more changes before being ready for merging. I just need time to get round to it!

As is, I’m not sure there’s much benefit complicating the situation even further with that PR because it’s been open for almost a year now (oops..) As for these improvements, they can probably be retroactively applied after the fact (either by myself or you).

@samueltardieu
Copy link
Contributor Author

Fine with me, I'll keep this PR in my own version for the time being so that I can benefit from the lint (I detected some usages of .map_or(false, …) in some of my code and got it fixed using it) until your PR is merged, and I'll close it when your is in.

@Manishearth Manishearth removed their assignment Oct 7, 2024
@Manishearth
Copy link
Member

On vacation for a week. Feel free to retarget review with r? clippy

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.

5 participants