fix: `needless_for_each` suggests wrongly when closure has no braces

4 files changed, 71 insertions, 5 deletions

diff --git a/clippy_lints/src/needless_for_each.rs b/clippy_lints/src/needless_for_each.rs
index 7dd96f1f037..6a7c8436bad 100644
--- a/clippy_lints/src/needless_for_each.rs
+++ b/clippy_lints/src/needless_for_each.rs
@@ -74,7 +74,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessForEach {
             && let body = cx.tcx.hir_body(body)
             // Skip the lint if the body is not safe, so as not to suggest `for … in … unsafe {}`
             // and suggesting `for … in … { unsafe { } }` is a little ugly.
-            && let ExprKind::Block(Block { rules: BlockCheckMode::DefaultBlock, .. }, ..) = body.value.kind
+            && !matches!(body.value.kind, ExprKind::Block(Block { rules: BlockCheckMode::UnsafeBlock(_), .. }, ..))
         {
             let mut ret_collector = RetCollector::default();
             ret_collector.visit_expr(body.value);
@@ -99,11 +99,21 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessForEach {
                 )
             };
 
+            let body_param_sugg = snippet_with_applicability(cx, body.params[0].pat.span, "..", &mut applicability);
+            let for_each_rev_sugg = snippet_with_applicability(cx, for_each_recv.span, "..", &mut applicability);
+            let body_value_sugg = snippet_with_applicability(cx, body.value.span, "..", &mut applicability);
+
             let sugg = format!(
                 "for {} in {} {}",
-                snippet_with_applicability(cx, body.params[0].pat.span, "..", &mut applicability),
-                snippet_with_applicability(cx, for_each_recv.span, "..", &mut applicability),
-                snippet_with_applicability(cx, body.value.span, "..", &mut applicability),
+                body_param_sugg,
+                for_each_rev_sugg,
+                match body.value.kind {
+                    ExprKind::Block(block, _) if is_let_desugar(block) => {
+                        format!("{{ {body_value_sugg} }}")
+                    },
+                    ExprKind::Block(_, _) => body_value_sugg.to_string(),
+                    _ => format!("{{ {body_value_sugg}; }}"),
+                }
             );
 
             span_lint_and_then(cx, NEEDLESS_FOR_EACH, stmt.span, "needless use of `for_each`", |diag| {
@@ -116,6 +126,20 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessForEach {
     }
 }
 
+/// Check if the block is a desugared `_ = expr` statement.
+fn is_let_desugar(block: &Block<'_>) -> bool {
+    matches!(
+        block,
+        Block {
+            stmts: [Stmt {
+                kind: StmtKind::Let(_),
+                ..
+            },],
+            ..
+        }
+    )
+}
+
 /// This type plays two roles.
 /// 1. Collect spans of `return` in the closure body.
 /// 2. Detect use of `return` in `Loop` in the closure body.
diff --git a/tests/ui/needless_for_each_fixable.fixed b/tests/ui/needless_for_each_fixable.fixed
index fa23e18318f..a73aff55639 100644
--- a/tests/ui/needless_for_each_fixable.fixed
+++ b/tests/ui/needless_for_each_fixable.fixed
@@ -128,3 +128,18 @@ fn should_not_lint() {
 }
 
 fn main() {}
+
+mod issue14734 {
+    fn let_desugar(rows: &[u8]) {
+        let mut v = vec![];
+        for x in rows.iter() { _ = v.push(x) }
+        //~^ needless_for_each
+    }
+
+    fn do_something(_: &u8, _: u8) {}
+
+    fn single_expr(rows: &[u8]) {
+        for x in rows.iter() { do_something(x, 1u8); }
+        //~^ needless_for_each
+    }
+}
diff --git a/tests/ui/needless_for_each_fixable.rs b/tests/ui/needless_for_each_fixable.rs
index 2c7e68a6f51..d92f055d3f4 100644
--- a/tests/ui/needless_for_each_fixable.rs
+++ b/tests/ui/needless_for_each_fixable.rs
@@ -128,3 +128,18 @@ fn should_not_lint() {
 }
 
 fn main() {}
+
+mod issue14734 {
+    fn let_desugar(rows: &[u8]) {
+        let mut v = vec![];
+        rows.iter().for_each(|x| _ = v.push(x));
+        //~^ needless_for_each
+    }
+
+    fn do_something(_: &u8, _: u8) {}
+
+    fn single_expr(rows: &[u8]) {
+        rows.iter().for_each(|x| do_something(x, 1u8));
+        //~^ needless_for_each
+    }
+}
diff --git a/tests/ui/needless_for_each_fixable.stderr b/tests/ui/needless_for_each_fixable.stderr
index 013a3fa3e36..f8014456097 100644
--- a/tests/ui/needless_for_each_fixable.stderr
+++ b/tests/ui/needless_for_each_fixable.stderr
@@ -136,5 +136,17 @@ LL +         acc += elem;
 LL +     }
    |
 
-error: aborting due to 8 previous errors
+error: needless use of `for_each`
+  --> tests/ui/needless_for_each_fixable.rs:135:9
+   |
+LL |         rows.iter().for_each(|x| _ = v.push(x));
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x in rows.iter() { _ = v.push(x) }`
+
+error: needless use of `for_each`
+  --> tests/ui/needless_for_each_fixable.rs:142:9
+   |
+LL |         rows.iter().for_each(|x| do_something(x, 1u8));
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x in rows.iter() { do_something(x, 1u8); }`
+
+error: aborting due to 10 previous errors