From c0126939617dc86b0c5d29a712ce9e8fc67d2f0f Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Tue, 4 Feb 2025 13:01:32 -0700 Subject: needless_collect: avoid warning if non-iterator methods are used It can make sense to `collect()` an iterator and then immediately iterate over it if the iterator has special methods that you need. For example, the Map iterator doesn't implement Clone, but the collection iterator might. Or the collection iterator might support slicing. --- clippy_lints/src/methods/needless_collect.rs | 71 +++++++++++++++++++++++++--- tests/ui/needless_collect.fixed | 14 ++++++ tests/ui/needless_collect.rs | 14 ++++++ 3 files changed, 93 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/methods/needless_collect.rs b/clippy_lints/src/methods/needless_collect.rs index 2780c3f8af5..6732aeeb498 100644 --- a/clippy_lints/src/methods/needless_collect.rs +++ b/clippy_lints/src/methods/needless_collect.rs @@ -1,3 +1,5 @@ +use std::ops::ControlFlow; + use super::NEEDLESS_COLLECT; use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then}; use clippy_utils::source::{snippet, snippet_with_applicability}; @@ -9,9 +11,9 @@ use clippy_utils::{ }; use rustc_data_structures::fx::FxHashMap; use rustc_errors::{Applicability, MultiSpan}; -use rustc_hir::intravisit::{Visitor, walk_block, walk_expr}; +use rustc_hir::intravisit::{Visitor, walk_block, walk_expr, walk_stmt}; use rustc_hir::{ - BindingMode, Block, Expr, ExprKind, HirId, HirIdSet, LetStmt, Mutability, Node, PatKind, Stmt, StmtKind, + BindingMode, Block, Expr, ExprKind, HirId, HirIdSet, LetStmt, Mutability, Node, Pat, PatKind, Stmt, StmtKind, }; use rustc_lint::LateContext; use rustc_middle::hir::nested_filter; @@ -103,6 +105,12 @@ pub(super) fn check<'tcx>( return; } + if let IterFunctionKind::IntoIter(hir_id) = iter_call.func + && !check_iter_expr_used_only_as_iterator(cx, hir_id, block) + { + return; + } + // Suggest replacing iter_call with iter_replacement, and removing stmt let mut span = MultiSpan::from_span(name_span); span.push_span_label(iter_call.span, "the iterator could be used here instead"); @@ -253,7 +261,7 @@ struct IterFunction { impl IterFunction { fn get_iter_method(&self, cx: &LateContext<'_>) -> String { match &self.func { - IterFunctionKind::IntoIter => String::new(), + IterFunctionKind::IntoIter(_) => String::new(), IterFunctionKind::Len => String::from(".count()"), IterFunctionKind::IsEmpty => String::from(".next().is_none()"), IterFunctionKind::Contains(span) => { @@ -268,7 +276,7 @@ impl IterFunction { } fn get_suggestion_text(&self) -> &'static str { match &self.func { - IterFunctionKind::IntoIter => { + IterFunctionKind::IntoIter(_) => { "use the original Iterator instead of collecting it and then producing a new one" }, IterFunctionKind::Len => { @@ -284,7 +292,7 @@ impl IterFunction { } } enum IterFunctionKind { - IntoIter, + IntoIter(HirId), Len, IsEmpty, Contains(Span), @@ -343,7 +351,7 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor<'_, 'tcx> { } match method_name.ident.name.as_str() { "into_iter" => self.uses.push(Some(IterFunction { - func: IterFunctionKind::IntoIter, + func: IterFunctionKind::IntoIter(expr.hir_id), span: expr.span, })), "len" => self.uses.push(Some(IterFunction { @@ -520,3 +528,54 @@ fn get_captured_ids(cx: &LateContext<'_>, ty: Ty<'_>) -> HirIdSet { set } + +struct IteratorMethodCheckVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + hir_id_of_expr: HirId, + hir_id_of_let_binding: Option, +} + +impl<'tcx> Visitor<'tcx> for IteratorMethodCheckVisitor<'_, 'tcx> { + type Result = ControlFlow<()>; + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) -> ControlFlow<()> { + if let ExprKind::MethodCall(_method_name, recv, _args, _) = &expr.kind + && (recv.hir_id == self.hir_id_of_expr + || self + .hir_id_of_let_binding + .is_some_and(|hid| path_to_local_id(recv, hid))) + && !is_trait_method(self.cx, expr, sym::Iterator) + { + return ControlFlow::Break(()); + } + walk_expr(self, expr) + } + fn visit_stmt(&mut self, stmt: &'tcx Stmt<'tcx>) -> ControlFlow<()> { + if let StmtKind::Let(LetStmt { + init: Some(expr), + pat: + Pat { + kind: PatKind::Binding(BindingMode::NONE | BindingMode::MUT, id, _, None), + .. + }, + .. + }) = &stmt.kind + && expr.hir_id == self.hir_id_of_expr + { + self.hir_id_of_let_binding = Some(*id); + } + walk_stmt(self, stmt) + } +} + +fn check_iter_expr_used_only_as_iterator<'tcx>( + cx: &LateContext<'tcx>, + hir_id_of_expr: HirId, + block: &'tcx Block<'tcx>, +) -> bool { + let mut visitor = IteratorMethodCheckVisitor { + cx, + hir_id_of_expr, + hir_id_of_let_binding: None, + }; + visitor.visit_block(block).is_continue() +} diff --git a/tests/ui/needless_collect.fixed b/tests/ui/needless_collect.fixed index bd83581bdd9..e33b984a44b 100644 --- a/tests/ui/needless_collect.fixed +++ b/tests/ui/needless_collect.fixed @@ -73,6 +73,20 @@ fn main() { let mut out = vec![]; values.iter().cloned().map(|x| out.push(x)).collect::>(); let _y = values.iter().cloned().map(|x| out.push(x)).collect::>(); // this is fine + + // Don't write a warning if we call `clone()` on the iterator + // https://github.com/rust-lang/rust-clippy/issues/13430 + let my_collection: Vec<()> = vec![()].into_iter().map(|()| {}).collect(); + let _cloned = my_collection.into_iter().clone(); + let my_collection: Vec<()> = vec![()].into_iter().map(|()| {}).collect(); + let my_iter = my_collection.into_iter(); + let _cloned = my_iter.clone(); + // Same for `as_slice()`, for same reason. + let my_collection: Vec<()> = vec![()].into_iter().map(|()| {}).collect(); + let _sliced = my_collection.into_iter().as_slice(); + let my_collection: Vec<()> = vec![()].into_iter().map(|()| {}).collect(); + let my_iter = my_collection.into_iter(); + let _sliced = my_iter.as_slice(); } fn foo(_: impl IntoIterator) {} diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs index 6a81a767bbb..fab00d67dbf 100644 --- a/tests/ui/needless_collect.rs +++ b/tests/ui/needless_collect.rs @@ -73,6 +73,20 @@ fn main() { let mut out = vec![]; values.iter().cloned().map(|x| out.push(x)).collect::>(); let _y = values.iter().cloned().map(|x| out.push(x)).collect::>(); // this is fine + + // Don't write a warning if we call `clone()` on the iterator + // https://github.com/rust-lang/rust-clippy/issues/13430 + let my_collection: Vec<()> = vec![()].into_iter().map(|()| {}).collect(); + let _cloned = my_collection.into_iter().clone(); + let my_collection: Vec<()> = vec![()].into_iter().map(|()| {}).collect(); + let my_iter = my_collection.into_iter(); + let _cloned = my_iter.clone(); + // Same for `as_slice()`, for same reason. + let my_collection: Vec<()> = vec![()].into_iter().map(|()| {}).collect(); + let _sliced = my_collection.into_iter().as_slice(); + let my_collection: Vec<()> = vec![()].into_iter().map(|()| {}).collect(); + let my_iter = my_collection.into_iter(); + let _sliced = my_iter.as_slice(); } fn foo(_: impl IntoIterator) {} -- cgit 1.4.1-3-g733a5 From 6321ac79e1a24b4be336a8b6f3dd426251b773d4 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Mon, 17 Feb 2025 13:06:57 -0700 Subject: needless_collect: look through assignments, not just declarations --- clippy_lints/src/methods/needless_collect.rs | 7 +++++++ tests/ui/needless_collect.fixed | 9 +++++++++ tests/ui/needless_collect.rs | 9 +++++++++ 3 files changed, 25 insertions(+) diff --git a/clippy_lints/src/methods/needless_collect.rs b/clippy_lints/src/methods/needless_collect.rs index 6732aeeb498..def7bf635da 100644 --- a/clippy_lints/src/methods/needless_collect.rs +++ b/clippy_lints/src/methods/needless_collect.rs @@ -546,6 +546,12 @@ impl<'tcx> Visitor<'tcx> for IteratorMethodCheckVisitor<'_, 'tcx> { && !is_trait_method(self.cx, expr, sym::Iterator) { return ControlFlow::Break(()); + } else if let ExprKind::Assign(place, value, _span) = &expr.kind + && value.hir_id == self.hir_id_of_expr + && let Some(id) = path_to_local(place) + { + // our iterator was directly assigned to a variable + self.hir_id_of_let_binding = Some(id); } walk_expr(self, expr) } @@ -561,6 +567,7 @@ impl<'tcx> Visitor<'tcx> for IteratorMethodCheckVisitor<'_, 'tcx> { }) = &stmt.kind && expr.hir_id == self.hir_id_of_expr { + // our iterator was directly assigned to a variable self.hir_id_of_let_binding = Some(*id); } walk_stmt(self, stmt) diff --git a/tests/ui/needless_collect.fixed b/tests/ui/needless_collect.fixed index e33b984a44b..8a0208e4a7e 100644 --- a/tests/ui/needless_collect.fixed +++ b/tests/ui/needless_collect.fixed @@ -87,6 +87,15 @@ fn main() { let my_collection: Vec<()> = vec![()].into_iter().map(|()| {}).collect(); let my_iter = my_collection.into_iter(); let _sliced = my_iter.as_slice(); + // Assignment outside of main scope + { + let x; + { + let xxx: Vec<()> = vec![()].into_iter().map(|()| {}).collect(); + x = xxx.into_iter(); + for i in x.as_slice() {} + } + } } fn foo(_: impl IntoIterator) {} diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs index fab00d67dbf..b7e8e7b90b1 100644 --- a/tests/ui/needless_collect.rs +++ b/tests/ui/needless_collect.rs @@ -87,6 +87,15 @@ fn main() { let my_collection: Vec<()> = vec![()].into_iter().map(|()| {}).collect(); let my_iter = my_collection.into_iter(); let _sliced = my_iter.as_slice(); + // Assignment outside of main scope + { + let x; + { + let xxx: Vec<()> = vec![()].into_iter().map(|()| {}).collect(); + x = xxx.into_iter(); + for i in x.as_slice() {} + } + } } fn foo(_: impl IntoIterator) {} -- cgit 1.4.1-3-g733a5