Remove method_calls

5 files changed, 92 insertions, 55 deletions

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 8c9247d9781..11716afe11c 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -1326,7 +1326,6 @@ 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),
@@ -1409,6 +1408,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&misc_early::UNSEPARATED_LITERAL_SUFFIX),
         LintId::of(&mut_mut::MUT_MUT),
         LintId::of(&needless_continue::NEEDLESS_CONTINUE),
+        LintId::of(&needless_for_each::NEEDLESS_FOR_EACH),
         LintId::of(&needless_pass_by_value::NEEDLESS_PASS_BY_VALUE),
         LintId::of(&non_expressive_names::SIMILAR_NAMES),
         LintId::of(&option_if_let_else::OPTION_IF_LET_ELSE),
diff --git a/clippy_lints/src/needless_for_each.rs b/clippy_lints/src/needless_for_each.rs
index a7b0a1ca082..f60b09898ab 100644
--- a/clippy_lints/src/needless_for_each.rs
+++ b/clippy_lints/src/needless_for_each.rs
@@ -10,9 +10,7 @@ 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,
-};
+use crate::utils::{has_iter_method, is_trait_method, 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
@@ -41,7 +39,7 @@ declare_clippy_lint! {
     /// }
     /// ```
     pub NEEDLESS_FOR_EACH,
-    restriction,
+    pedantic,
     "using `for_each` where a `for` loop would be simpler"
 }
 
@@ -55,22 +53,28 @@ impl LateLintPass<'_> for NeedlessForEach {
             _ => 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);
+            // Check the method name is `for_each`.
+            if let ExprKind::MethodCall(method_name, _, for_each_args, _) = expr.kind;
+            if method_name.ident.name == Symbol::intern("for_each");
+            // Check `for_each` is an associated function of `Iterator`.
+            if is_trait_method(cx, expr, sym::Iterator);
+            // Checks the receiver of `for_each` is also a method call.
+            if let Some(for_each_receiver) = for_each_args.get(0);
+            if let ExprKind::MethodCall(_, _, iter_args, _) = for_each_receiver.kind;
+            // Skip the lint if the call chain is too long. e.g. `v.field.iter().for_each()` or
+            // `v.foo().iter().for_each()` must be skipped.
+            if let Some(iter_receiver) = iter_args.get(0);
+            if matches!(
+                iter_receiver.kind,
+                ExprKind::Array(..) | ExprKind::Call(..) | ExprKind::Path(..)
+            );
+            // Checks the type of the `iter` method receiver is NOT a user defined type.
+            if has_iter_method(cx, cx.typeck_results().expr_ty(&iter_receiver)).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.
+            if let ExprKind::Closure(_, _, body_id, ..) = for_each_args[1].kind;
+            let body = cx.tcx.hir().body(body_id);
             if let ExprKind::Block(..) = body.value.kind;
             then {
                 let mut ret_collector = RetCollector::default();
@@ -99,7 +103,7 @@ impl LateLintPass<'_> for NeedlessForEach {
                 )));
 
                 for span in &ret_collector.spans {
-                    suggs.push((*span, "return".to_string()));
+                    suggs.push((*span, "continue".to_string()));
                 }
 
                 span_lint_and_then(
diff --git a/tests/ui/needless_for_each_fixable.fixed b/tests/ui/needless_for_each_fixable.fixed
index 0caa95a9f53..a4d4937a19a 100644
--- a/tests/ui/needless_for_each_fixable.fixed
+++ b/tests/ui/needless_for_each_fixable.fixed
@@ -14,6 +14,10 @@ fn should_lint() {
         acc += elem;
     }
 
+    for elem in [1, 2, 3].iter() {
+        acc += elem;
+    }
+
     let mut hash_map: HashMap<i32, i32> = HashMap::new();
     for (k, v) in hash_map.iter() {
         acc += k + v;
@@ -46,11 +50,30 @@ fn should_not_lint() {
     }
     v.iter().for_each(print);
 
+    // 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;
+    });
+
     // `for_each` follows long iterator chain.
