Auto merge of #10336 - samueltardieu:issue-10241, r=llogiq

manual_let_else: do not suggest semantically different replacements

The problem is that this lint does not consider the possibility that the divergent branch can come first and that the patterns may overlap. This led to incorrect suggestions, previously registered as correct in the tests themselves:

```rust
let v = match build_enum() {
    _ => continue,
    Variant::Bar(v) | Variant::Baz(v) => v,
};
```

had a `let Variant::Bar(v) | Variant::Baz(v) = v else { continue; }` suggestion, which is obviously wrong as the original code `continue`s in any case. Issue #10241 gives another example.

The code now checks that the divergent branch comes second. It could be extended later (I've added a TODO) to check for non-overlapping patterns.

Fixes #10241.

changelog: [`manual_let_else`] do not suggest non equivalent replacements in `match`

3 files changed, 46 insertions, 5 deletions

diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs
index 5db5c644736..98e698c6c2a 100644
--- a/clippy_lints/src/manual_let_else.rs
+++ b/clippy_lints/src/manual_let_else.rs
@@ -116,6 +116,13 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
                     .enumerate()
                     .find(|(_, arm)| expr_diverges(cx, arm.body) && pat_allowed_for_else(cx, arm.pat, check_types));
                 let Some((idx, diverging_arm)) = diverging_arm_opt else { return; };
+                // If the non-diverging arm is the first one, its pattern can be reused in a let/else statement.
+                // However, if it arrives in second position, its pattern may cover some cases already covered
+                // by the diverging one.
+                // TODO: accept the non-diverging arm as a second position if patterns are disjointed.
+                if idx == 0 {
+                    return;
+                }
                 let pat_arm = &arms[1 - idx];
                 if !expr_is_simple_identity(pat_arm.pat, pat_arm.body) {
                     return;
diff --git a/tests/ui/manual_let_else_match.rs b/tests/ui/manual_let_else_match.rs
index 28caed9d79d..73b74679125 100644
--- a/tests/ui/manual_let_else_match.rs
+++ b/tests/ui/manual_let_else_match.rs
@@ -42,13 +42,13 @@ fn fire() {
     loop {
         // More complex pattern for the identity arm and diverging arm
         let v = match h() {
-            (Some(_), Some(_)) | (None, None) => continue,
             (Some(v), None) | (None, Some(v)) => v,
+            (Some(_), Some(_)) | (None, None) => continue,
         };
         // Custom enums are supported as long as the "else" arm is a simple _
         let v = match build_enum() {
-            _ => continue,
             Variant::Bar(v) | Variant::Baz(v) => v,
+            _ => continue,
         };
     }
 
@@ -71,6 +71,12 @@ fn fire() {
         Variant::Bar(_) | Variant::Baz(_) => (),
         _ => return,
     };
+
+    let data = [1_u8, 2, 3, 4, 0, 0, 0, 0];
+    let data = match data.as_slice() {
+        [data @ .., 0, 0, 0, 0] | [data @ .., 0, 0] | [data @ .., 0] => data,
+        _ => return,
+    };
 }
 
 fn not_fire() {
@@ -125,4 +131,23 @@ fn not_fire() {
         Ok(v) | Err(Variant::Bar(v) | Variant::Baz(v)) => v,
         Err(Variant::Foo) => return,
     };
+
+    // Issue 10241
+    // The non-divergent arm arrives in second position and
+    // may cover values already matched in the first arm.
+    let v = match h() {
+        (Some(_), Some(_)) | (None, None) => return,
+        (Some(v), _) | (None, Some(v)) => v,
+    };
+
+    let v = match build_enum() {
+        _ => return,
+        Variant::Bar(v) | Variant::Baz(v) => v,
+    };
+
+    let data = [1_u8, 2, 3, 4, 0, 0, 0, 0];
+    let data = match data.as_slice() {
+        [] | [0, 0] => return,
+        [data @ .., 0, 0, 0, 0] | [data @ .., 0, 0] | [data @ ..] => data,
+    };
 }
diff --git a/tests/ui/manual_let_else_match.stderr b/tests/ui/manual_let_else_match.stderr
index cd5e9a9ac39..7abaa0b85d2 100644
--- a/tests/ui/manual_let_else_match.stderr
+++ b/tests/ui/manual_let_else_match.stderr
@@ -22,8 +22,8 @@ error: this could be rewritten as `let...else`
   --> $DIR/manual_let_else_match.rs:44:9
    |
 LL | /         let v = match h() {
-LL | |             (Some(_), Some(_)) | (None, None) => continue,
 LL | |             (Some(v), None) | (None, Some(v)) => v,
+LL | |             (Some(_), Some(_)) | (None, None) => continue,
 LL | |         };
    | |__________^ help: consider writing: `let ((Some(v), None) | (None, Some(v))) = h() else { continue };`
 
@@ -31,8 +31,8 @@ error: this could be rewritten as `let...else`
   --> $DIR/manual_let_else_match.rs:49:9
    |
 LL | /         let v = match build_enum() {
-LL | |             _ => continue,
 LL | |             Variant::Bar(v) | Variant::Baz(v) => v,
+LL | |             _ => continue,
 LL | |         };
    | |__________^ help: consider writing: `let (Variant::Bar(v) | Variant::Baz(v)) = build_enum() else { continue };`
 
@@ -63,5 +63,14 @@ LL | |         _ => return,
 LL | |     };
    | |______^ help: consider writing: `let (Variant::Bar(_) | Variant::Baz(_)) = f else { return };`
 
-error: aborting due to 7 previous errors
+error: this could be rewritten as `let...else`
+  --> $DIR/manual_let_else_match.rs:76:5
+   |
+LL | /     let data = match data.as_slice() {
+LL | |         [data @ .., 0, 0, 0, 0] | [data @ .., 0, 0] | [data @ .., 0] => data,
+LL | |         _ => return,
+LL | |     };
+   | |______^ help: consider writing: `let ([data @ .., 0, 0, 0, 0] | [data @ .., 0, 0] | [data @ .., 0]) = data.as_slice() else { return };`
+
+error: aborting due to 8 previous errors