New Lint: excessive_for_each

Y-Nak · Y-Nak · commit 0cbec62aaa20 · 2021-02-09T20:57:07.000+09:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1937,6 +1937,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
diff --git a/clippy_lints/src/iter_for_each.rs b/clippy_lints/src/iter_for_each.rs
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -739,6 +739,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,
@@ -1535,6 +1536,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),
@@ -1751,6 +1753,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&mem_replace::MEM_REPLACE_WITH_DEFAULT),
         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),
diff --git a/clippy_lints/src/methods/excessive_for_each.rs b/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 {
+    for target in TARGET_ITER_RECEIVER_TY {
+        if match_type(cx, expr_ty, target) {
+            return true;
+        }
+    }
+
+    if_chain! {
+        if let ty::Ref(_, inner_ty, _) = expr_ty.kind();
+        if matches!(inner_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
+    }
+}
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -1,4 +1,5 @@
 mod bind_instead_of_map;
+mod excessive_for_each;
 mod filter_map_identity;
 mod inefficient_to_string;
 mod inspect_for_each;
@@ -927,6 +928,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`
@@ -1538,6 +1566,7 @@ impl_lint_pass!(Methods => [
     ITER_NTH,
     ITER_NTH_ZERO,
     ITER_SKIP_NEXT,
+    EXCESSIVE_FOR_EACH,
     GET_UNWRAP,
     STRING_EXTEND_CHARS,
     ITER_CLONED_COLLECT,
@@ -1645,6 +1674,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
             ["ok_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "ok_or"),
             ["collect", "map"] => lint_map_collect(cx, expr, arg_lists[1], arg_lists[0]),
             ["for_each", "inspect"] => inspect_for_each::lint(cx, expr, method_spans[1]),
+            ["for_each", ..] => excessive_for_each::lint(cx, expr, &arg_lists),
             _ => {},
         }
 
diff --git a/tests/ui/excessive_for_each.rs b/tests/ui/excessive_for_each.rs
@@ -0,0 +1,92 @@
+#![warn(clippy::excessive_for_each)]
+#![allow(clippy::needless_return)]
+
+use std::collections::*;
+
+fn main() {
+    // Should trigger this lint.
+    let vec: Vec<i32> = Vec::new();
+    vec.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint.
+    let vec_deq: VecDeque<i32> = VecDeque::new();
+    vec_deq.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint.
+    let list: LinkedList<i32> = LinkedList::new();
+    list.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint.
+    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.
+    let hash_set: HashSet<i32> = HashSet::new();
+    hash_set.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint.
+    let btree_set: BTreeSet<i32> = BTreeSet::new();
+    btree_set.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint.
+    let binary_heap: BinaryHeap<i32> = BinaryHeap::new();
+    binary_heap.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint.
+    let s = &[1, 2, 3];
+    s.iter().for_each(|v| println!("{}", v));
+
+    // Should trigger this lint.
+    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()
+    }
+}
diff --git a/tests/ui/excessive_for_each.stderr b/tests/ui/excessive_for_each.stderr
