hoist statements before break out of the loop

add test cases for semicolon_less and multiple stmts

extract could_be_while_let fn param into a struct

fix clippy warnings

Author: borngraced

clippy_lints/src/loops/while_let_loop.rs

@@ -1,13 +1,17 @@
 use super::WHILE_LET_LOOP;
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::source::{snippet, snippet_indent, snippet_opt};
+use clippy_utils::source::{snippet, snippet_indent, snippet_opt, snippet_with_context};
 use clippy_utils::ty::needs_ordered_drop;
-use clippy_utils::visitors::any_temporaries_need_ordered_drop;
+use clippy_utils::visitors::{any_temporaries_need_ordered_drop, for_each_expr_without_closures};
 use clippy_utils::{higher, peel_blocks};
 use rustc_ast::BindingMode;
 use rustc_errors::Applicability;
-use rustc_hir::{Block, Expr, ExprKind, LetStmt, MatchSource, Pat, PatKind, Path, QPath, StmtKind, Ty};
+use rustc_hir::{
+    Block, Destination, Expr, ExprKind, LetStmt, MatchSource, Pat, PatKind, Path, QPath, Stmt, StmtKind, Ty,
+};
 use rustc_lint::LateContext;
+use std::fmt::Write;
+use std::ops::ControlFlow;
 
 pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, loop_block: &'tcx Block<'_>) {
     let (init, let_info, els) = match (loop_block.stmts, loop_block.expr) {
@@ -34,16 +38,46 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, loop_blo
         could_be_while_let(
             cx,
             expr,
-            if_let.let_pat,
-            if_let.let_expr,
-            has_trailing_exprs,
-            let_info,
-            Some(if_let.if_then),
+            WhileLetInfo {
+                let_pat: if_let.let_pat,
+                let_expr: if_let.let_expr,
+                has_trailing_exprs,
+                let_info,
+                inner_expr: Some(if_let.if_then),
+                hoistable_stmts: None,
+            },
         );
     } else if els.is_some_and(is_simple_break_block)
         && let Some((pat, _)) = let_info
     {
-        could_be_while_let(cx, expr, pat, init, has_trailing_exprs, let_info, None);
+        could_be_while_let(
+            cx,
+            expr,
+            WhileLetInfo {
+                let_pat: pat,
+                let_expr: init,
+                has_trailing_exprs,
+                let_info,
+                inner_expr: None,
+                hoistable_stmts: None,
+            },
+        );
+    } else if let Some(els_block) = els
+        && let Some((pat, _)) = let_info
+        && let Some(hoistable) = extract_hoistable_stmts(els_block)
+    {
+        could_be_while_let(
+            cx,
+            expr,
+            WhileLetInfo {
+                let_pat: pat,
+                let_expr: init,
+                has_trailing_exprs,
+                let_info,
+                inner_expr: None,
+                hoistable_stmts: Some(hoistable),
+            },
+        );
     } else if let ExprKind::Match(scrutinee, [arm1, arm2], MatchSource::Normal) = init.kind
         && arm1.guard.is_none()
         && arm2.guard.is_none()
@@ -52,11 +86,14 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, loop_blo
         could_be_while_let(
             cx,
             expr,
-            arm1.pat,
-            scrutinee,
-            has_trailing_exprs,
-            let_info,
-            Some(arm1.body),
+            WhileLetInfo {
+                let_pat: arm1.pat,
+                let_expr: scrutinee,
+                has_trailing_exprs,
+                let_info,
+                inner_expr: Some(arm1.body),
+                hoistable_stmts: None,
+            },
         );
     }
 }
@@ -81,15 +118,26 @@ fn is_simple_break_expr(expr: &Expr<'_>) -> bool {
     }
 }
 
-fn could_be_while_let<'tcx>(
-    cx: &LateContext<'tcx>,
-    expr: &'tcx Expr<'_>,
-    let_pat: &'tcx Pat<'_>,
-    let_expr: &'tcx Expr<'_>,
+#[derive(Copy, Clone)]
+struct WhileLetInfo<'tcx> {
+    let_pat: &'tcx Pat<'tcx>,
+    let_expr: &'tcx Expr<'tcx>,
     has_trailing_exprs: bool,
