Auto merge of rust-lang#11522 - y21:redundant_guards_pat_lhs, r=giraf…

…fate [`redundant_guards`]: lint if the pattern is on the left side A tiny improvement to the `redundant_guards` lint. There's no associated issue for this, just noticed it while going through the code. Right now it warns on `Some(x) if x == 2` and suggests `Some(2)`, but it didn't do that for `Some(x) if 2 == x` (i.e. when the local is on the right side and the pattern on the left side). changelog: [`redundant_guards`]: also lint if the pattern is on the left side
lengyijun · Sep 20, 2023 · c556695 · c556695
2 parents ddbe110 + 558ae4c
commit c556695
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 12 deletions.
diff --git a/clippy_lints/src/matches/redundant_guards.rs b/clippy_lints/src/matches/redundant_guards.rs
@@ -70,18 +70,23 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
             );
         }
         // `Some(x) if x == Some(2)`
+        // `Some(x) if Some(2) == x`
         else if let Guard::If(if_expr) = guard
             && let ExprKind::Binary(bin_op, local, pat) = if_expr.kind
             && matches!(bin_op.node, BinOpKind::Eq)
-            && expr_can_be_pat(cx, pat)
             // Ensure they have the same type. If they don't, we'd need deref coercion which isn't
             // possible (currently) in a pattern. In some cases, you can use something like
             // `as_deref` or similar but in general, we shouldn't lint this as it'd create an
             // extraordinary amount of FPs.
             //
             // This isn't necessary in the other two checks, as they must be a pattern already.
             && cx.typeck_results().expr_ty(local) == cx.typeck_results().expr_ty(pat)