-    v.iter().chain(v.iter()).for_each(|v| println!("{}", v));
+    v.iter().chain(v.iter()).for_each(|v| {
+        acc += v;
+    });
     v.as_slice().iter().for_each(|v| {
         acc += v;
     });
+    s.v.iter().for_each(|v| {
+        acc += v;
+    });
 
     // `return` is used in `Loop` of the closure.
     v.iter().for_each(|v| {
@@ -68,20 +91,6 @@ fn should_not_lint() {
         }
     });
 
-    // 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| {
diff --git a/tests/ui/needless_for_each_fixable.rs b/tests/ui/needless_for_each_fixable.rs
index a04243de27a..b374128f253 100644
--- a/tests/ui/needless_for_each_fixable.rs
+++ b/tests/ui/needless_for_each_fixable.rs
@@ -14,6 +14,10 @@ fn should_lint() {
         acc += elem;
     });
 
+    [1, 2, 3].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;
@@ -46,11 +50,30 @@ fn should_not_lint() {
     }
     v.iter().for_each(print);
 
+    // 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;
+    });
+
     // `for_each` follows long iterator chain.
-    v.iter().chain(v.iter()).for_each(|v| println!("{}", v));
+    v.iter().chain(v.iter()).for_each(|v| {
+        acc += v;
+    });
     v.as_slice().iter().for_each(|v| {
         acc += v;
     });
+    s.v.iter().for_each(|v| {
+        acc += v;
+    });
 
     // `return` is used in `Loop` of the closure.
     v.iter().for_each(|v| {
@@ -68,20 +91,6 @@ fn should_not_lint() {
         }
     });
 
-    // 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| {
diff --git a/tests/ui/needless_for_each_fixable.stderr b/tests/ui/needless_for_each_fixable.stderr
index 214e357a208..483a5e6d61d 100644
--- a/tests/ui/needless_for_each_fixable.stderr
+++ b/tests/ui/needless_for_each_fixable.stderr
@@ -30,7 +30,22 @@ LL |     }
    |
 
 error: needless use of `for_each`
-  --> $DIR/needless_for_each_fixable.rs:18:5
+  --> $DIR/needless_for_each_fixable.rs:17:5
+   |
+LL | /     [1, 2, 3].iter().for_each(|elem| {
+LL | |         acc += elem;
+LL | |     });
+   | |_______^
+   |
+help: try
+   |
+LL |     for elem in [1, 2, 3].iter() {
+LL |         acc += elem;
+LL |     }
+   |
+
+error: needless use of `for_each`
+  --> $DIR/needless_for_each_fixable.rs:22:5
    |
 LL | /     hash_map.iter().for_each(|(k, v)| {
 LL | |         acc += k + v;
@@ -45,7 +60,7 @@ LL |     }
    |
 
 error: needless use of `for_each`
-  --> $DIR/needless_for_each_fixable.rs:21:5
+  --> $DIR/needless_for_each_fixable.rs:25:5
    |
 LL | /     hash_map.iter_mut().for_each(|(k, v)| {
 LL | |         acc += *k + *v;
@@ -60,7 +75,7 @@ LL |     }
    |
 
 error: needless use of `for_each`
-  --> $DIR/needless_for_each_fixable.rs:24:5
+  --> $DIR/needless_for_each_fixable.rs:28:5
    |
 LL | /     hash_map.keys().for_each(|k| {
 LL | |         acc += k;
@@ -75,7 +90,7 @@ LL |     }
    |
 
 error: needless use of `for_each`
-  --> $DIR/needless_for_each_fixable.rs:27:5
+  --> $DIR/needless_for_each_fixable.rs:31:5
    |
 LL | /     hash_map.values().for_each(|v| {
 LL | |         acc += v;
@@ -90,7 +105,7 @@ LL |     }
    |
 
 error: needless use of `for_each`
-  --> $DIR/needless_for_each_fixable.rs:34:5
+  --> $DIR/needless_for_each_fixable.rs:38:5
    |
 LL | /     my_vec().iter().for_each(|elem| {
 LL | |         acc += elem;
@@ -104,5 +119,5 @@ LL |         acc += elem;
 LL |     }
    |
 
-error: aborting due to 7 previous errors
+error: aborting due to 8 previous errors