Warning about needless collect but suggestion does not compile

[kvrhdn]

Clippy warns about a needless collect, but the suggested code does not compile.

I have something similar to this code (condensed):

let keys = map.keys().copied().collect::<Vec<_>>();

for key in keys.into_iter() {
    // since .keys() has been copied and collected, I'm not
    // borrowing the map anymore and I can mutate the map here.
    map.remove(key);
}

Suggestion from clippy:

warning: avoid using `collect()` when not needed
  --> src/main.rs:18:5
   |
18 | /     let keys = map.keys().copied().collect::<Vec<_>>();
19 | |
20 | |     for key in keys.into_iter() {
   | |_______________^
   |
   = note: `#[warn(clippy::needless_collect)]` on by default
help: Use the original Iterator instead of collecting it and then producing a new one
   |
18 |     
19 | 
20 |     for key in map.keys().copied() {
   |

This suggestion will not compile because maps.keys().copied() borrows the map.

Full code example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=711425060fb8ac794003067454010a69

Expectation: the warning does not appear because removing the .collect() would break the code.

Instead, this happened: the needless_collect warning triggered. Additionally, it's not possible to mute this warning for a specific line, so I resorted to adding #![allow(clippy::needless_collect)] to the entire file.

Meta

• cargo +nightly clippy -V: clippy 0.0.212 (1c389ffe 2020-11-24) (I use the nightly clippy to get more warnings)
• rustc -Vv:

  rustc 1.48.0 (7eac88abb 2020-11-16)
  binary: rustc
  commit-hash: 7eac88abb2e57e752f3302f02be5f3ce3d7adfb4
  commit-date: 2020-11-16
  host: x86_64-apple-darwin
  release: 1.48.0
  LLVM version: 11.0

[giraffate]

@rustbot modify labels: +L-suggestion-causes-error

[flip1995]

Couldn't you use the filter method instead? So fileter the map and collect the remainders afterwards. (I just assume here, that in your actual code, you don't remove each element one by one, but only if some condition is met).

[kvrhdn]

Yeah, that would be better in most cases, but the actual code is more complicated unfortunately.

In my case I want to loop over all keys of the map and for every key delete and add entries into the map until there is only one key left. Actual code is posted here: https://github.com/kvrhdn/Advent-of-Code/blob/main/advent-of-code-2019/src/day14.rs#L60-L116, this is a solution for the Advent of Code 2019, day 14.

[tigregalis]

I'm running into a very similar issue.

Simplified example:

// much simplified
fn get_index(id: usize) -> usize {
    id
}

fn main() {
    let actual_ids = [0, 2, 4, 1];

    // effectively `vec![Some(0), Some(1), ..., Some(4)]`
    let mut possible_ids = (0..5).map(Some).collect::<Vec<_>>();

    actual_ids.iter().for_each(|id| {
        // "remove" the *actual* ids from the `possible_ids`
        possible_ids[get_index(*id)] = None;
    });

    let missing_id = possible_ids.into_iter().find_map(|id| id).unwrap();

    println!("missing_id: {}", missing_id);
}

produces the following warning:

warning: avoid using `collect()` when not needed
  --> something-broken\src\main.rs:10:5
   |
10 | /     let mut possible_ids = (0..5).map(Some).collect::<Vec<_>>();
11 | |
12 | |     actual_ids.iter().for_each(|id| {
13 | |         // "remove" the *actual* ids from the `possible_ids`
...  |
16 | |
17 | |     let missing_id = possible_ids.into_iter().find_map(|id| id).unwrap(); 
   | |_____________________^
   |
   = note: `#[warn(clippy::needless_collect)]` on by default
help: Use the original Iterator instead of collecting it and then producing a new one
   |
10 | 
11 | 
12 |     actual_ids.iter().for_each(|id| {
13 |         // "remove" the *actual* ids from the `possible_ids`
14 |         possible_ids[get_index(*id)] = None;
15 |     });
 ...

warning: 1 warning emitted

How would you rewrite this?

If you remove the collect it leaves an iterator - if so, you can't index into it.

The intent of the way I've written it is that you only visit each item in each list at most once.

[giraffate]

@tigregalis The lint wasn't emitted at nightly rustc 1.50.0-nightly (d32c320d7 2020-12-10). That case seems to be fixed at https://github.com/rust-lang/rust-clippy/pull/6313.

[tigregalis]

@tigregalis The lint wasn't emitted at nightly rustc 1.50.0-nightly (d32c320d7 2020-12-10). That case seems to be fixed at #6313.

@giraffate So it is. Thanks.

[dmilith]

I've found an even worse case: Code compiles… but the code logic changes: https://twitter.com/dmilith/status/1424370582179692550?s=20

[flip1995]

Oh, so the collect in your case forces the iterator to evaluate. So removing it makes the statement lazy and it won't evaluate. Detecting this is even harder than detecting if the underlying iterator is mutated. Is it time to move this lint to nursery?

[giraffate]

Is it time to move this lint to nursery?

Yes, I think so.

Many false positives have been reported for needless_collect in the indirect usage, but not for the direct usage. So, IMO it would be better that needless_collect is split into two and the indirect usage is downgraded to nursery.

[reivilibre]

I too have hit a few instances where this lint triggers and the suggestion is not valid because the original collection the iterator was collected from has been moved (e.g. with into_iter() before the result of collect() has been used).

I've obfuscated the identifiers a bit but hopefully this will paint a picture:

let a_b_indices: Vec<Option<usize>> = as
    .iter()
    .map(|a| {
        bs
            .iter()
            .position(|b| b.id == a.id)
    })
    .collect(); // <-- clippy thinks this collect is needless

let b_a_indices: Vec<Option<usize>> = bs
    .iter()
    .map(|b| {
        as
            .iter()
            .position(|a| b.id == a.id)
    })
    .collect(); // <-- clippy thinks this collect is needless

let as_with_contexts: Vec<_> =
    as
        .into_iter() // <-- this moves `as`
        .zip(a_b_indices.into_iter()) // <-- clippy thinks the iterator that was collect()ed could be inlined here
        .enumerate()
        .map(|(a_index, (a, b_index))| {
            let context = (a.id.clone(), a_index, b_index);

            (a, context)
        })
        .collect();

let bs_with_contexts: Vec<_> =
    bs
        .into_iter() // <-- this moves `bs`
        .zip(b_a_indices.into_iter()) // <-- clippy thinks the iterator that was collect()ed could be inlined here
        .enumerate()
        .map(|(b_index, (b, a_index))| {
            let context = (b.id.clone(), a_index, b_index);

            (b, context)
        })
        .collect();

I could probably do it in a nicer way I'm sure, but at least I can't see any way of avoiding the collect (at least not without cloneing as and bs :))