inconsistent needless_collect lint for turbofish syntax vs type hint

[felix-pb]

I tried this code:

fn main() {
    let squares: Vec<_> = (1..5).map(|n| n * n).collect();
    squares.into_iter().for_each(|s| println!("{}", s));
}

I also tried this code:

fn main() {
    let squares = (1..5).map(|n| n * n).collect::<Vec<_>>();
    squares.into_iter().for_each(|s| println!("{}", s));
}

I expected to see this happen:
I guess it's debatable whether the two snippets above should emit a needless_collect warning. In my opinion, it should in the scenario above but it shouldn't in other scenarios (e.g. when collecting a vector of JoinHandle<T>). However, I think it's clear that both snippets should behave identically.

Instead, this happened:
For the first snippet (i.e. type hint), running cargo clippy doesn't generate any warning. However, for the second snippet (i.e. turbofish syntax), running cargo clippy generates the following warning:

warning: avoid using `collect()` when not needed
 --> src/main.rs:2:5
  |
2 | /     let squares = (1..5).map(|n| n * n).collect::<Vec<_>>();
3 | |     squares.into_iter().for_each(|s| println!("{}", s));
  | |____^
  |
  = 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| n * n).for_each(|s| println!("{}", s));
  |

warning: 1 warning emitted

I am willing to contribute to a pull request, but I want to make sure that we agree on the expected behavior: should they both emit a warning, or should they both NOT emit a warning? Or perhaps, I'm mistaken and they should behave differently...

Meta

• cargo clippy -V: clippy 0.1.52 (88f19c6d 2021-05-03)
• rustc -Vv:

rustc 1.52.0 (88f19c6da 2021-05-03)
binary: rustc
commit-hash: 88f19c6dab716c6281af7602e30f413e809c5974
commit-date: 2021-05-03
host: x86_64-apple-darwin
release: 1.52.0
LLVM version: 12.0.0

[giraffate]

This has been already fixed by #7163, so I'm closing this. FYI You can check it work on playground.

[felix-pb]

@giraffate Great! Indeed, it works with clippy 0.1.54 on playground.

However, I noticed that, even with clippy 0.1.54 on playground, you get the same warning for the following snippet:

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

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. The same is true for std::process::Child.

Should I open a separate issue for this? Or is this not considered a bug with clippy, and the intention is that we should use #[allow(clippy::needless_collect)] in those cases?

[llogiq]

Reopened for further discussion.

It looks like a false positive.

[felix-pb]

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).

I don't know if the code in rust-clippy/clippy_lints/src/loops/needless_collect.rs is aware of the type of the elements being collected. If so, having a "whitelist" of types that are okay to collect could be an easy implementation.

[camsteffen]

A new issue for this would be good. Yikes, this looks like a hard one. 😬

[felix-pb]

@camsteffen: I've opened a separate issue (#7207) for it. Feel free to close this one if appropriate!