make match_like_matches_macro only apply to matches with a wildcard

rust-lang · Jul 8, 2020 · 37d75da · 37d75da
1 parent 1740dda
commit 37d75da
Show file tree

Hide file tree

Showing 8 changed files with 77 additions and 54 deletions.
diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs
@@ -237,7 +237,6 @@ fn is_commutative(op: hir::BinOpKind) -> bool {
     use rustc_hir::BinOpKind::{
         Add, And, BitAnd, BitOr, BitXor, Div, Eq, Ge, Gt, Le, Lt, Mul, Ne, Or, Rem, Shl, Shr, Sub,
     };
-    #[allow(clippy::match_like_matches_macro)]
     match op {
         Add | Mul | And | Or | BitXor | BitAnd | BitOr | Eq | Ne => true,
         Sub | Div | Rem | Shl | Shr | Lt | Le | Ge | Gt => false,

diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs
@@ -446,11 +446,12 @@ declare_clippy_lint! {
 }
 
 declare_clippy_lint! {
-    /// **What it does:** Checks for `match` expressions producing a `bool` that could be written using `matches!`
+    /// **What it does:** Checks for `match`  or `if let` expressions producing a
+    /// `bool` that could be written using `matches!`
     ///
     /// **Why is this bad?** Readability and needless complexity.
     ///
-    /// **Known problems:** This can turn an intentionally exhaustive match into a non-exhaustive one.
+    /// **Known problems:** None
     ///
     /// **Example:**
     /// ```rust
@@ -462,8 +463,14 @@ declare_clippy_lint! {
     ///     _ => false,
     /// };
     ///
+    /// let a = if let Some(0) = x {
+    ///     true
+    /// } else {
+    ///     false
+    /// };
+    ///
     /// // Good
-    /// let a = matches!(x, Some(5));
+    /// let a = matches!(x, Some(0));
     /// ```
     pub MATCH_LIKE_MATCHES_MACRO,
     style,
@@ -499,9 +506,8 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
             return;
         }
 
-        if !redundant_pattern_match::check(cx, expr) {
-            check_match_like_matches(cx, expr);
-        }
+        redundant_pattern_match::check(cx, expr);
+        check_match_like_matches(cx, expr);
 
         if let ExprKind::Match(ref ex, ref arms, MatchSource::Normal) = expr.kind {
             check_single_match(cx, ex, arms, expr);
@@ -1068,6 +1074,7 @@ fn find_matches_sugg(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr
     if_chain! {
         if arms.len() == 2;
         if cx.tables().expr_ty(expr).is_bool();
+        if is_wild(&arms[1].pat);
         if let Some(first) = find_bool_lit(&arms[0].body.kind, desugared);
         if let Some(second) = find_bool_lit(&arms[1].body.kind, desugared);
         if first != second;
@@ -1437,16 +1444,14 @@ mod redundant_pattern_match {
     use rustc_mir::const_eval::is_const_fn;
     use rustc_span::source_map::Symbol;
 
-    pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
+    pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         if let ExprKind::Match(op, arms, ref match_source) = &expr.kind {
             match match_source {
                 MatchSource::Normal => find_sugg_for_match(cx, expr, op, arms),
                 MatchSource::IfLetDesugar { .. } => find_sugg_for_if_let(cx, expr, op, arms, "if"),
                 MatchSource::WhileLetDesugar => find_sugg_for_if_let(cx, expr, op, arms, "while"),
-                _ => false,
+                _ => {},
             }
-        } else {
-            false
         }
     }
 
@@ -1456,7 +1461,7 @@ mod redundant_pattern_match {
         op: &Expr<'_>,
         arms: &[Arm<'_>],
         keyword: &'static str,
-    ) -> bool {
+    ) {
         fn find_suggestion(cx: &LateContext<'_>, hir_id: HirId, path: &QPath<'_>) -> Option<&'static str> {
             if match_qpath(path, &paths::RESULT_OK) && can_suggest(cx, hir_id, sym!(result_type), "is_ok") {
                 return Some("is_ok()");
@@ -1487,7 +1492,7 @@ mod redundant_pattern_match {
         };
         let good_method = match good_method {
             Some(method) => method,
-            None => return false,
+            None => return,
         };
 
         // check that `while_let_on_iterator` lint does not trigger
@@ -1497,7 +1502,7 @@ mod redundant_pattern_match {
             if method_path.ident.name == sym!(next);
             if match_trait_method(cx, op, &paths::ITERATOR);
             then {
-                return false;
+                return;
             }
         }
 
@@ -1526,15 +1531,9 @@ mod redundant_pattern_match {
                 );
             },
         );
-        true
     }
 
-    fn find_sugg_for_match<'tcx>(
-        cx: &LateContext<'tcx>,
-        expr: &'tcx Expr<'_>,
-        op: &Expr<'_>,
-        arms: &[Arm<'_>],
-    ) -> bool {
+    fn find_sugg_for_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, op: &Expr<'_>, arms: &[Arm<'_>]) {
         if arms.len() == 2 {
             let node_pair = (&arms[0].pat.kind, &arms[1].pat.kind);
 
@@ -1599,10 +1598,8 @@ mod redundant_pattern_match {
                         );
                     },
                 );
-                return true;
             }
         }
-        false
     }
 
     #[allow(clippy::too_many_arguments)]

diff --git a/tests/ui/match_expr_like_matches_macro.fixed b/tests/ui/match_expr_like_matches_macro.fixed
@@ -1,32 +1,36 @@
 // run-rustfix
 
 #![warn(clippy::match_like_matches_macro)]
+#![allow(unreachable_patterns)]
 
 fn main() {
     let x = Some(5);
 
     // Lint
     let _y = matches!(x, Some(0));
 
+    // Lint
+    let _w = matches!(x, Some(_));
+
     // Turn into is_none
     let _z = x.is_none();
 
     // Lint
-    let _z = !matches!(x, Some(r) if r == 0);
+    let _zz = !matches!(x, Some(r) if r == 0);
 
     // Lint
-    let _zz = matches!(x, Some(5));
+    let _zzz = matches!(x, Some(5));
 
     // No lint
     let _a = match x {
         Some(_) => false,
-        None => false,
+        _ => false,
     };
 
     // No lint
-    let _a = match x {
+    let _ab = match x {
         Some(0) => false,
-        Some(_) => true,
+        _ => true,
         None => false,
     };
 }
diff --git a/tests/ui/match_expr_like_matches_macro.rs b/tests/ui/match_expr_like_matches_macro.rs
@@ -1,6 +1,7 @@
 // run-rustfix
 
 #![warn(clippy::match_like_matches_macro)]
+#![allow(unreachable_patterns)]
 
 fn main() {
     let x = Some(5);
@@ -11,31 +12,37 @@ fn main() {
         _ => false,
     };
 
+    // Lint
+    let _w = match x {
+        Some(_) => true,
+        _ => false,
+    };
+
     // Turn into is_none
     let _z = match x {
         Some(_) => false,
         None => true,
     };
 
     // Lint
-    let _z = match x {
+    let _zz = match x {
         Some(r) if r == 0 => false,
         _ => true,
     };
 
     // Lint
-    let _zz = if let Some(5) = x { true } else { false };
+    let _zzz = if let Some(5) = x { true } else { false };
 
     // No lint
     let _a = match x {
         Some(_) => false,
-        None => false,
+        _ => false,
     };
 
     // No lint
-    let _a = match x {
+    let _ab = match x {
         Some(0) => false,
-        Some(_) => true,
+        _ => true,
         None => false,
     };
 }
diff --git a/tests/ui/match_expr_like_matches_macro.stderr b/tests/ui/match_expr_like_matches_macro.stderr
@@ -1,5 +1,5 @@
 error: match expression looks like `matches!` macro
-  --> $DIR/match_expr_like_matches_macro.rs:9:14
+  --> $DIR/match_expr_like_matches_macro.rs:10:14
    |
 LL |       let _y = match x {
    |  ______________^
@@ -10,8 +10,18 @@ LL | |     };
    |
    = note: `-D clippy::match-like-matches-macro` implied by `-D warnings`
 
+error: match expression looks like `matches!` macro
+  --> $DIR/match_expr_like_matches_macro.rs:16:14
+   |
+LL |       let _w = match x {
+   |  ______________^
+LL | |         Some(_) => true,
+LL | |         _ => false,
+LL | |     };
+   | |_____^ help: try this: `matches!(x, Some(_))`
+
 error: redundant pattern matching, consider using `is_none()`
-  --> $DIR/match_expr_like_matches_macro.rs:15:14
+  --> $DIR/match_expr_like_matches_macro.rs:22:14
    |
 LL |       let _z = match x {
    |  ______________^
@@ -23,20 +33,20 @@ LL | |     };
    = note: `-D clippy::redundant-pattern-matching` implied by `-D warnings`
 
 error: match expression looks like `matches!` macro
-  --> $DIR/match_expr_like_matches_macro.rs:21:14
+  --> $DIR/match_expr_like_matches_macro.rs:28:15
    |
-LL |       let _z = match x {
-   |  ______________^
+LL |       let _zz = match x {
+   |  _______________^
 LL | |         Some(r) if r == 0 => false,
 LL | |         _ => true,
 LL | |     };
    | |_____^ help: try this: `!matches!(x, Some(r) if r == 0)`
 
 error: if let .. else expression looks like `matches!` macro
-  --> $DIR/match_expr_like_matches_macro.rs:27:15
+  --> $DIR/match_expr_like_matches_macro.rs:34:16
    |
-LL |     let _zz = if let Some(5) = x { true } else { false };
-   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `matches!(x, Some(5))`
+LL |     let _zzz = if let Some(5) = x { true } else { false };
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `matches!(x, Some(5))`
 
-error: aborting due to 4 previous errors
+error: aborting due to 5 previous errors
 
diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed
@@ -23,7 +23,10 @@ pub enum SeemsOption<T> {
 
 impl<T> SeemsOption<T> {
     pub fn is_none(&self) -> bool {
-        matches!(*self, SeemsOption::None)
+        match *self {
+            SeemsOption::None => true,
+            SeemsOption::Some(_) => false,
+        }
     }
 }
 

diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs
@@ -25,7 +25,10 @@ pub enum SeemsOption<T> {
 
 impl<T> SeemsOption<T> {
     pub fn is_none(&self) -> bool {
-        matches!(*self, SeemsOption::None)
+        match *self {
+            SeemsOption::None => true,
+            SeemsOption::Some(_) => false,
+        }
     }
 }
 

diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr
@@ -9,23 +9,23 @@ LL | |     }
    = note: `-D clippy::question-mark` implied by `-D warnings`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:47:9
+  --> $DIR/question_mark.rs:50:9
    |
 LL | /         if (self.opt).is_none() {
 LL | |             return None;
 LL | |         }
    | |_________^ help: replace it with: `(self.opt)?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:51:9
+  --> $DIR/question_mark.rs:54:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None
 LL | |         }
    | |_________^ help: replace it with: `self.opt?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:55:17
+  --> $DIR/question_mark.rs:58:17
    |
 LL |           let _ = if self.opt.is_none() {
    |  _________________^
@@ -36,7 +36,7 @@ LL | |         };
    | |_________^ help: replace it with: `Some(self.opt?)`
 
 error: this if-let-else may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:61:17
+  --> $DIR/question_mark.rs:64:17
    |
 LL |           let _ = if let Some(x) = self.opt {
    |  _________________^
@@ -47,31 +47,31 @@ LL | |         };
    | |_________^ help: replace it with: `self.opt?`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:78:9
+  --> $DIR/question_mark.rs:81:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None;
 LL | |         }
    | |_________^ help: replace it with: `self.opt.as_ref()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:86:9
+  --> $DIR/question_mark.rs:89:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None;
 LL | |         }
    | |_________^ help: replace it with: `self.opt.as_ref()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:94:9
+  --> $DIR/question_mark.rs:97:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None;
 LL | |         }
    | |_________^ help: replace it with: `self.opt.as_ref()?;`
 
 error: this if-let-else may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:101:26
+  --> $DIR/question_mark.rs:104:26
    |
 LL |           let v: &Vec<_> = if let Some(ref v) = self.opt {
    |  __________________________^
@@ -82,7 +82,7 @@ LL | |         };
    | |_________^ help: replace it with: `self.opt.as_ref()?`
 
 error: this if-let-else may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:111:17
+  --> $DIR/question_mark.rs:114:17
    |
 LL |           let v = if let Some(v) = self.opt {
    |  _________________^
@@ -93,7 +93,7 @@ LL | |         };
    | |_________^ help: replace it with: `self.opt?`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:126:5
+  --> $DIR/question_mark.rs:129:5
    |
 LL | /     if f().is_none() {
 LL | |         return None;