diff --git a/CHANGELOG.md b/CHANGELOG.md index c35ab6c94bf..a2193f0eab9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2192,7 +2192,6 @@ 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 @@ -2370,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 diff --git a/clippy_lints/src/iter_for_each.rs b/clippy_lints/src/iter_for_each.rs deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2f2ccdb310a..8c9247d9781 100644 --- a/clippy_lints/src/lib.rs +++ b/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; @@ -781,7 +782,6 @@ 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, @@ -868,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, @@ -1046,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); @@ -1314,7 +1316,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&matches::WILDCARD_ENUM_MATCH_ARM), LintId::of(&mem_forget::MEM_FORGET), LintId::of(&methods::CLONE_ON_REF_PTR), - LintId::of(&methods::EXCESSIVE_FOR_EACH), LintId::of(&methods::EXPECT_USED), LintId::of(&methods::FILETYPE_IS_FILE), LintId::of(&methods::GET_UNWRAP), @@ -1325,6 +1326,7 @@ 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), diff --git a/clippy_lints/src/methods/excessive_for_each.rs b/clippy_lints/src/methods/excessive_for_each.rs deleted file mode 100644 index bcb7cf1491f..00000000000 --- a/clippy_lints/src/methods/excessive_for_each.rs +++ /dev/null @@ -1,122 +0,0 @@ -use rustc_errors::Applicability; -use rustc_hir::{ - intravisit::{walk_expr, NestedVisitorMap, Visitor}, - Expr, ExprKind, -}; -use rustc_lint::LateContext; -use rustc_middle::hir::map::Map; -use rustc_span::source_map::Span; - -use if_chain::if_chain; - -use crate::utils::{has_iter_method, match_trait_method, paths, snippet, span_lint_and_then}; - -use super::EXCESSIVE_FOR_EACH; - -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 has_iter_method(cx, cx.typeck_results().expr_ty(iter_receiver)).is_some(); - if match_trait_method(cx, expr, &paths::ITERATOR); - 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::new(); - 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 sugg = format!( - "for {} in {} {{ .. }}", - snippet(cx, body.params[0].pat.span, ".."), - snippet(cx, for_each_receiver.span, "..") - ); - - span_lint_and_then( - cx, - EXCESSIVE_FOR_EACH, - expr.span, - "excessive use of `for_each`", - |diag| { - diag.span_suggestion(expr.span, "try", sugg, Applicability::HasPlaceholders); - for span in ret_collector.spans { - diag.span_note(span, "change `return` to `continue` in the loop body"); - } - } - ) - } - } -} - -/// 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`. -struct RetCollector { - spans: Vec, - ret_in_loop: bool, - - loop_depth: u16, -} - -impl RetCollector { - fn new() -> Self { - Self { - spans: Vec::new(), - ret_in_loop: false, - loop_depth: 0, - } - } -} - -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 { - NestedVisitorMap::None - } -} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index f0c99528fe1..fccdee07877 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -974,33 +974,6 @@ declare_clippy_lint! { "using `.skip(x).next()` on an iterator" } -declare_clippy_lint! { - /// **What it does:** Checks for use of `.method(..).for_each(closure)` if the reciever of `.method(..)` doesn't - /// implement `Iterator` and the return type of `.method(..)` implements `Iterator`. - /// - /// **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, - restriction, - "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` @@ -1688,7 +1661,6 @@ impl_lint_pass!(Methods => [ ITER_NTH_ZERO, BYTES_NTH, ITER_SKIP_NEXT, - EXCESSIVE_FOR_EACH, GET_UNWRAP, STRING_EXTEND_CHARS, ITER_CLONED_COLLECT, @@ -1835,7 +1807,6 @@ 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), _ => {}, } diff --git a/clippy_lints/src/needless_for_each.rs b/clippy_lints/src/needless_for_each.rs new file mode 100644 index 00000000000..a7b0a1ca082 --- /dev/null +++ b/clippy_lints/src/needless_for_each.rs @@ -0,0 +1,170 @@ +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 crate::utils::{ + has_iter_method, is_diagnostic_assoc_item, method_calls, 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 + /// `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, + restriction, + "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, + StmtKind::Local(local) if local.init.is_some() => local.init.unwrap(), + _ => 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); + // 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::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; + } + + // We can't use `Applicability::MachineApplicable` when the closure contains `return` + // because `Diagnostic::multipart_suggestion` doesn't work with multiple overlapped + // spans. + let mut applicability = if ret_collector.spans.is_empty() { + Applicability::MachineApplicable + } else { + Applicability::MaybeIncorrect + }; + + let mut suggs = vec![]; + suggs.push((stmt.span, format!( + "for {} in {} {}", + snippet_with_applicability(cx, body.params[0].pat.span, "..", &mut applicability), + snippet_with_applicability(cx, for_each_args[0].span, "..", &mut applicability), + snippet_with_applicability(cx, body.value.span, "..", &mut applicability), + ))); + + for span in &ret_collector.spans { + suggs.push((*span, "return".to_string())); + } + + span_lint_and_then( + cx, + NEEDLESS_FOR_EACH, + stmt.span, + "needless use of `for_each`", + |diag| { + diag.multipart_suggestion("try", suggs, applicability); + // `Diagnostic::multipart_suggestion` ignores the second and subsequent overlapped spans, + // so `span_note` is needed here even though `suggs` includes the replacements. + for span in ret_collector.spans { + diag.span_note(span, "replace `return` with `continue`"); + } + } + ) + } + } + } +} + +/// 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, + 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 { + NestedVisitorMap::None + } +} diff --git a/tests/ui/excessive_for_each.rs b/tests/ui/excessive_for_each.rs deleted file mode 100644 index 0800bef71e9..00000000000 --- a/tests/ui/excessive_for_each.rs +++ /dev/null @@ -1,126 +0,0 @@ -#![warn(clippy::excessive_for_each)] -#![allow(clippy::needless_return)] - -use std::collections::*; - -fn main() { - // Should trigger this lint: Vec. - let vec: Vec = Vec::new(); - let mut acc = 0; - vec.iter().for_each(|v| { - acc += v; - }); - - // Should trigger this lint: &Vec. - let vec_ref = &vec; - vec_ref.iter().for_each(|v| { - acc += v; - }); - - // Should trigger this lint: VecDeque. - let vec_deq: VecDeque = VecDeque::new(); - vec_deq.iter().for_each(|v| { - acc += v; - }); - - // Should trigger this lint: LinkedList. - let list: LinkedList = LinkedList::new(); - list.iter().for_each(|v| { - acc += v; - }); - - // Should trigger this lint: HashMap. - let mut hash_map: HashMap = 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; - }); - - // Should trigger this lint: HashSet. - let hash_set: HashSet = HashSet::new(); - hash_set.iter().for_each(|v| { - acc += v; - }); - - // Should trigger this lint: BTreeSet. - let btree_set: BTreeSet = BTreeSet::new(); - btree_set.iter().for_each(|v| { - acc += v; - }); - - // Should trigger this lint: BinaryHeap. - let binary_heap: BinaryHeap = BinaryHeap::new(); - binary_heap.iter().for_each(|v| { - acc += v; - }); - - // Should trigger this lint: Array. - let s = [1, 2, 3]; - s.iter().for_each(|v| { - acc += v; - }); - - // Should trigger this lint. Slice. - vec.as_slice().iter().for_each(|v| { - acc += 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 NOT trigger this lint in case `return` is used in `Loop` of the closure. - 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)); - - // Should NOT trigger this lint in case the closure body is not a `ExprKind::Block`. - vec.iter().for_each(|x| acc += x); -} - -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 deleted file mode 100644 index f5799484e03..00000000000 --- a/tests/ui/excessive_for_each.stderr +++ /dev/null @@ -1,126 +0,0 @@ -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:10:5 - | -LL | / vec.iter().for_each(|v| { -LL | | acc += v; -LL | | }); - | |______^ 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:16:5 - | -LL | / vec_ref.iter().for_each(|v| { -LL | | acc += v; -LL | | }); - | |______^ help: try: `for v in vec_ref.iter() { .. }` - -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:22:5 - | -LL | / vec_deq.iter().for_each(|v| { -LL | | acc += v; -LL | | }); - | |______^ help: try: `for v in vec_deq.iter() { .. }` - -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:28:5 - | -LL | / list.iter().for_each(|v| { -LL | | acc += v; -LL | | }); - | |______^ help: try: `for v in list.iter() { .. }` - -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:34:5 - | -LL | / hash_map.iter().for_each(|(k, v)| { -LL | | acc += k + v; -LL | | }); - | |______^ help: try: `for (k, v) in hash_map.iter() { .. }` - -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:37:5 - | -LL | / hash_map.iter_mut().for_each(|(k, v)| { -LL | | acc += *k + *v; -LL | | }); - | |______^ help: try: `for (k, v) in hash_map.iter_mut() { .. }` - -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:40:5 - | -LL | / hash_map.keys().for_each(|k| { -LL | | acc += k; -LL | | }); - | |______^ help: try: `for k in hash_map.keys() { .. }` - -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:43:5 - | -LL | / hash_map.values().for_each(|v| { -LL | | acc += v; -LL | | }); - | |______^ help: try: `for v in hash_map.values() { .. }` - -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:49:5 - | -LL | / hash_set.iter().for_each(|v| { -LL | | acc += v; -LL | | }); - | |______^ help: try: `for v in hash_set.iter() { .. }` - -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:55:5 - | -LL | / btree_set.iter().for_each(|v| { -LL | | acc += v; -LL | | }); - | |______^ help: try: `for v in btree_set.iter() { .. }` - -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:61:5 - | -LL | / binary_heap.iter().for_each(|v| { -LL | | acc += v; -LL | | }); - | |______^ help: try: `for v in binary_heap.iter() { .. }` - -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:67:5 - | -LL | / s.iter().for_each(|v| { -LL | | acc += v; -LL | | }); - | |______^ help: try: `for v in s.iter() { .. }` - -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:72:5 - | -LL | / vec.as_slice().iter().for_each(|v| { -LL | | acc += v; -LL | | }); - | |______^ help: try: `for v in vec.as_slice().iter() { .. }` - -error: excessive use of `for_each` - --> $DIR/excessive_for_each.rs:77: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:79:13 - | -LL | return; - | ^^^^^^ - -error: aborting due to 14 previous errors - diff --git a/tests/ui/needless_for_each_fixable.fixed b/tests/ui/needless_for_each_fixable.fixed new file mode 100644 index 00000000000..0caa95a9f53 --- /dev/null +++ b/tests/ui/needless_for_each_fixable.fixed @@ -0,0 +1,99 @@ +// 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 = Vec::new(); + let mut acc = 0; + for elem in v.iter() { + acc += elem; + } + for elem in v.into_iter() { + acc += elem; + } + + let mut hash_map: HashMap = 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 { + Vec::new() + } + for elem in my_vec().iter() { + acc += elem; + } +} + +fn should_not_lint() { + let v: Vec = Vec::new(); + let mut acc = 0; + + // `for_each` argument is not closure. + fn print(x: &i32) { + println!("{}", x); + } + v.iter().for_each(print); + + // `for_each` follows long iterator chain. + v.iter().chain(v.iter()).for_each(|v| println!("{}", v)); + v.as_slice().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); + } + }); + + // 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| { + acc += elem; + }); + + // `for_each` is not directly in a statement. + match 1 { + _ => v.iter().for_each(|elem| { + acc += elem; + }), + } +} + +fn main() {} diff --git a/tests/ui/needless_for_each_fixable.rs b/tests/ui/needless_for_each_fixable.rs new file mode 100644 index 00000000000..a04243de27a --- /dev/null +++ b/tests/ui/needless_for_each_fixable.rs @@ -0,0 +1,99 @@ +// 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 = Vec::new(); + let mut acc = 0; + v.iter().for_each(|elem| { + acc += elem; + }); + v.into_iter().for_each(|elem| { + acc += elem; + }); + + let mut hash_map: HashMap = 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 { + Vec::new() + } + my_vec().iter().for_each(|elem| { + acc += elem; + }); +} + +fn should_not_lint() { + let v: Vec = Vec::new(); + let mut acc = 0; + + // `for_each` argument is not closure. + fn print(x: &i32) { + println!("{}", x); + } + v.iter().for_each(print); + + // `for_each` follows long iterator chain. + v.iter().chain(v.iter()).for_each(|v| println!("{}", v)); + v.as_slice().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); + } + }); + + // 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| { + acc += elem; + }); + + // `for_each` is not directly in a statement. + match 1 { + _ => v.iter().for_each(|elem| { + acc += elem; + }), + } +} + +fn main() {} diff --git a/tests/ui/needless_for_each_fixable.stderr b/tests/ui/needless_for_each_fixable.stderr new file mode 100644 index 00000000000..214e357a208 --- /dev/null +++ b/tests/ui/needless_for_each_fixable.stderr @@ -0,0 +1,108 @@ +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:18: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:21: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:24: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:27: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:34: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 7 previous errors + diff --git a/tests/ui/needless_for_each_unfixable.rs b/tests/ui/needless_for_each_unfixable.rs new file mode 100644 index 00000000000..d765d7dab65 --- /dev/null +++ b/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 = Vec::new(); + // This is unfixable because the closure includes `return`. + v.iter().for_each(|v| { + if *v == 10 { + return; + } else { + println!("{}", v); + } + }); +} diff --git a/tests/ui/needless_for_each_unfixable.stderr b/tests/ui/needless_for_each_unfixable.stderr new file mode 100644 index 00000000000..58d107062bc --- /dev/null +++ b/tests/ui/needless_for_each_unfixable.stderr @@ -0,0 +1,30 @@ +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` +note: replace `return` with `continue` + --> $DIR/needless_for_each_unfixable.rs:9:13 + | +LL | return; + | ^^^^^^ +help: try + | +LL | for v in v.iter() { +LL | if *v == 10 { +LL | return; +LL | } else { +LL | println!("{}", v); +LL | } + ... + +error: aborting due to previous error +