-    let_info: Option<(&Pat<'_>, Option<&Ty<'_>>)>,
-    inner_expr: Option<&Expr<'_>>,
-) {
+    let_info: Option<(&'tcx Pat<'tcx>, Option<&'tcx Ty<'tcx>>)>,
+    inner_expr: Option<&'tcx Expr<'tcx>>,
+    hoistable_stmts: Option<&'tcx [Stmt<'tcx>]>,
+}
+
+fn could_be_while_let<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, info: WhileLetInfo<'tcx>) {
+    let WhileLetInfo {
+        let_pat,
+        let_expr,
+        has_trailing_exprs,
+        let_info,
+        inner_expr,
+        hoistable_stmts,
+    } = info;
+
     if has_trailing_exprs
         && (needs_ordered_drop(cx, cx.typeck_results().expr_ty(let_expr))
             || any_temporaries_need_ordered_drop(cx, let_expr))
@@ -98,11 +146,13 @@ fn could_be_while_let<'tcx>(
         return;
     }
 
+    let indent = snippet_indent(cx, expr.span).unwrap_or_default();
+
     // NOTE: we used to build a body here instead of using
     // ellipsis, this was removed because:
     // 1) it was ugly with big bodies;
     // 2) it was not indented properly;
-    // 3) it wasn’t very smart (see #675).
+    // 3) it wasn't very smart (see #675).
     let inner_content = if let Some(((pat, ty), inner_expr)) = let_info.zip(inner_expr)
         // Prevent trivial reassignments such as `let x = x;` or `let _ = …;`, but
         // keep them if the type has been explicitly specified.
@@ -113,22 +163,37 @@ fn could_be_while_let<'tcx>(
         let ty_str = ty
             .map(|ty| format!(": {}", snippet(cx, ty.span, "_")))
             .unwrap_or_default();
-        format!(
-            "\n{indent}    let {pat_str}{ty_str} = {init_str};\n{indent}    ..\n{indent}",
-            indent = snippet_indent(cx, expr.span).unwrap_or_default(),
-        )
+        format!("\n{indent}    let {pat_str}{ty_str} = {init_str};\n{indent}    ..\n{indent}")
     } else {
         " .. ".into()
     };
 
+    // Build the hoisted statements string in case we have statements to hoist
+    let hoisted_content = if let Some(stmts) = hoistable_stmts {
+        let mut hoisted = String::new();
+        let outer_ctxt = expr.span.ctxt();
+        for stmt in stmts {
+            let (stmt_str, _) = snippet_with_context(cx, stmt.span, outer_ctxt, "..", &mut Applicability::Unspecified);
+            let semi = if matches!(stmt.kind, StmtKind::Semi(_)) {
+                ";"
+            } else {
+                ""
+            };
+            let _ = write!(hoisted, "\n{indent}{stmt_str}{semi}");
+        }
+        hoisted
+    } else {
+        String::new()
+    };
+
     span_lint_and_sugg(
         cx,
         WHILE_LET_LOOP,
         expr.span,
         "this loop could be written as a `while let` loop",
         "try",
         format!(
-            "while let {} = {} {{{inner_content}}}",
+            "while let {} = {} {{{inner_content}}}{hoisted_content}",
             snippet(cx, let_pat.span, ".."),
             snippet(cx, let_expr.span, ".."),
         ),
@@ -146,3 +211,38 @@ fn is_trivial_assignment(pat: &Pat<'_>, init: &Expr<'_>) -> bool {
         _ => false,
     }
 }
