Auto merge of #6706 - Y-Nak:excessive-for-each, r=camsteffen

New Lint: needless_for_each

resolves: #6543

changelog: Added pedantic lint: `needless_for_each`

CHANGELOG.md

@@ -2369,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

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;
@@ -867,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,
@@ -1045,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);
@@ -1405,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),

clippy_lints/src/matches.rs

@@ -924,7 +924,7 @@ fn check_wild_err_arm<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'tcx>, arms: &[Arm
                     let mut ident_bind_name = String::from("_");
                     if !matching_wild {
                         // Looking for unused bindings (i.e.: `_e`)
-                        inner.iter().for_each(|pat| {
+                        for pat in inner.iter() {
                             if let PatKind::Binding(_, id, ident, None) = pat.kind {
                                 if ident.as_str().starts_with('_')
                                     && !LocalUsedVisitor::new(cx, id).check_expr(arm.body)
@@ -933,7 +933,7 @@ fn check_wild_err_arm<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'tcx>, arms: &[Arm
                                     matching_wild = true;
                                 }
                             }
-                        });
+                        }
                     }
                     if_chain! {
                         if matching_wild;

clippy_lints/src/needless_for_each.rs

@@ -0,0 +1,167 @@
+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 clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::is_trait_method;
+use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::ty::has_iter_method;
+
+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,
+    pedantic,
+    "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,
+            _ => return,
+        };
+
+        if_chain! {
+            // Check the method name is `for_each`.
+            if let ExprKind::MethodCall(method_name, _, [for_each_recv, for_each_arg], _) = 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 ExprKind::MethodCall(_, _, [iter_recv], _) = for_each_recv.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 matches!(
+                iter_recv.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_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.
+            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::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;
+                }
+
+                let (mut applicability, ret_suggs) = if ret_collector.spans.is_empty() {
+                    (Applicability::MachineApplicable, None)
+                } else {
+                    (
+                        Applicability::MaybeIncorrect,
+                        Some(
+                            ret_collector
+                                .spans
+                                .into_iter()
+                                .map(|span| (span, "continue".to_string()))
+                                .collect(),
+                        ),
+                    )
+                };
+
+                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),
+                );
+
+                span_lint_and_then(cx, NEEDLESS_FOR_EACH, stmt.span, "needless use of `for_each`", |diag| {
+                    diag.span_suggestion(stmt.span, "try", sugg, applicability);
+                    if let Some(ret_suggs) = ret_suggs {
+                        diag.multipart_suggestion("...and replace `return` with `continue`", ret_suggs, applicability);
+                    }
+                })
+            }
+        }
+    }
+}
+
+/// 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
+    }
+}

tests/lint_message_convention.rs

@@ -89,14 +89,14 @@ fn lint_message_convention() {
         .filter(|message| !message.bad_lines.is_empty())
         .collect();
 
-    bad_tests.iter().for_each(|message| {
+    for message in &bad_tests {
         eprintln!(
             "error: the test '{}' contained the following nonconforming lines :",
             message.path.display()
         );
         message.bad_lines.iter().for_each(|line| eprintln!("{}", line));
         eprintln!("\n\n");
-    });
+    }
 
     eprintln!(
         "\n\n\nLint message should not start with a capital letter and should not have punctuation at the end of the message unless multiple sentences are needed."

tests/ui/needless_for_each_fixable.fixed

@@ -0,0 +1,113 @@
+// 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;
+    }
+
+    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;
+    }
+    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);
+
+    // 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| {
+        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| {
+        for i in 0..*v {
+            if i == 10 {
+                return;
+            } else {
+                println!("{}", v);
+            }
+        }
+        if *v == 20 {
+            return;
+        } else {
+            println!("{}", v);
+        }
+    });
+
+    // 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;
+        }),
+    }
+
+    // `for_each` is in a let bingind.
+    let _ = v.iter().for_each(|elem| {
+        acc += elem;
+    });
+}
+
+fn main() {}

tests/ui/needless_for_each_fixable.rs

@@ -0,0 +1,113 @@
+// 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;
+    });
+
+    [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;
+    });
+    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);
+
+    // 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| {
+        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| {
+        for i in 0..*v {
+            if i == 10 {
+                return;
+            } else {
+                println!("{}", v);
+            }
+        }
+        if *v == 20 {
+            return;
+        } else {
+            println!("{}", v);
+        }
+    });
+
+    // 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;
+        }),
+    }
+
+    // `for_each` is in a let bingind.
+    let _ = v.iter().for_each(|elem| {
+        acc += elem;
+    });
+}
+
+fn main() {}

tests/ui/needless_for_each_fixable.stderr

@@ -0,0 +1,123 @@
+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: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;
+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:25: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:28: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:31: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:38: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 8 previous errors
+

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

tests/ui/needless_for_each_unfixable.stderr

@@ -0,0 +1,29 @@
+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`
+help: try
+   |
+LL |     for v in v.iter() {
+LL |         if *v == 10 {
+LL |             return;
+LL |         } else {
+LL |             println!("{}", v);
+LL |         }
+ ...
+help: ...and replace `return` with `continue`
+   |
+LL |             continue;
+   |             ^^^^^^^^
+
+error: aborting due to previous error
+