Uplift `clippy::for_loops_over_fallibles` to `rustc`

[WaffleLapkin]

@jyn514 showed an interesting bug:

for x in rx.recv().await {
    // ...
}

The sneaky bug is that rx.recv().await returns an Option and Option implements IntoIterator. So instead of calling rx.recv().await repeatedly until it returns None (intended usage), this calls it once and executes the loop body if it's Some(x).

This can easily be written by a newcomer or even an experienced user, it compiles and does an unexpected thing. This is especially easy to write with async code since we don't have async version of for loop and while let is commonly used instead. Option: IntoIterator is useful in generic code or with iterator combinators (e.g. .chain(opt)), but I think using it in a for loop is never intended.

Clippy has a lint for that: for_loops_over_fallibles which, in my opinion, should be uplifted to rustc.

Clippy currently only suggests using if let Some (i.e. preserving code behavior), but we could also suggest using while let Some (i.e. changing code behavior, in recv case to the good) and, in case of for x in i.next() we can suggest for x in i (iterator specific fix).

---

I'm willing to work on this, but want to first get a signoff that we indeed want to uplift the lint :)

@rustbot label +A-lint +T-compiler +D-newcomer-roadblock

[jyn514]

According to #53224 (comment) this needs approval by the lang team, so

cc @rust-lang/lang, how do you feel about uplifting this lint?

[scottmcm]

(Lang hat on, but not after a team discussion) That sounds eminently reasonable to me. Using a loop feels wrong when something weaker (like if let) would suffice -- and would give fewer move/borrow errors, too, since for bodies (unless there's an unconditional break) are always considered to potentially run multiple times.

Exactly what the best suggestion would be I'm less confident. Especially for Result, since that's also iterable, but for x in foo().await might be if let Ok(x) in foo().await but it might also be for x in foo().await?.

---

EDIT after the lang meeting: This feels to me like a general idea of "well the trait bound is nice (for things like .flatten()), but in non-generic situations you should use the more specific thing".

[jyn514]

We can suggest the latter iff foo() is iterable.

Honestly I think any of these suggestions would be extremely helpful - the hard part is realizing this doesn't do what you want, I think it's ok for the suggestion not to be ideal since the fix will usually be pretty obvious to the author.

[joshtriplett]

This looks like a great lint. Looking at the example where it would warn made me shudder; catching that seems like a big help.

I agree that just flagging suffices to call attention to the problem.

In terms of suggestions, I think it would make sense to suggest:

• Both if let Some(x) = and while let Some(x) = if it's Option<T>, with an explanation of the difference.
• ? if it's a Result<T, E> for which T is iterable. (If there's a way to filter for "and T would otherwise type-check" that might help reduce false positives.)

There are other possibilities, such as Option<T> where T is iterable, but that seems likely to produce many false positives. Even the Result<T, E> heuristic seems likely to produce false positives; for instance, you might have Result<Vec<u8>, E> and want to process the byte string as a whole rather than iterating over it. But as you said, calling attention to the problem seems like the most important part.

I think it'd be appropriate to make this warn-by-default.

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting, and we're in favor. Let's do it.

[scottmcm]

(Procedural Note: We don't need an FCP for this because it's not a breaking change to remove it later, should that be necessary.)

[WaffleLapkin]

fyi: I've opened an uplifting PR: #99696