needless_collect: avoid warning if non-iterator methods are used

notriddle · notriddle · commit c0126939617d · 2025-02-04T13:07:06.000-07:00
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.
diff --git 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<HirId>,
+}
+
+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
@@ -73,6 +73,20 @@ fn main() {
     let mut out = vec![];
     values.iter().cloned().map(|x| out.push(x)).collect::<Vec<_>>();
     let _y = values.iter().cloned().map(|x| out.push(x)).collect::<Vec<_>>(); // 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<Item = usize>) {}
diff --git 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::<Vec<_>>();
     let _y = values.iter().cloned().map(|x| out.push(x)).collect::<Vec<_>>(); // 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<Item = usize>) {}
