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.

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<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
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::<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
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::<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>) {}