From 0cbec62aaa20a7f546c1afd9c19b70e47eb4c9fc Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Tue, 9 Feb 2021 00:24:23 +0900 Subject: [PATCH] New Lint: excessive_for_each --- CHANGELOG.md | 1 + clippy_lints/src/iter_for_each.rs | 0 clippy_lints/src/lib.rs | 3 + .../src/methods/excessive_for_each.rs | 149 ++++++++++++++++++ clippy_lints/src/methods/mod.rs | 30 ++++ tests/ui/excessive_for_each.rs | 92 +++++++++++ tests/ui/excessive_for_each.stderr | 117 ++++++++++++++ 7 files changed, 392 insertions(+) create mode 100644 clippy_lints/src/iter_for_each.rs create mode 100644 clippy_lints/src/methods/excessive_for_each.rs create mode 100644 tests/ui/excessive_for_each.rs create mode 100644 tests/ui/excessive_for_each.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index b74841c77940..9f5ecaaf7218 100644 --- 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 new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fe4aa584b187..906d79169ebc 100644 --- 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 new file mode 100644 index 000000000000..2c12923b3c61 --- /dev/null +++ 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, ¬e.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 { + NestedVisitorMap::None + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 3e34fc1aed16..5e0c9061f66b 100644 --- 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 new file mode 100644 index 000000000000..931fdf4583f9 --- /dev/null +++ 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 = Vec::new(); + vec.iter().for_each(|v| println!("{}", v)); + + // Should trigger this lint. + let vec_deq: VecDeque = VecDeque::new(); + vec_deq.iter().for_each(|v| println!("{}", v)); + + // Should trigger this lint. + let list: LinkedList = LinkedList::new(); + list.iter().for_each(|v| println!("{}", v)); + + // Should trigger this lint. + let mut hash_map: HashMap = 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 = HashSet::new(); + hash_set.iter().for_each(|v| println!("{}", v)); + + // Should trigger this lint. + let btree_set: BTreeSet = BTreeSet::new(); + btree_set.iter().for_each(|v| println!("{}", v)); + + // Should trigger this lint. + let binary_heap: BinaryHeap = 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, +} + +impl MyCollection { + fn iter(&self) -> impl Iterator { + self.v.iter() + } +} diff --git a/tests/ui/excessive_for_each.stderr b/tests/ui/excessive_for_each.stderr new file mode 100644 index 000000000000..ef54962d3377 --- /dev/null +++ b/tests/ui/excessive_for_each.stderr @@ -0,0 +1,117 @@ +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_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:17: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:21: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:22: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:23: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:24: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:28: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:32: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:36: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:40: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:43: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:46: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:48:13 + | +LL | return; + | ^^^^^^ + +error: excessive use of `for_each` + --> $DIR/excessive_for_each.rs:55: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:58:17 + | +LL | return; + | ^^^^^^ +note: change `return` to `continue` in the loop body + --> $DIR/excessive_for_each.rs:64:13 + | +LL | return; + | ^^^^^^ + +error: aborting due to 14 previous errors +