iter_with_drain false positive when vec capacity should be retained

[dtolnay]

Summary

The iter_with_drain lint is a performance lint that says to replace vec.drain(..) with vec.into_iter(). This is unfortunate because into_iter destroys the capacity of a vector that you might have been planning to reuse, which is the opposite of performance.

Lint Name

iter_with_drain

Reproducer

fn main() {
    let mut out = Struct(Vec::new());
    let mut pending = Vec::new();

    for i in 0..20 {
        if i % (pending.len() + 1) == 2 {
            out.extend(pending.drain(..));
        } else {
            pending.push(i);
            pending.reverse();
        }
    }

    println!("{:?}", out);
}

#[derive(Debug)]
struct Struct<T>(Vec<T>);

impl<T> Extend<T> for Struct<T> {
    fn extend<I: IntoIterator<Item = T>>(&mut self, i: I) {
        self.0.extend(i);
    }
}

warning: `drain(..)` used on a `Vec`
 --> src/main.rs:7:32
  |
7 |             out.extend(pending.drain(..));
  |                                ^^^^^^^^^ help: try this: `into_iter()`
  |
  = note: `#[warn(clippy::iter_with_drain)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#iter_with_drain

Following clippy's suggestion makes the code fail to build.

error[E0382]: borrow of moved value: `pending`
   --> src/main.rs:6:17
    |
3   |     let mut pending = Vec::new();
    |         ----------- move occurs because `pending` has type `Vec<usize>`, which does not implement the `Copy` trait
...
6   |         if i % (pending.len() + 1) == 2 {
    |                 ^^^^^^^^^^^^^ value borrowed here after move
7   |             out.extend(pending.into_iter());
    |                                ----------- `pending` moved due to this method call, in previous iteration of loop
    |
note: this function takes ownership of the receiver `self`, which moves `pending`
   --> /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/collect.rs:267:18
    |
267 |     fn into_iter(self) -> Self::IntoIter;
    |                  ^^^^

It's possible to get around that by recreating a new vector after into_iter has dropped the old one, but the resulting code is slower than we started with because the old capacity is being destroyed and reallocated in each loop.

-             out.extend(pending.drain(..));
+             out.extend(pending.into_iter());
+             pending = Vec::new();

Version

rustc 1.61.0-nightly (285fa7ecd 2022-03-14)
binary: rustc
commit-hash: 285fa7ecd05dcbfdaf2faaf20400f5f92b39b3c6
commit-date: 2022-03-14
host: x86_64-unknown-linux-gnu
release: 1.61.0-nightly
LLVM version: 14.0.0

Additional Labels

@rustbot label +I-suggestion-causes-error

[dtolnay]

Mentioning @ldm0 @flip1995 @giraffate since the lint is from #8483.

[ldm0]

@dtolnay Could you try std::mem::take(&mut pending).into_iter()?

[ldm0]

.into_iter() usually gets better optimization: https://rust.godbolt.org/z/saGc3Tjqr

[ldm0]

#8538 This one can also be replaced with mem::take + into_iter. 🤔 I didn't suggest it automatically due to the hardness of ownership checking and otherwise usage checking. So false positive is expected.

[hrxi]

@dtolnay Could you try std::mem::take(&mut pending).into_iter()?

That also destroys the capacity because it replaces the old vector with an empty one that doesn't have any capacity.

[flip1995]

I also had concerns about this lint triggering on owned vs referenced vecs, but couldn't come up with a concrete example at the time (I probably should have thought about this harder).

Together with #8538, I think this lint is not ready to be warn-by-default. But I also think this lint still needs some work, so I'll put it in nursery. (next sync is in a little over a week from now, so I won't do a out-of-cycle sync for this)

[SUPERCILEX]

I just ran into the same issue:

    let mut results = results.into_vec();
    results.sort_unstable();
    let entries = results
        .drain(..)
        .map_stuff...
        .collect();
    *search_result_buf = results;

I want to reuse the results buffer, so into_iter is wrong.

[ViliamVadocz]

I wrote another example of the false positive in an older issue: #8538