New Lint: excessive_for_each

CHANGELOG.md

@@ -2192,6 +2192,7 @@ 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

clippy_lints/src/lib.rs

@@ -781,6 +781,7 @@ 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,
@@ -1581,6 +1582,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::CHARS_NEXT_CMP),
         LintId::of(&methods::CLONE_DOUBLE_REF),
         LintId::of(&methods::CLONE_ON_COPY),
+        LintId::of(&methods::EXCESSIVE_FOR_EACH),
         LintId::of(&methods::EXPECT_FUN_CALL),
         LintId::of(&methods::FILTER_MAP_IDENTITY),
         LintId::of(&methods::FILTER_NEXT),
@@ -1797,6 +1799,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::BYTES_NTH),
         LintId::of(&methods::CHARS_LAST_CMP),
         LintId::of(&methods::CHARS_NEXT_CMP),
+        LintId::of(&methods::EXCESSIVE_FOR_EACH),
         LintId::of(&methods::FROM_ITER_INSTEAD_OF_COLLECT),
         LintId::of(&methods::INTO_ITER_ON_REF),
         LintId::of(&methods::ITER_CLONED_COLLECT),

clippy_lints/src/methods/excessive_for_each.rs

@@ -0,0 +1,149 @@
+use rustc_errors::Applicability;
+use rustc_hir::{
+    intravisit::{walk_expr, NestedVisitorMap, Visitor},
+    Expr, ExprKind,
+};
+use rustc_lint::LateContext;
+use rustc_middle::{hir::map::Map, ty, ty::Ty};
+use rustc_span::source_map::Span;
+
+use crate::utils::{match_trait_method, match_type, paths, snippet, span_lint_and_then};
+
+use if_chain::if_chain;
+
+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 match_trait_method(cx, expr, &paths::ITERATOR);
+        if !match_trait_method(cx, for_each_receiver, &paths::ITERATOR);
+        if is_target_ty(cx, cx.typeck_results().expr_ty(iter_receiver));
+        if let ExprKind::Closure(_, _, body_id, ..) = for_each_arg.kind;
+        then {
+            let body = cx.tcx.hir().body(body_id);
+            let mut ret_span_collector = RetSpanCollector::new();
+            ret_span_collector.visit_expr(&body.value);
+
+            let label = "'outer";
+            let loop_label = if ret_span_collector.need_label {
+                format!("{}: ", label)
+            } else {
+                "".to_string()
+            };
+            let sugg =
+                format!("{}for {} in {} {{ .. }}", loop_label, snippet(cx, body.params[0].pat.span, ""), snippet(cx, for_each_receiver.span, ""));
+
+            let mut notes = vec![];
+            for (span, need_label) in ret_span_collector.spans {
+                let cont_label = if need_label {
+                    format!(" {}", label)
+                } else {
+                    "".to_string()
+                };
+                let note = format!("change `return` to `continue{}` in the loop body", cont_label);
+                notes.push((span, note));
+            }
+
+            span_lint_and_then(cx,
+                      super::EXCESSIVE_FOR_EACH,
+                      expr.span,
+                      "excessive use of `for_each`",
+                      |diag| {
+                          diag.span_suggestion(expr.span, "try", sugg, Applicability::HasPlaceholders);
+                          for note in notes {
+                              diag.span_note(note.0, &note.1);
+                          }
+                      }
+                );
+        }
+    }
+}
+
+type PathSegment = &'static [&'static str];
+
+const TARGET_ITER_RECEIVER_TY: &[PathSegment] = &[
+    &paths::VEC,
+    &paths::VEC_DEQUE,
+    &paths::LINKED_LIST,
+    &paths::HASHMAP,
+    &paths::BTREEMAP,
+    &paths::HASHSET,
+    &paths::BTREESET,
+    &paths::BINARY_HEAP,
+];
+
+fn is_target_ty(cx: &LateContext<'_>, expr_ty: Ty<'_>) -> bool {
+    let expr_ty = expr_ty.peel_refs();
+    for target in TARGET_ITER_RECEIVER_TY {
+        if match_type(cx, expr_ty, target) {
+            return true;
+        }
+    }
+
+    if_chain! {
+        if matches!(expr_ty.kind(), ty::Slice(_) | ty::Array(..));
+        then {
+            return true;
+        }
+    }
+
+    false
+}
+
+/// Collect spans of `return` in the closure body.
+struct RetSpanCollector {
+    spans: Vec<(Span, bool)>,
+    loop_depth: u16,
+    need_label: bool,
+}
+
+impl RetSpanCollector {
+    fn new() -> Self {
+        Self {
+            spans: Vec::new(),
+            loop_depth: 0,
+            need_label: false,
+        }
+    }
+}
+
+impl<'tcx> Visitor<'tcx> for RetSpanCollector {
+    type Map = Map<'tcx>;
+
+    fn visit_expr(&mut self, expr: &Expr<'_>) {
+        match expr.kind {
+            ExprKind::Ret(..) => {
+                if self.loop_depth > 0 && !self.need_label {
+                    self.need_label = true
+                }
+
+                self.spans.push((expr.span, self.loop_depth > 0))
+            },
+
+            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
+    }
+}

clippy_lints/src/methods/mod.rs

@@ -974,6 +974,33 @@ declare_clippy_lint! {
     "using `.skip(x).next()` on an iterator"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for use of `obj.method().for_each(closure)` if obj doesn't
+    /// implelement `Iterator` and `method()` returns `Impl Iterator` type.
+    ///
+    /// **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,
+    style,
+    "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`
@@ -1661,6 +1688,7 @@ impl_lint_pass!(Methods => [
     ITER_NTH_ZERO,
     BYTES_NTH,
     ITER_SKIP_NEXT,
+    EXCESSIVE_FOR_EACH,
     GET_UNWRAP,
     STRING_EXTEND_CHARS,
     ITER_CLONED_COLLECT,
@@ -1807,6 +1835,7 @@ 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),
             _ => {},
         }
 

tests/ui/excessive_for_each.rs

@@ -0,0 +1,96 @@
+#![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();
+    vec.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint: &Vec.
+    let vec_ref = &vec;
+    vec_ref.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint: VecDeque.
+    let vec_deq: VecDeque<i32> = VecDeque::new();
+    vec_deq.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint: LinkedList.
+    let list: LinkedList<i32> = LinkedList::new();
+    list.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint: HashMap.
+    let mut hash_map: HashMap<i32, i32> = HashMap::new();
+    hash_map.iter().for_each(|(k, v)| println!("{}: {}", k, v));
+    hash_map.iter_mut().for_each(|(k, v)| println!("{}: {}", k, v));
+    hash_map.keys().for_each(|k| println!("{}", k));
+    hash_map.values().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint: HashSet.
+    let hash_set: HashSet<i32> = HashSet::new();
+    hash_set.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint: BTreeSet.
+    let btree_set: BTreeSet<i32> = BTreeSet::new();
+    btree_set.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint: BinaryHeap.
+    let binary_heap: BinaryHeap<i32> = BinaryHeap::new();
+    binary_heap.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint: Array.
+    let s = [1, 2, 3];
+    s.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint. Slice.
+    vec.as_slice().iter().for_each(|v| println!("{}", 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 trigger this lint with notes that say "change `return` to `continue 'outer`".
+    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));
+}
+
+struct MyCollection {
+    v: Vec<i32>,
+}
+
+impl MyCollection {
+    fn iter(&self) -> impl Iterator<Item = &i32> {
+        self.v.iter()
+    }
+}

tests/ui/excessive_for_each.stderr

@@ -0,0 +1,123 @@
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:9:5
+   |
+LL |     vec.iter().for_each(|v| println!("{}", v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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:13:5
+   |
+LL |     vec_ref.iter().for_each(|v| println!("{}", v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for v in vec_ref.iter() { .. }`
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:17:5
+   |
+LL |     vec_deq.iter().for_each(|v| println!("{}", v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for v in vec_deq.iter() { .. }`
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:21:5
+   |
+LL |     list.iter().for_each(|v| println!("{}", v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for v in list.iter() { .. }`
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:25:5
+   |
+LL |     hash_map.iter().for_each(|(k, v)| println!("{}: {}", k, v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for (k, v) in hash_map.iter() { .. }`
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:26:5
+   |
+LL |     hash_map.iter_mut().for_each(|(k, v)| println!("{}: {}", k, v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for (k, v) in hash_map.iter_mut() { .. }`
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:27:5
+   |
+LL |     hash_map.keys().for_each(|k| println!("{}", k));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for k in hash_map.keys() { .. }`
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:28:5
+   |
+LL |     hash_map.values().for_each(|v| println!("{}", v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for v in hash_map.values() { .. }`
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:32:5
+   |
+LL |     hash_set.iter().for_each(|v| println!("{}", v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for v in hash_set.iter() { .. }`
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:36:5
+   |
+LL |     btree_set.iter().for_each(|v| println!("{}", v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for v in btree_set.iter() { .. }`
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:40:5
+   |
+LL |     binary_heap.iter().for_each(|v| println!("{}", v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for v in binary_heap.iter() { .. }`
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:44:5
+   |
+LL |     s.iter().for_each(|v| println!("{}", v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for v in s.iter() { .. }`
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:47:5
+   |
+LL |     vec.as_slice().iter().for_each(|v| println!("{}", v));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for v in vec.as_slice().iter() { .. }`
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:50: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:52:13
+   |
+LL |             return;
+   |             ^^^^^^
+
+error: excessive use of `for_each`
+  --> $DIR/excessive_for_each.rs:59:5
+   |
+LL | /     vec.iter().for_each(|v| {
+LL | |         for i in 0..*v {
+LL | |             if i == 10 {
+LL | |                 return;
+...  |
+LL | |         }
+LL | |     });
+   | |______^ help: try: `'outer: for v in vec.iter() { .. }`
+   |
+note: change `return` to `continue 'outer` in the loop body
+  --> $DIR/excessive_for_each.rs:62:17
+   |
+LL |                 return;
+   |                 ^^^^^^
+note: change `return` to `continue` in the loop body
+  --> $DIR/excessive_for_each.rs:68:13
+   |
+LL |             return;
+   |             ^^^^^^
+
+error: aborting due to 15 previous errors
+