Skip to content

Commit

Permalink
Use clearer multipart suggestions for unnecessary_map_or lint
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@samueltardieu
samueltardieu committed Jan 19, 2025
1 parent 6ab6c3c commit 7f37b2a
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 46 deletions.
2 changes: 1 addition & 1 deletion clippy_lints/src/methods/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
35 changes: 14 additions & 21 deletions clippy_lints/src/methods/unnecessary_map_or.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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 {
Expand All @@ -60,6 +60,8 @@ pub(super) fn check<'a>(
Some(_) | None => return,
};

let ext_def_span = def.span.until(map.span);

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()
Expand Down Expand Up @@ -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,
)
Expand All @@ -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_verbose(format!("use {method} instead"), sugg, applicability);
},
);
}
156 changes: 132 additions & 24 deletions tests/ui/unnecessary_map_or.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,25 @@ error: this `map_or` can be simplified
--> tests/ui/unnecessary_map_or.rs:13:13
|
LL | let _ = Some(5).map_or(false, |n| n == 5);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a standard comparison instead: `Some(5) == Some(5)`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::unnecessary-map-or` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_map_or)]`
help: use a standard comparison instead
|
LL | let _ = Some(5) == Some(5);
| ~~~~~~~~~~~~~~~~~~

error: this `map_or` can be simplified
--> tests/ui/unnecessary_map_or.rs:14:13
|
LL | let _ = Some(5).map_or(true, |n| n != 5);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a standard comparison instead: `Some(5) != Some(5)`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use a standard comparison instead
|
LL | let _ = Some(5) != Some(5);
| ~~~~~~~~~~~~~~~~~~

error: this `map_or` can be simplified
--> tests/ui/unnecessary_map_or.rs:15:13
Expand All @@ -21,7 +30,12 @@ LL | let _ = Some(5).map_or(false, |n| {
LL | | let _ = 1;
LL | | n == 5
LL | | });
| |______^ help: use a standard comparison instead: `Some(5) == Some(5)`
| |______^
|
help: use a standard comparison instead
|
LL | let _ = Some(5) == Some(5);
| ~~~~~~~~~~~~~~~~~~

error: this `map_or` can be simplified
--> tests/ui/unnecessary_map_or.rs:19:13
Expand All @@ -35,113 +49,207 @@ 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::<Vec<i32>, i32>(vec![5]).map_or(false, |n| n == [5]);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_ok_and instead: `Ok::<Vec<i32>, i32>(vec![5]).is_ok_and(|n| n == [5])`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use is_ok_and instead
|
LL - let _ = Ok::<Vec<i32>, i32>(vec![5]).map_or(false, |n| n == [5]);
LL + let _ = Ok::<Vec<i32>, i32>(vec![5]).is_ok_and(|n| n == [5]);
|

error: this `map_or` can be simplified
--> tests/ui/unnecessary_map_or.rs:28:13
|
LL | let _ = Ok::<i32, i32>(5).map_or(false, |n| n == 5);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a standard comparison instead: `Ok::<i32, i32>(5) == Ok(5)`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use a standard comparison instead
|
LL | let _ = Ok::<i32, i32>(5) == Ok(5);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~

error: this `map_or` can be simplified
--> tests/ui/unnecessary_map_or.rs:29:13
|
LL | let _ = Some(5).map_or(false, |n| n == 5).then(|| 1);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a standard comparison instead: `(Some(5) == Some(5))`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use a standard comparison instead
|
LL | let _ = (Some(5) == Some(5)).then(|| 1);
| ~~~~~~~~~~~~~~~~~~~~

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
|
LL | let _ = !Some(5).map_or(false, |n| n == 5);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a standard comparison instead: `(Some(5) == Some(5))`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use a standard comparison instead
|
LL | let _ = !(Some(5) == Some(5));
| ~~~~~~~~~~~~~~~~~~~~

error: this `map_or` can be simplified
--> tests/ui/unnecessary_map_or.rs:33:13
|
LL | let _ = Some(5).map_or(false, |n| n == 5) || false;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a standard comparison instead: `(Some(5) == Some(5))`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use a standard comparison instead
|
LL | let _ = (Some(5) == Some(5)) || false;
| ~~~~~~~~~~~~~~~~~~~~

error: this `map_or` can be simplified
--> tests/ui/unnecessary_map_or.rs:34:13
|
LL | let _ = Some(5).map_or(false, |n| n == 5) as usize;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a standard comparison instead: `(Some(5) == Some(5))`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use a standard comparison instead
|
LL | let _ = (Some(5) == Some(5)) as usize;
| ~~~~~~~~~~~~~~~~~~~~

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
|
LL | let _ = r.map_or(false, |x| x == 8);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a standard comparison instead: `r == Ok(8)`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use a standard comparison instead
|
LL | let _ = r == Ok(8);
| ~~~~~~~~~~

error: aborting due to 21 previous errors

0 comments on commit 7f37b2a

Please sign in to comment.