Lint .iter().for_each()

[kangalio]

What it does

A thing I see really often is beginners attempting to embrace functional style programming by replacing all for-loops with collection.iter().for_each(|item| ...). Because this happens so often, it might be beneficial to introduce a clippy lint to reassure those people that there's nothing wrong with the traditional for-loop.

This lint would be triggered when the following criteria match:

• a method is called on a binding (e.g. foo.method() matches, make_foo().method() doesn't. That's because long iterator chains ending with for_each should not trigger this lint)
• the methods returns an Iterator object
• for_each is called on the Iterator object, with a closure parameter (e.g. vec.iter().for_each(process) should not trigger because the alternative is (arguably) inferior: for item in &vec { process(item) })

Categories (optional)

• Kind: clippy::complexity

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

The code is easier to digest than the syntax-heavy for_each with the embedded closure.

Drawbacks

None.

Example

hash_map.keys().for_each(|key| {
    // ...
});

Could be written as:

for key in hash_map.keys() {
    // ...
}

[llogiq]

One downside is that iter().for_each(..) can be faster, for example with chain(_)s. So unless the iterator is of a type where we know the perf to be the same, this should not lint.

[camsteffen]

@llogiq I think the intention is to only lint non_iterator.iter().for_each().

[kangalio]

Yes, camsteffen is right. Actually, with the criteria I wrote above, the normal usage of chain() won't even trigger the lint:

// doesn't lint
collection.iter().chain(...).for_each(...);

The two situations in which it does falsely lint are these:

let temp = collection.iter();
temp.chain(&collection2).for_each(...);

impl MyCollection {
    pub fn iter(&self) -> impl Iterator<Item = MyType> {
        // returning a chaining iterator as impl Iterator from a method
        self.collection.iter().chain(&self.collection2)
    }
}
my_collection.iter().for_each(...);

I think both of these patterns are somewhat exotic. It's probably unlikely that false positives from this lint will be a problem in practice.

If it does turn out to be a problem, the lint can be further restricted to only trigger when for_each is called on a known Iterator implementation like std::slice::Iter, std::slice::IterMut, std::vec::IntoIter (and equivalent for all other standard library collections).

[camsteffen]

Good points. Actually I think returning impl Iterator could be somewhat common. It's a very practical use of impl Trait. Restricting to slices and std collections seems good to me.

[Y-Nak]

I'm working on it!
@rustbot claim

[llogiq]

Please note that any return in the loop body needs to be changed to a continue. And we'd need a label for that if the return is within an inner loop.

[Y-Nak]

Good point! Thanks!