finalise implementation of match_or lint case

Author: Jacherr

clippy_lints/src/methods/unnecessary_pattern_matching.rs

@@ -3,6 +3,7 @@ use clippy_utils::eager_or_lazy::switch_to_eager_eval;
 use clippy_utils::path_to_local_id;
 use clippy_utils::source::snippet;
 use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::visitors::is_local_used;
 use rustc_ast::LitKind::Bool;
 use rustc_errors::Applicability;
 use rustc_hir::{BinOpKind, Expr, ExprKind, PatKind};
@@ -14,20 +15,13 @@ use super::UNNECESSARY_PATTERN_MATCHING;
 // Only checking map_or for now
 pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def: &Expr<'_>, map: &Expr<'_>) {
     if let ExprKind::Lit(def_kind) = def.kind
-    // only works on Result and Option
     && let recv_ty = cx.typeck_results().expr_ty(recv)
     && (is_type_diagnostic_item(cx, recv_ty, sym::Option)
         || is_type_diagnostic_item(cx, recv_ty, sym::Result))
-    // check we are dealing with boolean map_or
     && let Bool(def_bool) = def_kind.node
     && 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()
-    // switching from closure to direct comparison changes the comparison
-    // from lazy (when map_or evaluates to first param) to eager, so we force eager here
-    // this stops the lint running whenever the closure has a potentially expensive
-    // computation inside of it
-    && switch_to_eager_eval(cx, closure_body_value)
     && let ExprKind::Binary(op, l, r) = closure_body_value.kind
     && let Some(param) = closure_body.params.first()
     && let PatKind::Binding(_, hir_id, _, _) = param.pat.kind
@@ -45,17 +39,21 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def:
         } else {
             "Result"
         };
-
         let comparator = if BinOpKind::Eq == op.node { "==" } else { "!=" };
-
         // we already checked either l or r is the local id earlier
         let non_binding_location = if path_to_local_id(l, hir_id) { r } else { l };
-
+        // we switch to eager in the event that the closure has an
+        // expensive computation inside of it
+        if is_local_used(cx, non_binding_location, hir_id) 
+            || !switch_to_eager_eval(cx, non_binding_location)
+            || l.kind {
+            return;
+        };
         span_lint_and_sugg(
             cx,
             UNNECESSARY_PATTERN_MATCHING,
             expr.span,
-            "`map_or` here is redundant, use standard comparison instead",
+            "`map_or` is redundant, use standard comparison instead",
             "try",
             format!(
                 "{} {} {}({})",

tests/ui/unnecessary_pattern_matching.fixed

@@ -0,0 +1,21 @@
+#![warn(clippy::unnecessary_pattern_matching)]
+#![allow(clippy::no_effect)]
+#![allow(clippy::eq_op)]
+
+fn main() {
+    // should trigger
+    let _ = Some(5) == Some(5);
+    let _ = Some(5) == Some(5);
+    let _ = Some(5) != Some(5);
+    let _ = Some(5) != Some(5);
+    let _ = Some(vec![5]) == Some([5]);
+
+    // shouldnt trigger
+    let _ = Some(5).map_or(true, |n| n == 5);
+    let _ = Some(5).map_or(true, |n| 5 == n);
+    let _ = Some(5).map_or(false, |n| n != 5);
+    let _ = Some(5).map_or(false, |n| 5 != n);
+    let _ = Some(5).map_or(false, |n| n == n);
+    let _ = Some(vec![1]).map_or(false, |n| vec![2] == n);
+    let _ = Some(5).map_or(false, |n| n == if 2 > 1 { n } else { 0 });
+}

tests/ui/unnecessary_pattern_matching.rs

@@ -1,5 +1,21 @@
 #![warn(clippy::unnecessary_pattern_matching)]
+#![allow(clippy::no_effect)]
+#![allow(clippy::eq_op)]
 
 fn main() {
-    // test code goes here
+    // should trigger
+    let _ = Some(5).map_or(false, |n| n == 5);
+    let _ = Some(5).map_or(false, |n| 5 == n);
+    let _ = Some(5).map_or(true, |n| n != 5);
+    let _ = Some(5).map_or(true, |n| 5 != n);
+    let _ = Some(vec![5]).map_or(false, |n| n == [5]);
+
+    // shouldnt trigger
+    let _ = Some(5).map_or(true, |n| n == 5);
+    let _ = Some(5).map_or(true, |n| 5 == n);
+    let _ = Some(5).map_or(false, |n| n != 5);
+    let _ = Some(5).map_or(false, |n| 5 != n);
+    let _ = Some(5).map_or(false, |n| n == n);
+    let _ = Some(vec![1]).map_or(false, |n| vec![2] == n);
+    let _ = Some(5).map_or(false, |n| n == if 2 > 1 { n } else { 0 });
 }

tests/ui/unnecessary_pattern_matching.stderr

@@ -0,0 +1,35 @@
+error: `map_or` is redundant, use standard comparison instead
+  --> $DIR/unnecessary_pattern_matching.rs:7:13
+   |
+LL |     let _ = Some(5).map_or(false, |n| n == 5);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(5) == Some(5)`
+   |
+   = note: `-D clippy::unnecessary-pattern-matching` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::unnecessary_pattern_matching)]`
+
+error: `map_or` is redundant, use standard comparison instead
+  --> $DIR/unnecessary_pattern_matching.rs:8:13
+   |
+LL |     let _ = Some(5).map_or(false, |n| 5 == n);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(5) == Some(5)`
+
+error: `map_or` is redundant, use standard comparison instead
+  --> $DIR/unnecessary_pattern_matching.rs:9:13
+   |
+LL |     let _ = Some(5).map_or(true, |n| n != 5);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(5) != Some(5)`
+
+error: `map_or` is redundant, use standard comparison instead
+  --> $DIR/unnecessary_pattern_matching.rs:10:13
+   |
+LL |     let _ = Some(5).map_or(true, |n| 5 != n);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(5) != Some(5)`
+
+error: `map_or` is redundant, use standard comparison instead
+  --> $DIR/unnecessary_pattern_matching.rs:11:13
+   |
+LL |     let _ = Some(vec![5]).map_or(false, |n| n == [5]);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(vec![5]) == Some([5])`
+
+error: aborting due to 5 previous errors
+