unnecessary_fold: lint folding over an Option's iterator

[bugprone]

Fixes #1658

Folding over an Option's iterator visits zero or one items, which is map_or in disguise:

opt.iter().fold(init, |acc, x| f(acc, x))
// is
opt.as_ref().map_or(init, |x| f(init, x))

(as_mut() for iter_mut(), plain map_or for into_iter(), which consumes the Option.)

This extends unnecessary_fold rather than adding a new lint, and only runs when none of the existing any/all/sum/product cases fired, so existing behavior (and the existing test expectations) are unchanged.

Suggestion details:

• accumulator uses in the closure body are substituted with the init expression, parenthesized when needed
• machine-applicable when the accumulator is unused, or init is a literal or a Copy binding
• MaybeIncorrect when substituting duplicates an arbitrary expression (it becomes evaluated only in the Some case, which changes behavior for side-effecting init)
• omitted entirely when it would move a non-Copy binding twice — no compiling suggestion exists without restructuring (covered in unnecessary_fold_unfixable.rs)
• the whole suggestion is spliced from source text, so it bails out of suggesting when any part comes from a different syntax context

changelog: [unnecessary_fold]: lint fold over an Option's iterator and suggest map_or

[rustbot]

Thanks for the pull request, and welcome!

You should hear from one of our reviewers after this PR gets at least 2 reviews from the community.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[Gri-ffin]

Also why not move the logic to a function then have something like this?

if check_standard_fold(...) {
    return;
}
check_fold_option(...);

or something similar instead of tracking state using fired?

View changes since this review

[Gri-ffin]

compute() is evaluated here twice, which can return a different result each time if the function returns a different value on each call, although its marked as MaybeIncorrect, suggesting this can still change the code behavior.

[bugprone]

Agreed — resolved in 413edb1 by no longer linting this case at all: substituting a call re-evaluates it only in the Some case, so there is no correct machine fix. Every emitted suggestion is now MachineApplicable.

[Gri-ffin]

The other folds handle only literals, so why not keep that behavior too? Since handling other cases can be unsafe.

[bugprone]

Done in 413edb1, with one deliberate extension: literals plus bindings of Copy types. The issue's motivating example folds with a binding init (opt.iter().fold(my_val, ...)), and reading a Copy binding twice is exactly as safe as duplicating a literal — no move, no side effects. Everything else no longer lints. Happy to narrow to literals only if you or the reviewer prefer; it's a one-line change.

[Gri-ffin]

Aren't we linting here but giving no suggestion? is this intentional? why not avoid linting at all?

[bugprone]

It was intentional (lint the smell even when we can't fix it), but you're right that it's inconsistent with how the rest of this lint behaves — gone in 413edb1. Cases without a correct suggestion are no longer linted.

[bugprone]

Thanks for the review! All four points addressed in 413edb1:

• replaced the fired flag with an early return from an extracted check_standard_fold
• the Option case now only lints when init is a literal or a binding of a Copy type, so every suggestion is MachineApplicable — the MaybeIncorrect tier, the suggestion-less path, and the unnecessary_fold_unfixable test are all gone

[Gri-ffin]

I would personally prefer to have a back and forth with a human if possible, I don't mind if you use LLMs as a tool to help implement an initial draft or to help you understand concepts, but I would prefer to not be the first person to have read this code and I'd much rather discuss things directly with you (even if the implementation isn't right) than debate an LLM.

[bugprone]

@Gri-ffin I strongly agree with you. English isn't my first language, so I mainly use an LLM to help with writing. I believe every decision is mine. Btw, I'll try to use my own words.