iter() -> into_iter()

[tschuett]

What it does

Propose to change iter() to into_iter()

Categories (optional)

• Kind: See https://github.com/rust-lang/rust-clippy/blob/master/README.md#clippy for list of lint kinds

What is the advantage of the recommended code over the original code

For example:

My old code: foo.unwrap_or_default().iter().... By hand I changed it to: foo.unwrap_or_default().into_iter().... Then clippy proposed to remove some clone()s When I removed the clone()s, clippy proposed to change filter + map to filter_map clippy is awesome. The code looks beautiful now.

The suggestion to change iter() to into_iter() was missing and blocking the other lints

Drawbacks

None.

Example

foo.unwrap_or_default().iter()....

Could be written as:

foo.unwrap_or_default().into_iter()....

[xFrednet]

iter() and into_iter() behave differently, as iter() iterates over item references while into_iter() takes ownership of the collection and iterates over the actual items (See docs).

The lint should therefore only suggest the usage of into_iter() if the collection is owned in the current context and not used later. This would basically be the opposite of the into_iter_on_ref lint, that implementation can most likely be taken as a reference.

I like the lint suggestion, and these are only some pointers for the implementation.

---

@rustbot label +good-first-issue

[tschuett]

I am aware of the semantic differences. I just want to say if iter() and into_iter() are valid options, then the later one may allow to write more beautiful/idomatic code.

[matthiaskrgr]

Could be a duplicate of https://github.com/rust-lang/rust-clippy/issues/5305 ^^

We should be careful though. I could imagine that this lint fires in a lot of places but if we change v.iter() into v.into_iter(), and want to usevagain afterwards (for adbg!()` for example or just because we continue coding in the file) it could be annoying because clippy does not know we are going to use it somewhere else and complains all the time.

[Luro02]

Using into_iter instead of iter caused the issue which made it impossible to implement IntoIterator for [T; N]:

https://blog.rust-lang.org/2021/06/17/Rust-1.53.0.html#intoiterator-for-arrays

[xFrednet]

Hey @Luro02, could you elaborate your point a bit? I'm sadly not quite sure what you mean. :upside_down_face:

[rsmantini]

@rustbot claim

[rsmantini]

I had a first try to tackle this issue and it seems to me that mir analysis will be need to solve it as it needs to guarantee that:

1. The collection where iter is called is owned
2. It is not used afterwards nor has any borrows that are used (Am I missing any case?)

So unlike into_iter_on_ref which is an expression lint this one should be a function wide lint somewhat similar to redundant_clone but a bit less complex I think.

Do I make sense or am I missing something and going to the wrong direction? Thanks :)

[rsmantini]

I played a bit with this one but it is way above my current knowledge. This what I got https://github.com/rsmantini/rust-clippy/pull/2 It works for some simple cases but fails to things like:

    let v7: Vec<String> = vec!["a".into()];
    // this triggers the lint :(
    let _: Vec<&str> = v7.iter().map(|x| x.as_ref()).collect();

The code very naive, probably inefficient and might have reinvented the wheel in some dumb ways.

Also I think this is way too complicated from a good-first-issue although I had fun with it :)

[xFrednet]

Hey @rsmantini, sorry, I missed your first comment. These things sadly sometimes get lost. If you get stuck and no one answers you, please ping us directly or ask on Zulip :upside_down_face:

Anyway, I labeled this as a good first issue, my rough idea was outlined in this comment. The idea had these steps:

1. Find occurrences of <expr>.iter() (This can be done in the methods module)
2. Check the origin of <expr> to see if it's owned. Basically, check if <expr> is a variable and if the type is an object or a reference
3. Check if the type implements std::iter::IntoIterator
4. Lastly, check that the value is never used after this call to .iter(). This is the more tricky part, but we have some examples with Visitors like clippy_utils::visitors::for_each_expr

If you would rather not continue this issue, it's totally fine. It's good to hear that you at least had fun :upside_down_face:

[rsmantini]

Hi @xFrednet no worries :)

About the implementation, I had trouble with step 4. Maybe I'm over complicating things, but I think it's analogous to what the borrow checker does, so it needs to be done at the mir level. For instance in the example I gave above:

fn foo() {
    let v: Vec<String> = vec!["a".into()];
    let _: Vec<&str> = v.iter().map(|x| x.as_ref()).collect();
}

The collection v is not used anymore after iter is called but it's still invalid to change it to into_iter. So there is one more check on step 4 that is if no references of the origin are still alive after the iter call Does it make sense?

[xFrednet]

Ohh, I didn't think about the fact that the new target value could still reference the old one from iter(). In that case, this issue becomes a lot harder. You're right :sweat_smile:

In Clippy we rarely interact with the borrow checker. I can't think of a lint where we actually do that. Often we just check for these rules by checking ownership based on types. But I'm unsure how that would be done in this case. Thank you for pointing that out :upside_down_face:

@rustbot label -good-first-issue

[jplatte]

Often we just check for these rules by checking ownership based on types. But I'm unsure how that would be done in this case.

As a cheap solution in this case, the lint could only trigger when the item type of the iterator before collect() (or the output type of collect()) is 'static? That would result in false negatives when lifetimes captured by the output type refer to something outside the input of .iter(), but still be better than not having this lint at all.

[rsmantini]

I can't think of a lint where we actually do that

I think redundant_clone does something similar. I took some inspiration there but I still don't quite understand how the GenKillAnalysis work :(

As a cheap solution in this case, the lint could only trigger when the item type of the iterator before collect() (or the output type of collect()) is 'static? That would result in false negatives when lifetimes captured by the output type refer to something outside the input of .iter(), but still be better than not having this lint at all.

Interesting idea, shouldn't be too hard to do. I wonder if there are methods other than collect that can get us into trouble. I'll try to add that check and see what dogfood says about my patch

[jplatte]

Ah, my idea doesn't actually work.. This (silly) example compiles fine, but breaks with into_iter:

fn foo() {
    let v: Vec<String> = vec!["1".into()];
    let _: Vec<u32> = v
        .iter()
        .map(|x| x.as_str())
        .map(|x| x.parse().unwrap())
        .collect();
}