Skip to content

needless_collect doesn't take into account side-effects #8055

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.

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

Closed
jsgf opened this issue Dec 1, 2021 · 2 comments · Fixed by #14490
Closed

needless_collect doesn't take into account side-effects #8055

jsgf opened this issue Dec 1, 2021 · 2 comments · Fixed by #14490
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have L-nursery Lint: Currently in the nursery group

Comments

@jsgf
Copy link
Contributor

jsgf commented Dec 1, 2021

Summary

needless_collect does not take into account side effects of the iterator, and so spuriously reports a necessary collect as needless.

Lint Name

needless_collect

Reproducer

I tried this code:

pub fn filter(v: impl IntoIterator<Item = i32>) -> Result<impl Iterator<Item = i32>, usize> {
    let mut zeros = 0;

    let res: Vec<_> = v
        .into_iter()
        .filter(|i| {
            if *i == 0 {
                zeros += 1
            };
            *i != 0
        })
        .collect();

    if zeros != 0 {
        return Err(zeros);
    } 
    Ok(res.into_iter())
}

playground https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=35738bf7ad2c8bcba6b8431afd61e527

I saw this happen:

warning: avoid using `collect()` when not needed
  --> src/lib.rs:12:10
   |
12 |         .collect();
   |          ^^^^^^^
...
17 |     Ok(res.into_iter())
   |        --------------- the iterator could be used here instead
   |
   = note: `#[warn(clippy::needless_collect)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect
help: use the original Iterator instead of collecting it and then producing a new one
   |
4  ~     
5  | 
6  |     if zeros != 0 {
7  |         return Err(zeros);
8  |     } 
9  ~     Ok(v
 ...

I expected to see this happen: no warning

Version

rustc 1.56.1 (59eed8a2a 2021-11-01)
binary: rustc
commit-hash: 59eed8a2aac0230a8b53e89d4e99d55912ba6b35
commit-date: 2021-11-01
host: x86_64-unknown-linux-gnu
release: 1.56.1
LLVM version: 13.0.0

Additional Labels

No response

@jsgf jsgf added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Dec 1, 2021
@esoterra
Copy link

As another example of this, I got the same warning when collecting join handles into a Vec from an iterator.

let join_handles: Vec<JoinHandle<_>> = actors
   .into_iter()
   .map(|actor|
      thread::spawn(|| actor.run())
   ).collect();

If I do not collect and simply use the iterator later, then the threads are not spawned when they should be.

@surfingreg
Copy link

surfingreg commented Jul 11, 2022

Same here...I think...

// remove "remove_me"
let a_vec:Vec<String> = a_vec.into_iter().map(|x|
    regex::Regex::new(r"remove_me").unwrap().replace(&x, "").into_owned()
).collect();

@J-ZhengLi J-ZhengLi added the L-nursery Lint: Currently in the nursery group label Jul 9, 2024
github-merge-queue bot pushed a commit that referenced this issue Apr 22, 2025
Closes #9191
Closes #14444
Closes #8055

Adds a new helper to partly check for side effects by recursively
checking if the iterator type contains closures with mutable captures.

changelog: [`double_ended_iterator_last`] fix FP when iter has side
effects
changelog: [`needless_collect`] fix lint not consider side effects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have L-nursery Lint: Currently in the nursery group
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants