needless_collect: false positive when collecting std::thread::JoinHandle or similar elements

[felix-pb]

Lint name: needless_collect

I tried this code:

fn main() {
    let threads = (1..5).map(|n| std::thread::spawn(move || n * n)).collect::<Vec<_>>();
    threads.into_iter().for_each(|thread| println!("{}", thread.join().unwrap()));
}

I expected to see this happen:

In my opinion, no warning should be emitted here because, in almost all cases, the user wants for the threads to run in parallel and "fixing" the lint would make them run sequentially. Unfortunately, I'm not sure how feasible it would be to fix those false positives in the general case. It's somewhat subjective when "collecting" is valid for parallelism. The obvious cases from the standard library are when collecting std::thread::JoinHandle or std::process::Child elements. However, the same could be true for other crates (e.g. crossbeam::thread::ScopedJoinHandle).

Instead, this happened:

Running cargo clippy, I get the following warning:

warning: avoid using `collect()` when not needed
 --> src/main.rs:2:5
  |
2 | /     let threads = (1..5).map(|n| std::thread::spawn(move || n * n)).collect::<Vec<_>>();
3 | |     threads.into_iter().for_each(|thread| println!("{}", thread.join().unwrap()));
  | |____^
  |
  = note: `#[warn(clippy::needless_collect)]` on by default
help: use the original Iterator instead of collecting it and then producing a new one
  |
2 |     
3 |     (1..5).map(|n| std::thread::spawn(move || n * n)).for_each(|thread| println!("{}", thread.join().unwrap()));
  |

warning: 1 warning emitted

Note that using a type hint (i.e. let threads: Vec<_>) instead of turbofish syntax doesn't produce the warning with clippy 0.1.52 because of an inconsistency bug. However, that bug has been fixed in clippy 0.1.54, which emits the warning in both cases. See issue #7194.

Meta

• cargo clippy -V: clippy 0.1.52 (9bc8c42b 2021-05-09)
  (Note that I get the same result on the Rust Playground with clippy 0.1.54)
• rustc -Vv:

rustc 1.52.1 (9bc8c42bb 2021-05-09)
binary: rustc
commit-hash: 9bc8c42bb2f19e745a63f3445f1ac248fb015e53
commit-date: 2021-05-09
host: x86_64-apple-darwin
release: 1.52.1
LLVM version: 12.0.0

[felix-pb]

As for a solution, I'm not very familiar with rust-clippy but I can't think of anything better than having a "whitelist" of types that are okay to collect in this manner. It could start pretty conservative (e.g. just with std::thread::JoinHandle and std::process::Child, and be expanded over time if the community agrees on certain non-std crate types such as crossbeam::thread::ScopedJoinHandle). However, I don't even know if the code in needless_collect.rs has such an awareness of the type of the elements being collected.

[hniksic]

We hit this one on our code base. The insidious thing about this false positive is that a careless reviewer can apply the suggestion and get code that:

• compiles without warnings;
• "works", i.e. passes all tests;
• but is now much slower than intended because it's effectively sequential.

[llogiq]

Yeah, I think having a whitelist would be the way to go for now. I think in the long run there would be value in having an annotation for handle types whose drop impls have special semantics, such as locks, processes, etc.

[stouset]

Maybe the two are isomorphic to one another, but to me the issue with the threading example is that the loop iterations have side effects (thread creation) and thus we need to force them to execute at a specific point in time rather than allowing the loop bodies to be lazily run only when needed.

I haven't had enough coffee today, so maybe that's equivalent to drop impls having those kinds of special semantics though.