unnecessary_map_or: lint .map_or(true, …) as well

rust-lang · Nov 7, 2024 · c4cf340 · c4cf340
1 parent f8080d6
commit c4cf340
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 17 deletions.
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -4107,24 +4107,32 @@ declare_clippy_lint! {
     /// ### Why is this bad?
     /// Calls such as `opt.map_or(false, |val| val == 5)` are needlessly long and cumbersome,
     /// and can be reduced to, for example, `opt == Some(5)` assuming `opt` implements `PartialEq`.
+    /// Also, calls such as `opt.map_or(true, |val| val == 5)` can be reduced to
+    /// `opt.is_none_or(|val| val == 5)`.
     /// This lint offers readability and conciseness improvements.
     ///
     /// ### Example
     /// ```no_run
-    /// pub fn a(x: Option<i32>) -> bool {
-    ///     x.map_or(false, |n| n == 5)
+    /// pub fn a(x: Option<i32>) -> (bool, bool) {
+    ///     (
+    ///         x.map_or(false, |n| n == 5),
+    ///         x.map_or(true, |n| n > 5),
+    ///     )
     /// }
     /// ```
     /// Use instead:
     /// ```no_run
-    /// pub fn a(x: Option<i32>) -> bool {
-    ///     x == Some(5)
+    /// pub fn a(x: Option<i32>) -> (bool, bool) {
+    ///     (
+    ///         x == Some(5),
+    ///         x.is_none_or(|n| n > 5),
+    ///     )
     /// }
     /// ```
     #[clippy::version = "1.75.0"]
     pub UNNECESSARY_MAP_OR,
     style,
-    "reduce unnecessary pattern matching for constructs that implement `PartialEq`"
+    "reduce unnecessary calls to `.map_or(bool, …)`"
 }
 
 declare_clippy_lint! {

diff --git a/clippy_lints/src/methods/unnecessary_map_or.rs b/clippy_lints/src/methods/unnecessary_map_or.rs
@@ -60,7 +60,7 @@ pub(super) fn check<'a>(
         Some(_) | None => return,
     };
 
-    let (sugg, method) = if let ExprKind::Closure(map_closure) = map.kind
+    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()
             && let ExprKind::Binary(op, l, r) = closure_body_value.kind
@@ -100,7 +100,7 @@ pub(super) fn check<'a>(
             .maybe_par()
             .into_string();
 
-        (binop, "a standard comparison")
+        (binop, "a standard comparison", Applicability::MaybeIncorrect)
     } else if !def_bool
         && msrv.meets(msrvs::OPTION_RESULT_IS_VARIANT_AND)
         && let Some(recv_callsite) = snippet_opt(cx, recv.span.source_callsite())
@@ -110,6 +110,18 @@ pub(super) fn check<'a>(
         (
             format!("{recv_callsite}.{suggested_name}({span_callsite})",),
             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())
+    {
+        (
+            format!("{recv_callsite}.is_none_or({span_callsite})"),
+            "is_none_or",
+            Applicability::MachineApplicable,
         )
     } else {
         return;
@@ -126,6 +138,6 @@ pub(super) fn check<'a>(
         "this `map_or` is redundant",
         format!("use {method} instead"),
         sugg,
-        Applicability::MaybeIncorrect,
+        applicability,
     );
 }
