never_loop lint: properly handle break from block

samueltardieu · samueltardieu · commit 346a9f231d5c · 2023-02-13T20:32:15.000+01:00
It is not sufficient to ignore break from a block inside the loop.
Instructions after the break must be ignored, as they are unreachable.
This is also true for all instructions in outer blocks and loops
until the right block is reached.
diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs
@@ -39,6 +39,7 @@ pub(super) fn check(
             });
         },
         NeverLoopResult::MayContinueMainLoop | NeverLoopResult::Otherwise => (),
+        NeverLoopResult::IgnoreUntilEnd(_) => unreachable!(),
     }
 }
 
@@ -48,6 +49,8 @@ enum NeverLoopResult {
     AlwaysBreak,
     // A continue may occur for the main loop.
     MayContinueMainLoop,
+    // Ignore everything until the end of the block with this id
+    IgnoreUntilEnd(HirId),
     Otherwise,
 }
 
@@ -56,35 +59,36 @@ fn absorb_break(arg: NeverLoopResult) -> NeverLoopResult {
     match arg {
         NeverLoopResult::AlwaysBreak | NeverLoopResult::Otherwise => NeverLoopResult::Otherwise,
         NeverLoopResult::MayContinueMainLoop => NeverLoopResult::MayContinueMainLoop,
+        NeverLoopResult::IgnoreUntilEnd(id) => NeverLoopResult::IgnoreUntilEnd(id),
     }
 }
 
 // Combine two results for parts that are called in order.
 #[must_use]
 fn combine_seq(first: NeverLoopResult, second: NeverLoopResult) -> NeverLoopResult {
     match first {
-        NeverLoopResult::AlwaysBreak | NeverLoopResult::MayContinueMainLoop => first,
-        NeverLoopResult::Otherwise => second,
-    }
-}
-
-// Combine two results where both parts are called but not necessarily in order.
-#[must_use]
-fn combine_both(left: NeverLoopResult, right: NeverLoopResult) -> NeverLoopResult {
-    match (left, right) {
-        (NeverLoopResult::MayContinueMainLoop, _) | (_, NeverLoopResult::MayContinueMainLoop) => {
-            NeverLoopResult::MayContinueMainLoop
+        NeverLoopResult::AlwaysBreak | NeverLoopResult::MayContinueMainLoop | NeverLoopResult::IgnoreUntilEnd(_) => {
+            first
         },
-        (NeverLoopResult::AlwaysBreak, _) | (_, NeverLoopResult::AlwaysBreak) => NeverLoopResult::AlwaysBreak,
-        (NeverLoopResult::Otherwise, NeverLoopResult::Otherwise) => NeverLoopResult::Otherwise,
+        NeverLoopResult::Otherwise => second,
     }
 }
 
 // Combine two results where only one of the part may have been executed.
 #[must_use]
-fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult {
+fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult, ignore_ids: &[HirId]) -> NeverLoopResult {
     match (b1, b2) {
-        (NeverLoopResult::AlwaysBreak, NeverLoopResult::AlwaysBreak) => NeverLoopResult::AlwaysBreak,
+        (NeverLoopResult::IgnoreUntilEnd(a), NeverLoopResult::IgnoreUntilEnd(b)) => {
+            if ignore_ids.iter().find(|&e| e == &a || e == &b).unwrap() == &a {
+                NeverLoopResult::IgnoreUntilEnd(b)
+            } else {
+                NeverLoopResult::IgnoreUntilEnd(a)
+            }
+        },
+        (NeverLoopResult::IgnoreUntilEnd(_), NeverLoopResult::AlwaysBreak)
+        | (NeverLoopResult::AlwaysBreak, NeverLoopResult::IgnoreUntilEnd(_) | NeverLoopResult::AlwaysBreak) => {
+            NeverLoopResult::AlwaysBreak
+        },
         (NeverLoopResult::MayContinueMainLoop, _) | (_, NeverLoopResult::MayContinueMainLoop) => {
             NeverLoopResult::MayContinueMainLoop
         },
@@ -103,7 +107,7 @@ fn never_loop_block(block: &Block<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id
         let e = never_loop_expr(e, ignore_ids, main_loop_id);
         // els is an else block in a let...else binding
         els.map_or(e, |els| {
-            combine_branches(e, never_loop_block(els, ignore_ids, main_loop_id))
+            combine_branches(e, never_loop_block(els, ignore_ids, main_loop_id), ignore_ids)
         })
     })
     .fold(NeverLoopResult::Otherwise, combine_seq)
@@ -139,7 +143,7 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
         ExprKind::Struct(_, fields, base) => {
             let fields = never_loop_expr_all(&mut fields.iter().map(|f| f.expr), ignore_ids, main_loop_id);
             if let Some(base) = base {
-                combine_both(fields, never_loop_expr(base, ignore_ids, main_loop_id))
+                combine_seq(fields, never_loop_expr(base, ignore_ids, main_loop_id))
             } else {
                 fields
             }
@@ -159,7 +163,7 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
             let e3 = e3.as_ref().map_or(NeverLoopResult::Otherwise, |e| {
                 never_loop_expr(e, ignore_ids, main_loop_id)
             });
-            combine_seq(e1, combine_branches(e2, e3))
+            combine_seq(e1, combine_branches(e2, e3, ignore_ids))
         },
         ExprKind::Match(e, arms, _) => {
             let e = never_loop_expr(e, ignore_ids, main_loop_id);
@@ -176,7 +180,10 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
             }
             let ret = never_loop_block(b, ignore_ids, main_loop_id);
             ignore_ids.pop();
-            ret
+            match ret {
+                NeverLoopResult::IgnoreUntilEnd(a) if a == b.hir_id => NeverLoopResult::Otherwise,
+                _ => ret,
+            }
         },
         ExprKind::Continue(d) => {
             let id = d
@@ -190,7 +197,7 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
         },
         // checks if break targets a block instead of a loop
         ExprKind::Break(Destination { target_id: Ok(t), .. }, e) if ignore_ids.contains(&t) => e
-            .map_or(NeverLoopResult::Otherwise, |e| {
+            .map_or(NeverLoopResult::IgnoreUntilEnd(t), |e| {
                 never_loop_expr(e, ignore_ids, main_loop_id)
             }),
         ExprKind::Break(_, e) | ExprKind::Ret(e) => e.as_ref().map_or(NeverLoopResult::AlwaysBreak, |e| {
@@ -218,7 +225,7 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
                 | InlineAsmOperand::SymFn { .. }
                 | InlineAsmOperand::SymStatic { .. } => NeverLoopResult::Otherwise,
             })
-            .fold(NeverLoopResult::Otherwise, combine_both),
+            .fold(NeverLoopResult::Otherwise, combine_seq),
         ExprKind::Yield(_, _)
         | ExprKind::Closure { .. }
         | ExprKind::Path(_)
@@ -234,16 +241,19 @@ fn never_loop_expr_all<'a, T: Iterator<Item = &'a Expr<'a>>>(
     main_loop_id: HirId,
 ) -> NeverLoopResult {
     es.map(|e| never_loop_expr(e, ignore_ids, main_loop_id))
-        .fold(NeverLoopResult::Otherwise, combine_both)
+        .fold(NeverLoopResult::Otherwise, combine_seq)
 }
 
 fn never_loop_expr_branch<'a, T: Iterator<Item = &'a Expr<'a>>>(
     e: &mut T,
     ignore_ids: &mut Vec<HirId>,
     main_loop_id: HirId,
 ) -> NeverLoopResult {
+    let ignore_ids_saved = ignore_ids.clone();
     e.map(|e| never_loop_expr(e, ignore_ids, main_loop_id))
-        .fold(NeverLoopResult::AlwaysBreak, combine_branches)
+        .fold(NeverLoopResult::AlwaysBreak, |a, b| {
+            combine_branches(a, b, &ignore_ids_saved)
+        })
 }
 
 fn for_to_if_let_sugg(cx: &LateContext<'_>, iterator: &Expr<'_>, pat: &Pat<'_>) -> String {
diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs
@@ -250,6 +250,30 @@ pub fn test20() {
     }
 }
 
+// Issue 10304: code after break from block was not considered
+// unreachable code and was considered for further analysis of
+// whether the loop would ever be executed or not.
+pub fn test21() {
+    for _ in 0..10 {
+        'block: {
+            break 'block;
+            return;
+        }
+        println!("looped");
+    }
+}
+
+pub fn test22() {
+    for _ in 0..10 {
+        'block: {
+            for _ in 0..20 {
+                break 'block;
+            }
+        }
+        println!("looped");
+    }
+}
+
 fn main() {
     test1();
     test2();
@@ -265,4 +289,6 @@ fn main() {
     test12(true, false);
     test13();
     test14();
+    test21();
+    test22();
 }
diff --git a/tests/ui/never_loop.stderr b/tests/ui/never_loop.stderr
@@ -126,5 +126,18 @@ LL | |         }
 LL | |     }
    | |_____^
 
-error: aborting due to 11 previous errors
+error: this loop never actually loops
+  --> $DIR/never_loop.rs:269:13
+   |
+LL | /             for _ in 0..20 {
+LL | |                 break 'block;
+LL | |             }
+   | |_____________^
+   |
+help: if you need the first element of the iterator, try writing
+   |
+LL |             if let Some(_) = (0..20).next() {
+   |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+error: aborting due to 12 previous errors
 
