Auto merge of #9496 - yotamofek:never_loop_let_else, r=Jarcho

[`never_loop`]: Fix FP with let..else statements. Fixes #9356 This has been bugging me for a while, so I thought I'd take a stab at it! I'm completely uncertain about the quality of my code, but I think it's an alright start, so opening this PR to get some feedback from more experienced clippy people :) changelog: [`never_loop`]: Fix FP with let..else statements
rust-lang · Sep 23, 2022 · ff65eec · ff65eec
2 parents c8f2f38 + d63aece
commit ff65eec
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 20 deletions.
diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs
@@ -42,6 +42,7 @@ pub(super) fn check(
     }
 }
 
+#[derive(Copy, Clone)]
 enum NeverLoopResult {
     // A break/return always get triggered but not necessarily for the main loop.
     AlwaysBreak,
@@ -51,8 +52,8 @@ enum NeverLoopResult {
 }
 
 #[must_use]
-fn absorb_break(arg: &NeverLoopResult) -> NeverLoopResult {
-    match *arg {
+fn absorb_break(arg: NeverLoopResult) -> NeverLoopResult {
+    match arg {
         NeverLoopResult::AlwaysBreak | NeverLoopResult::Otherwise => NeverLoopResult::Otherwise,
         NeverLoopResult::MayContinueMainLoop => NeverLoopResult::MayContinueMainLoop,
     }
@@ -92,19 +93,29 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult
 }
 
 fn never_loop_block(block: &Block<'_>, main_loop_id: HirId) -> NeverLoopResult {
-    let mut iter = block.stmts.iter().filter_map(stmt_to_expr).chain(block.expr);
+    let mut iter = block
+        .stmts
+        .iter()
+        .filter_map(stmt_to_expr)
+        .chain(block.expr.map(|expr| (expr, None)));
     never_loop_expr_seq(&mut iter, main_loop_id)
 }
 
-fn never_loop_expr_seq<'a, T: Iterator<Item = &'a Expr<'a>>>(es: &mut T, main_loop_id: HirId) -> NeverLoopResult {
-    es.map(|e| never_loop_expr(e, main_loop_id))
-        .fold(NeverLoopResult::Otherwise, combine_seq)
+fn never_loop_expr_seq<'a, T: Iterator<Item = (&'a Expr<'a>, Option<&'a Block<'a>>)>>(
+    es: &mut T,
+    main_loop_id: HirId,
+) -> NeverLoopResult {
+    es.map(|(e, els)| {
+        let e = never_loop_expr(e, main_loop_id);
+        els.map_or(e, |els| combine_branches(e, never_loop_block(els, main_loop_id)))
+    })
+    .fold(NeverLoopResult::Otherwise, combine_seq)
 }
 
-fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<&'tcx Expr<'tcx>> {
+fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<(&'tcx Expr<'tcx>, Option<&'tcx Block<'tcx>>)> {
     match stmt.kind {
-        StmtKind::Semi(e, ..) | StmtKind::Expr(e, ..) => Some(e),
-        StmtKind::Local(local) => local.init,
+        StmtKind::Semi(e, ..) | StmtKind::Expr(e, ..) => Some((e, None)),
+        StmtKind::Local(local) => local.init.map(|init| (init, local.els)),
         StmtKind::Item(..) => None,
     }
 }
@@ -139,7 +150,7 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
         | ExprKind::Index(e1, e2) => never_loop_expr_all(&mut [e1, e2].iter().copied(), main_loop_id),
         ExprKind::Loop(b, _, _, _) => {
             // Break can come from the inner loop so remove them.
-            absorb_break(&never_loop_block(b, main_loop_id))
+            absorb_break(never_loop_block(b, main_loop_id))
         },
         ExprKind::If(e, e2, e3) => {
             let e1 = never_loop_expr(e, main_loop_id);

diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs
@@ -1,3 +1,4 @@
+#![feature(let_else)]
 #![allow(
     clippy::single_match,
     unused_assignments,
@@ -203,6 +204,32 @@ pub fn test17() {
     };
 }
 
+// Issue #9356: `continue` in else branch of let..else
+pub fn test18() {
+    let x = Some(0);
+    let y = 0;
+    // might loop
+    let _ = loop {
+        let Some(x) = x else {
+            if y > 0 {
+                continue;
+            } else {
+                return;
+            }
+        };
+
+        break x;
+    };
+    // never loops
+    let _ = loop {
+        let Some(x) = x else {
+            return;
+        };
+
+        break x;
+    };
+}
+
 fn main() {
     test1();
     test2();

diff --git a/tests/ui/never_loop.stderr b/tests/ui/never_loop.stderr
@@ -1,5 +1,5 @@
 error: this loop never actually loops
-  --> $DIR/never_loop.rs:10:5
+  --> $DIR/never_loop.rs:11:5
    |
 LL | /     loop {
 LL | |         // clippy::never_loop
@@ -13,7 +13,7 @@ LL | |     }
    = note: `#[deny(clippy::never_loop)]` on by default
 
 error: this loop never actually loops
-  --> $DIR/never_loop.rs:32:5
+  --> $DIR/never_loop.rs:33:5
    |
 LL | /     loop {
 LL | |         // never loops
@@ -23,7 +23,7 @@ LL | |     }
    | |_____^
 
 error: this loop never actually loops
-  --> $DIR/never_loop.rs:52:5
+  --> $DIR/never_loop.rs:53:5
    |
 LL | /     loop {
 LL | |         // never loops
@@ -35,7 +35,7 @@ LL | |     }
    | |_____^
 
 error: this loop never actually loops
-  --> $DIR/never_loop.rs:54:9
+  --> $DIR/never_loop.rs:55:9
    |
 LL | /         while i == 0 {
 LL | |             // never loops
@@ -44,7 +44,7 @@ LL | |         }
    | |_________^
 
 error: this loop never actually loops
-  --> $DIR/never_loop.rs:66:9
+  --> $DIR/never_loop.rs:67:9
    |
 LL | /         loop {
 LL | |             // never loops
@@ -56,7 +56,7 @@ LL | |         }
    | |_________^
 
 error: this loop never actually loops
-  --> $DIR/never_loop.rs:102:5
+  --> $DIR/never_loop.rs:103:5
    |
 LL | /     while let Some(y) = x {
 LL | |         // never loops
@@ -65,7 +65,7 @@ LL | |     }
    | |_____^
 
 error: this loop never actually loops
-  --> $DIR/never_loop.rs:109:5
+  --> $DIR/never_loop.rs:110:5
    |
 LL | /     for x in 0..10 {
 LL | |         // never loops
@@ -82,7 +82,7 @@ LL |     if let Some(x) = (0..10).next() {
    |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 error: this loop never actually loops
-  --> $DIR/never_loop.rs:157:5
+  --> $DIR/never_loop.rs:158:5
    |
 LL | /     'outer: while a {
 LL | |         // never loops
@@ -94,12 +94,25 @@ LL | |     }
    | |_____^
 
 error: this loop never actually loops
-  --> $DIR/never_loop.rs:172:9
+  --> $DIR/never_loop.rs:173:9
    |
 LL | /         while false {
 LL | |             break 'label;
 LL | |         }
    | |_________^
 
-error: aborting due to 9 previous errors
+error: this loop never actually loops
+  --> $DIR/never_loop.rs:224:13
+   |
+LL |       let _ = loop {
+   |  _____________^
+LL | |         let Some(x) = x else {
+LL | |             return;
+LL | |         };
+LL | |
+LL | |         break x;
+LL | |     };
+   | |_____^
+
+error: aborting due to 10 previous errors