diff --git a/tests/ui/unnecessary_map_or.fixed b/tests/ui/unnecessary_map_or.fixed
@@ -23,10 +23,9 @@ fn main() {
     let _ = Ok::<Vec<i32>, i32>(vec![5]).is_ok_and(|n| n == [5]);
     let _ = (Ok::<i32, i32>(5) == Ok(5));
     let _ = (Some(5) == Some(5)).then(|| 1);
+    let _ = Some(5).is_none_or(|n| n == 5);
+    let _ = Some(5).is_none_or(|n| 5 == n);
 
-    // shouldnt trigger
-    let _ = Some(5).map_or(true, |n| n == 5);
-    let _ = Some(5).map_or(true, |n| 5 == n);
     macro_rules! x {
         () => {
             Some(1)
@@ -56,15 +55,26 @@ fn main() {
     let r: Result<i32, S> = Ok(3);
     let _ = r.is_ok_and(func);
     let _ = Some(5).is_some_and(func);
+    let _ = Some(5).is_none_or(func);
 
     #[derive(PartialEq)]
     struct S2;
     let r: Result<i32, S2> = Ok(4);
     let _ = (r == Ok(8));
+
+    // do not lint `Result::map_or(true, …)`
+    let r: Result<i32, S2> = Ok(4);
+    let _ = r.map_or(true, |x| x == 8);
 }
 
 #[clippy::msrv = "1.69.0"]
 fn msrv_1_69() {
     // is_some_and added in 1.70.0
     let _ = Some(5).map_or(false, |n| n == if 2 > 1 { n } else { 0 });
 }
+
+#[clippy::msrv = "1.81.0"]
+fn msrv_1_81() {
+    // is_none_or added in 1.82.0
+    let _ = Some(5).map_or(true, |n| n == if 2 > 1 { n } else { 0 });
+}
diff --git a/tests/ui/unnecessary_map_or.rs b/tests/ui/unnecessary_map_or.rs
@@ -26,10 +26,9 @@ fn main() {
     let _ = Ok::<Vec<i32>, i32>(vec![5]).map_or(false, |n| n == [5]);
     let _ = Ok::<i32, i32>(5).map_or(false, |n| n == 5);
     let _ = Some(5).map_or(false, |n| n == 5).then(|| 1);
-
-    // shouldnt trigger
     let _ = Some(5).map_or(true, |n| n == 5);
     let _ = Some(5).map_or(true, |n| 5 == n);
+
     macro_rules! x {
         () => {
             Some(1)
@@ -59,15 +58,26 @@ fn main() {
     let r: Result<i32, S> = Ok(3);
     let _ = r.map_or(false, func);
     let _ = Some(5).map_or(false, func);
+    let _ = Some(5).map_or(true, func);
 
     #[derive(PartialEq)]
     struct S2;
     let r: Result<i32, S2> = Ok(4);
     let _ = r.map_or(false, |x| x == 8);
+
+    // do not lint `Result::map_or(true, …)`
+    let r: Result<i32, S2> = Ok(4);
+    let _ = r.map_or(true, |x| x == 8);
 }
 
 #[clippy::msrv = "1.69.0"]
 fn msrv_1_69() {
     // is_some_and added in 1.70.0
     let _ = Some(5).map_or(false, |n| n == if 2 > 1 { n } else { 0 });
 }
+
+#[clippy::msrv = "1.81.0"]
+fn msrv_1_81() {
+    // is_none_or added in 1.82.0
+    let _ = Some(5).map_or(true, |n| n == if 2 > 1 { n } else { 0 });
+}
diff --git a/tests/ui/unnecessary_map_or.stderr b/tests/ui/unnecessary_map_or.stderr
@@ -84,28 +84,46 @@ LL |     let _ = Some(5).map_or(false, |n| n == 5).then(|| 1);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a standard comparison instead: `(Some(5) == Some(5))`
 
 error: this `map_or` is redundant
-  --> tests/ui/unnecessary_map_or.rs:55:13
+  --> tests/ui/unnecessary_map_or.rs:29: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)`
+
+error: this `map_or` is redundant
+  --> tests/ui/unnecessary_map_or.rs:30: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)`
+
+error: this `map_or` is redundant
+  --> tests/ui/unnecessary_map_or.rs:54:13
    |
 LL |     let _ = r.map_or(false, |x| x == 7);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_ok_and instead: `r.is_ok_and(|x| x == 7)`
 
 error: this `map_or` is redundant
-  --> tests/ui/unnecessary_map_or.rs:60:13
+  --> tests/ui/unnecessary_map_or.rs:59:13
    |
 LL |     let _ = r.map_or(false, func);
    |             ^^^^^^^^^^^^^^^^^^^^^ help: use is_ok_and instead: `r.is_ok_and(func)`
 
 error: this `map_or` is redundant
-  --> tests/ui/unnecessary_map_or.rs:61:13
+  --> tests/ui/unnecessary_map_or.rs:60:13
    |
 LL |     let _ = Some(5).map_or(false, func);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_some_and instead: `Some(5).is_some_and(func)`
 
+error: this `map_or` is redundant
+  --> tests/ui/unnecessary_map_or.rs:61:13
+   |
+LL |     let _ = Some(5).map_or(true, func);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_none_or instead: `Some(5).is_none_or(func)`
+
 error: this `map_or` is redundant
   --> tests/ui/unnecessary_map_or.rs:66:13
    |
 LL |     let _ = r.map_or(false, |x| x == 8);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a standard comparison instead: `(r == Ok(8))`
 
-error: aborting due to 15 previous errors
+error: aborting due to 18 previous errors