+
+/// Checks if a block ends with an unlabeled `break` and returns the statements before it..
+/// or `None` if the block doesn't end with a simple break or if any statement before
+/// the break could exit the loop (via return, labeled break, etc.).
+fn extract_hoistable_stmts<'tcx>(block: &'tcx Block<'tcx>) -> Option<&'tcx [Stmt<'tcx>]> {
+    let stmts_before_break = match (block.stmts, block.expr) {
+        (stmts, Some(e)) if is_simple_break_expr(e) => stmts,
+        (stmts, None) if !stmts.is_empty() => {
+            let (last, rest) = stmts.split_last()?;
+            match last.kind {
+                StmtKind::Expr(e) | StmtKind::Semi(e) if is_simple_break_expr(e) => rest,
+                _ => return None,
+            }
+        },
+        _ => return None,
+    };
+
+    if stmts_before_break.is_empty() {
+        return None;
+    }
+
+    // Check that none of the statements before break contain return, labeled break,
+    // or other control flow that could exit the loop differently
+    let has_early_exit = stmts_before_break.iter().any(|stmt| {
+        for_each_expr_without_closures(stmt, |e| match e.kind {
+            ExprKind::Ret(..)
+            | ExprKind::Break(Destination { label: Some(_), .. }, _)
+            | ExprKind::Continue(Destination { label: Some(_), .. }) => ControlFlow::Break(()),
+            _ => ControlFlow::Continue(()),
+        })
+        .is_some()
+    });
+
+    (!has_early_exit).then_some(stmts_before_break)
+}

tests/ui/manual_take_nocore.stderr

@@ -28,7 +28,7 @@ fn main() {
     }
 
     loop {
-        // no error, else branch does something other than break
+        //~^ while_let_loop
         let Some(_x) = y else {
             let _z = 1;
             break;
@@ -255,22 +255,91 @@ fn let_assign() {
 }
 
 fn issue16378() {
-    // This does not lint today because of the extra statement(s)
-    // before the `break`.
-    // TODO: When the `break` statement/expr in the `let`/`else` is the
-    // only way to leave the loop, the lint could trigger and move
-    // the statements preceeding the `break` after the loop, as in:
-    // ```rust
-    // while let Some(x) = std::hint::black_box(None::<i32>) {
-    //     println!("x = {x}");
-    // }
-    // println!("fail");
-    // ```
     loop {
+        //~^ while_let_loop
         let Some(x) = std::hint::black_box(None::<i32>) else {
             println!("fail");
             break;
         };
         println!("x = {x}");
     }
 }
+
+fn test_hoist_with_multiple_stmts() {
+    let y = Some(true);
+    loop {
+        //~^ while_let_loop
+        let Some(x) = y else {
+            let a = 1;
+            let b = 2;
+            let _c = a + b;
+            println!("sum: {}", a + b);
+            eprintln!("failed");
+            break;
+        };
+        println!("x = {x}");
+    }
+}
+
+fn test_hoist_with_semicolon_less_stmt() {
+    let y = Some(true);
+    loop {
+        //~^ while_let_loop
+        let Some(x) = y else {
+            if std::hint::black_box(true) {
+                println!("pass");
+            }
+            match 42 {
+                0 => println!("zero"),
+                _ => println!("non-zero"),
+            }
+            break;
+        };
+        println!("x = {x}");
+    }
+}
+
+fn no_hoist_with_return() -> Option<i32> {
+    // Should NOT lint: the else block contains a return statement
+    loop {
+        let Some(x) = std::hint::black_box(None::<i32>) else {
+            if true {
+                return None;
+            }
+            break;
+        };
+        println!("x = {x}");
+    }
+    Some(42)
+}
+
+fn no_hoist_with_labeled_break() {
+    // Should NOT lint: the else block contains a labeled break to outer loop
+    'outer: loop {
+        loop {
+            let Some(x) = std::hint::black_box(None::<i32>) else {
+                if true {
+                    break 'outer;
+                }
+                break;
+            };
+            println!("x = {x}");
+        }
+    }
+}
+
+fn no_hoist_with_labeled_continue() {
+    // Should NOT lint: the else block contains a labeled continue to outer loop
+    'outer: loop {
+        loop {
+            let Some(x) = std::hint::black_box(None::<i32>) else {
+                if true {
+                    continue 'outer;
+                }
+                break;
+            };
+            println!("x = {x}");
+        }
+        break;
+    }
+}