From 0ff495c7fa5c0503017c88464c735b1aebd0de99 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Tue, 14 Jan 2025 00:21:16 +0100 Subject: [PATCH] Use clearer multipart suggestions for `unnecessary_map_or` lint A multipart suggestion will be used whenever the method call can be replaced by another one with the first argument removed. It helps place the new method call in context, especially when it is part of a larger expression. --- clippy_lints/src/methods/mod.rs | 2 +- .../src/methods/unnecessary_map_or.rs | 35 +++---- tests/ui/unnecessary_map_or.stderr | 94 ++++++++++++++++--- 3 files changed, 94 insertions(+), 37 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index f66466896e5b..55c1e55a3001 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -5015,7 +5015,7 @@ impl Methods { option_map_or_none::check(cx, expr, recv, def, map); manual_ok_or::check(cx, expr, recv, def, map); option_map_or_err_ok::check(cx, expr, recv, def, map); - unnecessary_map_or::check(cx, expr, recv, def, map, &self.msrv); + unnecessary_map_or::check(cx, expr, recv, def, map, span, &self.msrv); }, ("map_or_else", [def, map]) => { result_map_or_else_none::check(cx, expr, recv, def, map); diff --git a/clippy_lints/src/methods/unnecessary_map_or.rs b/clippy_lints/src/methods/unnecessary_map_or.rs index b7dbebe60a42..5fd00c35db7c 100644 --- a/clippy_lints/src/methods/unnecessary_map_or.rs +++ b/clippy_lints/src/methods/unnecessary_map_or.rs @@ -1,9 +1,8 @@ use std::borrow::Cow; -use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::eager_or_lazy::switch_to_eager_eval; use clippy_utils::msrvs::{self, Msrv}; -use clippy_utils::source::snippet_opt; use clippy_utils::sugg::{Sugg, make_binop}; use clippy_utils::ty::{get_type_diagnostic_name, implements_trait}; use clippy_utils::visitors::is_local_used; @@ -12,7 +11,7 @@ use rustc_ast::LitKind::Bool; use rustc_errors::Applicability; use rustc_hir::{BinOpKind, Expr, ExprKind, PatKind}; use rustc_lint::LateContext; -use rustc_span::sym; +use rustc_span::{Span, sym}; use super::UNNECESSARY_MAP_OR; @@ -42,6 +41,7 @@ pub(super) fn check<'a>( recv: &Expr<'_>, def: &Expr<'_>, map: &Expr<'_>, + method_span: Span, msrv: &Msrv, ) { let ExprKind::Lit(def_kind) = def.kind else { @@ -60,6 +60,8 @@ pub(super) fn check<'a>( Some(_) | None => return, }; + let ext_def_span = def.span.with_hi(map.span.lo()); + let (sugg, method, applicability) = if 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() @@ -114,26 +116,17 @@ pub(super) fn check<'a>( } .into_string(); - (sugg, "a standard comparison", app) - } else if !def_bool - && msrv.meets(msrvs::OPTION_RESULT_IS_VARIANT_AND) - && let Some(recv_callsite) = snippet_opt(cx, recv.span.source_callsite()) - && let Some(span_callsite) = snippet_opt(cx, map.span.source_callsite()) - { + (vec![(expr.span, sugg)], "a standard comparison", app) + } else if !def_bool && msrv.meets(msrvs::OPTION_RESULT_IS_VARIANT_AND) { let suggested_name = variant.method_name(); ( - format!("{recv_callsite}.{suggested_name}({span_callsite})",), + vec![(method_span, suggested_name.into()), (ext_def_span, String::default())], suggested_name, Applicability::MachineApplicable, ) - } else if def_bool - && matches!(variant, Variant::Some) - && msrv.meets(msrvs::IS_NONE_OR) - && let Some(recv_callsite) = snippet_opt(cx, recv.span.source_callsite()) - && let Some(span_callsite) = snippet_opt(cx, map.span.source_callsite()) - { + } else if def_bool && matches!(variant, Variant::Some) && msrv.meets(msrvs::IS_NONE_OR) { ( - format!("{recv_callsite}.is_none_or({span_callsite})"), + vec![(method_span, "is_none_or".into()), (ext_def_span, String::default())], "is_none_or", Applicability::MachineApplicable, ) @@ -145,13 +138,13 @@ pub(super) fn check<'a>( return; } - span_lint_and_sugg( + span_lint_and_then( cx, UNNECESSARY_MAP_OR, expr.span, "this `map_or` can be simplified", - format!("use {method} instead"), - sugg, - applicability, + |diag| { + diag.multipart_suggestion(format!("use {method} instead"), sugg, applicability); + }, ); } diff --git a/tests/ui/unnecessary_map_or.stderr b/tests/ui/unnecessary_map_or.stderr index 2b78996d5f3e..01793791d23d 100644 --- a/tests/ui/unnecessary_map_or.stderr +++ b/tests/ui/unnecessary_map_or.stderr @@ -35,41 +35,69 @@ LL | | }); | help: use is_some_and instead | -LL ~ let _ = Some(5).is_some_and(|n| { -LL + let _ = n; -LL + 6 >= 5 -LL ~ }); +LL - let _ = Some(5).map_or(false, |n| { +LL + let _ = Some(5).is_some_and(|n| { | error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:23:13 | LL | let _ = Some(vec![5]).map_or(false, |n| n == [5]); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_some_and instead: `Some(vec![5]).is_some_and(|n| n == [5])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +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]); + | error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:24:13 | LL | let _ = Some(vec![1]).map_or(false, |n| vec![2] == n); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_some_and instead: `Some(vec![1]).is_some_and(|n| vec![2] == n)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +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); + | error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:25:13 | LL | let _ = Some(5).map_or(false, |n| n == n); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_some_and instead: `Some(5).is_some_and(|n| n == n)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +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); + | error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:26:13 | LL | let _ = Some(5).map_or(false, |n| n == if 2 > 1 { n } else { 0 }); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_some_and instead: `Some(5).is_some_and(|n| n == if 2 > 1 { n } else { 0 })` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +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 }); + | error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:27:13 | LL | let _ = Ok::, i32>(vec![5]).map_or(false, |n| n == [5]); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_ok_and instead: `Ok::, i32>(vec![5]).is_ok_and(|n| n == [5])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +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]); + | error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:28:13 @@ -87,13 +115,25 @@ error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:30:13 | LL | let _ = Some(5).map_or(true, |n| n == 5); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_none_or instead: `Some(5).is_none_or(|n| n == 5)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +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); + | error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:31:13 | LL | let _ = Some(5).map_or(true, |n| 5 == n); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_none_or instead: `Some(5).is_none_or(|n| 5 == n)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +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); + | error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:32:14 @@ -117,25 +157,49 @@ error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:58:13 | LL | let _ = r.map_or(false, |x| x == 7); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_ok_and instead: `r.is_ok_and(|x| x == 7)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use is_ok_and instead + | +LL - let _ = r.map_or(false, |x| x == 7); +LL + let _ = r.is_ok_and(|x| x == 7); + | error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:63:13 | LL | let _ = r.map_or(false, func); - | ^^^^^^^^^^^^^^^^^^^^^ help: use is_ok_and instead: `r.is_ok_and(func)` + | ^^^^^^^^^^^^^^^^^^^^^ + | +help: use is_ok_and instead + | +LL - let _ = r.map_or(false, func); +LL + let _ = r.is_ok_and(func); + | error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:64:13 | LL | let _ = Some(5).map_or(false, func); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_some_and instead: `Some(5).is_some_and(func)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use is_some_and instead + | +LL - let _ = Some(5).map_or(false, func); +LL + let _ = Some(5).is_some_and(func); + | error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:65:13 | LL | let _ = Some(5).map_or(true, func); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_none_or instead: `Some(5).is_none_or(func)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use is_none_or instead + | +LL - let _ = Some(5).map_or(true, func); +LL + let _ = Some(5).is_none_or(func); + | error: this `map_or` can be simplified --> tests/ui/unnecessary_map_or.rs:70:13