Fix needless_for_each suggests wrongly when closure has no braces (#14735)

Closes #14734

changelog: [`needless_for_each`] wrong suggestions when closure has no
braces

Author: blyxyas

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.

lintcheck/src/output.rs

@@ -162,9 +162,9 @@ pub fn summarize_and_print_changes(
 fn gather_stats(warnings: &[ClippyWarning]) -> (String, HashMap<&String, usize>) {
     // count lint type occurrences
     let mut counter: HashMap<&String, usize> = HashMap::new();
-    warnings
-        .iter()
-        .for_each(|wrn| *counter.entry(&wrn.name).or_insert(0) += 1);
+    for wrn in warnings {
+        *counter.entry(&wrn.name).or_insert(0) += 1;
+    }
 
     // collect into a tupled list for sorting
     let mut stats: Vec<(&&String, &usize)> = counter.iter().collect();

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
+    }
+}

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
+    }
+}

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