Suggest using find rather than implementing it using a for loop

[teor2345]

What it does

Suggests replacing a for loop that searches for an item in an iterable, with the find iterator method.

Categories (optional)

• Kind: correctness

It's easier to make mistakes in a manual implementation. Using find is simpler and easier to maintain, focusing the attention on the predicate.

Drawbacks

Suggestions might not work in complex edge cases.

If the lookup does a mapping, it might be harder to create a good suggestion. If the lookup finds an index, we should suggest Iterator::index instead of find.

Example

const ARRAY: &[u32; 5] = &[2, 7, 1, 9, 3];

fn lookup(n: u32) -> Option<u32> {
    for &v in ARRAY {
        if v == n {
            return Some(v);
        }
    }
    None
}

Could be written as:

const ARRAY: &[u32; 5] = &[2, 7, 1, 9, 3];

fn lookup(n: u32) -> Option<u32> {
    ARRAY.iter().find(|&&v| v == n).cloned()
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=80518a6a20d1a57658a6c72bd7967a44

[llogiq]

That loop code example returns the element itself, not the index, so the suggestion would change behavior.

[teor2345]

That loop code example returns the element itself, not the index, so the suggestion would change behavior.

Thanks, I've made a playground and fixed a few other issues with the code.

[camsteffen]

This could be generalized a bit to suggest find_map when applicable (possibly under a different lint).

[ebobrow]

I thought I'd give this a try, but I've hit a bit of a wall. Currently, I have an implementation that replaces the for loop with an if let no matter what, so for something like the example code that would produce

const ARRAY: &[u32; 5] = &[2, 7, 1, 9, 3];
                                                        
fn lookup(n: u32) -> Option<u32> {
    if let Some(&v) = ARRAY.iter().find(|&&v| v == n) {
        return Some(v);
    }
    None
}

which is not great. Would it be worth it to hard code certain use cases (function return, variable assignments, etc.) to create better suggestions, and if so, what use cases? I'm drawing a blank because I'm guessing it would be more complicated than this in real code.

[xFrednet]

I think for this, it's reasonable to hard-code for certain use cases. You probably first want to write the detection part for the for-loop, and then identify the use case. As for the cases, I would first focus on one, like the return or value assignment, and then create a PR. More cases can be added afterwards.

From my feeling, I wouldn't even directly call this hard-coding. We simply restrict the lint to specific cases. It's usually better to have some false negatives than false positives. For this lint, the amount of use-cases is also small and should work out fine. :)

---

For the question of which use-cases should/can be supported. Currently, I can only think of three. The two you mentioned and one where a function or macro is called like this:

for &v in ARRAY {
    if v == n {
        println!("Print value {}", v);
    }
}

I think that the two use-cases you mentioned are probably the most common and useful once.