Implement a lint for .clone().into_iter()

[GnomedDev]

Closes #3302

changelog: [unnecessary_collection_clone] Add a lint for .clone().into_iter().

[rustbot]

r? @xFrednet

rustbot has assigned @xFrednet.
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

[samueltardieu]

This will lint this code and the suggestion will fail:

fn main() {
    let mut v = vec![1, 2, 1, 10];
    for (i, e) in v.clone().into_iter().enumerate() {
        for j in ((i+1)..v.len()).rev() {
            if v[j] == e {
                v.remove(j);
            }
        }
    }
    assert_eq!(v, vec![1, 2, 10]);
}

Maybe restricting the lint to immutable clone() receivers would be safer.

_{(yes, this code is ugly, but that is not the point)}

[samueltardieu]

In fact, it will probably be necessary to check whether any immutable component exist in the receiver ref chain if any because the lifetime of the receiver will be extended for the lifetime of the loop. This one will also cause a wrong suggestion for the time being, I'll let it here so that you can check that it is handled as well:

struct S {
    a: Vec<i32>,
}

fn main() {
    let mut s = S { a: vec![1, 2, 3] };
    (&s.a).clone().into_iter().for_each(|x| s.a[0] += x);
    assert_eq!(s.a[0], 7);
}

[GnomedDev]

Sorted out those issues, I think at least

[samueltardieu]

Sorted out those issues, I think at least

I think the example in #13595 (comment) still gives an incorrect suggestion:

warning: using clone on collection to own iterated items
 --> src/main.rs:7:5
  |
7 |     (&s.a).clone().into_iter().for_each(|x| s.a[0] += x);
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `(&s.a).iter().cloned()`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_collection_clone
  = note: `#[warn(clippy::unnecessary_collection_clone)]` on by default

Applying the suggestion doesn't compile:

error[E0502]: cannot borrow `s.a` as mutable because it is also borrowed as immutable
 --> src/main.rs:7:37
  |
7 |     (&s.a).iter().cloned().for_each(|x| s.a[0] += x);
  |     ------                 -------- ^^^ --- second borrow occurs due to use of `s.a` in closure
  |     |                      |        |
  |     |                      |        mutable borrow occurs here
  |     |                      immutable borrow later used by call
  |     immutable borrow occurs here

[GnomedDev]

Okay, I'm pretty sure that issue is now fixed and I have added more tests for partial borrow situations.

[bors]

☔ The latest upstream changes (presumably #13437) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #13034) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

Generally LGTM, some high-level comments, and questions and that's it :D

[xFrednet]

I really like this comment xD

I wonder if there is actually a reasonable use case for this, but let's not open that can of worms ^^

[xFrednet]

I wonder if this suggestion is always better. Have you benchmarked that individual clones are actually faster, or at least equivalent?

With a linked list (I know everyone's favorite structure) it might be legit better to clone it, thereby allocating all nodes close together and loading everything into cache.

This is not an argument against the lint, I still think it's a good idea, just a question about the category

[GnomedDev]

I haven't benchmarked this, and I may be falling into the "it's somewhat obvious" trap. It's possible we could maintain a list of collections it would be quicker to do this, or even have a #[clippy::iter_cloned_instead_of_clone)] to mark collections that would benefit from this lint. But it will be quite the low FP rate for collections that are faster to clone than iterate and clone.

[xFrednet]

I'm guessing this highly depends on the iterator usage. For a vector, it makes a difference of allocating a big chunk of memory once against several small allocations. For vectors it might actually be the other way around (meaning .clone().into_iter() being faster), but this is just guesstimating, since I haven't benchmarked this.

One thing that's safe to say, is that cloning each item individually will reduce the memory footprint, but only if the items are dropped and not stored somewhere afterward.

Could you try to benchmark this?

I've just checked the issue, and that one was suggestion to lint on cloned().into_iter().filter(). Linting on that should be safe again

[xFrednet]

There is this lint trigger from Lintcheck:

Added clippy::unnecessary_collection_clone at reqwest-0.12.5/src/dns/resolve.rs:100

warning: using clone on collection to own iterated items
   --> target/lintcheck/sources/reqwest-0.12.5/src/dns/resolve.rs:100:45
    |
100 |                 let addrs: Addrs = Box::new(dest.clone().into_iter());
    |                                             ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `dest.iter().cloned()`
    |
    = note: `--force-warn clippy::unnecessary-collection-clone` implied by `--force-warn clippy::all`

These cases might not be MachineApplicable as they change the type of the expression. To fully avoid FPs, we probably would need to track how the value is finally used, which would be hard and also prone to errors. I think this is an argument for changing the Applicability to MaybeIncorrect and having it in pedantic instead.

[GnomedDev]

Hmm, that's a shame, I hate relegating useful lints to pedantic where they won't be hit by the beginners this lint is targeting.

Would it be good if I swap the suggestion to MaybeIncorrect but keep in the perf category? Since we now have lint reasons, it would be good for a codebase to document "we don't do Iter::cloned due to XYZ" with an allow/expect.

[xFrednet]

The applicability is only given to tools like rustfix and not directly visible to the user. So changing the applicability, shouldn't have an effect on the lint category.

The category usually depends on the FP rate and the trigger rate when we introduce the lint. If a lint is reasonable, but has too many lint triggers, it will sadly still often land in pedantic. We can leave the lint group discussion open for the RFC. But the applicability should still be changed.

[bors]

☔ The latest upstream changes (presumably b829d53) made this pull request unmergeable. Please resolve the merge conflicts.