Auto merge of #12059 - roife:fix/issue-11223, r=Alexendoo

Fix issue 11223: add check for identical guards in lint `match_same_arms`

fixes #11223

In the current `match_same_arms` implementation, if arms have guards, they are considered different. This commit adds equality checking for guards: arms are now considered equivalent only when they either both have no guards or their guards are identical.

The portion responsible for checking guard equality is refactored to reuse the existing code for checking body equality. This is abstracted into a function called `check_eq_with_pat`. To optimize performance, `check_same_guard` and `check_same_body` here use closures for lazy evaluation, ensuring that the calculation is only performed when `!(backwards_blocking_idxs...)` is true.

changelog: [`match_same_arms`]: Add check for identical guards

4 files changed, 87 insertions, 47 deletions

diff --git a/clippy_lints/src/casts/cast_possible_truncation.rs b/clippy_lints/src/casts/cast_possible_truncation.rs
index f99a51e2b88..fbbb559319e 100644
--- a/clippy_lints/src/casts/cast_possible_truncation.rs
+++ b/clippy_lints/src/casts/cast_possible_truncation.rs
@@ -131,8 +131,7 @@ pub(super) fn check(
 
             let cast_from_ptr_size = def.repr().int.map_or(true, |ty| matches!(ty, IntegerType::Pointer(_),));
             let suffix = match (cast_from_ptr_size, is_isize_or_usize(cast_to)) {
-                (false, false) if from_nbits > to_nbits => "",
-                (true, false) if from_nbits > to_nbits => "",
+                (_, false) if from_nbits > to_nbits => "",
                 (false, true) if from_nbits > 64 => "",
                 (false, true) if from_nbits > 32 => " on targets with 32-bit wide pointers",
                 _ => return,
diff --git a/clippy_lints/src/matches/match_same_arms.rs b/clippy_lints/src/matches/match_same_arms.rs
index d645e6c6c05..ae302f0f075 100644
--- a/clippy_lints/src/matches/match_same_arms.rs
+++ b/clippy_lints/src/matches/match_same_arms.rs
@@ -64,38 +64,50 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) {
         let min_index = usize::min(lindex, rindex);
         let max_index = usize::max(lindex, rindex);
 
-        let mut local_map: HirIdMap<HirId> = HirIdMap::default();
-        let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
-            if let Some(a_id) = path_to_local(a)
-                && let Some(b_id) = path_to_local(b)
-                && let entry = match local_map.entry(a_id) {
-                    HirIdMapEntry::Vacant(entry) => entry,
-                    // check if using the same bindings as before
-                    HirIdMapEntry::Occupied(entry) => return *entry.get() == b_id,
-                }
+        let check_eq_with_pat = |expr_a: &Expr<'_>, expr_b: &Expr<'_>| {
+            let mut local_map: HirIdMap<HirId> = HirIdMap::default();
+            let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
+                if let Some(a_id) = path_to_local(a)
+                    && let Some(b_id) = path_to_local(b)
+                    && let entry = match local_map.entry(a_id) {
+                        HirIdMapEntry::Vacant(entry) => entry,
+                        // check if using the same bindings as before
+                        HirIdMapEntry::Occupied(entry) => return *entry.get() == b_id,
+                    }
                 // the names technically don't have to match; this makes the lint more conservative
                 && cx.tcx.hir().name(a_id) == cx.tcx.hir().name(b_id)
-                && cx.typeck_results().expr_ty(a) == cx.typeck_results().expr_ty(b)
-                && pat_contains_local(lhs.pat, a_id)
-                && pat_contains_local(rhs.pat, b_id)
-            {
-                entry.insert(b_id);
-                true
-            } else {
-                false
-            }
-        };
-        // Arms with a guard are ignored, those can’t always be merged together
-        // If both arms overlap with an arm in between then these can't be merged either.
-        !(backwards_blocking_idxs[max_index] > min_index && forwards_blocking_idxs[min_index] < max_index)
-                && lhs.guard.is_none()
-                && rhs.guard.is_none()
-                && SpanlessEq::new(cx)
-                    .expr_fallback(eq_fallback)
-                    .eq_expr(lhs.body, rhs.body)
+                    && cx.typeck_results().expr_ty(a) == cx.typeck_results().expr_ty(b)
+                    && pat_contains_local(lhs.pat, a_id)
+                    && pat_contains_local(rhs.pat, b_id)
+                {
+                    entry.insert(b_id);
+                    true
+                } else {
+                    false
+                }
+            };
+
+            SpanlessEq::new(cx)
+                .expr_fallback(eq_fallback)
+                .eq_expr(expr_a, expr_b)
                 // these checks could be removed to allow unused bindings
                 && bindings_eq(lhs.pat, local_map.keys().copied().collect())
                 && bindings_eq(rhs.pat, local_map.values().copied().collect())
+        };
+
+        let check_same_guard = || match (&lhs.guard, &rhs.guard) {
+            (None, None) => true,
+            (Some(lhs_guard), Some(rhs_guard)) => check_eq_with_pat(lhs_guard, rhs_guard),
+            _ => false,
+        };
+
+        let check_same_body = || check_eq_with_pat(lhs.body, rhs.body);
+
+        // Arms with different guard are ignored, those can’t always be merged together
+        // If both arms overlap with an arm in between then these can't be merged either.
+        !(backwards_blocking_idxs[max_index] > min_index && forwards_blocking_idxs[min_index] < max_index)
+            && check_same_guard()
+            && check_same_body()
     };
 
     let indexed_arms: Vec<(usize, &Arm<'_>)> = arms.iter().enumerate().collect();
diff --git a/tests/ui/match_same_arms2.rs b/tests/ui/match_same_arms2.rs
index 3428ff45906..85ad0962eb4 100644
--- a/tests/ui/match_same_arms2.rs
+++ b/tests/ui/match_same_arms2.rs
@@ -67,6 +67,20 @@ fn match_same_arms() {
         _ => (),
     }
 
+    // No warning because guards are different
+    let _ = match Some(42) {
+        Some(a) if a == 42 => a,
+        Some(a) if a == 24 => a,
+        Some(_) => 24,
+        None => 0,
+    };
+
+    let _ = match (Some(42), Some(42)) {
+        (Some(a), None) if a == 42 => a,
+        (None, Some(a)) if a == 42 => a, //~ ERROR: this match arm has an identical body to another arm
+        _ => 0,
+    };
+
     match (Some(42), Some(42)) {
         (Some(a), ..) => bar(a), //~ ERROR: this match arm has an identical body to another arm
         (.., Some(a)) => bar(a),
diff --git a/tests/ui/match_same_arms2.stderr b/tests/ui/match_same_arms2.stderr
index 6abbb582763..f4c38c1af89 100644
--- a/tests/ui/match_same_arms2.stderr
+++ b/tests/ui/match_same_arms2.stderr
@@ -71,7 +71,22 @@ LL |         (Some(a), None) => bar(a),
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: this match arm has an identical body to another arm
-  --> tests/ui/match_same_arms2.rs:71:9
+  --> tests/ui/match_same_arms2.rs:80:9
+   |
+LL |         (None, Some(a)) if a == 42 => a,
+   |         ---------------^^^^^^^^^^^^^^^^
+   |         |
+   |         help: try merging the arm patterns: `(None, Some(a)) | (Some(a), None)`
+   |
+   = help: or try changing either arm body
+note: other arm here
+  --> tests/ui/match_same_arms2.rs:79:9
+   |
+LL |         (Some(a), None) if a == 42 => a,
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: this match arm has an identical body to another arm
+  --> tests/ui/match_same_arms2.rs:85:9
    |
 LL |         (Some(a), ..) => bar(a),
    |         -------------^^^^^^^^^^
@@ -80,13 +95,13 @@ LL |         (Some(a), ..) => bar(a),
    |
    = help: or try changing either arm body
 note: other arm here
-  --> tests/ui/match_same_arms2.rs:72:9
+  --> tests/ui/match_same_arms2.rs:86:9
    |
 LL |         (.., Some(a)) => bar(a),
    |         ^^^^^^^^^^^^^^^^^^^^^^^
 
 error: this match arm has an identical body to another arm
-  --> tests/ui/match_same_arms2.rs:105:9
+  --> tests/ui/match_same_arms2.rs:119:9
    |
 LL |         (Ok(x), Some(_)) => println!("ok {}", x),
    |         ----------------^^^^^^^^^^^^^^^^^^^^^^^^
@@ -95,13 +110,13 @@ LL |         (Ok(x), Some(_)) => println!("ok {}", x),
    |
    = help: or try changing either arm body
 note: other arm here
-  --> tests/ui/match_same_arms2.rs:106:9
+  --> tests/ui/match_same_arms2.rs:120:9
    |
 LL |         (Ok(_), Some(x)) => println!("ok {}", x),
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: this match arm has an identical body to another arm
-  --> tests/ui/match_same_arms2.rs:121:9
+  --> tests/ui/match_same_arms2.rs:135:9
    |
 LL |         Ok(_) => println!("ok"),
    |         -----^^^^^^^^^^^^^^^^^^
@@ -110,13 +125,13 @@ LL |         Ok(_) => println!("ok"),
    |
    = help: or try changing either arm body
 note: other arm here
-  --> tests/ui/match_same_arms2.rs:120:9
+  --> tests/ui/match_same_arms2.rs:134:9
    |
 LL |         Ok(3) => println!("ok"),
    |         ^^^^^^^^^^^^^^^^^^^^^^^
 
 error: this match arm has an identical body to another arm
-  --> tests/ui/match_same_arms2.rs:148:9
+  --> tests/ui/match_same_arms2.rs:162:9
    |
 LL |           1 => {
    |           ^ help: try merging the arm patterns: `1 | 0`
@@ -128,7 +143,7 @@ LL | |         },
    |
    = help: or try changing either arm body
 note: other arm here
-  --> tests/ui/match_same_arms2.rs:145:9
+  --> tests/ui/match_same_arms2.rs:159:9
    |
 LL | /         0 => {
 LL | |             empty!(0);
@@ -136,7 +151,7 @@ LL | |         },
    | |_________^
 
 error: match expression looks like `matches!` macro
-  --> tests/ui/match_same_arms2.rs:167:16
+  --> tests/ui/match_same_arms2.rs:181:16
    |
 LL |       let _ans = match x {
    |  ________________^
@@ -150,7 +165,7 @@ LL | |     };
    = help: to override `-D warnings` add `#[allow(clippy::match_like_matches_macro)]`
 
 error: this match arm has an identical body to another arm
-  --> tests/ui/match_same_arms2.rs:199:9
+  --> tests/ui/match_same_arms2.rs:213:9
    |
 LL |         Foo::X(0) => 1,
    |         ---------^^^^^
@@ -159,13 +174,13 @@ LL |         Foo::X(0) => 1,
    |
    = help: or try changing either arm body
 note: other arm here
-  --> tests/ui/match_same_arms2.rs:201:9
+  --> tests/ui/match_same_arms2.rs:215:9
    |
 LL |         Foo::Z(_) => 1,
    |         ^^^^^^^^^^^^^^
 
 error: this match arm has an identical body to another arm
-  --> tests/ui/match_same_arms2.rs:209:9
+  --> tests/ui/match_same_arms2.rs:223:9
    |
 LL |         Foo::Z(_) => 1,
    |         ---------^^^^^
@@ -174,13 +189,13 @@ LL |         Foo::Z(_) => 1,
    |
    = help: or try changing either arm body
 note: other arm here
-  --> tests/ui/match_same_arms2.rs:207:9
+  --> tests/ui/match_same_arms2.rs:221:9
    |
 LL |         Foo::X(0) => 1,
    |         ^^^^^^^^^^^^^^
 
 error: this match arm has an identical body to another arm
-  --> tests/ui/match_same_arms2.rs:232:9
+  --> tests/ui/match_same_arms2.rs:246:9
    |
 LL |         Some(Bar { y: 0, x: 5, .. }) => 1,
    |         ----------------------------^^^^^
@@ -189,13 +204,13 @@ LL |         Some(Bar { y: 0, x: 5, .. }) => 1,
    |
    = help: or try changing either arm body
 note: other arm here
-  --> tests/ui/match_same_arms2.rs:229:9
+  --> tests/ui/match_same_arms2.rs:243:9
    |
 LL |         Some(Bar { x: 0, y: 5, .. }) => 1,
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: this match arm has an identical body to another arm
-  --> tests/ui/match_same_arms2.rs:246:9
+  --> tests/ui/match_same_arms2.rs:260:9
    |
 LL |         1 => cfg!(not_enable),
    |         -^^^^^^^^^^^^^^^^^^^^
@@ -204,10 +219,10 @@ LL |         1 => cfg!(not_enable),
    |
    = help: or try changing either arm body
 note: other arm here
-  --> tests/ui/match_same_arms2.rs:245:9
+  --> tests/ui/match_same_arms2.rs:259:9
    |
 LL |         0 => cfg!(not_enable),
    |         ^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 13 previous errors
+error: aborting due to 14 previous errors