manual_pop_if: avoid invalid automatic suggestions

[cuishuang]

changelog: [manual_pop_if]: avoid invalid automatic suggestions when the predicate borrows the collection

manual_pop_if could emit a machine-applicable suggestion when the predicate also referenced the collection being popped from.

For example, rewriting:

if vec
    .last()
    .is_some_and(|x| vec.len() > 1 && *x > 10)
{
    vec.pop().unwrap();
}

to vec.pop_if(...) requires a mutable borrow of vec while the predicate closure also immutably borrows it, causing E0502.

Only provide an automatic suggestion when the collection is a simple local path and the predicate does not reference that local. Continue emitting the lint with a help message for captured or non-local collection expressions.

Also retain the parameter pattern and avoid automatic rewrites for non-default binding modes such as ref and mut, since the predicate parameter changes from &T to &mut T.

Add UI coverage for all supported condition forms, complex collection expressions, and non-default parameter bindings.

[rustbot]

Thanks for the pull request. A reviewer will take a look after it receives 2 community reviews.

In the meantime, we would highly appreciate if you could try to review any of PRs waiting on community reviews.

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

[NicDevTV]

Could we also handle indirect borrows here? For example, let r = &vec; ... |x| r.len() > 1 ... isn’t detected, but rewriting to vec.pop_if(...) fails with E0502. The automatic suggestion should be suppressed in this case.

View changes since this review

[CommanderStorm]

community review:
Code wise good start, I am not quite happy with the wording of the new diags...

View changes since this review

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[cuishuang]

@CommanderStorm Thanks for the detailed feedback. I addressed the comments as follows:

• Removed the confusing explanation about automatic rewrites for non-default binding patterns.
• Added a span label at the collection use inside predicates that borrow the collection.
• Simplified the help for complex collection expressions.
• Moved suggestion classification into check_pop_unwrap.
• Kept pattern-preserving rewrites such as |ref x| out of this PR to keep the scope focused on avoiding invalid MachineApplicable suggestions. This can be handled in a follow-up.

I also rebased onto the latest upstream master and resolved the merge conflict. Please review it again.

[CommanderStorm]

is the span here entierly correct? It also includes the if...

View changes since the review

[cuishuang]

Yes, the secondary span is intended to cover only the vec path in vec.len(). The surrounding ^ characters belong to the primary lint span, which covers the whole if condition. The --- section is the secondary label for the collection borrow, so it does not include the if.
I agree that the overlapping spans make the rendered output a little confusing.

[CommanderStorm]

the span here also seems a bit suspect.. You sure that this is correct?

View changes since the review

[cuishuang]

The same applies here. The secondary span is taken from the HIR span of the vec path used in the predicate. The ^^^ before it belongs to the existing primary span for the inner condition, while the --- marks only the collection use.

So the labeled span is limited to vec, not the surrounding if. I can adjust the diagnostic presentation if the overlapping spans are considered too confusing.