to_vec on for loop expression

[hellow554]

What it does

Warns about a to_vec() on a for loop expression.

There is this real world example (please don't see as an offence!):

fn check_files(fm: &FileManager, file_types: &[FileType]) -> bool {
    for t in file_types.to_vec() {
         ...
    }
}

I know, that there are similar lints, e.g. for a String, that gets imediatly dereferenced to a &str, so maybe one could extend that lint?

Lint Name

intermediate_vec_on_loop_expression

Category

perf

Advantage

Remove the creation of an intermediate object and allocation

Drawbacks

no as far as I can tell

Example

for t in file_types.to_vec() {
     ...
}

Could be written as:

for t in &file_types {
     ...
}

[llogiq]

Note that in for i in &file_types, i is not the same as in for i in file_types.to_vec(), the former borrows each item, while the latter consumes them.

[hellow554]

Also i without to_vec is &T, while with it it's T.

But nevertheless it should be linted

[llogiq]

But then we should be careful while linting:

• If the items are only used by reference, we're likely OK with borrowck (e.g. calling methods that take &self), but note that with trait lookup, even then we may break compilation, as some traits may be available for Item but not &Item.
• Otherwise, we may use .iter().cloned() (or even copied(), depending on whether the type implements Copy).

Even then there is no guarantee of the type's Clone impl being side-effect free. So the proposed fix could change the behavior from cloning all items before entering the loop to cloning each item directly before the loop body consuming it. We'd need to consider this a false positive.

[camsteffen]

This can be covered by unnecessary_to_owned, currently in progress #7978

[smoelius]

Are we proposing that there be a code suggestion to rewrite the entire loop?

[llogiq]

No. I would either add .cloned or .copied to the iterator if necessary.

[smoelius]

Please forgive me, @llogiq, but it sounds like you have some idea as to how you would make this determination. If that's right, could you please share? (If not, that's cool too.)

EDIT: Determine whether .cloned or .copied is necessary, I mean.

[llogiq]

I was thinking about letting an ExprUseVisitor visit the loop body to determine what locals are used. Then compare that with the item pattern (the i in for i in ..), which notably may have multiple bindings. If there is at least one binding used by value, get its type and check if that type is Copy; if so, use .copied(), else .cloned(). Otherwise check if items are taken by mutable ref or immutable ref. In the former case, check if the slice is mutable; then you can .iter_mut(), otherwise keep iterating by value, in the latter case, just .iter() (or borrow the slice).

[hellow554]

In the specific example in my opening post it isn't necessary to make a copy or clone, because the elements are use as reference only.

[smoelius]

Thanks, @llogiq. I'll give this a shot.

[smoelius]

I am wondering if what we're describing should be two lints.

There's essentially two things going on here:

• the composition of to_vec with IntoIterator::into_iter
• for x in iter.cloned()/copied() { ... } where the cloned()/copied() call is unnecessary

Notionally, what we're saying is: notice the first bullet, transform it into the form of the second, and then determine whether the second bullet applies.

But each of the above bullets could appear in code, and so each could be linted for independently.

So what I am proposing concretely is to include just the first bullet in unnecessary_to_owned (#7978), and to have a separate lint for the second bullet.

I am happy to tackle both. I just think it might make sense to separate them into two lints.

Thoughts?

[llogiq]

I think both variants could be unnecessary_to_owned – it's just that with .copied()/.cloned() only the slice itself is needlessly converted to an owned instance (in this case a Vec) while in the reference-only case both the slice and its contents are needlessly converted.

[smoelius]

Consider this example (which is meant to be similar to the original example):

fn check_files(file_types: &[FileType]) -> bool {
    for t in file_types.to_vec() {
        let path = match get_file_path(&t) {
            Ok(p) => p,
            Err(_) => {
                return false;
            },
        };
        if !path.is_file() {
            return false;
        }
    }
    true
}

fn get_file_path(_file_type: &FileType) -> Result<std::path::PathBuf, std::io::Error> {
    Ok(std::path::PathBuf::new())
}

If you drop the .to_vec() in the for loop, the resulting code compiles, but needless_borrow triggers on &t in the call to get_file_path. Is that okay?

[llogiq]

Yes. Otherwise the lint would have to suggest both removing the .to_vec() and the borrows, but those suggestions fail in isolation.

[smoelius]

So, I guess I could add suggestions to remove those &, e.g., (typed by hand):

   |
LL |        let path = match get_file_path(&t) {
   |                                       ^ help: remove this
   |

But that would be overkill?

(Sorry, I should have thought about this more.)

[hellow554]

I think doing two lints in one, could be error prone and sometimes not replicable because a lint is triggered from two different places. So, ... ?

[smoelius]

Sorry, @hellow554, I'm not sure what you're suggesting. What I wrote was probably unclear.

I meant the lint could suggest to change multiple lines. To elaborate, the error message would look something like this:

error: unnecessary use of `to_vec`
  --> $DIR/unnecessary_to_owned.rs:197:14
   |
LL |     for t in file_types.to_vec() {
   |              ^^^^^^^^^^^^^^^^^^^ help: use: `file_types`
   |
  --> $DIR/unnecessary_to_owned.rs:198:39
   |
LL |        let path = match get_file_path(&t) {
   |                                       ^ help: remove this
   |

[smoelius]

So I think the question is: would it be too unruly to have a remove this for each relevant & in the loop body? Because, in principle, this number could be large.

[llogiq]

Either this or leave it to the needless_ref lint.

[smoelius]

I think I have this working in #7978.

[smoelius]

@hellow554 #7978 was merged. Maybe we can close this?

[hellow554]

@smoelius you could have added this issue to your close keyword so it would have been automatically closed ;)

thanks for the hard work! I really appreciate this! That's amazing work you have done.