diff --git a/clippy_lints/src/loops/while_let_loop.rs b/clippy_lints/src/loops/while_let_loop.rs index 53df9a358b0e..d13d0b9ccd5c 100644 --- a/clippy_lints/src/loops/while_let_loop.rs +++ b/clippy_lints/src/loops/while_let_loop.rs @@ -1,13 +1,18 @@ 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::diagnostics::span_lint_and_then; +use clippy_utils::source::{reindent_multiline, 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, HirId, LetStmt, LoopSource, MatchSource, Pat, PatKind, Path, QPath, Stmt, + StmtKind, Ty, +}; use rustc_lint::LateContext; +use std::fmt::Write as _; +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) { @@ -27,38 +32,47 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, loop_blo }; let has_trailing_exprs = loop_block.stmts.len() + usize::from(loop_block.expr.is_some()) > 1; - if let Some(if_let) = higher::IfLet::hir(cx, init) + let (let_pat, let_expr, inner_expr, hoistable_stmts) = if let Some(if_let) = higher::IfLet::hir(cx, init) && let Some(else_expr) = if_let.if_else && is_simple_break_expr(else_expr) { - could_be_while_let( - cx, - expr, - if_let.let_pat, - if_let.let_expr, - has_trailing_exprs, - let_info, - Some(if_let.if_then), - ); + (if_let.let_pat, if_let.let_expr, Some(if_let.if_then), 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); + (pat, init, None, None) + } else if let Some(els_block) = els + && let Some((pat, _)) = let_info + && let Some(hoistable) = extract_hoistable_stmts(els_block, expr.hir_id) + { + (pat, init, None, Some(hoistable)) } else if let ExprKind::Match(scrutinee, [arm1, arm2], MatchSource::Normal) = init.kind && arm1.guard.is_none() && arm2.guard.is_none() && is_simple_break_expr(arm2.body) { - could_be_while_let( - cx, - expr, - arm1.pat, - scrutinee, - has_trailing_exprs, - let_info, - Some(arm1.body), - ); + (arm1.pat, scrutinee, Some(arm1.body), None) + } else { + return; + }; + + if (has_trailing_exprs || hoistable_stmts.is_some()) + && (needs_ordered_drop(cx, cx.typeck_results().expr_ty(let_expr)) + || any_temporaries_need_ordered_drop(cx, let_expr)) + { + return; } + + could_be_while_let( + cx, + expr, + loop_block, + let_info, + let_pat, + let_expr, + inner_expr, + hoistable_stmts, + ); } /// Checks if `block` contains a single unlabeled `break` expression or statement, possibly embedded @@ -81,28 +95,30 @@ fn is_simple_break_expr(expr: &Expr<'_>) -> bool { } } +#[expect(clippy::too_many_arguments)] fn could_be_while_let<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, - let_pat: &'tcx Pat<'_>, - let_expr: &'tcx Expr<'_>, - has_trailing_exprs: bool, - let_info: Option<(&Pat<'_>, Option<&Ty<'_>>)>, - inner_expr: Option<&Expr<'_>>, + loop_block: &'tcx Block<'_>, + let_info: Option<(&'tcx Pat<'tcx>, Option<&'tcx Ty<'tcx>>)>, + let_pat: &'tcx Pat<'tcx>, + let_expr: &'tcx Expr<'tcx>, + inner_expr: Option<&'tcx Expr<'tcx>>, + hoistable_stmts: Option<&'tcx [Stmt<'tcx>]>, ) { - if has_trailing_exprs - && (needs_ordered_drop(cx, cx.typeck_results().expr_ty(let_expr)) - || any_temporaries_need_ordered_drop(cx, let_expr)) - { - // Switching to a `while let` loop will extend the lifetime of some values. - return; - } + let indent = snippet_indent(cx, expr.span).unwrap_or_default(); + + let label_prefix = if let ExprKind::Loop(_, Some(label), LoopSource::Loop, _) = expr.kind { + format!("{}: ", label.ident) + } else { + String::new() + }; // 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. @@ -111,26 +127,62 @@ fn could_be_while_let<'tcx>( && let Some(init_str) = snippet_opt(cx, peel_blocks(inner_expr).span) { let ty_str = ty.map_or_default(|ty| format!(": {}", snippet(cx, ty.span, "_"))); - 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() }; - span_lint_and_sugg( + 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 reindented = reindent_multiline(&stmt_str, true, Some(indent.len())); + let _ = write!(hoisted, "\n{indent}{reindented}{semi}"); + } + hoisted + } else { + String::new() + }; + + let human_suggestion = format!( + "{label_prefix}while let {} = {} {{{inner_content}}}{hoisted_content}", + snippet(cx, let_pat.span, ".."), + snippet(cx, let_expr.span, ".."), + ); + + span_lint_and_then( cx, WHILE_LET_LOOP, expr.span, "this loop could be written as a `while let` loop", - "try", - format!( - "while let {} = {} {{{inner_content}}}", - snippet(cx, let_pat.span, ".."), - snippet(cx, let_expr.span, ".."), - ), - Applicability::HasPlaceholders, + |diag| { + diag.span_suggestion(expr.span, "try", &human_suggestion, Applicability::HasPlaceholders); + + if inner_expr.is_none() { + let while_let_header = format!( + "{label_prefix}while let {} = {} {{", + snippet(cx, let_pat.span, ".."), + snippet(cx, let_expr.span, ".."), + ); + + let first_stmt_span = loop_block.stmts[0].span; + let replace_span = expr.span.with_hi(first_stmt_span.hi()); + + let mut parts = vec![(replace_span, while_let_header)]; + + if !hoisted_content.is_empty() { + parts.push((expr.span.shrink_to_hi(), hoisted_content.clone())); + } + + diag.tool_only_multipart_suggestion("try", parts, Applicability::MachineApplicable); + } + }, ); } @@ -144,3 +196,41 @@ 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 any statement before the break contains a `break` or `continue` targeting +/// the loop identified by `loop_id`. +fn extract_hoistable_stmts<'tcx>(block: &'tcx Block<'tcx>, loop_id: HirId) -> 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; + } + + // Reject statements containing a `break`/`continue` targeting the loop + // being transformed. Breaks/continues to other loops and returns are fine to hoist. + let has_problematic_control_flow = stmts_before_break.iter().any(|stmt| { + for_each_expr_without_closures(stmt, |e| match e.kind { + ExprKind::Break(Destination { target_id: Ok(id), .. }, _) + | ExprKind::Continue(Destination { target_id: Ok(id), .. }) + if id == loop_id => + { + ControlFlow::Break(()) + }, + _ => ControlFlow::Continue(()), + }) + .is_some() + }); + + (!has_problematic_control_flow).then_some(stmts_before_break) +} diff --git a/tests/ui/while_let_loop.rs b/tests/ui/while_let_loop.rs index 0100be4ec137..dc7df017a609 100644 --- a/tests/ui/while_let_loop.rs +++ b/tests/ui/while_let_loop.rs @@ -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; @@ -71,7 +71,7 @@ fn main() { } loop { - // no error, else branch does something other than break + // no error, hoisting from match arms isn't supported match y { Some(_x) => true, _ => { @@ -255,18 +255,8 @@ 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::) { - // println!("x = {x}"); - // } - // println!("fail"); - // ``` loop { + //~^ while_let_loop let Some(x) = std::hint::black_box(None::) else { println!("fail"); break; @@ -274,3 +264,160 @@ fn issue16378() { println!("x = {x}"); } } + +fn 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 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 hoist_with_return() -> Option { + loop { + //~^ while_let_loop + let Some(x) = std::hint::black_box(None::) else { + if true { + return None; + } + break; + }; + println!("x = {x}"); + } + Some(42) +} + +fn hoist_with_labeled_break() { + 'outer: loop { + loop { + //~^ while_let_loop + let Some(x) = std::hint::black_box(None::) else { + if true { + break 'outer; + } + break; + }; + println!("x = {x}"); + } + } +} + +fn hoist_with_labeled_continue() { + 'outer: loop { + loop { + //~^ while_let_loop + let Some(x) = std::hint::black_box(None::) else { + if true { + continue 'outer; + } + break; + }; + println!("x = {x}"); + } + break; + } +} + +fn hoist_with_label_on_transformed_loop() { + let y = Some(true); + 'my_loop: loop { + //~^ while_let_loop + let Some(x) = y else { + println!("done"); + break; + }; + println!("x = {x}"); + } +} + +fn no_hoist_break_targets_transformed_loop() { + // Should NOT lint: hoisted stmt contains a break targeting the loop being transformed + loop { + let Some(x) = std::hint::black_box(None::) else { + if true { + break; + } + println!("msg"); + break; + }; + println!("x = {x}"); + } +} + +fn no_hoist_continue_targets_transformed_loop() { + // no error, unlabeled continue targets the loop being transformed + loop { + let Some(x) = std::hint::black_box(None::) else { + if true { + continue; + } + break; + }; + println!("x = {x}"); + } +} + +fn no_hoist_labeled_break_targets_transformed_loop() { + // no error, labeled break targets the loop being transformed + 'my_loop: loop { + let Some(x) = std::hint::black_box(None::) else { + if true { + break 'my_loop; + } + break; + }; + println!("x = {x}"); + } +} + +fn no_hoist_break_with_value() { + // no error, break with a value is not a simple break + let _result = loop { + let Some(x) = std::hint::black_box(None::) else { + break 42; + }; + println!("x = {x}"); + }; +} + +fn hoist_with_nested_inner_loop() { + loop { + //~^ while_let_loop + let Some(x) = std::hint::black_box(None::) else { + for i in 0..3 { + if i == 1 { + break; + } + println!("{i}"); + } + break; + }; + println!("x = {x}"); + } +} diff --git a/tests/ui/while_let_loop.stderr b/tests/ui/while_let_loop.stderr index b9aee6eb42ec..cda2473151ee 100644 --- a/tests/ui/while_let_loop.stderr +++ b/tests/ui/while_let_loop.stderr @@ -19,7 +19,34 @@ LL | / loop { LL | | LL | | let Some(_x) = y else { break }; LL | | } - | |_____^ help: try: `while let Some(_x) = y { .. }` + | |_____^ + | +help: try + | +LL - loop { +LL - +LL - let Some(_x) = y else { break }; +LL - } +LL + while let Some(_x) = y { .. } + | + +error: this loop could be written as a `while let` loop + --> tests/ui/while_let_loop.rs:30:5 + | +LL | / loop { +LL | | +LL | | let Some(_x) = y else { +LL | | let _z = 1; +LL | | break; +LL | | }; +LL | | } + | |_____^ + | +help: try + | +LL ~ while let Some(_x) = y { .. } +LL + let _z = 1; + | error: this loop could be written as a `while let` loop --> tests/ui/while_let_loop.rs:38:5 @@ -186,5 +213,170 @@ LL + .. LL + } | -error: aborting due to 12 previous errors +error: this loop could be written as a `while let` loop + --> tests/ui/while_let_loop.rs:258:5 + | +LL | / loop { +LL | | +LL | | let Some(x) = std::hint::black_box(None::) else { +LL | | println!("fail"); +... | +LL | | println!("x = {x}"); +LL | | } + | |_____^ + | +help: try + | +LL ~ while let Some(x) = std::hint::black_box(None::) { .. } +LL + println!("fail"); + | + +error: this loop could be written as a `while let` loop + --> tests/ui/while_let_loop.rs:270:5 + | +LL | / loop { +LL | | +LL | | let Some(x) = y else { +LL | | let a = 1; +... | +LL | | println!("x = {x}"); +LL | | } + | |_____^ + | +help: try + | +LL ~ while let Some(x) = y { .. } +LL + let a = 1; +LL + let b = 2; +LL + let _c = a + b; +LL + println!("sum: {}", a + b); +LL + eprintln!("failed"); + | + +error: this loop could be written as a `while let` loop + --> tests/ui/while_let_loop.rs:286:5 + | +LL | / loop { +LL | | +LL | | let Some(x) = y else { +LL | | if std::hint::black_box(true) { +... | +LL | | println!("x = {x}"); +LL | | } + | |_____^ + | +help: try + | +LL ~ while let Some(x) = y { .. } +LL + if std::hint::black_box(true) { +LL + println!("pass"); +LL + } +LL + match 42 { +LL + 0 => println!("zero"), +LL + _ => println!("non-zero"), +LL + } + | + +error: this loop could be written as a `while let` loop + --> tests/ui/while_let_loop.rs:303:5 + | +LL | / loop { +LL | | +LL | | let Some(x) = std::hint::black_box(None::) else { +LL | | if true { +... | +LL | | println!("x = {x}"); +LL | | } + | |_____^ + | +help: try + | +LL ~ while let Some(x) = std::hint::black_box(None::) { .. } +LL + if true { +LL + return None; +LL + } + | + +error: this loop could be written as a `while let` loop + --> tests/ui/while_let_loop.rs:318:9 + | +LL | / loop { +LL | | +LL | | let Some(x) = std::hint::black_box(None::) else { +LL | | if true { +... | +LL | | println!("x = {x}"); +LL | | } + | |_________^ + | +help: try + | +LL ~ while let Some(x) = std::hint::black_box(None::) { .. } +LL + if true { +LL + break 'outer; +LL + } + | + +error: this loop could be written as a `while let` loop + --> tests/ui/while_let_loop.rs:333:9 + | +LL | / loop { +LL | | +LL | | let Some(x) = std::hint::black_box(None::) else { +LL | | if true { +... | +LL | | println!("x = {x}"); +LL | | } + | |_________^ + | +help: try + | +LL ~ while let Some(x) = std::hint::black_box(None::) { .. } +LL + if true { +LL + continue 'outer; +LL + } + | + +error: this loop could be written as a `while let` loop + --> tests/ui/while_let_loop.rs:349:5 + | +LL | / 'my_loop: loop { +LL | | +LL | | let Some(x) = y else { +LL | | println!("done"); +... | +LL | | println!("x = {x}"); +LL | | } + | |_____^ + | +help: try + | +LL ~ 'my_loop: while let Some(x) = y { .. } +LL + println!("done"); + | + +error: this loop could be written as a `while let` loop + --> tests/ui/while_let_loop.rs:410:5 + | +LL | / loop { +LL | | +LL | | let Some(x) = std::hint::black_box(None::) else { +LL | | for i in 0..3 { +... | +LL | | println!("x = {x}"); +LL | | } + | |_____^ + | +help: try + | +LL ~ while let Some(x) = std::hint::black_box(None::) { .. } +LL + for i in 0..3 { +LL + if i == 1 { +LL + break; +LL + } +LL + println!("{i}"); +LL + } + | + +error: aborting due to 21 previous errors