diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8c9247d97818..11716afe11cd 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 a7b0a1ca0820..f60b09898abc 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 0caa95a9f53f..a4d4937a19a1 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 = 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, + } + impl MyStruct { + fn iter(&self) -> impl Iterator { + 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, - } - impl MyStruct { - fn iter(&self) -> impl Iterator { - 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 a04243de27a0..b374128f2532 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 = 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, + } + impl MyStruct { + fn iter(&self) -> impl Iterator { + 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, - } - impl MyStruct { - fn iter(&self) -> impl Iterator { - 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 214e357a2080..483a5e6d61d7 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