Fix `never_loop` forget to remove `break` in suggestion (#15064)

Closes rust-lang/rust-clippy#15007

Removes the `break` stmt and the dead code after it when appropriate.

changelog: [`never_loop`] Make sure to remove `break` in suggestions

3 files changed, 210 insertions, 32 deletions

diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs
index 69c84bc7038..8a253ae5810 100644
--- a/clippy_lints/src/loops/never_loop.rs
+++ b/clippy_lints/src/loops/never_loop.rs
@@ -6,9 +6,11 @@ use clippy_utils::macros::root_macro_call_first_node;
 use clippy_utils::source::snippet;
 use clippy_utils::visitors::{Descend, for_each_expr_without_closures};
 use rustc_errors::Applicability;
-use rustc_hir::{Block, Destination, Expr, ExprKind, HirId, InlineAsmOperand, Pat, Stmt, StmtKind, StructTailExpr};
+use rustc_hir::{
+    Block, Destination, Expr, ExprKind, HirId, InlineAsmOperand, Node, Pat, Stmt, StmtKind, StructTailExpr,
+};
 use rustc_lint::LateContext;
-use rustc_span::{Span, sym};
+use rustc_span::{BytePos, Span, sym};
 use std::iter::once;
 use std::ops::ControlFlow;
 
@@ -20,7 +22,7 @@ pub(super) fn check<'tcx>(
     for_loop: Option<&ForLoop<'_>>,
 ) {
     match never_loop_block(cx, block, &mut Vec::new(), loop_id) {
-        NeverLoopResult::Diverging => {
+        NeverLoopResult::Diverging { ref break_spans } => {
             span_lint_and_then(cx, NEVER_LOOP, span, "this loop never actually loops", |diag| {
                 if let Some(ForLoop {
                     arg: iterator,
@@ -38,10 +40,15 @@ pub(super) fn check<'tcx>(
                         Applicability::Unspecified
                     };
 
-                    diag.span_suggestion_verbose(
+                    let mut suggestions = vec![(
                         for_span.with_hi(iterator.span.hi()),
-                        "if you need the first element of the iterator, try writing",
                         for_to_if_let_sugg(cx, iterator, pat),
+                    )];
+                    // Make sure to clear up the diverging sites when we remove a loopp.
+                    suggestions.extend(break_spans.iter().map(|span| (*span, String::new())));
+                    diag.multipart_suggestion_verbose(
+                        "if you need the first element of the iterator, try writing",
+                        suggestions,
                         app,
                     );
                 }
@@ -70,22 +77,22 @@ fn contains_any_break_or_continue(block: &Block<'_>) -> bool {
 /// The first two bits of information are in this enum, and the last part is in the
 /// `local_labels` variable, which contains a list of `(block_id, reachable)` pairs ordered by
 /// scope.
-#[derive(Copy, Clone)]
+#[derive(Clone)]
 enum NeverLoopResult {
     /// A continue may occur for the main loop.
     MayContinueMainLoop,
     /// We have not encountered any main loop continue,
     /// but we are diverging (subsequent control flow is not reachable)
-    Diverging,
+    Diverging { break_spans: Vec<Span> },
     /// We have not encountered any main loop continue,
     /// and subsequent control flow is (possibly) reachable
     Normal,
 }
 
 #[must_use]
-fn absorb_break(arg: NeverLoopResult) -> NeverLoopResult {
+fn absorb_break(arg: &NeverLoopResult) -> NeverLoopResult {
     match arg {
-        NeverLoopResult::Diverging | NeverLoopResult::Normal => NeverLoopResult::Normal,
+        NeverLoopResult::Diverging { .. } | NeverLoopResult::Normal => NeverLoopResult::Normal,
         NeverLoopResult::MayContinueMainLoop => NeverLoopResult::MayContinueMainLoop,
     }
 }
@@ -94,7 +101,7 @@ fn absorb_break(arg: NeverLoopResult) -> NeverLoopResult {
 #[must_use]
 fn combine_seq(first: NeverLoopResult, second: impl FnOnce() -> NeverLoopResult) -> NeverLoopResult {
     match first {
-        NeverLoopResult::Diverging | NeverLoopResult::MayContinueMainLoop => first,
+        NeverLoopResult::Diverging { .. } | NeverLoopResult::MayContinueMainLoop => first,
         NeverLoopResult::Normal => second(),
     }
 }
@@ -103,7 +110,7 @@ fn combine_seq(first: NeverLoopResult, second: impl FnOnce() -> NeverLoopResult)
 #[must_use]
 fn combine_seq_many(iter: impl IntoIterator<Item = NeverLoopResult>) -> NeverLoopResult {
     for e in iter {
-        if let NeverLoopResult::Diverging | NeverLoopResult::MayContinueMainLoop = e {
+        if let NeverLoopResult::Diverging { .. } | NeverLoopResult::MayContinueMainLoop = e {
             return e;
         }
     }
@@ -118,7 +125,19 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult
             NeverLoopResult::MayContinueMainLoop
         },
         (NeverLoopResult::Normal, _) | (_, NeverLoopResult::Normal) => NeverLoopResult::Normal,
-        (NeverLoopResult::Diverging, NeverLoopResult::Diverging) => NeverLoopResult::Diverging,
+        (
+            NeverLoopResult::Diverging {
+                break_spans: mut break_spans1,
+            },
+            NeverLoopResult::Diverging {
+                break_spans: mut break_spans2,
+            },
+        ) => {
+            break_spans1.append(&mut break_spans2);
+            NeverLoopResult::Diverging {
+                break_spans: break_spans1,
+            }
+        },
     }
 }
 
@@ -136,7 +155,7 @@ fn never_loop_block<'tcx>(
     combine_seq_many(iter.map(|(e, els)| {
         let e = never_loop_expr(cx, e, local_labels, main_loop_id);
         // els is an else block in a let...else binding
-        els.map_or(e, |els| {
+        els.map_or(e.clone(), |els| {
             combine_seq(e, || match never_loop_block(cx, els, local_labels, main_loop_id) {
                 // Returning MayContinueMainLoop here means that
                 // we will not evaluate the rest of the body
@@ -144,7 +163,7 @@ fn never_loop_block<'tcx>(
                 // An else block always diverges, so the Normal case should not happen,
                 // but the analysis is approximate so it might return Normal anyway.
                 // Returning Normal here says that nothing more happens on the main path
-                NeverLoopResult::Diverging | NeverLoopResult::Normal => NeverLoopResult::Normal,
+                NeverLoopResult::Diverging { .. } | NeverLoopResult::Normal => NeverLoopResult::Normal,
             })
         })
     }))
@@ -159,6 +178,45 @@ fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<(&'tcx Expr<'tcx>, Option<&'t
     }
 }
 
+fn stmt_source_span(stmt: &Stmt<'_>) -> Span {
+    let call_span = stmt.span.source_callsite();
+    // if it is a macro call, the span will be missing the trailing semicolon
+    if stmt.span == call_span {
+        return call_span;
+    }
+
+    // An expression without a trailing semi-colon (must have unit type).
+    if let StmtKind::Expr(..) = stmt.kind {
+        return call_span;
+    }
+
+    call_span.with_hi(call_span.hi() + BytePos(1))
+}
+
+/// Returns a Vec of all the individual spans after the highlighted expression in a block
+fn all_spans_after_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> Vec<Span> {
+    if let Node::Stmt(stmt) = cx.tcx.parent_hir_node(expr.hir_id) {
+        if let Node::Block(block) = cx.tcx.parent_hir_node(stmt.hir_id) {
+            return block
+                .stmts
+                .iter()
+                .skip_while(|inner| inner.hir_id != stmt.hir_id)
+                .map(stmt_source_span)
+                .chain(if let Some(e) = block.expr { vec![e.span] } else { vec![] })
+                .collect();
+        }
+
+        return vec![stmt.span];
+    }
+
+    vec![]
+}
+
+fn is_label_for_block(cx: &LateContext<'_>, dest: &Destination) -> bool {
+    dest.target_id
+        .is_ok_and(|hir_id| matches!(cx.tcx.hir_node(hir_id), Node::Block(_)))
+}
+
 #[allow(clippy::too_many_lines)]
 fn never_loop_expr<'tcx>(
     cx: &LateContext<'tcx>,
@@ -197,7 +255,7 @@ fn never_loop_expr<'tcx>(
         ExprKind::Loop(b, _, _, _) => {
             // We don't attempt to track reachability after a loop,
             // just assume there may have been a break somewhere
-            absorb_break(never_loop_block(cx, b, local_labels, main_loop_id))
+            absorb_break(&never_loop_block(cx, b, local_labels, main_loop_id))
         },
         ExprKind::If(e, e2, e3) => {
             let e1 = never_loop_expr(cx, e, local_labels, main_loop_id);
@@ -212,9 +270,10 @@ fn never_loop_expr<'tcx>(
         ExprKind::Match(e, arms, _) => {
             let e = never_loop_expr(cx, e, local_labels, main_loop_id);
             combine_seq(e, || {
-                arms.iter().fold(NeverLoopResult::Diverging, |a, b| {
-                    combine_branches(a, never_loop_expr(cx, b.body, local_labels, main_loop_id))
-                })
+                arms.iter()
+                    .fold(NeverLoopResult::Diverging { break_spans: vec![] }, |a, b| {
+                        combine_branches(a, never_loop_expr(cx, b.body, local_labels, main_loop_id))
+                    })
             })
         },
         ExprKind::Block(b, _) => {
@@ -224,7 +283,7 @@ fn never_loop_expr<'tcx>(
             let ret = never_loop_block(cx, b, local_labels, main_loop_id);
             let jumped_to = b.targeted_by_break && local_labels.pop().unwrap().1;
             match ret {
-                NeverLoopResult::Diverging if jumped_to => NeverLoopResult::Normal,
+                NeverLoopResult::Diverging { .. } if jumped_to => NeverLoopResult::Normal,
                 _ => ret,
             }
         },
@@ -235,25 +294,39 @@ fn never_loop_expr<'tcx>(
             if id == main_loop_id {
                 NeverLoopResult::MayContinueMainLoop
             } else {
-                NeverLoopResult::Diverging
+                NeverLoopResult::Diverging {
+                    break_spans: all_spans_after_expr(cx, expr),
+                }
             }
         },
-        ExprKind::Break(_, e) | ExprKind::Ret(e) => {
+        ExprKind::Ret(e) => {
             let first = e.as_ref().map_or(NeverLoopResult::Normal, |e| {
                 never_loop_expr(cx, e, local_labels, main_loop_id)
             });
             combine_seq(first, || {
                 // checks if break targets a block instead of a loop
-                if let ExprKind::Break(Destination { target_id: Ok(t), .. }, _) = expr.kind
-                    && let Some((_, reachable)) = local_labels.iter_mut().find(|(label, _)| *label == t)
-                {
-                    *reachable = true;
+                mark_block_as_reachable(expr, local_labels);
+                NeverLoopResult::Diverging { break_spans: vec![] }
+            })
+        },
+        ExprKind::Break(dest, e) => {
+            let first = e.as_ref().map_or(NeverLoopResult::Normal, |e| {
+                never_loop_expr(cx, e, local_labels, main_loop_id)
+            });
+            combine_seq(first, || {
+                // checks if break targets a block instead of a loop
+                mark_block_as_reachable(expr, local_labels);
+                NeverLoopResult::Diverging {
+                    break_spans: if is_label_for_block(cx, &dest) {
+                        vec![]
+                    } else {
+                        all_spans_after_expr(cx, expr)
+                    },
                 }
-                NeverLoopResult::Diverging
             })
         },
         ExprKind::Become(e) => combine_seq(never_loop_expr(cx, e, local_labels, main_loop_id), || {
-            NeverLoopResult::Diverging
+            NeverLoopResult::Diverging { break_spans: vec![] }
         }),
         ExprKind::InlineAsm(asm) => combine_seq_many(asm.operands.iter().map(|(o, _)| match o {
             InlineAsmOperand::In { expr, .. } | InlineAsmOperand::InOut { expr, .. } => {
@@ -283,12 +356,12 @@ fn never_loop_expr<'tcx>(
     };
     let result = combine_seq(result, || {
         if cx.typeck_results().expr_ty(expr).is_never() {
-            NeverLoopResult::Diverging
+            NeverLoopResult::Diverging { break_spans: vec![] }
         } else {
             NeverLoopResult::Normal
         }
     });
-    if let NeverLoopResult::Diverging = result
+    if let NeverLoopResult::Diverging { .. } = result
         && let Some(macro_call) = root_macro_call_first_node(cx, expr)
         && let Some(sym::todo_macro) = cx.tcx.get_diagnostic_name(macro_call.def_id)
     {
@@ -316,3 +389,11 @@ fn for_to_if_let_sugg(cx: &LateContext<'_>, iterator: &Expr<'_>, pat: &Pat<'_>)
 
     format!("if let Some({pat_snippet}) = {iter_snippet}.next()")
 }
+
+fn mark_block_as_reachable(expr: &Expr<'_>, local_labels: &mut [(HirId, bool)]) {
+    if let ExprKind::Break(Destination { target_id: Ok(t), .. }, _) = expr.kind
+        && let Some((_, reachable)) = local_labels.iter_mut().find(|(label, _)| *label == t)
+    {
+        *reachable = true;
+    }
+}
diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs
index e0f54ef899b..48d4b8ad151 100644
--- a/tests/ui/never_loop.rs
+++ b/tests/ui/never_loop.rs
@@ -466,3 +466,35 @@ fn main() {
     test13();
     test14();
 }
+
+fn issue15059() {
+    'a: for _ in 0..1 {
+        //~^ never_loop
+        break 'a;
+    }
+
+    let mut b = 1;
+    'a: for i in 0..1 {
+        //~^ never_loop
+        match i {
+            0 => {
+                b *= 2;
+                break 'a;
+            },
+            x => {
+                b += x;
+                break 'a;
+            },
+        }
+    }
+
+    #[allow(clippy::unused_unit)]
+    for v in 0..10 {
+        //~^ never_loop
+        break;
+        println!("{v}");
+        // This is comment and should be kept
+        println!("This is a comment");
+        ()
+    }
+}
diff --git a/tests/ui/never_loop.stderr b/tests/ui/never_loop.stderr
index bc9a7ec48b4..54b463266a3 100644
--- a/tests/ui/never_loop.stderr
+++ b/tests/ui/never_loop.stderr
@@ -176,8 +176,10 @@ LL | |     }
    |
 help: if you need the first element of the iterator, try writing
    |
-LL -     for v in 0..10 {
-LL +     if let Some(v) = (0..10).next() {
+LL ~     if let Some(v) = (0..10).next() {
+LL |
+LL ~         
+LL ~         
    |
 
 error: this loop never actually loops
@@ -232,5 +234,68 @@ LL | |             break 'inner;
 LL | |         }
    | |_________^
 
-error: aborting due to 21 previous errors
+error: this loop never actually loops
+  --> tests/ui/never_loop.rs:471:5
+   |
+LL | /     'a: for _ in 0..1 {
+LL | |
+LL | |         break 'a;
+LL | |     }
+   | |_____^
+   |
+help: if you need the first element of the iterator, try writing
+   |
+LL ~     if let Some(_) = (0..1).next() {
+LL |
+LL ~         
+   |
+
+error: this loop never actually loops
+  --> tests/ui/never_loop.rs:477:5
+   |
+LL | /     'a: for i in 0..1 {
+LL | |
+LL | |         match i {
+LL | |             0 => {
+...  |
+LL | |     }
+   | |_____^
+   |
+help: if you need the first element of the iterator, try writing
+   |
+LL ~     if let Some(i) = (0..1).next() {
+LL |
+...
+LL |                 b *= 2;
+LL ~                 
+LL |             },
+LL |             x => {
+LL |                 b += x;
+LL ~                 
+   |
+
+error: this loop never actually loops
+  --> tests/ui/never_loop.rs:492:5
+   |
+LL | /     for v in 0..10 {
+LL | |
+LL | |         break;
+LL | |         println!("{v}");
+...  |
+LL | |         ()
+LL | |     }
+   | |_____^
+   |
+help: if you need the first element of the iterator, try writing
+   |
+LL ~     if let Some(v) = (0..10).next() {
+LL |
+LL ~         
+LL ~         
+LL |         // This is comment and should be kept
+LL ~         
+LL ~         
+   |
+
+error: aborting due to 24 previous errors