fix: needless_for_each suggests wrongly with explicit closure input types (rust-lang#15595)

Fixes rust-lang#9912

changelog: [`needless_for_each`]: fix wrong suggestion with explicit
closure input types

Author: dswij

clippy_lints/src/needless_for_each.rs

@@ -1,6 +1,6 @@
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::{Visitor, walk_expr};
-use rustc_hir::{Block, BlockCheckMode, Closure, Expr, ExprKind, Stmt, StmtKind};
+use rustc_hir::{Block, BlockCheckMode, Closure, Expr, ExprKind, Stmt, StmtKind, TyKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
 use rustc_span::Span;
@@ -70,12 +70,24 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessForEach {
             && has_iter_method(cx, cx.typeck_results().expr_ty(iter_recv)).is_some()
             // Skip the lint if the body is not block because this is simpler than `for` loop.
             // e.g. `v.iter().for_each(f)` is simpler and clearer than using `for` loop.
-            && let ExprKind::Closure(&Closure { body, .. }) = for_each_arg.kind
+            && let ExprKind::Closure(&Closure { body, fn_decl, .. }) = for_each_arg.kind
             && 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.
             && !matches!(body.value.kind, ExprKind::Block(Block { rules: BlockCheckMode::UnsafeBlock(_), .. }, ..))
         {
+            let mut applicability = Applicability::MachineApplicable;
+
+            // If any closure parameter has an explicit type specified, applying the lint would necessarily
+            // remove that specification, possibly breaking type inference
+            if fn_decl
+                .inputs
+                .iter()
+                .any(|input| matches!(input.kind, TyKind::Infer(..)))
+            {
+                applicability = Applicability::MaybeIncorrect;
+            }
+
             let mut ret_collector = RetCollector::default();
             ret_collector.visit_expr(body.value);
 
@@ -84,18 +96,16 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessForEach {
                 return;
             }
 
-            let (mut applicability, ret_suggs) = if ret_collector.spans.is_empty() {
-                (Applicability::MachineApplicable, None)
+            let ret_suggs = if ret_collector.spans.is_empty() {
+                None
             } else {
-                (
-                    Applicability::MaybeIncorrect,
-                    Some(
-                        ret_collector
-                            .spans
-                            .into_iter()
-                            .map(|span| (span, "continue".to_string()))
-                            .collect(),
-                    ),
+                applicability = Applicability::MaybeIncorrect;
+                Some(
+                    ret_collector
+                        .spans
+                        .into_iter()
+                        .map(|span| (span, "continue".to_string()))
+                        .collect(),
                 )
             };
 

tests/ui/needless_for_each_unfixable.rs

@@ -1,6 +1,6 @@
 //@no-rustfix: overlapping suggestions
 #![warn(clippy::needless_for_each)]
-#![allow(clippy::needless_return, clippy::uninlined_format_args)]
+#![allow(clippy::needless_return)]
 
 fn main() {
     let v: Vec<i32> = Vec::new();
@@ -11,7 +11,22 @@ fn main() {
         if *v == 10 {
             return;
         } else {
-            println!("{}", v);
+            println!("{v}");
         }
     });
 }
+
+fn issue9912() {
+    let mut i = 0;
+    // Changing this to a `for` loop would break type inference
+    [].iter().for_each(move |_: &i32| {
+        //~^ needless_for_each
+        i += 1;
+    });
+
+    // Changing this would actually be okay, but we still suggest `MaybeIncorrect`ly
+    [1i32].iter().for_each(move |_: &i32| {
+        //~^ needless_for_each
+        i += 1;
+    });
+}

tests/ui/needless_for_each_unfixable.stderr

@@ -19,7 +19,7 @@ LL +
 LL +         if *v == 10 {
 LL +             return;
 LL +         } else {
-LL +             println!("{}", v);
+LL +             println!("{v}");
 LL +         }
 LL +     }
    |
@@ -29,5 +29,39 @@ LL -             return;
 LL +             continue;
    |
 
-error: aborting due to 1 previous error
+error: needless use of `for_each`
+  --> tests/ui/needless_for_each_unfixable.rs:22:5
+   |
+LL | /     [].iter().for_each(move |_: &i32| {
+LL | |
+LL | |         i += 1;
+LL | |     });
+   | |_______^
+   |
+help: try
+   |
+LL ~     for _ in [].iter() {
+LL +
+LL +         i += 1;
+LL +     }
+   |
+
+error: needless use of `for_each`
+  --> tests/ui/needless_for_each_unfixable.rs:28:5
+   |
+LL | /     [1i32].iter().for_each(move |_: &i32| {
+LL | |
+LL | |         i += 1;
+LL | |     });
+   | |_______^
+   |
+help: try
+   |
+LL ~     for _ in [1i32].iter() {
+LL +
+LL +         i += 1;
+LL +     }
+   |
+
+error: aborting due to 3 previous errors