-            && let Some(binding) = get_pat_binding(cx, local, outer_arm)
+            // Since we want to lint on both `x == Some(2)` and `Some(2) == x`, we might have to "swap"
+            // `local` and `pat`, depending on which side they are.
+            && let Some((binding, pat)) = get_pat_binding(cx, local, outer_arm)
+                .map(|binding| (binding, pat))
+                .or_else(|| get_pat_binding(cx, pat, outer_arm).map(|binding| (binding, local)))
+            && expr_can_be_pat(cx, pat)
         {
             let pat_span = match (pat.kind, binding.byref_ident) {
                 (ExprKind::AddrOf(BorrowKind::Ref, _, expr), Some(_)) => expr.span,

diff --git a/tests/ui/redundant_guards.fixed b/tests/ui/redundant_guards.fixed
@@ -43,6 +43,7 @@ fn main() {
         },
         Some(Some(1)) => ..,
         Some(Some(2)) => ..,
+        Some(Some(2)) => ..,
         // Don't lint, since x is used in the body
         Some(x) if let Some(1) = x => {
             x;
@@ -56,11 +57,13 @@ fn main() {
         Some(x) if matches!(y, 1 if true) => ..,
         Some(x) if let 1 = y => ..,
         Some(x) if y == 2 => ..,
+        Some(x) if 2 == y => ..,
         _ => todo!(),
     };
     let a = A(1);
     match a {
         _ if a.0 == 1 => {},
+        _ if 1 == a.0 => {},
         _ => todo!(),
     }
     let b = B { e: Some(A(0)) };
@@ -119,6 +122,7 @@ fn h(v: Option<u32>) {
 fn f(s: Option<std::ffi::OsString>) {
     match s {
         Some(x) if x == "a" => {},
+        Some(x) if "a" == x => {},
         _ => {},
     }
 }
@@ -140,6 +144,7 @@ static CONST_S: S = S { a: 1 };
 fn g(opt_s: Option<S>) {
     match opt_s {
         Some(x) if x == CONST_S => {},
+        Some(x) if CONST_S == x => {},
         _ => {},
     }
 }
@@ -157,6 +162,7 @@ mod issue11465 {
     fn issue11465() {
         let c = Some(1);
         match c {
+            Some(1) => {},
             Some(1) => {},
             Some(2) => {},
             Some(3) => {},
@@ -166,6 +172,7 @@ mod issue11465 {
         let enum_a = A::Foo([98, 97, 114]);
         match enum_a {
             A::Foo(ref arr) if arr == b"foo" => {},
+            A::Foo(ref arr) if b"foo" == arr => {},
             A::Foo(ref arr) if let b"bar" = arr => {},
             A::Foo(ref arr) if matches!(arr, b"baz") => {},
             _ => {},
@@ -177,6 +184,8 @@ mod issue11465 {
         };
         match struct_b {
             B { ref b, .. } if b == "bar" => {},
+            B { ref b, .. } if "bar" == b => {},
+            B { c: 1, .. } => {},
             B { c: 1, .. } => {},
             B { c: 1, .. } => {},
             B { c: 1, .. } => {},

diff --git a/tests/ui/redundant_guards.rs b/tests/ui/redundant_guards.rs
@@ -43,6 +43,7 @@ fn main() {
         },
         Some(x) if let Some(1) = x => ..,
         Some(x) if x == Some(2) => ..,
+        Some(x) if Some(2) == x => ..,
         // Don't lint, since x is used in the body
         Some(x) if let Some(1) = x => {
             x;
@@ -56,11 +57,13 @@ fn main() {
         Some(x) if matches!(y, 1 if true) => ..,
         Some(x) if let 1 = y => ..,
         Some(x) if y == 2 => ..,
+        Some(x) if 2 == y => ..,
         _ => todo!(),
     };
     let a = A(1);
     match a {
         _ if a.0 == 1 => {},
+        _ if 1 == a.0 => {},
         _ => todo!(),
     }
     let b = B { e: Some(A(0)) };
@@ -119,6 +122,7 @@ fn h(v: Option<u32>) {
 fn f(s: Option<std::ffi::OsString>) {
     match s {
         Some(x) if x == "a" => {},
+        Some(x) if "a" == x => {},
         _ => {},
     }
 }
@@ -140,6 +144,7 @@ static CONST_S: S = S { a: 1 };
 fn g(opt_s: Option<S>) {
     match opt_s {
         Some(x) if x == CONST_S => {},
+        Some(x) if CONST_S == x => {},
         _ => {},
     }
 }
@@ -158,6 +163,7 @@ mod issue11465 {
         let c = Some(1);
         match c {
             Some(ref x) if x == &1 => {},
+            Some(ref x) if &1 == x => {},
             Some(ref x) if let &2 = x => {},
             Some(ref x) if matches!(x, &3) => {},
             _ => {},
@@ -166,6 +172,7 @@ mod issue11465 {
         let enum_a = A::Foo([98, 97, 114]);
         match enum_a {
             A::Foo(ref arr) if arr == b"foo" => {},
+            A::Foo(ref arr) if b"foo" == arr => {},
             A::Foo(ref arr) if let b"bar" = arr => {},
             A::Foo(ref arr) if matches!(arr, b"baz") => {},
             _ => {},
@@ -177,7 +184,9 @@ mod issue11465 {
         };
         match struct_b {
             B { ref b, .. } if b == "bar" => {},
+            B { ref b, .. } if "bar" == b => {},
             B { ref c, .. } if c == &1 => {},
+            B { ref c, .. } if &1 == c => {},
             B { ref c, .. } if let &1 = c => {},
             B { ref c, .. } if matches!(c, &1) => {},
             _ => {},

diff --git a/tests/ui/redundant_guards.stderr b/tests/ui/redundant_guards.stderr
@@ -60,7 +60,19 @@ LL +         Some(Some(2)) => ..,
    |
 
 error: redundant guard
-  --> $DIR/redundant_guards.rs:68:20
+  --> $DIR/redundant_guards.rs:46:20
+   |
+LL |         Some(x) if Some(2) == x => ..,
+   |                    ^^^^^^^^^^^^
+   |
+help: try
+   |
+LL -         Some(x) if Some(2) == x => ..,
+LL +         Some(Some(2)) => ..,
+   |
+
+error: redundant guard
+  --> $DIR/redundant_guards.rs:71:20
    |
 LL |         B { e } if matches!(e, Some(A(2))) => ..,
    |                    ^^^^^^^^^^^^^^^^^^^^^^^
@@ -72,7 +84,7 @@ LL +         B { e: Some(A(2)) } => ..,
    |
 
 error: redundant guard
-  --> $DIR/redundant_guards.rs:105:20
+  --> $DIR/redundant_guards.rs:108:20
    |
 LL |         E::A(y) if y == "not from an or pattern" => {},
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -84,7 +96,7 @@ LL +         E::A("not from an or pattern") => {},
    |
 
 error: redundant guard
-  --> $DIR/redundant_guards.rs:112:14
+  --> $DIR/redundant_guards.rs:115:14
    |
 LL |         x if matches!(x, Some(0)) => ..,
    |              ^^^^^^^^^^^^^^^^^^^^
@@ -96,7 +108,7 @@ LL +         Some(0) => ..,
    |
 
 error: redundant guard
-  --> $DIR/redundant_guards.rs:160:28
+  --> $DIR/redundant_guards.rs:165:28
    |
 LL |             Some(ref x) if x == &1 => {},
    |                            ^^^^^^^
@@ -108,7 +120,19 @@ LL +             Some(1) => {},
    |
 
 error: redundant guard
-  --> $DIR/redundant_guards.rs:161:28
+  --> $DIR/redundant_guards.rs:166:28
+   |
+LL |             Some(ref x) if &1 == x => {},
+   |                            ^^^^^^^
+   |
+help: try
+   |
+LL -             Some(ref x) if &1 == x => {},
+LL +             Some(1) => {},
+   |
+
+error: redundant guard
+  --> $DIR/redundant_guards.rs:167:28
    |
 LL |             Some(ref x) if let &2 = x => {},
    |                            ^^^^^^^^^^
@@ -120,7 +144,7 @@ LL +             Some(2) => {},
    |
 
 error: redundant guard
-  --> $DIR/redundant_guards.rs:162:28
+  --> $DIR/redundant_guards.rs:168:28
    |
 LL |             Some(ref x) if matches!(x, &3) => {},
    |                            ^^^^^^^^^^^^^^^
@@ -132,7 +156,7 @@ LL +             Some(3) => {},
    |
 
 error: redundant guard
-  --> $DIR/redundant_guards.rs:180:32
+  --> $DIR/redundant_guards.rs:188:32
    |
 LL |             B { ref c, .. } if c == &1 => {},
    |                                ^^^^^^^
@@ -144,7 +168,19 @@ LL +             B { c: 1, .. } => {},
    |
 
 error: redundant guard
-  --> $DIR/redundant_guards.rs:181:32
+  --> $DIR/redundant_guards.rs:189:32
+   |
+LL |             B { ref c, .. } if &1 == c => {},
+   |                                ^^^^^^^
+   |
+help: try
+   |
+LL -             B { ref c, .. } if &1 == c => {},
+LL +             B { c: 1, .. } => {},
+   |
+
+error: redundant guard
+  --> $DIR/redundant_guards.rs:190:32
    |
 LL |             B { ref c, .. } if let &1 = c => {},
    |                                ^^^^^^^^^^
@@ -156,7 +192,7 @@ LL +             B { c: 1, .. } => {},
    |
 
 error: redundant guard
-  --> $DIR/redundant_guards.rs:182:32
+  --> $DIR/redundant_guards.rs:191:32
    |
 LL |             B { ref c, .. } if matches!(c, &1) => {},
    |                                ^^^^^^^^^^^^^^^
@@ -167,5 +203,5 @@ LL -             B { ref c, .. } if matches!(c, &1) => {},
 LL +             B { c: 1, .. } => {},
    |
 
-error: aborting due to 14 previous errors
+error: aborting due to 17 previous errors