Skip to content
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

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

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 all commits
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
78 changes: 72 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,61 @@ 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(());
} 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)
}
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
{
// our iterator was directly assigned to a variable
self.hir_id_of_let_binding = Some(*id);
}
walk_stmt(self, stmt)
}
}

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()
}
23 changes: 23 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,29 @@ 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();
// 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<Item = usize>) {}
Expand Down
23 changes: 23 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,29 @@ 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();
// 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<Item = usize>) {}
Expand Down
Loading