From 05bb6e6bdb1894de5803f729339a631a9222499f Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Mon, 20 Jul 2020 08:58:55 -0700 Subject: [PATCH 1/7] Create test for wanted behavior --- tests/ui/needless_collect.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs index 7ee603afeb07..1577e7a46edd 100644 --- a/tests/ui/needless_collect.rs +++ b/tests/ui/needless_collect.rs @@ -8,6 +8,9 @@ use std::collections::{BTreeSet, HashMap, HashSet}; #[allow(unused_variables, clippy::iter_cloned_collect)] fn main() { let sample = [1; 5]; + let indirect_with_into_iter = sample.iter().collect::>(); + let indirect_with_iter = sample.iter().collect::>();; + let indirect_negative = sample.iter().collect::>();; let len = sample.iter().collect::>().len(); if sample.iter().collect::>().is_empty() { // Empty @@ -18,4 +21,8 @@ fn main() { sample.iter().collect::>().len(); // Neither should this sample.iter().collect::>().len(); + indirect_with_into_iter.into_iter().map(|x| (x, x+1)).collect::>(); + indirect_with_iter.iter().map(|x| (x, x+1)).collect::>(); + indirect_negative.iter().map(|x| (x, x+1)).collect::>(); + indirect_negative.iter().map(|x| (x, x+1)).collect::>(); } From 3ee61373fe056efb46b6b1b243b31cec0d7e6099 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Wed, 22 Jul 2020 22:46:23 -0700 Subject: [PATCH 2/7] Write the lint and write tests --- clippy_lints/src/loops.rs | 107 +++++++++++++++++++++++++++++-- tests/ui/needless_collect.fixed | 11 ++++ tests/ui/needless_collect.rs | 18 ++++-- tests/ui/needless_collect.stderr | 17 ++++- 4 files changed, 137 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 7e3876ff49b4..231c440463d6 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1,14 +1,15 @@ use crate::consts::constant; use crate::reexport::Name; use crate::utils::paths; +use crate::utils::sugg::Sugg; use crate::utils::usage::{is_unused, mutated_variables}; use crate::utils::{ get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, - is_integer_const, is_no_std_crate, is_refutable, last_path_segment, match_trait_method, match_type, match_var, - multispan_sugg, snippet, snippet_opt, snippet_with_applicability, span_lint, span_lint_and_help, - span_lint_and_sugg, span_lint_and_then, SpanlessEq, + is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, last_path_segment, match_path, + match_trait_method, match_type, match_var, multispan_sugg, qpath_res, snippet, snippet_opt, + snippet_with_applicability, span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, + SpanlessEq, }; -use crate::utils::{is_type_diagnostic_item, qpath_res, sugg}; use if_chain::if_chain; use rustc_ast::ast; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; @@ -17,7 +18,7 @@ use rustc_hir::def::{DefKind, Res}; use rustc_hir::intravisit::{walk_block, walk_expr, walk_pat, walk_stmt, NestedVisitorMap, Visitor}; use rustc_hir::{ def_id, BinOpKind, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, GenericArg, HirId, InlineAsmOperand, - LoopSource, MatchSource, Mutability, Node, Pat, PatKind, QPath, Stmt, StmtKind, + Local, LoopSource, MatchSource, Mutability, Node, Pat, PatKind, QPath, Stmt, StmtKind, }; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{LateContext, LateLintPass, LintContext}; @@ -27,7 +28,7 @@ use rustc_middle::middle::region; use rustc_middle::ty::{self, Ty, TyS}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; -use rustc_span::symbol::Symbol; +use rustc_span::symbol::{Ident, Symbol}; use rustc_typeck::expr_use_visitor::{ConsumeMode, Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; use std::iter::{once, Iterator}; use std::mem; @@ -2358,6 +2359,7 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> { const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed"; fn check_needless_collect<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { + // Check for direct, immediate usage if_chain! { if let ExprKind::MethodCall(ref method, _, ref args, _) = expr.kind; if let ExprKind::MethodCall(ref chain_method, _, _, _) = args[0].kind; @@ -2423,6 +2425,99 @@ fn check_needless_collect<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { } } } + // Check for collecting it and then turning it back into an iterator later + if let ExprKind::Block(ref block, _) = expr.kind { + for ref stmt in block.stmts { + if_chain! { + // TODO also work for assignments to an existing variable + if let StmtKind::Local( + Local { pat: Pat { kind: PatKind::Binding(_, _, ident, .. ), .. }, + init: Some(ref init_expr), .. } + ) = stmt.kind; + if let ExprKind::MethodCall(ref method_name, _, &[ref iter_source], ..) = init_expr.kind; + if method_name.ident.name == sym!(collect) && match_trait_method(cx, &init_expr, &paths::ITERATOR); + if let Some(ref generic_args) = method_name.args; + if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0); + if let ty = cx.typeck_results().node_type(ty.hir_id); + if is_type_diagnostic_item(cx, ty, sym!(vec_type)) || + is_type_diagnostic_item(cx, ty, sym!(vecdeque_type)) || + match_type(cx, ty, &paths::LINKED_LIST); + if let Some(iter_calls) = detect_iter_and_into_iters(block, *ident); + if iter_calls.len() == 1; + then { + // Suggest replacing iter_call with iter_replacement, and removing stmt + span_lint_and_then( + cx, + NEEDLESS_COLLECT, + stmt.span, + NEEDLESS_COLLECT_MSG, + |diag| { + let iter_replacement = Sugg::hir(cx, iter_source, "..").to_string(); + diag.multipart_suggestion( + "Use the original Iterator instead of collecting it and then producing a new one", + vec![ + (stmt.span, String::new()), + (iter_calls[0].span, iter_replacement) + ], + Applicability::MaybeIncorrect, + ); + }, + ); + } + } + } + } +} + +struct IntoIterVisitor<'tcx> { + iters: Vec<&'tcx Expr<'tcx>>, + seen_other: bool, + target: String, +} +impl<'tcx> Visitor<'tcx> for IntoIterVisitor<'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + match &expr.kind { + ExprKind::MethodCall( + method_name, + _, + &[Expr { + kind: ExprKind::Path(QPath::Resolved(_, ref path)), + .. + }], + _, + ) if match_path(path, &[&self.target]) => { + // TODO Check what method is being called, if it's called on target, and act + // accordingly + if method_name.ident.name == sym!(into_iter) { + self.iters.push(expr); + } else { + self.seen_other = true; + } + }, + _ => walk_expr(self, expr), + } + } + + type Map = Map<'tcx>; + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } +} + +/// Detect the occurences of calls to `iter` or `into_iter` for the +/// given identifier +fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, identifier: Ident) -> Option>> { + let mut visitor = IntoIterVisitor { + iters: Vec::new(), + target: identifier.name.to_ident_string(), + seen_other: false, + }; + visitor.visit_block(block); + if visitor.seen_other { + None + } else { + Some(visitor.iters) + } } fn shorten_span(expr: &Expr<'_>, target_fn_name: Symbol) -> Span { diff --git a/tests/ui/needless_collect.fixed b/tests/ui/needless_collect.fixed index be37dc16b9a3..60a3e206283f 100644 --- a/tests/ui/needless_collect.fixed +++ b/tests/ui/needless_collect.fixed @@ -18,4 +18,15 @@ fn main() { sample.iter().collect::>().len(); // Neither should this sample.iter().collect::>().len(); + let indirect_positive = sample.iter().collect::>(); + indirect_positive + .into_iter() + .map(|x| (x, x + 1)) + .collect::>(); + let indirect_negative = sample.iter().collect::>(); + indirect_negative.len(); + indirect_negative + .iter() + .map(|x| (*x, *x + 1)) + .collect::>(); } diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs index 1577e7a46edd..33a1ea360959 100644 --- a/tests/ui/needless_collect.rs +++ b/tests/ui/needless_collect.rs @@ -8,9 +8,6 @@ use std::collections::{BTreeSet, HashMap, HashSet}; #[allow(unused_variables, clippy::iter_cloned_collect)] fn main() { let sample = [1; 5]; - let indirect_with_into_iter = sample.iter().collect::>(); - let indirect_with_iter = sample.iter().collect::>();; - let indirect_negative = sample.iter().collect::>();; let len = sample.iter().collect::>().len(); if sample.iter().collect::>().is_empty() { // Empty @@ -21,8 +18,15 @@ fn main() { sample.iter().collect::>().len(); // Neither should this sample.iter().collect::>().len(); - indirect_with_into_iter.into_iter().map(|x| (x, x+1)).collect::>(); - indirect_with_iter.iter().map(|x| (x, x+1)).collect::>(); - indirect_negative.iter().map(|x| (x, x+1)).collect::>(); - indirect_negative.iter().map(|x| (x, x+1)).collect::>(); + let indirect_positive = sample.iter().collect::>(); + indirect_positive + .into_iter() + .map(|x| (x, x + 1)) + .collect::>(); + let indirect_negative = sample.iter().collect::>(); + indirect_negative.len(); + indirect_negative + .iter() + .map(|x| (*x, *x + 1)) + .collect::>(); } diff --git a/tests/ui/needless_collect.stderr b/tests/ui/needless_collect.stderr index 9113aad90dd7..bb67bfa83e91 100644 --- a/tests/ui/needless_collect.stderr +++ b/tests/ui/needless_collect.stderr @@ -1,10 +1,21 @@ +error: avoid using `collect()` when not needed + --> $DIR/needless_collect.rs:21:5 + | +LL | let indirect_positive = sample.iter().collect::>(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::needless-collect` implied by `-D warnings` +help: Use the original Iterator instead of collecting it and then producing a new one + | +LL | +LL | sample.iter() + | + error: avoid using `collect()` when not needed --> $DIR/needless_collect.rs:11:29 | LL | let len = sample.iter().collect::>().len(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `count()` - | - = note: `-D clippy::needless-collect` implied by `-D warnings` error: avoid using `collect()` when not needed --> $DIR/needless_collect.rs:12:15 @@ -24,5 +35,5 @@ error: avoid using `collect()` when not needed LL | sample.iter().map(|x| (x, x)).collect::>().len(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `count()` -error: aborting due to 4 previous errors +error: aborting due to 5 previous errors From 3657c92ac978f69667b9c8bb46e51bc602b3d7ee Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Thu, 23 Jul 2020 09:14:10 -0700 Subject: [PATCH 3/7] Check for other things which can be used indirectly --- clippy_lints/src/loops.rs | 94 ++++++++++++++++++++++++++------------- 1 file changed, 62 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 231c440463d6..11a9b1e531c8 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -2429,7 +2429,6 @@ fn check_needless_collect<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { if let ExprKind::Block(ref block, _) = expr.kind { for ref stmt in block.stmts { if_chain! { - // TODO also work for assignments to an existing variable if let StmtKind::Local( Local { pat: Pat { kind: PatKind::Binding(_, _, ident, .. ), .. }, init: Some(ref init_expr), .. } @@ -2446,21 +2445,22 @@ fn check_needless_collect<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { if iter_calls.len() == 1; then { // Suggest replacing iter_call with iter_replacement, and removing stmt + let iter_call = &iter_calls[0]; span_lint_and_then( cx, NEEDLESS_COLLECT, - stmt.span, + stmt.span.until(iter_call.span), NEEDLESS_COLLECT_MSG, |diag| { - let iter_replacement = Sugg::hir(cx, iter_source, "..").to_string(); + let iter_replacement = format!("{}{}", Sugg::hir(cx, iter_source, ".."), iter_call.get_iter_method(cx)); diag.multipart_suggestion( - "Use the original Iterator instead of collecting it and then producing a new one", + iter_call.get_suggestion_text(), vec![ (stmt.span, String::new()), - (iter_calls[0].span, iter_replacement) + (iter_call.span, iter_replacement) ], - Applicability::MaybeIncorrect, - ); + Applicability::MachineApplicable,// MaybeIncorrect, + ).emit(); }, ); } @@ -2469,32 +2469,62 @@ fn check_needless_collect<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { } } -struct IntoIterVisitor<'tcx> { - iters: Vec<&'tcx Expr<'tcx>>, +struct IterFunction { + func: IterFunctionKind, + span: Span, +} +impl IterFunction { + fn get_iter_method(&self, cx: &LateContext<'_>) -> String { + match &self.func { + IterFunctionKind::IntoIter => String::new(), + IterFunctionKind::Len => String::from(".count()"), + IterFunctionKind::IsEmpty => String::from(".next().is_none()"), + IterFunctionKind::Contains(span) => format!(".any(|x| x == {})", snippet(cx, *span, "..")), + } + } + fn get_suggestion_text(&self) -> &'static str { + match &self.func { + IterFunctionKind::IntoIter => "Use the original Iterator instead of collecting it and then producing a new one", + IterFunctionKind::Len => "Take the original Iterator's count instead of collecting it and finding the length", + IterFunctionKind::IsEmpty => "Check if the original Iterator has anything instead of collecting it and seeing if it's empty", + IterFunctionKind::Contains(_) => "Check if the original Iterator contains an element instead of collecting then checking", + } + } +} +enum IterFunctionKind { + IntoIter, + Len, + IsEmpty, + Contains(Span), +} + +struct IterFunctionVisitor { + uses: Vec, seen_other: bool, target: String, } -impl<'tcx> Visitor<'tcx> for IntoIterVisitor<'tcx> { +impl<'tcx> Visitor<'tcx> for IterFunctionVisitor { fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { - match &expr.kind { - ExprKind::MethodCall( - method_name, - _, - &[Expr { - kind: ExprKind::Path(QPath::Resolved(_, ref path)), - .. - }], - _, - ) if match_path(path, &[&self.target]) => { - // TODO Check what method is being called, if it's called on target, and act - // accordingly - if method_name.ident.name == sym!(into_iter) { - self.iters.push(expr); - } else { - self.seen_other = true; + if_chain! { + if let ExprKind::MethodCall(method_name, _, ref args, _) = &expr.kind; + if let Some(Expr { kind: ExprKind::Path(QPath::Resolved(_, ref path)), .. }) = args.get(0); + if match_path(path, &[&self.target]); + then { + let into_iter = sym!(into_iter); + let len = sym!(len); + let is_empty = sym!(is_empty); + let contains = sym!(contains); + match method_name.ident.name { + name if name == into_iter => self.uses.push(IterFunction { func: IterFunctionKind::IntoIter, span: expr.span }), + name if name == len => self.uses.push(IterFunction { func: IterFunctionKind::Len, span: expr.span }), + name if name == is_empty => self.uses.push(IterFunction { func: IterFunctionKind::IsEmpty, span: expr.span }), + name if name == contains => self.uses.push(IterFunction { func: IterFunctionKind::Contains(args[1].span), span: expr.span }), + _ => self.seen_other = true, } - }, - _ => walk_expr(self, expr), + } + else { + walk_expr(self, expr); + } } } @@ -2506,9 +2536,9 @@ impl<'tcx> Visitor<'tcx> for IntoIterVisitor<'tcx> { /// Detect the occurences of calls to `iter` or `into_iter` for the /// given identifier -fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, identifier: Ident) -> Option>> { - let mut visitor = IntoIterVisitor { - iters: Vec::new(), +fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, identifier: Ident) -> Option> { + let mut visitor = IterFunctionVisitor { + uses: Vec::new(), target: identifier.name.to_ident_string(), seen_other: false, }; @@ -2516,7 +2546,7 @@ fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, identifier: Ident) if visitor.seen_other { None } else { - Some(visitor.iters) + Some(visitor.uses) } } From c86f4109fdd83fef1ea69c0f3c878ace0aa7c56f Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Thu, 23 Jul 2020 09:15:16 -0700 Subject: [PATCH 4/7] Split indirect collects into their own test case --- tests/ui/needless_collect.fixed | 11 ----- tests/ui/needless_collect.rs | 11 ----- tests/ui/needless_collect.stderr | 17 ++----- tests/ui/needless_collect_indirect.fixed | 26 +++++++++++ tests/ui/needless_collect_indirect.rs | 26 +++++++++++ tests/ui/needless_collect_indirect.stderr | 55 +++++++++++++++++++++++ 6 files changed, 110 insertions(+), 36 deletions(-) create mode 100644 tests/ui/needless_collect_indirect.fixed create mode 100644 tests/ui/needless_collect_indirect.rs create mode 100644 tests/ui/needless_collect_indirect.stderr diff --git a/tests/ui/needless_collect.fixed b/tests/ui/needless_collect.fixed index 60a3e206283f..be37dc16b9a3 100644 --- a/tests/ui/needless_collect.fixed +++ b/tests/ui/needless_collect.fixed @@ -18,15 +18,4 @@ fn main() { sample.iter().collect::>().len(); // Neither should this sample.iter().collect::>().len(); - let indirect_positive = sample.iter().collect::>(); - indirect_positive - .into_iter() - .map(|x| (x, x + 1)) - .collect::>(); - let indirect_negative = sample.iter().collect::>(); - indirect_negative.len(); - indirect_negative - .iter() - .map(|x| (*x, *x + 1)) - .collect::>(); } diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs index 33a1ea360959..7ee603afeb07 100644 --- a/tests/ui/needless_collect.rs +++ b/tests/ui/needless_collect.rs @@ -18,15 +18,4 @@ fn main() { sample.iter().collect::>().len(); // Neither should this sample.iter().collect::>().len(); - let indirect_positive = sample.iter().collect::>(); - indirect_positive - .into_iter() - .map(|x| (x, x + 1)) - .collect::>(); - let indirect_negative = sample.iter().collect::>(); - indirect_negative.len(); - indirect_negative - .iter() - .map(|x| (*x, *x + 1)) - .collect::>(); } diff --git a/tests/ui/needless_collect.stderr b/tests/ui/needless_collect.stderr index bb67bfa83e91..9113aad90dd7 100644 --- a/tests/ui/needless_collect.stderr +++ b/tests/ui/needless_collect.stderr @@ -1,21 +1,10 @@ -error: avoid using `collect()` when not needed - --> $DIR/needless_collect.rs:21:5 - | -LL | let indirect_positive = sample.iter().collect::>(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = note: `-D clippy::needless-collect` implied by `-D warnings` -help: Use the original Iterator instead of collecting it and then producing a new one - | -LL | -LL | sample.iter() - | - error: avoid using `collect()` when not needed --> $DIR/needless_collect.rs:11:29 | LL | let len = sample.iter().collect::>().len(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `count()` + | + = note: `-D clippy::needless-collect` implied by `-D warnings` error: avoid using `collect()` when not needed --> $DIR/needless_collect.rs:12:15 @@ -35,5 +24,5 @@ error: avoid using `collect()` when not needed LL | sample.iter().map(|x| (x, x)).collect::>().len(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `count()` -error: aborting due to 5 previous errors +error: aborting due to 4 previous errors diff --git a/tests/ui/needless_collect_indirect.fixed b/tests/ui/needless_collect_indirect.fixed new file mode 100644 index 000000000000..136af42a9fef --- /dev/null +++ b/tests/ui/needless_collect_indirect.fixed @@ -0,0 +1,26 @@ +// run-rustfix + +#[allow(unused)] + +use std::collections::{HashMap, VecDeque}; + +fn main() { + let sample = [1; 5]; + let indirect_iter = sample.iter().collect::>(); + indirect_iter + .into_iter() + .map(|x| (x, x + 1)) + .collect::>(); + let indirect_len = sample.iter().collect::>(); + indirect_len.len(); + let indirect_empty = sample.iter().collect::>(); + indirect_empty.is_empty(); + let indirect_contains = sample.iter().collect::>(); + indirect_contains.contains(&&5); + let indirect_negative = sample.iter().collect::>(); + indirect_negative.len(); + indirect_negative + .into_iter() + .map(|x| (*x, *x + 1)) + .collect::>(); +} diff --git a/tests/ui/needless_collect_indirect.rs b/tests/ui/needless_collect_indirect.rs new file mode 100644 index 000000000000..136af42a9fef --- /dev/null +++ b/tests/ui/needless_collect_indirect.rs @@ -0,0 +1,26 @@ +// run-rustfix + +#[allow(unused)] + +use std::collections::{HashMap, VecDeque}; + +fn main() { + let sample = [1; 5]; + let indirect_iter = sample.iter().collect::>(); + indirect_iter + .into_iter() + .map(|x| (x, x + 1)) + .collect::>(); + let indirect_len = sample.iter().collect::>(); + indirect_len.len(); + let indirect_empty = sample.iter().collect::>(); + indirect_empty.is_empty(); + let indirect_contains = sample.iter().collect::>(); + indirect_contains.contains(&&5); + let indirect_negative = sample.iter().collect::>(); + indirect_negative.len(); + indirect_negative + .into_iter() + .map(|x| (*x, *x + 1)) + .collect::>(); +} diff --git a/tests/ui/needless_collect_indirect.stderr b/tests/ui/needless_collect_indirect.stderr new file mode 100644 index 000000000000..5058c171ac23 --- /dev/null +++ b/tests/ui/needless_collect_indirect.stderr @@ -0,0 +1,55 @@ +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:9:5 + | +LL | / let indirect_iter = sample.iter().collect::>(); +LL | | indirect_iter + | |____^ + | + = note: `-D clippy::needless-collect` implied by `-D warnings` +help: Use the original Iterator instead of collecting it and then producing a new one + | +LL | +LL | sample.iter() + | + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:14:5 + | +LL | / let indirect_len = sample.iter().collect::>(); +LL | | indirect_len.len(); + | |____^ + | +help: Take the original Iterator's count instead of collecting it and finding the length + | +LL | +LL | sample.iter().count(); + | + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:16:5 + | +LL | / let indirect_empty = sample.iter().collect::>(); +LL | | indirect_empty.is_empty(); + | |____^ + | +help: Check if the original Iterator has anything instead of collecting it and seeing if it's empty + | +LL | +LL | sample.iter().next().is_none(); + | + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:18:5 + | +LL | / let indirect_contains = sample.iter().collect::>(); +LL | | indirect_contains.contains(&&5); + | |____^ + | +help: Check if the original Iterator contains an element instead of collecting then checking + | +LL | +LL | sample.iter().any(|x| x == &&5); + | + +error: aborting due to 4 previous errors + From a84948329459e650af4eb2f8cff8f55282316ea5 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Thu, 23 Jul 2020 09:44:44 -0700 Subject: [PATCH 5/7] Fix formatting and dogfood fallout --- clippy_lints/src/loops.rs | 43 +++++++++++++++++------ tests/ui/needless_collect_indirect.fixed | 6 +--- tests/ui/needless_collect_indirect.rs | 6 +--- tests/ui/needless_collect_indirect.stderr | 12 +++---- 4 files changed, 41 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 11a9b1e531c8..2181304f0064 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -2359,7 +2359,10 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> { const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed"; fn check_needless_collect<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { - // Check for direct, immediate usage + check_needless_collect_direct_usage(expr, cx); + check_needless_collect_indirect_usage(expr, cx); +} +fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { if_chain! { if let ExprKind::MethodCall(ref method, _, ref args, _) = expr.kind; if let ExprKind::MethodCall(ref chain_method, _, _, _) = args[0].kind; @@ -2425,7 +2428,9 @@ fn check_needless_collect<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { } } } - // Check for collecting it and then turning it back into an iterator later +} + +fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { if let ExprKind::Block(ref block, _) = expr.kind { for ref stmt in block.stmts { if_chain! { @@ -2484,10 +2489,18 @@ impl IterFunction { } fn get_suggestion_text(&self) -> &'static str { match &self.func { - IterFunctionKind::IntoIter => "Use the original Iterator instead of collecting it and then producing a new one", - IterFunctionKind::Len => "Take the original Iterator's count instead of collecting it and finding the length", - IterFunctionKind::IsEmpty => "Check if the original Iterator has anything instead of collecting it and seeing if it's empty", - IterFunctionKind::Contains(_) => "Check if the original Iterator contains an element instead of collecting then checking", + IterFunctionKind::IntoIter => { + "Use the original Iterator instead of collecting it and then producing a new one" + }, + IterFunctionKind::Len => { + "Take the original Iterator's count instead of collecting it and finding the length" + }, + IterFunctionKind::IsEmpty => { + "Check if the original Iterator has anything instead of collecting it and seeing if it's empty" + }, + IterFunctionKind::Contains(_) => { + "Check if the original Iterator contains an element instead of collecting then checking" + }, } } } @@ -2505,6 +2518,8 @@ struct IterFunctionVisitor { } impl<'tcx> Visitor<'tcx> for IterFunctionVisitor { fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + // TODO Check if the target identifier is being used in something other + // than a function call if_chain! { if let ExprKind::MethodCall(method_name, _, ref args, _) = &expr.kind; if let Some(Expr { kind: ExprKind::Path(QPath::Resolved(_, ref path)), .. }) = args.get(0); @@ -2515,10 +2530,18 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor { let is_empty = sym!(is_empty); let contains = sym!(contains); match method_name.ident.name { - name if name == into_iter => self.uses.push(IterFunction { func: IterFunctionKind::IntoIter, span: expr.span }), - name if name == len => self.uses.push(IterFunction { func: IterFunctionKind::Len, span: expr.span }), - name if name == is_empty => self.uses.push(IterFunction { func: IterFunctionKind::IsEmpty, span: expr.span }), - name if name == contains => self.uses.push(IterFunction { func: IterFunctionKind::Contains(args[1].span), span: expr.span }), + name if name == into_iter => self.uses.push( + IterFunction { func: IterFunctionKind::IntoIter, span: expr.span } + ), + name if name == len => self.uses.push( + IterFunction { func: IterFunctionKind::Len, span: expr.span } + ), + name if name == is_empty => self.uses.push( + IterFunction { func: IterFunctionKind::IsEmpty, span: expr.span } + ), + name if name == contains => self.uses.push( + IterFunction { func: IterFunctionKind::Contains(args[1].span), span: expr.span } + ), _ => self.seen_other = true, } } diff --git a/tests/ui/needless_collect_indirect.fixed b/tests/ui/needless_collect_indirect.fixed index 136af42a9fef..163eaf965dd6 100644 --- a/tests/ui/needless_collect_indirect.fixed +++ b/tests/ui/needless_collect_indirect.fixed @@ -1,16 +1,12 @@ // run-rustfix #[allow(unused)] - use std::collections::{HashMap, VecDeque}; fn main() { let sample = [1; 5]; let indirect_iter = sample.iter().collect::>(); - indirect_iter - .into_iter() - .map(|x| (x, x + 1)) - .collect::>(); + indirect_iter.into_iter().map(|x| (x, x + 1)).collect::>(); let indirect_len = sample.iter().collect::>(); indirect_len.len(); let indirect_empty = sample.iter().collect::>(); diff --git a/tests/ui/needless_collect_indirect.rs b/tests/ui/needless_collect_indirect.rs index 136af42a9fef..163eaf965dd6 100644 --- a/tests/ui/needless_collect_indirect.rs +++ b/tests/ui/needless_collect_indirect.rs @@ -1,16 +1,12 @@ // run-rustfix #[allow(unused)] - use std::collections::{HashMap, VecDeque}; fn main() { let sample = [1; 5]; let indirect_iter = sample.iter().collect::>(); - indirect_iter - .into_iter() - .map(|x| (x, x + 1)) - .collect::>(); + indirect_iter.into_iter().map(|x| (x, x + 1)).collect::>(); let indirect_len = sample.iter().collect::>(); indirect_len.len(); let indirect_empty = sample.iter().collect::>(); diff --git a/tests/ui/needless_collect_indirect.stderr b/tests/ui/needless_collect_indirect.stderr index 5058c171ac23..700c73b0b223 100644 --- a/tests/ui/needless_collect_indirect.stderr +++ b/tests/ui/needless_collect_indirect.stderr @@ -1,19 +1,19 @@ error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:9:5 + --> $DIR/needless_collect_indirect.rs:8:5 | LL | / let indirect_iter = sample.iter().collect::>(); -LL | | indirect_iter +LL | | indirect_iter.into_iter().map(|x| (x, x + 1)).collect::>(); | |____^ | = note: `-D clippy::needless-collect` implied by `-D warnings` help: Use the original Iterator instead of collecting it and then producing a new one | LL | -LL | sample.iter() +LL | sample.iter().map(|x| (x, x + 1)).collect::>(); | error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:14:5 + --> $DIR/needless_collect_indirect.rs:10:5 | LL | / let indirect_len = sample.iter().collect::>(); LL | | indirect_len.len(); @@ -26,7 +26,7 @@ LL | sample.iter().count(); | error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:16:5 + --> $DIR/needless_collect_indirect.rs:12:5 | LL | / let indirect_empty = sample.iter().collect::>(); LL | | indirect_empty.is_empty(); @@ -39,7 +39,7 @@ LL | sample.iter().next().is_none(); | error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:18:5 + --> $DIR/needless_collect_indirect.rs:14:5 | LL | / let indirect_contains = sample.iter().collect::>(); LL | | indirect_contains.contains(&&5); From bb2c14e92b5a0eafe9c9f43f3a02e33b544b3f91 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Thu, 23 Jul 2020 10:07:51 -0700 Subject: [PATCH 6/7] Fix a bug causing it to be too trigger-happy --- clippy_lints/src/loops.rs | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 2181304f0064..c931c212735f 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -5,10 +5,9 @@ use crate::utils::sugg::Sugg; use crate::utils::usage::{is_unused, mutated_variables}; use crate::utils::{ get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, - is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, last_path_segment, match_path, - match_trait_method, match_type, match_var, multispan_sugg, qpath_res, snippet, snippet_opt, - snippet_with_applicability, span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, - SpanlessEq, + is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, last_path_segment, match_trait_method, + match_type, match_var, multispan_sugg, qpath_res, snippet, snippet_opt, snippet_with_applicability, span_lint, + span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, SpanlessEq, }; use if_chain::if_chain; use rustc_ast::ast; @@ -2514,16 +2513,16 @@ enum IterFunctionKind { struct IterFunctionVisitor { uses: Vec, seen_other: bool, - target: String, + target: Ident, } impl<'tcx> Visitor<'tcx> for IterFunctionVisitor { fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { - // TODO Check if the target identifier is being used in something other - // than a function call + // Check function calls on our collection if_chain! { if let ExprKind::MethodCall(method_name, _, ref args, _) = &expr.kind; if let Some(Expr { kind: ExprKind::Path(QPath::Resolved(_, ref path)), .. }) = args.get(0); - if match_path(path, &[&self.target]); + if let &[name] = &path.segments; + if name.ident == self.target; then { let into_iter = sym!(into_iter); let len = sym!(len); @@ -2544,8 +2543,17 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor { ), _ => self.seen_other = true, } + return } - else { + } + // Check if the collection is used for anything else + if_chain! { + if let Expr { kind: ExprKind::Path(QPath::Resolved(_, ref path)), .. } = expr; + if let &[name] = &path.segments; + if name.ident == self.target; + then { + self.seen_other = true; + } else { walk_expr(self, expr); } } @@ -2562,7 +2570,7 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor { fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, identifier: Ident) -> Option> { let mut visitor = IterFunctionVisitor { uses: Vec::new(), - target: identifier.name.to_ident_string(), + target: identifier, seen_other: false, }; visitor.visit_block(block); From 5e10b039a3f215b48a54ce7dd38f95115f92e55a Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Sun, 2 Aug 2020 21:46:18 -0700 Subject: [PATCH 7/7] Implement review suggestions --- clippy_lints/src/loops.rs | 7 +++---- tests/ui/needless_collect_indirect.fixed | 22 ---------------------- tests/ui/needless_collect_indirect.rs | 3 --- tests/ui/needless_collect_indirect.stderr | 8 ++++---- 4 files changed, 7 insertions(+), 33 deletions(-) delete mode 100644 tests/ui/needless_collect_indirect.fixed diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index c931c212735f..e9ce804320fc 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -27,7 +27,7 @@ use rustc_middle::middle::region; use rustc_middle::ty::{self, Ty, TyS}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; -use rustc_span::symbol::{Ident, Symbol}; +use rustc_span::symbol::{sym, Ident, Symbol}; use rustc_typeck::expr_use_visitor::{ConsumeMode, Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; use std::iter::{once, Iterator}; use std::mem; @@ -2442,7 +2442,7 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo if let Some(ref generic_args) = method_name.args; if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0); if let ty = cx.typeck_results().node_type(ty.hir_id); - if is_type_diagnostic_item(cx, ty, sym!(vec_type)) || + if is_type_diagnostic_item(cx, ty, sym::vec_type) || is_type_diagnostic_item(cx, ty, sym!(vecdeque_type)) || match_type(cx, ty, &paths::LINKED_LIST); if let Some(iter_calls) = detect_iter_and_into_iters(block, *ident); @@ -2524,12 +2524,11 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor { if let &[name] = &path.segments; if name.ident == self.target; then { - let into_iter = sym!(into_iter); let len = sym!(len); let is_empty = sym!(is_empty); let contains = sym!(contains); match method_name.ident.name { - name if name == into_iter => self.uses.push( + sym::into_iter => self.uses.push( IterFunction { func: IterFunctionKind::IntoIter, span: expr.span } ), name if name == len => self.uses.push( diff --git a/tests/ui/needless_collect_indirect.fixed b/tests/ui/needless_collect_indirect.fixed deleted file mode 100644 index 163eaf965dd6..000000000000 --- a/tests/ui/needless_collect_indirect.fixed +++ /dev/null @@ -1,22 +0,0 @@ -// run-rustfix - -#[allow(unused)] -use std::collections::{HashMap, VecDeque}; - -fn main() { - let sample = [1; 5]; - let indirect_iter = sample.iter().collect::>(); - indirect_iter.into_iter().map(|x| (x, x + 1)).collect::>(); - let indirect_len = sample.iter().collect::>(); - indirect_len.len(); - let indirect_empty = sample.iter().collect::>(); - indirect_empty.is_empty(); - let indirect_contains = sample.iter().collect::>(); - indirect_contains.contains(&&5); - let indirect_negative = sample.iter().collect::>(); - indirect_negative.len(); - indirect_negative - .into_iter() - .map(|x| (*x, *x + 1)) - .collect::>(); -} diff --git a/tests/ui/needless_collect_indirect.rs b/tests/ui/needless_collect_indirect.rs index 163eaf965dd6..4cf03e820352 100644 --- a/tests/ui/needless_collect_indirect.rs +++ b/tests/ui/needless_collect_indirect.rs @@ -1,6 +1,3 @@ -// run-rustfix - -#[allow(unused)] use std::collections::{HashMap, VecDeque}; fn main() { diff --git a/tests/ui/needless_collect_indirect.stderr b/tests/ui/needless_collect_indirect.stderr index 700c73b0b223..0c1e61d74966 100644 --- a/tests/ui/needless_collect_indirect.stderr +++ b/tests/ui/needless_collect_indirect.stderr @@ -1,5 +1,5 @@ error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:8:5 + --> $DIR/needless_collect_indirect.rs:5:5 | LL | / let indirect_iter = sample.iter().collect::>(); LL | | indirect_iter.into_iter().map(|x| (x, x + 1)).collect::>(); @@ -13,7 +13,7 @@ LL | sample.iter().map(|x| (x, x + 1)).collect::>(); | error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:10:5 + --> $DIR/needless_collect_indirect.rs:7:5 | LL | / let indirect_len = sample.iter().collect::>(); LL | | indirect_len.len(); @@ -26,7 +26,7 @@ LL | sample.iter().count(); | error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:12:5 + --> $DIR/needless_collect_indirect.rs:9:5 | LL | / let indirect_empty = sample.iter().collect::>(); LL | | indirect_empty.is_empty(); @@ -39,7 +39,7 @@ LL | sample.iter().next().is_none(); | error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:14:5 + --> $DIR/needless_collect_indirect.rs:11:5 | LL | / let indirect_contains = sample.iter().collect::>(); LL | | indirect_contains.contains(&&5);