Also consider match guards for divergence check

Plus, add some tests for the divergence check.
rust-lang · Oct 16, 2022 · 383a44f · 383a44f
1 parent 23121cc
commit 383a44f
Show file tree

Hide file tree

Showing 3 changed files with 120 additions and 16 deletions.
diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs
@@ -197,8 +197,11 @@ fn expr_diverges(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool {
                 ControlFlow::Continue(Descend::No)
             },
             ExprKind::Match(match_expr, match_arms, _) => {
-                let diverges =
-                    expr_diverges(cx, match_expr) || match_arms.iter().all(|arm| expr_diverges(cx, arm.body));
+                let diverges = expr_diverges(cx, match_expr)
+                    || match_arms.iter().all(|arm| {
+                        let guard_diverges = arm.guard.as_ref().map(|g| expr_diverges(cx, g.body())).unwrap_or(false);
+                        guard_diverges || expr_diverges(cx, arm.body)
+                    });
                 if diverges {
                     does_diverge = true;
                     return ControlFlow::Break(());

diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs
@@ -3,6 +3,7 @@
     clippy::collapsible_else_if,
     clippy::unused_unit,
     clippy::let_unit_value,
+    clippy::match_single_binding,
     clippy::never_loop
 )]
 #![warn(clippy::manual_let_else)]
@@ -55,6 +56,38 @@ fn fire() {
         if true { return } else { panic!() }
     };
 
+    // Diverging after an if still makes the block diverge:
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else {
+        if true {}
+        panic!();
+    };
+
+    // A match diverges if all branches diverge:
+    // Note: the corresponding let-else requires a ; at the end of the match
+    // as otherwise the type checker does not turn it into a ! type.
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else {
+        match () {
+            _ if panic!() => {},
+            _ => panic!(),
+        }
+    };
+
+    // An if's expression can cause divergence:
+    let v = if let Some(v_some) = g() { v_some } else { if panic!() {} };
+
+    // An expression of a match can cause divergence:
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else {
+        match panic!() {
+            _ => {},
+        }
+    };
+
     // Top level else if
     let v = if let Some(v_some) = g() {
         v_some

diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr
@@ -1,13 +1,13 @@
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:17:5
+  --> $DIR/manual_let_else.rs:18:5
    |
 LL |     let v = if let Some(v_some) = g() { v_some } else { return };
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { return };`
    |
    = note: `-D clippy::manual-let-else` implied by `-D warnings`
 
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:18:5
+  --> $DIR/manual_let_else.rs:19:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |         v_some
@@ -24,7 +24,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:24:5
+  --> $DIR/manual_let_else.rs:25:5
    |
 LL | /     let v = if let Some(v) = g() {
 LL | |         // Blocks around the identity should have no impact
@@ -45,25 +45,25 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:37:9
+  --> $DIR/manual_let_else.rs:38:9
    |
 LL |         let v = if let Some(v_some) = g() { v_some } else { continue };
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { continue };`
 
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:38:9
+  --> $DIR/manual_let_else.rs:39:9
    |
 LL |         let v = if let Some(v_some) = g() { v_some } else { break };
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { break };`
 
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:42:5
+  --> $DIR/manual_let_else.rs:43:5
    |
 LL |     let v = if let Some(v_some) = g() { v_some } else { panic!() };
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { panic!() };`
 
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:45:5
+  --> $DIR/manual_let_else.rs:46:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |         v_some
@@ -80,7 +80,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:52:5
+  --> $DIR/manual_let_else.rs:53:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |         v_some
@@ -97,7 +97,75 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:59:5
+  --> $DIR/manual_let_else.rs:60:5
+   |
+LL | /     let v = if let Some(v_some) = g() {
+LL | |         v_some
+LL | |     } else {
+LL | |         if true {}
+LL | |         panic!();
+LL | |     };
+   | |______^
+   |
+help: consider writing
+   |
+LL ~     let Some(v_some) = g() else {
+LL +         if true {}
+LL +         panic!();
+LL +     };
+   |
+
+error: this could be rewritten as `let...else`
+  --> $DIR/manual_let_else.rs:70:5
+   |
+LL | /     let v = if let Some(v_some) = g() {
+LL | |         v_some
+LL | |     } else {
+LL | |         match () {
+...  |
+LL | |         }
+LL | |     };
+   | |______^
+   |
+help: consider writing
+   |
+LL ~     let Some(v_some) = g() else {
+LL +         match () {
+LL +             _ if panic!() => {},
+LL +             _ => panic!(),
+LL +         }
+LL +     };
+   |
+
+error: this could be rewritten as `let...else`
+  --> $DIR/manual_let_else.rs:80:5
+   |
+LL |     let v = if let Some(v_some) = g() { v_some } else { if panic!() {} };
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { if panic!() {} };`
+
+error: this could be rewritten as `let...else`
+  --> $DIR/manual_let_else.rs:83:5
+   |
+LL | /     let v = if let Some(v_some) = g() {
+LL | |         v_some
+LL | |     } else {
+LL | |         match panic!() {
+LL | |             _ => {},
+LL | |         }
+LL | |     };
+   | |______^
+   |
+help: consider writing
+   |
+LL ~     let Some(v_some) = g() else {
+LL +         match panic!() {
+LL +             _ => {},
+LL +         }
+LL +     };
+   |
+
+error: this could be rewritten as `let...else`
+  --> $DIR/manual_let_else.rs:92:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |         v_some
@@ -118,7 +186,7 @@ LL +     } };
    |
 
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:68:5
+  --> $DIR/manual_let_else.rs:101:5
    |
 LL | /     let v = if let Some(v_some) = g() {
 LL | |         v_some
@@ -147,7 +215,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:85:5
+  --> $DIR/manual_let_else.rs:118:5
    |
 LL | /     let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) {
 LL | |         v_some
@@ -164,7 +232,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:92:5
+  --> $DIR/manual_let_else.rs:125:5
    |
 LL | /     let v = if let (Some(v_some), w_some) = (g(), 0) {
 LL | |         (w_some, v_some)
@@ -181,7 +249,7 @@ LL +     };
    |
 
 error: this could be rewritten as `let...else`
-  --> $DIR/manual_let_else.rs:101:13
+  --> $DIR/manual_let_else.rs:134:13
    |
 LL |             let $n = if let Some(v) = $e { v } else { return };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };`
@@ -191,5 +259,5 @@ LL |     create_binding_if_some!(w, g());
    |
    = note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info)
 
-error: aborting due to 13 previous errors
+error: aborting due to 17 previous errors