refactor(unnecessary_map_or): move `.map_or(false, ` -> `.is_some_and(` transformation to `manual_is_variant_and` by ada4a · Pull Request #16388 · rust-lang/rust-clippy

ada4a · 2026-01-12T11:46:53Z

This is the third commit of #16383

Resolves #15998

Diffs best viewed with whitespace ignored and --color-moved

changelog: [unnecessary_map_or]: move .map_or(false, -> .is_some_and( and similar transformations to manual_is_variant_and

rustbot · 2026-01-12T11:46:58Z

r? @dswij

rustbot has assigned @dswij.
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

ada4a · 2026-01-12T11:47:42Z

Depends on:

so @rustbot blocked

github-actions · 2026-01-12T11:56:28Z

Lintcheck changes for ad29684

Lint	Added	Removed	Changed
`clippy::manual_is_variant_and`	22	0	1
`clippy::unnecessary_map_or`	0	22	1

This comment will be updated if you push new changes

ada4a · 2026-01-12T14:08:33Z

Dependencies merged, so @rustbot ready

teofr

Just some questions and comments on it.

View changes since this review

teofr · 2026-01-22T11:27:56Z

+    let _ = mac!(some 2).map_or(true, |x| x % 2 == 0);
+    let _ = mac!(some 2).map_or(false, |x| x % 2 == 0);


I know this is not part of this PR, mainly my own curiosity, is there any reason why these shouldn't lint?

Well for the ones with mac! it's not that they should not lint, it's just that methods lints don't trigger on anything having macros.. as that can get a bit complicated

teofr · 2026-01-22T11:37:46Z

+    if let ExprKind::Lit(def_kind) = def.kind
+        && let LitKind::Bool(def_bool) = def_kind.node
+        && let typeck = cx.typeck_results()
+        && let recv_ty = typeck.expr_ty_adjusted(recv)
+        && let wrap = match recv_ty.opt_diag_name(cx) {
+            Some(sym::Option) => "Some",
+            Some(sym::Result) => "Ok",
+            Some(_) | None => return false,
+        }
+        && typeck.expr_adjustments(recv).is_empty()
+        && let ExprKind::Closure(map_closure) = map.kind
+        && let closure_body = cx.tcx.hir_body(map_closure.body)
+        && let closure_body_value = closure_body.value.peel_blocks()
+        && let ExprKind::Binary(op, l, r) = closure_body_value.kind
+        && let [param] = closure_body.params
+        && let PatKind::Binding(_, hir_id, _, _) = param.pat.kind
+        // checking that map_or is one of the following:
+        // .map_or(false, |x| x == y)
+        // .map_or(false, |x| y == x) - swapped comparison
+        // .map_or(true, |x| x != y)
+        // .map_or(true, |x| y != x) - swapped comparison
+        && ((BinOpKind::Eq == op.node && !def_bool) || (BinOpKind::Ne == op.node && def_bool))
+        && let non_binding_location = if l.res_local_id() == Some(hir_id) { r } else { l }
+        && switch_to_eager_eval(cx, non_binding_location)
+        // if it's both then that's a strange edge case and
+        // we can just ignore it, since by default clippy will error on this
+        && (l.res_local_id() == Some(hir_id)) != (r.res_local_id() == Some(hir_id))
+        && !is_local_used(cx, non_binding_location, hir_id)
+        && let l_ty = typeck.expr_ty(l)
+        && l_ty == typeck.expr_ty(r)
+        && let Some(partial_eq) = cx.tcx.lang_items().eq_trait()
+        && implements_trait(cx, recv_ty, partial_eq, &[recv_ty.into()])
+        && is_copy(cx, l_ty)
+        && !is_from_proc_macro(cx, expr)


You'd probably enjoy Scala's for comprehensions

Ha:) But yeah I like let-chains a lot, they're easily one of my favorite newer features in Rust. Don't you just love it when the whole lint definition is just one big chain:)

teofr · 2026-01-22T11:42:08Z

+    // Don't lint, `unnecessary_map_or` will take over
+    #[derive(PartialEq)]
+    struct S1;
+    let r: Result<i32, S1> = Ok(3);
+    let _ = r.map_or(false, |x| x == 7);
+    //~^ unnecessary_map_or


Note: If you allow unnecessary_map_or here, then manual_is_variant_and won't lint either.

teofr · 2026-01-22T11:46:11Z

+                    if !unnecessary_map_or::check(cx, expr, recv, def, map) {
+                        manual_is_variant_and::check_map_or(cx, expr, recv, span, def, map, self.msrv);
+                    }


I'm not a huge fan of this solution, but I also lack the expertise on Clippy to see a better solution.

I think my main issue is that it entangles two independent lints. My ideal solution in this case would be to produce both lints (and both suggestions), but I'm not sure if this is allowed by Clippy. And if it could take any kind of priority to choose which one to apply automatically.

As a note, a real issue with this approach is that disallowing the deciding lint (unnecessary_map_or) doesn't enable the secondary one. Check the note in the manual_is_variant_and testcase.

In any case I'd add a comment explaining that these two lints overlap each other, and why priority is given to one over the other.

I'm not a huge fan of this solution, but I also lack the expertise on Clippy to see a better solution.

I understand what you mean, and don't like this intertwining itself, but unfortunately I don't know of a better way myself (I'm relatively new here^^). I do need to note, though, that this approach is used here and there in the repository.

My ideal solution in this case would be to produce both lints (and both suggestions), but I'm not sure if this is allowed by Clippy

There isn't anything forbidding this, but if there are multiple possible conflicting suggestions on the same expression, we usually try to only show the most relevant one. In this case, transforming .map_or(false, |n| n == 5) to == Some(5) is a lot more helpful than just to .is_some_and(|n| n == 5).

a real issue with this approach is that disallowing the deciding [..] doesn't enable the secondary one

Right, that's a good catch! I think it could be solved by refining unnecessary_map_or::check to do something like:

if /* conditions */ { let mut fired = false; span_lint_and_then( /* .. */, |diag| { /* .. */ fired = true; } ); fired } else { false }

Though I agree that this is not very pretty.

In any case I'd add a comment explaining that these two lints overlap each other, and why priority is given to one over the other.

That's a good idea. Added a comment

I think it could be solved by refining unnecessary_map_or::check to do something like:

I agree it's not very elegant, it'd be interesting if span_lint_and_then returned a boolean itself, or even better the result of the closure wrapped in an Option<>.

But anyways, not a big deal and it seems to be in line with the rest of the codebase.

ada4a · 2026-02-23T20:56:07Z

r? clippy

rustbot · 2026-04-26T13:42:40Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

rustbot assigned dswij Jan 12, 2026

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 12, 2026

rustbot added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 12, 2026

ada4a mentioned this pull request Jan 12, 2026

feat(needless_is_variant_and): new lint #16389

Open