Refactor excessive_for_each

13 files changed, 525 insertions, 406 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c35ab6c94bf..a2193f0eab9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2192,7 +2192,6 @@ Released 2018-09-13
 [`eq_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#eq_op
 [`erasing_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#erasing_op
 [`eval_order_dependence`]: https://rust-lang.github.io/rust-clippy/master/index.html#eval_order_dependence
-[`excessive_for_each`]: https://rust-lang.github.io/rust-clippy/master/index.html#excessive_for_each
 [`excessive_precision`]: https://rust-lang.github.io/rust-clippy/master/index.html#excessive_precision
 [`exhaustive_enums`]: https://rust-lang.github.io/rust-clippy/master/index.html#exhaustive_enums
 [`exhaustive_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#exhaustive_structs
@@ -2370,6 +2369,7 @@ Released 2018-09-13
 [`needless_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect
 [`needless_continue`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_continue
 [`needless_doctest_main`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_doctest_main
+[`needless_for_each`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_for_each
 [`needless_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
 [`needless_pass_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value
 [`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark
diff --git a/clippy_lints/src/iter_for_each.rs b/clippy_lints/src/iter_for_each.rs
deleted file mode 100644
index e69de29bb2d..00000000000
--- a/clippy_lints/src/iter_for_each.rs
+++ /dev/null
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 2f2ccdb310a..8c9247d9781 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -291,6 +291,7 @@ mod needless_bool;
 mod needless_borrow;
 mod needless_borrowed_ref;
 mod needless_continue;
+mod needless_for_each;
 mod needless_pass_by_value;
 mod needless_question_mark;
 mod needless_update;
@@ -781,7 +782,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &methods::CLONE_DOUBLE_REF,
         &methods::CLONE_ON_COPY,
         &methods::CLONE_ON_REF_PTR,
-        &methods::EXCESSIVE_FOR_EACH,
         &methods::EXPECT_FUN_CALL,
         &methods::EXPECT_USED,
         &methods::FILETYPE_IS_FILE,
@@ -868,6 +868,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &needless_borrow::NEEDLESS_BORROW,
         &needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE,
         &needless_continue::NEEDLESS_CONTINUE,
+        &needless_for_each::NEEDLESS_FOR_EACH,
         &needless_pass_by_value::NEEDLESS_PASS_BY_VALUE,
         &needless_question_mark::NEEDLESS_QUESTION_MARK,
         &needless_update::NEEDLESS_UPDATE,
@@ -1046,6 +1047,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box ptr_eq::PtrEq);
     store.register_late_pass(|| box needless_bool::NeedlessBool);
     store.register_late_pass(|| box needless_bool::BoolComparison);
+    store.register_late_pass(|| box needless_for_each::NeedlessForEach);
     store.register_late_pass(|| box approx_const::ApproxConstant);
     store.register_late_pass(|| box misc::MiscLints);
     store.register_late_pass(|| box eta_reduction::EtaReduction);
@@ -1314,7 +1316,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&matches::WILDCARD_ENUM_MATCH_ARM),
         LintId::of(&mem_forget::MEM_FORGET),
         LintId::of(&methods::CLONE_ON_REF_PTR),
-        LintId::of(&methods::EXCESSIVE_FOR_EACH),
         LintId::of(&methods::EXPECT_USED),
         LintId::of(&methods::FILETYPE_IS_FILE),
         LintId::of(&methods::GET_UNWRAP),
@@ -1325,6 +1326,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS),
         LintId::of(&missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS),
         LintId::of(&modulo_arithmetic::MODULO_ARITHMETIC),
+        LintId::of(&needless_for_each::NEEDLESS_FOR_EACH),
         LintId::of(&panic_in_result_fn::PANIC_IN_RESULT_FN),
         LintId::of(&panic_unimplemented::PANIC),
         LintId::of(&panic_unimplemented::TODO),
diff --git a/clippy_lints/src/methods/excessive_for_each.rs b/clippy_lints/src/methods/excessive_for_each.rs
deleted file mode 100644
index bcb7cf1491f..00000000000
--- a/clippy_lints/src/methods/excessive_for_each.rs
+++ /dev/null
@@ -1,122 +0,0 @@
-use rustc_errors::Applicability;
-use rustc_hir::{
-    intravisit::{walk_expr, NestedVisitorMap, Visitor},
-    Expr, ExprKind,
-};
-use rustc_lint::LateContext;
-use rustc_middle::hir::map::Map;
-use rustc_span::source_map::Span;
-
-use if_chain::if_chain;
-
-use crate::utils::{has_iter_method, match_trait_method, paths, snippet, span_lint_and_then};
-
-use super::EXCESSIVE_FOR_EACH;
-
-pub(super) fn lint(cx: &LateContext<'_>, expr: &'tcx Expr<'_>, args: &[&[Expr<'_>]]) {
-    if args.len() < 2 {
-        return;
-    }
-
-    let for_each_args = args[0];
-    if for_each_args.len() < 2 {
-        return;
-    }
-    let for_each_receiver = &for_each_args[0];
-    let for_each_arg = &for_each_args[1];
-    let iter_receiver = &args[1][0];
-
-    if_chain! {
-        if has_iter_method(cx, cx.typeck_results().expr_ty(iter_receiver)).is_some();
-        if match_trait_method(cx, expr, &paths::ITERATOR);
-        if let ExprKind::Closure(_, _, body_id, ..) = for_each_arg.kind;
-        let body = cx.tcx.hir().body(body_id);
-        if let ExprKind::Block(..) = body.value.kind;
-        then {
-            let mut ret_collector = RetCollector::new();
-            ret_collector.visit_expr(&body.value);
-
-            // Skip the lint if `return` is used in `Loop` in order not to suggest using `'label`.
-            if ret_collector.ret_in_loop {
-                return;
-            }
-
-            let sugg = format!(
-                "for {} in {} {{ .. }}",
-                snippet(cx, body.params[0].pat.span, ".."),
-                snippet(cx, for_each_receiver.span, "..")
-            );
-
-            span_lint_and_then(
-                cx,
-                EXCESSIVE_FOR_EACH,
-                expr.span,
-                "excessive use of `for_each`",
-                |diag| {
-                    diag.span_suggestion(expr.span, "try", sugg, Applicability::HasPlaceholders);
-                    for span in ret_collector.spans {
-                        diag.span_note(span, "change `return` to `continue` in the loop body");
-                    }
-                }
-            )
-        }
-    }
-}
-
-/// 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.
-///
-/// NOTE: The functionality of this type is similar to
-/// [`crate::utilts::visitors::find_all_ret_expressions`], but we can't use
-/// `find_all_ret_expressions` instead of this type. The reasons are:
-/// 1. `find_all_ret_expressions` passes the argument of `ExprKind::Ret` to a callback, but what we
-///    need here is `ExprKind::Ret` itself.
-/// 2. We can't trace current loop depth with `find_all_ret_expressions`.
-struct RetCollector {
-    spans: Vec<Span>,
-    ret_in_loop: bool,
-
-    loop_depth: u16,
-}
-
-impl RetCollector {
-    fn new() -> Self {
-        Self {
-            spans: Vec::new(),
-            ret_in_loop: false,
-            loop_depth: 0,
-        }
-    }
-}
-
-impl<'tcx> Visitor<'tcx> for RetCollector {
-    type Map = Map<'tcx>;
-
-    fn visit_expr(&mut self, expr: &Expr<'_>) {
-        match expr.kind {
-            ExprKind::Ret(..) => {
-                if self.loop_depth > 0 && !self.ret_in_loop {
-                    self.ret_in_loop = true
-                }
-
-                self.spans.push(expr.span)
-            },
-
-            ExprKind::Loop(..) => {
-                self.loop_depth += 1;
-                walk_expr(self, expr);
-                self.loop_depth -= 1;
-                return;
-            },
-
-            _ => {},
-        }
-
-        walk_expr(self, expr);
-    }
-
-    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
-        NestedVisitorMap::None
-    }
-}
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index f0c99528fe1..fccdee07877 100644
--- a/clippy_lints/src/methods/mod.rs
+++ b/clippy_lints/src/methods/mod.rs
@@ -975,33 +975,6 @@ declare_clippy_lint! {
 }
 
 declare_clippy_lint! {
-    /// **What it does:** Checks for use of `.method(..).for_each(closure)` if the reciever of `.method(..)` doesn't
-    /// implement `Iterator` and the return type of `.method(..)` implements `Iterator`.
-    ///
-    /// **Why is this bad?** Excessive use of `for_each` reduces redability, using `for` loop is
-    /// clearer and more concise.
-    ///
-    /// **Known problems:** None.
-    ///
-    /// **Example:**
-    ///
-    /// ```rust
-    /// let v = vec![0, 1, 2];
-    /// v.iter().for_each(|elem| println!("{}", elem));
-    /// ```
-    /// Use instead:
-    /// ```rust
-    /// let v = vec![0, 1, 2];
-    /// for elem in v.iter() {
-    ///     println!("{}", elem);
-    /// }
-    /// ```
-    pub EXCESSIVE_FOR_EACH,
-    restriction,
-    "using `.iter().for_each(|x| {..})` when using `for` loop would work instead"
-}
-
-declare_clippy_lint! {
     /// **What it does:** Checks for use of `.get().unwrap()` (or
     /// `.get_mut().unwrap`) on a standard library type which implements `Index`
     ///
@@ -1688,7 +1661,6 @@ impl_lint_pass!(Methods => [
     ITER_NTH_ZERO,
     BYTES_NTH,
     ITER_SKIP_NEXT,
-    EXCESSIVE_FOR_EACH,
     GET_UNWRAP,
     STRING_EXTEND_CHARS,
     ITER_CLONED_COLLECT,
@@ -1835,7 +1807,6 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
             ["to_os_string", ..] => implicit_clone::check(cx, expr, sym::OsStr),
             ["to_path_buf", ..] => implicit_clone::check(cx, expr, sym::Path),
             ["to_vec", ..] => implicit_clone::check(cx, expr, sym::slice),
-            ["for_each", ..] => excessive_for_each::lint(cx, expr, &arg_lists),
             _ => {},
         }
 
diff --git a/clippy_lints/src/needless_for_each.rs b/clippy_lints/src/needless_for_each.rs
new file mode 100644
index 00000000000..a7b0a1ca082
--- /dev/null
+++ b/clippy_lints/src/needless_for_each.rs
@@ -0,0 +1,170 @@
+use rustc_errors::Applicability;
+use rustc_hir::{
+    intravisit::{walk_expr, NestedVisitorMap, Visitor},
+    Expr, ExprKind, Stmt, StmtKind,
+};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::hir::map::Map;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::{source_map::Span, sym, Symbol};
+
+use if_chain::if_chain;
+
+use crate::utils::{
+    has_iter_method, is_diagnostic_assoc_item, method_calls, snippet_with_applicability, span_lint_and_then,
+};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `for_each` that would be more simply written as a
+    /// `for` loop.
+    ///
+    /// **Why is this bad?** `for_each` may be used after applying iterator transformers like
+    /// `filter` for better readability and performance. It may also be used to fit a simple
+    /// operation on one line.
+    /// But when none of these apply, a simple `for` loop is more idiomatic.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// let v = vec![0, 1, 2];
+    /// v.iter().for_each(|elem| {
+    ///     println!("{}", elem);
+    /// })
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let v = vec![0, 1, 2];
+    /// for elem in v.iter() {
+    ///     println!("{}", elem);
+    /// }
+    /// ```
+    pub NEEDLESS_FOR_EACH,
+    restriction,
+    "using `for_each` where a `for` loop would be simpler"
+}
+
+declare_lint_pass!(NeedlessForEach => [NEEDLESS_FOR_EACH]);
+
+impl LateLintPass<'_> for NeedlessForEach {
+    fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
+        let expr = match stmt.kind {
+            StmtKind::Expr(expr) | StmtKind::Semi(expr) => expr,
+            StmtKind::Local(local) if local.init.is_some() => local.init.unwrap(),
+            _ => return,
+        };
+
+        // Max depth is set to 3 because we need to check the method chain length is just two.
+        let (method_names, arg_lists, _) = method_calls(expr, 3);
+
+        if_chain! {
+            // This assures the length of this method chain is two.
+            if let [for_each_args, iter_args] = arg_lists.as_slice();
+            if let Some(for_each_sym) = method_names.first();
+            if *for_each_sym == Symbol::intern("for_each");
+            if let Some(did) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
+            if is_diagnostic_assoc_item(cx, did, sym::Iterator);
+            // Checks the type of the first method receiver is NOT a user defined type.
+            if has_iter_method(cx, cx.typeck_results().expr_ty(&iter_args[0])).is_some();
+            if let ExprKind::Closure(_, _, body_id, ..) = for_each_args[1].kind;
+            let body = cx.tcx.hir().body(body_id);
+            // 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.
+            if let ExprKind::Block(..) = body.value.kind;
+            then {
+                let mut ret_collector = RetCollector::default();
+                ret_collector.visit_expr(&body.value);
+
+                // Skip the lint if `return` is used in `Loop` in order not to suggest using `'label`.
+                if ret_collector.ret_in_loop {
+                    return;
+                }
+
+                // We can't use `Applicability::MachineApplicable` when the closure contains `return`
+                // because `Diagnostic::multipart_suggestion` doesn't work with multiple overlapped
+                // spans.
+                let mut applicability = if ret_collector.spans.is_empty() {
+                    Applicability::MachineApplicable
+                } else {
+                    Applicability::MaybeIncorrect
+                };
+
+                let mut suggs = vec![];
+                suggs.push((stmt.span, format!(
+                    "for {} in {} {}",
+                    snippet_with_applicability(cx, body.params[0].pat.span, "..", &mut applicability),
+                    snippet_with_applicability(cx, for_each_args[0].span, "..", &mut applicability),
+                    snippet_with_applicability(cx, body.value.span, "..", &mut applicability),
+                )));
+
+                for span in &ret_collector.spans {
+                    suggs.push((*span, "return".to_string()));
+                }
+
+                span_lint_and_then(
+                    cx,
+                    NEEDLESS_FOR_EACH,
+                    stmt.span,
+                    "needless use of `for_each`",
+                    |diag| {
+                        diag.multipart_suggestion("try", suggs, applicability);
+                        // `Diagnostic::multipart_suggestion` ignores the second and subsequent overlapped spans,
+                        // so `span_note` is needed here even though `suggs` includes the replacements.
+                        for span in ret_collector.spans {
+                            diag.span_note(span, "replace `return` with `continue`");
+                        }
+                    }
+                )
+            }
+        }
+    }
+}
+
+/// 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.
+///
+/// NOTE: The functionality of this type is similar to
+/// [`crate::utilts::visitors::find_all_ret_expressions`], but we can't use
+/// `find_all_ret_expressions` instead of this type. The reasons are:
+/// 1. `find_all_ret_expressions` passes the argument of `ExprKind::Ret` to a callback, but what we
+///    need here is `ExprKind::Ret` itself.
+/// 2. We can't trace current loop depth with `find_all_ret_expressions`.
+#[derive(Default)]
+struct RetCollector {
+    spans: Vec<Span>,
+    ret_in_loop: bool,
+    loop_depth: u16,
+}
+
+impl<'tcx> Visitor<'tcx> for RetCollector {
+    type Map = Map<'tcx>;
+
+    fn visit_expr(&mut self, expr: &Expr<'_>) {
+        match expr.kind {
+            ExprKind::Ret(..) => {
+                if self.loop_depth > 0 && !self.ret_in_loop {
+                    self.ret_in_loop = true
+                }
+
+                self.spans.push(expr.span)
+            },
+
+            ExprKind::Loop(..) => {
+                self.loop_depth += 1;
+                walk_expr(self, expr);
+                self.loop_depth -= 1;
+                return;
+            },
+
+            _ => {},
+        }
+
+        walk_expr(self, expr);
+    }
+
+    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+        NestedVisitorMap::None
+    }
+}
diff --git a/tests/ui/excessive_for_each.rs b/tests/ui/excessive_for_each.rs
deleted file mode 100644
index 0800bef71e9..00000000000
--- a/tests/ui/excessive_for_each.rs
+++ /dev/null
@@ -1,126 +0,0 @@
-#![warn(clippy::excessive_for_each)]
-#![allow(clippy::needless_return)]
-
-use std::collections::*;
-
-fn main() {
-    // Should trigger this lint: Vec.
-    let vec: Vec<i32> = Vec::new();
-    let mut acc = 0;
-    vec.iter().for_each(|v| {
-        acc += v;
-    });
-
-    // Should trigger this lint: &Vec.
-    let vec_ref = &vec;
-    vec_ref.iter().for_each(|v| {
-        acc += v;
-    });
-
-    // Should trigger this lint: VecDeque.
-    let vec_deq: VecDeque<i32> = VecDeque::new();
-    vec_deq.iter().for_each(|v| {
-        acc += v;
-    });
-
-    // Should trigger this lint: LinkedList.
-    let list: LinkedList<i32> = LinkedList::new();
-    list.iter().for_each(|v| {
-        acc += v;
-    });
-
-    // Should trigger this lint: HashMap.
-    let mut hash_map: HashMap<i32, i32> = HashMap::new();
-    hash_map.iter().for_each(|(k, v)| {
-        acc += k + v;
-    });
-    hash_map.iter_mut().for_each(|(k, v)| {
-        acc += *k + *v;
-    });
-    hash_map.keys().for_each(|k| {
-        acc += k;
-    });
-    hash_map.values().for_each(|v| {
-        acc += v;
-    });
-
-    // Should trigger this lint: HashSet.
-    let hash_set: HashSet<i32> = HashSet::new();
-    hash_set.iter().for_each(|v| {
-        acc += v;
-    });
-
-    // Should trigger this lint: BTreeSet.
-    let btree_set: BTreeSet<i32> = BTreeSet::new();
-    btree_set.iter().for_each(|v| {
-        acc += v;
-    });
-
-    // Should trigger this lint: BinaryHeap.
-    let binary_heap: BinaryHeap<i32> = BinaryHeap::new();
-    binary_heap.iter().for_each(|v| {
-        acc += v;
-    });
-
-    // Should trigger this lint: Array.
-    let s = [1, 2, 3];
-    s.iter().for_each(|v| {
-        acc += v;
-    });
-
-    // Should trigger this lint. Slice.
-    vec.as_slice().iter().for_each(|v| {
-        acc += v;
-    });
-
-    // Should trigger this lint with notes that say "change `return` to `continue`".
-    vec.iter().for_each(|v| {
-        if *v == 10 {
-            return;
-        } else {
-            println!("{}", v);
-        }
-    });
-
-    // Should NOT trigger this lint in case `return` is used in `Loop` of the closure.
-    vec.iter().for_each(|v| {
-        for i in 0..*v {
-            if i == 10 {
-                return;
-            } else {
-                println!("{}", v);
-            }
-        }
-        if *v == 20 {
-            return;
-        } else {
-            println!("{}", v);
-        }
-    });
-
-    // Should NOT trigger this lint in case `for_each` follows long iterator chain.
-    vec.iter().chain(vec.iter()).for_each(|v| println!("{}", v));
-
-    // Should NOT trigger this lint in case a `for_each` argument is not closure.
-    fn print(x: &i32) {
-        println!("{}", x);
-    }
-    vec.iter().for_each(print);
-
-    // Should NOT trigger this lint in case the receiver of `iter` is a user defined type.
-    let my_collection = MyCollection { v: vec![] };
-    my_collection.iter().for_each(|v| println!("{}", v));
-
-    // Should NOT trigger this lint in case the closure body is not a `ExprKind::Block`.
-    vec.iter().for_each(|x| acc += x);
-}
-
-struct MyCollection {
-    v: Vec<i32>,
-}
-
-impl MyCollection {
-    fn iter(&self) -> impl Iterator<Item = &i32> {
-        self.v.iter()
-    }
-}
diff --git a/tests/ui/excessive_for_each.stderr b/tests/ui/excessive_for_each.stderr
deleted file mode 100644
index f5799484e03..00000000000
--- a/tests/ui/excessive_for_each.stderr
+++ /dev/null
@@ -1,126 +0,0 @@
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:10:5
-   |
-LL | /     vec.iter().for_each(|v| {
-LL | |         acc += v;
-LL | |     });
-   | |______^ help: try: `for v in vec.iter() { .. }`
-   |
-   = note: `-D clippy::excessive-for-each` implied by `-D warnings`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:16:5
-   |
-LL | /     vec_ref.iter().for_each(|v| {
-LL | |         acc += v;
-LL | |     });
-   | |______^ help: try: `for v in vec_ref.iter() { .. }`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:22:5
-   |
-LL | /     vec_deq.iter().for_each(|v| {
-LL | |         acc += v;
-LL | |     });
-   | |______^ help: try: `for v in vec_deq.iter() { .. }`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:28:5
-   |
-LL | /     list.iter().for_each(|v| {
-LL | |         acc += v;
-LL | |     });
-   | |______^ help: try: `for v in list.iter() { .. }`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:34:5
-   |
-LL | /     hash_map.iter().for_each(|(k, v)| {
-LL | |         acc += k + v;
-LL | |     });
-   | |______^ help: try: `for (k, v) in hash_map.iter() { .. }`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:37:5
-   |
-LL | /     hash_map.iter_mut().for_each(|(k, v)| {
-LL | |         acc += *k + *v;
-LL | |     });
-   | |______^ help: try: `for (k, v) in hash_map.iter_mut() { .. }`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:40:5
-   |
-LL | /     hash_map.keys().for_each(|k| {
-LL | |         acc += k;
-LL | |     });
-   | |______^ help: try: `for k in hash_map.keys() { .. }`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:43:5
-   |
-LL | /     hash_map.values().for_each(|v| {
-LL | |         acc += v;
-LL | |     });
-   | |______^ help: try: `for v in hash_map.values() { .. }`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:49:5
-   |
-LL | /     hash_set.iter().for_each(|v| {
-LL | |         acc += v;
-LL | |     });
-   | |______^ help: try: `for v in hash_set.iter() { .. }`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:55:5
-   |
-LL | /     btree_set.iter().for_each(|v| {
-LL | |         acc += v;
-LL | |     });
-   | |______^ help: try: `for v in btree_set.iter() { .. }`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:61:5
-   |
-LL | /     binary_heap.iter().for_each(|v| {
-LL | |         acc += v;
-LL | |     });
-   | |______^ help: try: `for v in binary_heap.iter() { .. }`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:67:5
-   |
-LL | /     s.iter().for_each(|v| {
-LL | |         acc += v;
-LL | |     });
-   | |______^ help: try: `for v in s.iter() { .. }`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:72:5
-   |
-LL | /     vec.as_slice().iter().for_each(|v| {
-LL | |         acc += v;
-LL | |     });
-   | |______^ help: try: `for v in vec.as_slice().iter() { .. }`
-
-error: excessive use of `for_each`
-  --> $DIR/excessive_for_each.rs:77:5
-   |
-LL | /     vec.iter().for_each(|v| {
-LL | |         if *v == 10 {
-LL | |             return;
-LL | |         } else {
-LL | |             println!("{}", v);
-LL | |         }
-LL | |     });
-   | |______^ help: try: `for v in vec.iter() { .. }`
-   |
-note: change `return` to `continue` in the loop body
-  --> $DIR/excessive_for_each.rs:79:13
-   |
-LL |             return;
-   |             ^^^^^^
-
-error: aborting due to 14 previous errors
-
diff --git a/tests/ui/needless_for_each_fixable.fixed b/tests/ui/needless_for_each_fixable.fixed
new file mode 100644
index 00000000000..0caa95a9f53
--- /dev/null
+++ b/tests/ui/needless_for_each_fixable.fixed
@@ -0,0 +1,99 @@
+// run-rustfix
+#![warn(clippy::needless_for_each)]
+#![allow(unused, clippy::needless_return, clippy::match_single_binding)]
+
+use std::collections::HashMap;
+
+fn should_lint() {
+    let v: Vec<i32> = Vec::new();
+    let mut acc = 0;
+    for elem in v.iter() {
+        acc += elem;
+    }
+    for elem in v.into_iter() {
+        acc += elem;
+    }
+
+    let mut hash_map: HashMap<i32, i32> = HashMap::new();
+    for (k, v) in hash_map.iter() {
+        acc += k + v;
+    }
+    for (k, v) in hash_map.iter_mut() {
+        acc += *k + *v;
+    }
+    for k in hash_map.keys() {
+        acc += k;
+    }
+    for v in hash_map.values() {
+        acc += v;
+    }
+
+    fn my_vec() -> Vec<i32> {
+        Vec::new()
+    }
+    for elem in my_vec().iter() {
+        acc += elem;
+    }
+}
+
+fn should_not_lint() {
+    let v: Vec<i32> = Vec::new();
+    let mut acc = 0;
+
+    // `for_each` argument is not closure.
+    fn print(x: &i32) {
+        println!("{}", x);
+    }
+    v.iter().for_each(print);
+
+    // `for_each` follows long iterator chain.
+    v.iter().chain(v.iter()).for_each(|v| println!("{}", v));
+    v.as_slice().iter().for_each(|v| {
+        acc += v;
+    });
+
+    // `return` is used in `Loop` of the closure.
+    v.iter().for_each(|v| {
+        for i in 0..*v {
+            if i == 10 {
+                return;
+            } else {
+                println!("{}", v);
+            }
+        }
+        if *v == 20 {
+            return;
+        } else {
+            println!("{}", v);
+        }
+    });
+
+    // User defined type.
+    struct MyStruct {
+        v: Vec<i32>,
+    }
+    impl MyStruct {
+        fn iter(&self) -> impl Iterator<Item = &i32> {
+            self.v.iter()
+        }
+    }
+    let s = MyStruct { v: Vec::new() };
+    s.iter().for_each(|elem| {
+        acc += elem;
+    });
+
+    // Previously transformed iterator variable.
+    let it = v.iter();
+    it.chain(v.iter()).for_each(|elem| {
+        acc += elem;
+    });
+
+    // `for_each` is not directly in a statement.
+    match 1 {
+        _ => v.iter().for_each(|elem| {
+            acc += elem;
+        }),
+    }
+}
+
+fn main() {}
diff --git a/tests/ui/needless_for_each_fixable.rs b/tests/ui/needless_for_each_fixable.rs
new file mode 100644
index 00000000000..a04243de27a
--- /dev/null
+++ b/tests/ui/needless_for_each_fixable.rs
@@ -0,0 +1,99 @@
+// run-rustfix
+#![warn(clippy::needless_for_each)]
+#![allow(unused, clippy::needless_return, clippy::match_single_binding)]
+
+use std::collections::HashMap;
+
+fn should_lint() {
+    let v: Vec<i32> = Vec::new();
+    let mut acc = 0;
+    v.iter().for_each(|elem| {
+        acc += elem;
+    });
+    v.into_iter().for_each(|elem| {
+        acc += elem;
+    });
+
+    let mut hash_map: HashMap<i32, i32> = HashMap::new();
+    hash_map.iter().for_each(|(k, v)| {
+        acc += k + v;
+    });
+    hash_map.iter_mut().for_each(|(k, v)| {
+        acc += *k + *v;
+    });
+    hash_map.keys().for_each(|k| {
+        acc += k;
+    });
+    hash_map.values().for_each(|v| {
+        acc += v;
+    });
+
+    fn my_vec() -> Vec<i32> {
+        Vec::new()
+    }
+    my_vec().iter().for_each(|elem| {
+        acc += elem;
+    });
+}
+
+fn should_not_lint() {
+    let v: Vec<i32> = Vec::new();
+    let mut acc = 0;
+
+    // `for_each` argument is not closure.
+    fn print(x: &i32) {
+        println!("{}", x);
+    }
+    v.iter().for_each(print);
+
+    // `for_each` follows long iterator chain.
+    v.iter().chain(v.iter()).for_each(|v| println!("{}", v));
+    v.as_slice().iter().for_each(|v| {
+        acc += v;
+    });
+
+    // `return` is used in `Loop` of the closure.
+    v.iter().for_each(|v| {
+        for i in 0..*v {
+            if i == 10 {
+                return;
+            } else {
+                println!("{}", v);
+            }
+        }
+        if *v == 20 {
+            return;
+        } else {
+            println!("{}", v);
+        }
+    });
+
+    // User defined type.
+    struct MyStruct {
+        v: Vec<i32>,
+    }
+    impl MyStruct {
+        fn iter(&self) -> impl Iterator<Item = &i32> {
+            self.v.iter()
+        }
+    }
+    let s = MyStruct { v: Vec::new() };
+    s.iter().for_each(|elem| {
+        acc += elem;
+    });
+
+    // Previously transformed iterator variable.
+    let it = v.iter();
+    it.chain(v.iter()).for_each(|elem| {
+        acc += elem;
+    });
+
+    // `for_each` is not directly in a statement.
+    match 1 {
+        _ => v.iter().for_each(|elem| {
+            acc += elem;
+        }),
+    }
+}
+
+fn main() {}
diff --git a/tests/ui/needless_for_each_fixable.stderr b/tests/ui/needless_for_each_fixable.stderr
new file mode 100644
index 00000000000..214e357a208
--- /dev/null
+++ b/tests/ui/needless_for_each_fixable.stderr
@@ -0,0 +1,108 @@
+error: needless use of `for_each`
+  --> $DIR/needless_for_each_fixable.rs:10:5
+   |
+LL | /     v.iter().for_each(|elem| {
+LL | |         acc += elem;
+LL | |     });
+   | |_______^
+   |
+   = note: `-D clippy::needless-for-each` implied by `-D warnings`
+help: try
+   |
+LL |     for elem in v.iter() {
+LL |         acc += elem;
+LL |     }
+   |
+
+error: needless use of `for_each`
+  --> $DIR/needless_for_each_fixable.rs:13:5
+   |
+LL | /     v.into_iter().for_each(|elem| {
+LL | |         acc += elem;
+LL | |     });
+   | |_______^
+   |
+help: try
+   |
+LL |     for elem in v.into_iter() {
+LL |         acc += elem;
+LL |     }
+   |
+
+error: needless use of `for_each`
+  --> $DIR/needless_for_each_fixable.rs:18:5
+   |
+LL | /     hash_map.iter().for_each(|(k, v)| {
+LL | |         acc += k + v;
+LL | |     });
+   | |_______^
+   |
+help: try
+   |
+LL |     for (k, v) in hash_map.iter() {
+LL |         acc += k + v;
+LL |     }
+   |
+
+error: needless use of `for_each`
+  --> $DIR/needless_for_each_fixable.rs:21:5
+   |
+LL | /     hash_map.iter_mut().for_each(|(k, v)| {
+LL | |         acc += *k + *v;
+LL | |     });
+   | |_______^
+   |
+help: try
+   |
+LL |     for (k, v) in hash_map.iter_mut() {
+LL |         acc += *k + *v;
+LL |     }
+   |
+
+error: needless use of `for_each`
+  --> $DIR/needless_for_each_fixable.rs:24:5
+   |
+LL | /     hash_map.keys().for_each(|k| {
+LL | |         acc += k;
+LL | |     });
+   | |_______^
+   |
+help: try
+   |
+LL |     for k in hash_map.keys() {
+LL |         acc += k;
+LL |     }
+   |
+
+error: needless use of `for_each`
+  --> $DIR/needless_for_each_fixable.rs:27:5
+   |
+LL | /     hash_map.values().for_each(|v| {
+LL | |         acc += v;
+LL | |     });
+   | |_______^
+   |
+help: try
+   |
+LL |     for v in hash_map.values() {
+LL |         acc += v;
+LL |     }
+   |
+
+error: needless use of `for_each`
+  --> $DIR/needless_for_each_fixable.rs:34:5
+   |
+LL | /     my_vec().iter().for_each(|elem| {
+LL | |         acc += elem;
+LL | |     });
+   | |_______^
+   |
+help: try
+   |
+LL |     for elem in my_vec().iter() {
+LL |         acc += elem;
+LL |     }
+   |
+
+error: aborting due to 7 previous errors
+
diff --git a/tests/ui/needless_for_each_unfixable.rs b/tests/ui/needless_for_each_unfixable.rs
new file mode 100644
index 00000000000..d765d7dab65
--- /dev/null
+++ b/tests/ui/needless_for_each_unfixable.rs
@@ -0,0 +1,14 @@
+#![warn(clippy::needless_for_each)]
+#![allow(clippy::needless_return)]
+
+fn main() {
+    let v: Vec<i32> = Vec::new();
+    // This is unfixable because the closure includes `return`.
+    v.iter().for_each(|v| {
+        if *v == 10 {
+            return;
+        } else {
+            println!("{}", v);
+        }
+    });
+}
diff --git a/tests/ui/needless_for_each_unfixable.stderr b/tests/ui/needless_for_each_unfixable.stderr
new file mode 100644
index 00000000000..58d107062bc
--- /dev/null
+++ b/tests/ui/needless_for_each_unfixable.stderr
@@ -0,0 +1,30 @@
+error: needless use of `for_each`
+  --> $DIR/needless_for_each_unfixable.rs:7:5
+   |
+LL | /     v.iter().for_each(|v| {
+LL | |         if *v == 10 {
+LL | |             return;
+LL | |         } else {
+LL | |             println!("{}", v);
+LL | |         }
+LL | |     });
+   | |_______^
+   |
+   = note: `-D clippy::needless-for-each` implied by `-D warnings`
+note: replace `return` with `continue`
+  --> $DIR/needless_for_each_unfixable.rs:9:13
+   |
+LL |             return;
+   |             ^^^^^^
+help: try
+   |
+LL |     for v in v.iter() {
+LL |         if *v == 10 {
+LL |             return;
+LL |         } else {
+LL |             println!("{}", v);
+LL |         }
+ ...
+
+error: aborting due to previous error
+