lint on unnecessary collections #15830

bend-n · 2025-10-07T12:23:41Z

lints on

fn squares() -> Vec<(u8, u8)> { (0..8u8) .flat_map(|x| (0..8).map(move |y| (x, y))) .collect::<Vec<_>>() }

changelog: [unnecessary_collect]: create lint

see also #clippy > linting on vector returns where return is collected

name can be bikeshed

rustbot · 2025-10-07T12:23:46Z

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

github-actions · 2025-10-07T12:40:27Z

Lintcheck changes for 7e91e18

Lint	Added	Removed	Changed
`clippy::unnecessary_collect`	36	0	0

This comment will be updated if you push new changes

rustbot · 2025-10-07T13:23:06Z

Some changes occurred in clippy_lints/src/doc

cc @notriddle

samueltardieu · 2025-10-07T13:45:47Z

clippy_lints/src/doc/suspicious_doc_comments.rs

- }
- })
- .collect()
+fn collect_doc_replacements(attrs: &[Attribute]) -> impl Iterator<Item = (Span, String)> {


Why make it more complicated when the caller collects it because it needs a Vec? This should probably a restriction lint.

samueltardieu · 2025-10-07T13:46:34Z

clippy_config/src/conf.rs

@@ -1,3 +1,4 @@
+#![allow(clippy::unnecessary_collect)]


Please use #[expect] to ignore a lint, and put it as close as possible to the lint emission.

bit difficult, its define_conf!

This convo seems smart I just wanna say smth so I feel like I'm a part of it

y21 · 2025-10-07T12:29:44Z

tests/ui/unnecessary_collect.stderr

+error: this collection may not be necessary. consider returning the iterator itself
+ --> tests/ui/unnecessary_collect.rs:5:12
+ |
+LL | fn bad() -> Vec<u32> {
+ | -------- help: perhaps: `impl Iterator<Item = u32>`


I'd suggest moving the "consider ..." part of the primary message into the help message to replace the "perhaps". IIRC at least rust-analyzer only shows the help message without the code suggestion in the quick fix tab, so just "help: perhaps" would look odd. It also makes the message itself more concise which is always nice

clippy_lints/src/unnecessary_collect.rs

y21 · 2025-10-07T13:43:28Z

clippy_lints/src/unnecessary_collect.rs

+ /// ```
+ #[clippy::version = "1.92.0"]
+ pub UNNECESSARY_COLLECT,
+ pedantic,


IMO this should be a restriction lint, at least as it's currently implemented because it seems too eager to me.

It's certainly useful in a lot of cases, but fundamentally I don't think there is anything wrong with this pattern. I get that it's for beginners, but clippy::pedantic is also enabled by experienced users, and this already introduces quite a few #[allow]s to the codebase.

There's also a bunch of false positives that seem fairly difficult to solve in a good way, like if the iterator has local lifetimes and can't be returned. Or if the method is part of a trait impl where the trait can't be controlled by the user in which case the signature can't be changed. The lint could be made weaker and checks could be added to avoid them, but maybe the goal is to strictly find all instances of this pattern without false negatives even if it means false positives, and you're willing to go through them manually and do some refactoring. If that's the case (which this sounds like to me), restriction sounds more fitting.

@ada4a was talking about how it would be good for pedantic..?

It's not like I'm a team member or anything though, just wanted to share my opinion^^

But yes, some of the linted cases being unsolvable is unfortunate, though pedantic lints are allowed to give more false-positives than the regular ones

justDeeevin · 2025-10-10T16:40:11Z

clippy_lints/src/unnecessary_collect.rs

+ ///
+ /// ### Why is this bad?
+ ///
+ /// This might not be necessary, and its more idiomatic to simply return the iterator, giving the caller the option to use it as they see fit.


Suggested change

/// This might not be necessary, and its more idiomatic to simply return the iterator, giving the caller the option to use it as they see fit.

/// This might not be necessary, and it's more idiomatic to simply return the iterator, giving the caller the option to use it as they see fit.

I disagree that it's more idiomatic to return the iterator. I have certainly seen and written a lot of methods that return Vec via collect. I will gladly admit that omitting the collect may sometimes benefit performance – if the allocation can be removed, which depends on the callers.

Also I would advice avoiding the word "simply". It can come off as condescending. I don't know what is simple for any person reading this, and I shouldn't want to guess.

llogiq · 2025-11-14T17:49:01Z

I personally think that the lint as is probably fits a restriction lint best (and should be documented to only show possible optimization opportunities), however, with an additional check if the result of that method ever is manipulated other than iterating it, and only linting if that's not the case would make the lint vastly more useful.

However, I don't think this PR needs to do the extension.

llogiq

This looks like the starting point of a cool lint, but I'm afraid it's not quite there yet.

I propose we either put it in nursery until we make it less false-positive-happy or extend it to check whether there is actually a caller of the linted item that only iterates on the result.

View changes since this review

llogiq · 2025-11-14T17:52:36Z

clippy_lints/src/lifetimes.rs

 visitor.visit_ty_unambig(self_ty);

- !visitor.all_lts().is_empty()
+ visitor.all_lts().next().is_some()


I don't understand how this is better than is_empty() here. You could also use impl Iterator<Item = Lifettime> + ExactSizeIterator.

you avoid iterating all of the items just to check the length, and it doesnt impl exactsizeiterator

llogiq · 2025-11-14T17:54:17Z

clippy_lints/src/lifetimes.rs

 // walk the type F, it may not contain LT refs
 walk_unambig_ty(&mut visitor, pred.bounded_ty);
- if !visitor.all_lts().is_empty() {
+ if visitor.all_lts().next().is_some() {


llogiq · 2025-11-14T17:54:52Z

clippy_lints/src/non_send_fields_in_send_ty.rs

 /// Given a type, collect all of its generic parameters.
 /// Example: `MyStruct<P, Box<Q, R>>` => `vec![P, Q, R]`
-fn collect_generic_params(ty: Ty<'_>) -> Vec<Ty<'_>> {
+fn collect_generic_params(ty: Ty<'_>) -> impl Iterator<Item = Ty<'_>> {


Wouldn't it make sense to rename this into iter_generic_params?

llogiq · 2025-11-14T17:58:05Z

clippy_lints/src/unnecessary_collect.rs

+ ///
+ /// ### Why is this bad?
+ ///
+ /// This might not be necessary, and its more idiomatic to simply return the iterator, giving the caller the option to use it as they see fit.


I disagree that it's more idiomatic to return the iterator. I have certainly seen and written a lot of methods that return Vec via collect. I will gladly admit that omitting the collect may sometimes benefit performance – if the allocation can be removed, which depends on the callers.

Also I would advice avoiding the word "simply". It can come off as condescending. I don't know what is simple for any person reading this, and I shouldn't want to guess.

llogiq · 2025-11-14T18:00:13Z

clippy_lints/src/unnecessary_collect.rs

+ ///
+ /// ### Potential Issues
+ ///
+ /// In some cases, there may be some lifetimes attached to your iterator that make it difficult, or even impossible, to avoid a collection.


Also if no caller can do without allocating a Vec, all you have done by applying this lint is moving one collect call into multiple other locations. Worse, even if there is one call that doesn't need to collect, the additional code produced may well negate any performance benefit gained by drowning out the L1 cache of hot path code, if there are multiple callers outside hot code paths.

llogiq · 2025-11-14T18:04:10Z

clippy_lints/src/unnecessary_collect.rs

+ pub fn new(conf: &'static Conf) -> Self {
+ Self {
+ avoid_breaking_exported_api: conf.avoid_breaking_exported_api,
+ }
+ }


A nice trick one can pull because function signatures are patterns:

Suggested change

pub fn new(conf: &'static Conf) -> Self {

Self {

avoid_breaking_exported_api: conf.avoid_breaking_exported_api,

}

}

pub fn new(Conf { avoid_breaking_exported_api, .. }: &'static Conf) -> Self {

Self { avoid_breaking_exported_api }

}

llogiq · 2025-11-14T18:05:24Z

clippy_lints/src/unnecessary_collect.rs

+ && let ExprKind::MethodCall(path, iter_expr, args, call_span) = expr.kind
+ && !(self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(def_id))
+ && !args.iter().any(|e| e.span.from_expansion())
+ // && !iter_expr.span.from_expansion()


Please remove commented out code.

llogiq · 2025-11-14T18:08:47Z

clippy_utils/src/lib.rs

 /// Returns all the comments a given span contains.
 ///
 /// Comments are returned wrapped with their relevant delimiters.
+#[allow(clippy::unnecessary_collect)]


That we have three allows in our code base should be testament to the fact that this lint is a bit too false-positive-happy to be widely useful as is.

Luckily, it is certainly possible to improve it, so that shouldn't discourage you.

rustbot · 2025-11-15T11:49:17Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

rustbot assigned llogiq Oct 7, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 7, 2025

bend-n force-pushed the returning_vector_produced_from_from_iterator branch from 1b7f7a8 to a7a20c0 Compare October 7, 2025 13:23

bend-n force-pushed the returning_vector_produced_from_from_iterator branch 3 times, most recently from a8d01c4 to da2cf18 Compare October 7, 2025 13:41

samueltardieu reviewed Oct 7, 2025

View reviewed changes

y21 reviewed Oct 7, 2025

View reviewed changes

bend-n force-pushed the returning_vector_produced_from_from_iterator branch from da2cf18 to 98f1059 Compare October 7, 2025 14:04

This comment has been minimized.

Sign in to view

bend-n force-pushed the returning_vector_produced_from_from_iterator branch from 98f1059 to b359f33 Compare October 8, 2025 10:56

justDeeevin reviewed Oct 10, 2025

View reviewed changes

bend-n force-pushed the returning_vector_produced_from_from_iterator branch from 2645d46 to d59599d Compare November 7, 2025 17:56

rustbot added the needs-fcp label Nov 7, 2025

This comment has been minimized.

Sign in to view

bend-n requested a review from samueltardieu November 8, 2025 09:23

bend-n force-pushed the returning_vector_produced_from_from_iterator branch from c07b6fd to c0d681d Compare November 8, 2025 09:30

This comment has been minimized.

Sign in to view

llogiq reviewed Nov 14, 2025

View reviewed changes

bend-n force-pushed the returning_vector_produced_from_from_iterator branch from c0d681d to cae7fc7 Compare November 15, 2025 11:49

bend-n force-pushed the returning_vector_produced_from_from_iterator branch from cae7fc7 to 5834ade Compare November 15, 2025 11:52

lint on unnecessary collections

0f63034

bend-n force-pushed the returning_vector_produced_from_from_iterator branch from 5834ade to b0cac50 Compare November 15, 2025 11:56

hmm

7e91e18

bend-n force-pushed the returning_vector_produced_from_from_iterator branch from b0cac50 to 7e91e18 Compare November 15, 2025 12:03

bend-n requested a review from llogiq December 11, 2025 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

lint on unnecessary collections #15830

lint on unnecessary collections #15830

Uh oh!

bend-n commented Oct 7, 2025

rustbot commented Oct 7, 2025

github-actions bot commented Oct 7, 2025 •

edited

Loading

rustbot commented Oct 7, 2025

samueltardieu Oct 7, 2025

samueltardieu Oct 7, 2025

bend-n Oct 7, 2025

SpinachPod Oct 10, 2025

y21 Oct 7, 2025

Uh oh!

y21 Oct 7, 2025

bend-n Oct 7, 2025

ada4a Oct 7, 2025

This comment has been minimized.

justDeeevin Oct 10, 2025

llogiq Nov 14, 2025

This comment has been minimized.

This comment has been minimized.

llogiq commented Nov 14, 2025

llogiq left a comment •

edited by rustbot

Loading

llogiq Nov 14, 2025

bend-n Nov 14, 2025 •

edited

Loading

llogiq Nov 14, 2025

llogiq Nov 14, 2025

llogiq Nov 14, 2025

llogiq Nov 14, 2025

llogiq Nov 14, 2025

llogiq Nov 14, 2025

llogiq Nov 14, 2025

rustbot commented Nov 15, 2025

Labels

8 participants

	/// This might not be necessary, and its more idiomatic to simply return the iterator, giving the caller the option to use it as they see fit.
	/// This might not be necessary, and it's more idiomatic to simply return the iterator, giving the caller the option to use it as they see fit.

lint on unnecessary collections #15830

Are you sure you want to change the base?

lint on unnecessary collections #15830

Uh oh!

Conversation

bend-n commented Oct 7, 2025

rustbot commented Oct 7, 2025

github-actions bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

rustbot commented Oct 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

llogiq commented Nov 14, 2025

llogiq left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bend-n Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Nov 15, 2025

Labels

8 participants

github-actions bot commented Oct 7, 2025 •

edited

Loading

llogiq left a comment •

edited by rustbot

Loading

bend-n Nov 14, 2025 •

edited

Loading