make `never_loop` applicability more flexible

6 files changed, 193 insertions, 5 deletions

diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs
index b679fdfadc3..3f8de774eef 100644
--- a/clippy_lints/src/loops/never_loop.rs
+++ b/clippy_lints/src/loops/never_loop.rs
@@ -4,11 +4,13 @@ use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::higher::ForLoop;
 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_lint::LateContext;
 use rustc_span::{Span, sym};
 use std::iter::once;
+use std::ops::ControlFlow;
 
 pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,
@@ -24,17 +26,23 @@ pub(super) fn check<'tcx>(
                     arg: iterator,
                     pat,
                     span: for_span,
+                    label,
                     ..
                 }) = for_loop
                 {
-                    // Suggests using an `if let` instead. This is `Unspecified` because the
-                    // loop may (probably) contain `break` statements which would be invalid
-                    // in an `if let`.
+                    // If the block contains a break or continue, or if the loop has a label, `MachineApplicable` is not
+                    // appropriate.
+                    let app = if !contains_any_break_or_continue(block) && label.is_none() {
+                        Applicability::MachineApplicable
+                    } else {
+                        Applicability::Unspecified
+                    };
+
                     diag.span_suggestion_verbose(
                         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),
-                        Applicability::Unspecified,
+                        app,
                     );
                 }
             });
@@ -43,6 +51,15 @@ pub(super) fn check<'tcx>(
     }
 }
 
+fn contains_any_break_or_continue(block: &Block<'_>) -> bool {
+    for_each_expr_without_closures(block, |e| match e.kind {
+        ExprKind::Break(..) | ExprKind::Continue(..) => ControlFlow::Break(()),
+        ExprKind::Loop(..) => ControlFlow::Continue(Descend::No),
+        _ => ControlFlow::Continue(Descend::Yes),
+    })
+    .is_some()
+}
+
 /// The `never_loop` analysis keeps track of three things:
 ///
 /// * Has any (reachable) code path hit a `continue` of the main loop?
diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs
index 2d8e04c192e..e0f54ef899b 100644
--- a/tests/ui/never_loop.rs
+++ b/tests/ui/never_loop.rs
@@ -422,6 +422,34 @@ pub fn issue12205() -> Option<()> {
     }
 }
 
+fn stmt_after_return() {
+    for v in 0..10 {
+        //~^ never_loop
+        break;
+        println!("{v}");
+    }
+}
+
+fn loop_label() {
+    'outer: for v in 0..10 {
+        //~^ never_loop
+        loop {
+            //~^ never_loop
+            break 'outer;
+        }
+        return;
+    }
+
+    for v in 0..10 {
+        //~^ never_loop
+        'inner: loop {
+            //~^ never_loop
+            break 'inner;
+        }
+        return;
+    }
+}
+
 fn main() {
     test1();
     test2();
diff --git a/tests/ui/never_loop.stderr b/tests/ui/never_loop.stderr
index f6d6d109542..bc9a7ec48b4 100644
--- a/tests/ui/never_loop.stderr
+++ b/tests/ui/never_loop.stderr
@@ -164,5 +164,73 @@ LL | |         unimplemented!("not yet");
 LL | |     }
    | |_____^
 
-error: aborting due to 16 previous errors
+error: this loop never actually loops
+  --> tests/ui/never_loop.rs:426:5
+   |
+LL | /     for v in 0..10 {
+LL | |
+LL | |         break;
+LL | |         println!("{v}");
+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() {
+   |
+
+error: this loop never actually loops
+  --> tests/ui/never_loop.rs:434:5
+   |
+LL | /     'outer: for v in 0..10 {
+LL | |
+LL | |         loop {
+...  |
+LL | |         return;
+LL | |     }
+   | |_____^
+   |
+help: if you need the first element of the iterator, try writing
+   |
+LL -     'outer: for v in 0..10 {
+LL +     if let Some(v) = (0..10).next() {
+   |
+
+error: this loop never actually loops
+  --> tests/ui/never_loop.rs:436:9
+   |
+LL | /         loop {
+LL | |
+LL | |             break 'outer;
+LL | |         }
+   | |_________^
+
+error: this loop never actually loops
+  --> tests/ui/never_loop.rs:443:5
+   |
+LL | /     for v in 0..10 {
+LL | |
+LL | |         'inner: loop {
+...  |
+LL | |         return;
+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() {
+   |
+
+error: this loop never actually loops
+  --> tests/ui/never_loop.rs:445:9
+   |
+LL | /         'inner: loop {
+LL | |
+LL | |             break 'inner;
+LL | |         }
+   | |_________^
+
+error: aborting due to 21 previous errors
 
diff --git a/tests/ui/never_loop_fixable.fixed b/tests/ui/never_loop_fixable.fixed
new file mode 100644
index 00000000000..5bc9ff1bb4d
--- /dev/null
+++ b/tests/ui/never_loop_fixable.fixed
@@ -0,0 +1,20 @@
+#![allow(clippy::iter_next_slice, clippy::needless_return)]
+
+fn no_break_or_continue_loop() {
+    if let Some(i) = [1, 2, 3].iter().next() {
+        //~^ never_loop
+        return;
+    }
+}
+
+fn no_break_or_continue_loop_outer() {
+    if let Some(i) = [1, 2, 3].iter().next() {
+        //~^ never_loop
+        return;
+        loop {
+            if true {
+                continue;
+            }
+        }
+    }
+}
diff --git a/tests/ui/never_loop_fixable.rs b/tests/ui/never_loop_fixable.rs
new file mode 100644
index 00000000000..9782bc107e9
--- /dev/null
+++ b/tests/ui/never_loop_fixable.rs
@@ -0,0 +1,20 @@
+#![allow(clippy::iter_next_slice, clippy::needless_return)]
+
+fn no_break_or_continue_loop() {
+    for i in [1, 2, 3].iter() {
+        //~^ never_loop
+        return;
+    }
+}
+
+fn no_break_or_continue_loop_outer() {
+    for i in [1, 2, 3].iter() {
+        //~^ never_loop
+        return;
+        loop {
+            if true {
+                continue;
+            }
+        }
+    }
+}
diff --git a/tests/ui/never_loop_fixable.stderr b/tests/ui/never_loop_fixable.stderr
new file mode 100644
index 00000000000..ee02d9a4297
--- /dev/null
+++ b/tests/ui/never_loop_fixable.stderr
@@ -0,0 +1,35 @@
+error: this loop never actually loops
+  --> tests/ui/never_loop_fixable.rs:4:5
+   |
+LL | /     for i in [1, 2, 3].iter() {
+LL | |
+LL | |         return;
+LL | |     }
+   | |_____^
+   |
+   = note: `#[deny(clippy::never_loop)]` on by default
+help: if you need the first element of the iterator, try writing
+   |
+LL -     for i in [1, 2, 3].iter() {
+LL +     if let Some(i) = [1, 2, 3].iter().next() {
+   |
+
+error: this loop never actually loops
+  --> tests/ui/never_loop_fixable.rs:11:5
+   |
+LL | /     for i in [1, 2, 3].iter() {
+LL | |
+LL | |         return;
+LL | |         loop {
+...  |
+LL | |     }
+   | |_____^
+   |
+help: if you need the first element of the iterator, try writing
+   |
+LL -     for i in [1, 2, 3].iter() {
+LL +     if let Some(i) = [1, 2, 3].iter().next() {
+   |
+
+error: aborting due to 2 previous errors
+