From 2b50456eb4b3b5abdb13a36b62a92a268b740d4e Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Sun, 23 Nov 2025 20:48:47 +0100 Subject: [PATCH] clean-up `unnecessary_map_or` and `manual_is_variant_and` - manual_is_variant_and: create `Flavor` using a function - removes one level of `for` --- .../src/methods/manual_is_variant_and.rs | 129 ++++++++++-------- clippy_lints/src/methods/mod.rs | 5 +- .../src/methods/unnecessary_map_or.rs | 34 ++--- tests/ui/unnecessary_map_or.fixed | 2 +- tests/ui/unnecessary_map_or.rs | 2 +- tests/ui/unnecessary_map_or.stderr | 38 +++--- 6 files changed, 111 insertions(+), 99 deletions(-) diff --git a/clippy_lints/src/methods/manual_is_variant_and.rs b/clippy_lints/src/methods/manual_is_variant_and.rs index 8f65858987b9..5ce9d364cdd8 100644 --- a/clippy_lints/src/methods/manual_is_variant_and.rs +++ b/clippy_lints/src/methods/manual_is_variant_and.rs @@ -1,12 +1,13 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::res::MaybeDef; -use clippy_utils::source::{snippet, snippet_with_applicability}; +use clippy_utils::source::snippet_with_applicability; use clippy_utils::sugg::Sugg; use clippy_utils::{get_parent_expr, sym}; use rustc_ast::LitKind; use rustc_errors::Applicability; use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res}; +use rustc_hir::def_id::DefId; use rustc_hir::{BinOpKind, Closure, Expr, ExprKind, QPath}; use rustc_lint::LateContext; use rustc_middle::ty; @@ -14,7 +15,37 @@ use rustc_span::{Span, Symbol}; use super::MANUAL_IS_VARIANT_AND; -pub(super) fn check( +#[derive(Clone, Copy, PartialEq)] +enum Flavor { + Option, + Result, +} + +impl Flavor { + fn new(cx: &LateContext<'_>, def_id: DefId) -> Option { + match cx.tcx.get_diagnostic_name(def_id)? { + sym::Option => Some(Self::Option), + sym::Result => Some(Self::Result), + _ => None, + } + } + + const fn diag_sym(self) -> Symbol { + match self { + Self::Option => sym::Option, + Self::Result => sym::Result, + } + } + + const fn positive_variant_name(self) -> Symbol { + match self { + Self::Option => sym::Some, + Self::Result => sym::Ok, + } + } +} + +pub(super) fn check_map_unwrap_or_default( cx: &LateContext<'_>, expr: &Expr<'_>, map_recv: &Expr<'_>, @@ -30,11 +61,13 @@ pub(super) fn check( } // 2. the caller of `map()` is neither `Option` nor `Result` - let is_option = cx.typeck_results().expr_ty(map_recv).is_diag_item(cx, sym::Option); - let is_result = cx.typeck_results().expr_ty(map_recv).is_diag_item(cx, sym::Result); - if !is_option && !is_result { + let Some(flavor) = (cx.typeck_results()) + .expr_ty(map_recv) + .opt_def_id() + .and_then(|did| Flavor::new(cx, did)) + else { return; - } + }; // 3. the caller of `unwrap_or_default` is neither `Option` nor `Result` if !cx.typeck_results().expr_ty(expr).is_bool() { @@ -46,44 +79,23 @@ pub(super) fn check( return; } - let lint_msg = if is_option { - "called `map().unwrap_or_default()` on an `Option` value" - } else { - "called `map().unwrap_or_default()` on a `Result` value" + let lint_span = expr.span.with_lo(map_span.lo()); + let lint_msg = match flavor { + Flavor::Option => "called `map().unwrap_or_default()` on an `Option` value", + Flavor::Result => "called `map().unwrap_or_default()` on a `Result` value", }; - let suggestion = if is_option { "is_some_and" } else { "is_ok_and" }; - span_lint_and_sugg( - cx, - MANUAL_IS_VARIANT_AND, - expr.span.with_lo(map_span.lo()), - lint_msg, - "use", - format!("{}({})", suggestion, snippet(cx, map_arg.span, "..")), - Applicability::MachineApplicable, - ); -} + span_lint_and_then(cx, MANUAL_IS_VARIANT_AND, lint_span, lint_msg, |diag| { + let method = match flavor { + Flavor::Option => "is_some_and", + Flavor::Result => "is_ok_and", + }; -#[derive(Clone, Copy, PartialEq)] -enum Flavor { - Option, - Result, -} + let mut app = Applicability::MachineApplicable; + let map_arg_snippet = snippet_with_applicability(cx, map_arg.span, "_", &mut app); -impl Flavor { - const fn symbol(self) -> Symbol { - match self { - Self::Option => sym::Option, - Self::Result => sym::Result, - } - } - - const fn positive(self) -> Symbol { - match self { - Self::Option => sym::Some, - Self::Result => sym::Ok, - } - } + diag.span_suggestion(lint_span, "use", format!("{method}({map_arg_snippet})"), app); + }); } #[derive(Clone, Copy, PartialEq)] @@ -178,7 +190,7 @@ fn emit_lint<'tcx>( cx, MANUAL_IS_VARIANT_AND, span, - format!("called `.map() {op} {pos}()`", pos = flavor.positive(),), + format!("called `.map() {op} {pos}()`", pos = flavor.positive_variant_name()), "use", format!( "{inversion}{recv}.{method}({body})", @@ -195,24 +207,23 @@ pub(super) fn check_map(cx: &LateContext<'_>, expr: &Expr<'_>) { && op.span.eq_ctxt(expr.span) && let Ok(op) = Op::try_from(op.node) { - // Check `left` and `right` expression in any order, and for `Option` and `Result` + // Check `left` and `right` expression in any order for (expr1, expr2) in [(left, right), (right, left)] { - for flavor in [Flavor::Option, Flavor::Result] { - if let ExprKind::Call(call, [arg]) = expr1.kind - && let ExprKind::Lit(lit) = arg.kind - && let LitKind::Bool(bool_cst) = lit.node - && let ExprKind::Path(QPath::Resolved(_, path)) = call.kind - && let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), _) = path.res - && let ty = cx.typeck_results().expr_ty(expr1) - && let ty::Adt(adt, args) = ty.kind() - && cx.tcx.is_diagnostic_item(flavor.symbol(), adt.did()) - && args.type_at(0).is_bool() - && let ExprKind::MethodCall(_, recv, [map_expr], _) = expr2.kind - && cx.typeck_results().expr_ty(recv).is_diag_item(cx, flavor.symbol()) - && let Ok(map_func) = MapFunc::try_from(map_expr) - { - return emit_lint(cx, parent_expr.span, op, flavor, bool_cst, map_func, recv); - } + if let ExprKind::Call(call, [arg]) = expr1.kind + && let ExprKind::Lit(lit) = arg.kind + && let LitKind::Bool(bool_cst) = lit.node + && let ExprKind::Path(QPath::Resolved(_, path)) = call.kind + && let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), _) = path.res + && let ExprKind::MethodCall(_, recv, [map_expr], _) = expr2.kind + && let ty = cx.typeck_results().expr_ty(expr1) + && let ty::Adt(adt, args) = ty.kind() + && let Some(flavor) = Flavor::new(cx, adt.did()) + && args.type_at(0).is_bool() + && cx.typeck_results().expr_ty(recv).is_diag_item(cx, flavor.diag_sym()) + && let Ok(map_func) = MapFunc::try_from(map_expr) + { + emit_lint(cx, parent_expr.span, op, flavor, bool_cst, map_func, recv); + return; } } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index ee0cd95a5021..659a704a1ecb 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3933,7 +3933,8 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for usage of `option.map(f).unwrap_or_default()` and `result.map(f).unwrap_or_default()` where f is a function or closure that returns the `bool` type. + /// Checks for usage of `option.map(f).unwrap_or_default()` and `result.map(f).unwrap_or_default()` where `f` is a function or closure that returns the `bool` type. + /// /// Also checks for equality comparisons like `option.map(f) == Some(true)` and `result.map(f) == Ok(true)`. /// /// ### Why is this bad? @@ -5629,7 +5630,7 @@ impl Methods { manual_saturating_arithmetic::check_sub_unwrap_or_default(cx, expr, lhs, rhs); }, Some((sym::map, m_recv, [arg], span, _)) => { - manual_is_variant_and::check(cx, expr, m_recv, arg, span, self.msrv); + manual_is_variant_and::check_map_unwrap_or_default(cx, expr, m_recv, arg, span, self.msrv); }, Some((then_method @ (sym::then | sym::then_some), t_recv, [t_arg], _, _)) => { obfuscated_if_else::check( diff --git a/clippy_lints/src/methods/unnecessary_map_or.rs b/clippy_lints/src/methods/unnecessary_map_or.rs index 7377b43571ce..21e112360aaf 100644 --- a/clippy_lints/src/methods/unnecessary_map_or.rs +++ b/clippy_lints/src/methods/unnecessary_map_or.rs @@ -8,7 +8,7 @@ use clippy_utils::sugg::{Sugg, make_binop}; use clippy_utils::ty::{implements_trait, is_copy}; use clippy_utils::visitors::is_local_used; use clippy_utils::{get_parent_expr, is_from_proc_macro}; -use rustc_ast::LitKind::Bool; +use rustc_ast::LitKind; use rustc_errors::Applicability; use rustc_hir::{BinOpKind, Expr, ExprKind, PatKind}; use rustc_lint::LateContext; @@ -48,13 +48,14 @@ pub(super) fn check<'a>( let ExprKind::Lit(def_kind) = def.kind else { return; }; - - let recv_ty = cx.typeck_results().expr_ty_adjusted(recv); - - let Bool(def_bool) = def_kind.node else { + let LitKind::Bool(def_bool) = def_kind.node else { return; }; + let typeck = cx.typeck_results(); + + let recv_ty = typeck.expr_ty_adjusted(recv); + let variant = match recv_ty.opt_diag_name(cx) { Some(sym::Option) => Variant::Some, Some(sym::Result) => Variant::Ok, @@ -63,12 +64,12 @@ pub(super) fn check<'a>( let ext_def_span = def.span.until(map.span); - let (sugg, method, applicability) = if cx.typeck_results().expr_adjustments(recv).is_empty() + let (sugg, method, applicability): (_, Cow<'_, _>, _) = if 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 Some(param) = closure_body.params.first() + && 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) @@ -78,14 +79,13 @@ pub(super) fn check<'a>( && ((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 its both then that's a strange edge case and + // 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 typeck_results = cx.typeck_results() - && let l_ty = typeck_results.expr_ty(l) - && l_ty == typeck_results.expr_ty(r) - && let Some(partial_eq) = cx.tcx.get_diagnostic_item(sym::PartialEq) + && 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) { @@ -126,18 +126,18 @@ pub(super) fn check<'a>( } .into_string(); - (vec![(expr.span, sugg)], "a standard comparison", app) + (vec![(expr.span, sugg)], "a standard comparison".into(), app) } else if !def_bool && msrv.meets(cx, msrvs::OPTION_RESULT_IS_VARIANT_AND) { let suggested_name = variant.method_name(); ( - vec![(method_span, suggested_name.into()), (ext_def_span, String::default())], - suggested_name, + vec![(method_span, suggested_name.into()), (ext_def_span, String::new())], + format!("`{suggested_name}`").into(), Applicability::MachineApplicable, ) } else if def_bool && matches!(variant, Variant::Some) && msrv.meets(cx, msrvs::IS_NONE_OR) { ( - vec![(method_span, "is_none_or".into()), (ext_def_span, String::default())], - "is_none_or", + vec![(method_span, "is_none_or".into()), (ext_def_span, String::new())], + "`is_none_or`".into(), Applicability::MachineApplicable, ) } else { diff --git a/tests/ui/unnecessary_map_or.fixed b/tests/ui/unnecessary_map_or.fixed index 10552431d65d..52c114339292 100644 --- a/tests/ui/unnecessary_map_or.fixed +++ b/tests/ui/unnecessary_map_or.fixed @@ -70,7 +70,7 @@ fn main() { let _ = r.is_ok_and(|x| x == 7); //~^ unnecessary_map_or - // lint constructs that are not comparaisons as well + // lint constructs that are not comparisons as well let func = |_x| true; let r: Result = Ok(3); let _ = r.is_ok_and(func); diff --git a/tests/ui/unnecessary_map_or.rs b/tests/ui/unnecessary_map_or.rs index 4b406ec2998b..dd2e1a569469 100644 --- a/tests/ui/unnecessary_map_or.rs +++ b/tests/ui/unnecessary_map_or.rs @@ -74,7 +74,7 @@ fn main() { let _ = r.map_or(false, |x| x == 7); //~^ unnecessary_map_or - // lint constructs that are not comparaisons as well + // lint constructs that are not comparisons as well let func = |_x| true; let r: Result = Ok(3); let _ = r.map_or(false, func); diff --git a/tests/ui/unnecessary_map_or.stderr b/tests/ui/unnecessary_map_or.stderr index b8a22346c378..d11e7179f921 100644 --- a/tests/ui/unnecessary_map_or.stderr +++ b/tests/ui/unnecessary_map_or.stderr @@ -56,7 +56,7 @@ LL | | 6 >= 5 LL | | }); | |______^ | -help: use is_some_and instead +help: use `is_some_and` instead | LL - let _ = Some(5).map_or(false, |n| { LL + let _ = Some(5).is_some_and(|n| { @@ -68,7 +68,7 @@ error: this `map_or` can be simplified LL | let _ = Some(vec![5]).map_or(false, |n| n == [5]); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_some_and instead +help: use `is_some_and` instead | LL - let _ = Some(vec![5]).map_or(false, |n| n == [5]); LL + let _ = Some(vec![5]).is_some_and(|n| n == [5]); @@ -80,7 +80,7 @@ error: this `map_or` can be simplified LL | let _ = Some(vec![1]).map_or(false, |n| vec![2] == n); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_some_and instead +help: use `is_some_and` instead | LL - let _ = Some(vec![1]).map_or(false, |n| vec![2] == n); LL + let _ = Some(vec![1]).is_some_and(|n| vec![2] == n); @@ -92,7 +92,7 @@ error: this `map_or` can be simplified LL | let _ = Some(5).map_or(false, |n| n == n); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_some_and instead +help: use `is_some_and` instead | LL - let _ = Some(5).map_or(false, |n| n == n); LL + let _ = Some(5).is_some_and(|n| n == n); @@ -104,7 +104,7 @@ error: this `map_or` can be simplified LL | let _ = Some(5).map_or(false, |n| n == if 2 > 1 { n } else { 0 }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_some_and instead +help: use `is_some_and` instead | LL - let _ = Some(5).map_or(false, |n| n == if 2 > 1 { n } else { 0 }); LL + let _ = Some(5).is_some_and(|n| n == if 2 > 1 { n } else { 0 }); @@ -116,7 +116,7 @@ error: this `map_or` can be simplified LL | let _ = Ok::, i32>(vec![5]).map_or(false, |n| n == [5]); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_ok_and instead +help: use `is_ok_and` instead | LL - let _ = Ok::, i32>(vec![5]).map_or(false, |n| n == [5]); LL + let _ = Ok::, i32>(vec![5]).is_ok_and(|n| n == [5]); @@ -152,7 +152,7 @@ error: this `map_or` can be simplified LL | let _ = Some(5).map_or(true, |n| n == 5); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_none_or instead +help: use `is_none_or` instead | LL - let _ = Some(5).map_or(true, |n| n == 5); LL + let _ = Some(5).is_none_or(|n| n == 5); @@ -164,7 +164,7 @@ error: this `map_or` can be simplified LL | let _ = Some(5).map_or(true, |n| 5 == n); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_none_or instead +help: use `is_none_or` instead | LL - let _ = Some(5).map_or(true, |n| 5 == n); LL + let _ = Some(5).is_none_or(|n| 5 == n); @@ -212,7 +212,7 @@ error: this `map_or` can be simplified LL | let _ = r.map_or(false, |x| x == 7); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_ok_and instead +help: use `is_ok_and` instead | LL - let _ = r.map_or(false, |x| x == 7); LL + let _ = r.is_ok_and(|x| x == 7); @@ -224,7 +224,7 @@ error: this `map_or` can be simplified LL | let _ = r.map_or(false, func); | ^^^^^^^^^^^^^^^^^^^^^ | -help: use is_ok_and instead +help: use `is_ok_and` instead | LL - let _ = r.map_or(false, func); LL + let _ = r.is_ok_and(func); @@ -236,7 +236,7 @@ error: this `map_or` can be simplified LL | let _ = Some(5).map_or(false, func); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_some_and instead +help: use `is_some_and` instead | LL - let _ = Some(5).map_or(false, func); LL + let _ = Some(5).is_some_and(func); @@ -248,7 +248,7 @@ error: this `map_or` can be simplified LL | let _ = Some(5).map_or(true, func); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_none_or instead +help: use `is_none_or` instead | LL - let _ = Some(5).map_or(true, func); LL + let _ = Some(5).is_none_or(func); @@ -272,7 +272,7 @@ error: this `map_or` can be simplified LL | o.map_or(true, |n| n > 5) || (o as &Option).map_or(true, |n| n < 5) | ^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_none_or instead +help: use `is_none_or` instead | LL - o.map_or(true, |n| n > 5) || (o as &Option).map_or(true, |n| n < 5) LL + o.is_none_or(|n| n > 5) || (o as &Option).map_or(true, |n| n < 5) @@ -284,7 +284,7 @@ error: this `map_or` can be simplified LL | o.map_or(true, |n| n > 5) || (o as &Option).map_or(true, |n| n < 5) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_none_or instead +help: use `is_none_or` instead | LL - o.map_or(true, |n| n > 5) || (o as &Option).map_or(true, |n| n < 5) LL + o.map_or(true, |n| n > 5) || (o as &Option).is_none_or(|n| n < 5) @@ -296,7 +296,7 @@ error: this `map_or` can be simplified LL | o.map_or(true, |n| n > 5) | ^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_none_or instead +help: use `is_none_or` instead | LL - o.map_or(true, |n| n > 5) LL + o.is_none_or(|n| n > 5) @@ -308,7 +308,7 @@ error: this `map_or` can be simplified LL | let x = a.map_or(false, |a| a == *s); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_some_and instead +help: use `is_some_and` instead | LL - let x = a.map_or(false, |a| a == *s); LL + let x = a.is_some_and(|a| a == *s); @@ -320,7 +320,7 @@ error: this `map_or` can be simplified LL | let y = b.map_or(true, |b| b == *s); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_none_or instead +help: use `is_none_or` instead | LL - let y = b.map_or(true, |b| b == *s); LL + let y = b.is_none_or(|b| b == *s); @@ -356,7 +356,7 @@ error: this `map_or` can be simplified LL | _ = s.lock().unwrap().map_or(false, |s| s == "foo"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_some_and instead +help: use `is_some_and` instead | LL - _ = s.lock().unwrap().map_or(false, |s| s == "foo"); LL + _ = s.lock().unwrap().is_some_and(|s| s == "foo"); @@ -368,7 +368,7 @@ error: this `map_or` can be simplified LL | _ = s.map_or(false, |s| s == "foo"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -help: use is_some_and instead +help: use `is_some_and` instead | LL - _ = s.map_or(false, |s| s == "foo"); LL + _ = s.is_some_and(|s| s == "foo");