Skip to content

needless_collect: avoid warning if non-iterator methods are used #14147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 2, 2025
Merged

needless_collect: avoid warning if non-iterator methods are used #14147

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c012693
needless_collect: avoid warning if non-iterator methods are used
notriddle Feb 4, 2025
6321ac7
needless_collect: look through assignments, not just declarations
notriddle Feb 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 65 additions & 6 deletions clippy_lints/src/methods/needless_collect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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};
Expand All @@ -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;
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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) => {
Expand All @@ -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 => {
Expand All @@ -284,7 +292,7 @@ impl IterFunction {
}
}
enum IterFunctionKind {
IntoIter,
IntoIter(HirId),
Len,
IsEmpty,
Contains(Span),
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been checking for already-computed alternatives to this visitor. Seeing it
gives you a "this has to be already computed somewhere", and there are liveness
checks throughout Rust that are near what we need, but it seems that there isn't
anywhere that we can check to traverse through all uses of a local.

Quite sad.


fn check_iter_expr_used_only_as_iterator<'tcx>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen if we were to do something like this:

let x;
{
    x = xxx.into_iter();
    for i in x.clone();
}

If I'm not wrong, the code that comes before this, only looks at the current
scope, first looking for the initializing local, and then assigning that to
some iterable.

Testing that, it seems that the x = part is a Path with a Def of Local, could
you also take that into acccount?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct. The code will also check assignment to a local now.

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()
}
14 changes: 14 additions & 0 deletions tests/ui/needless_collect.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>) {}
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/needless_collect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>) {}
Expand Down