manual_pop_if: avoid invalid automatic suggestions

[cuishuang]

View all comments

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

[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]

Community review:

Thanks, no blocking issues from my side.
collection_use_span could likey be modeled using a dedicated enum variant in SuggestionKind, but that is more of a nit than necceary change.

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]

Resolved the merge conflicts. @CommanderStorm Please review it again.

[CommanderStorm]

my review was already +1, you need another community +1 (maybe consider reviewing 1-2 PRs 😉).
See comment from above regarding this process #17561 (comment)

I think @NicDevTV code review from above still needs an answer, afterwards he likely also will +1.

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.

[cuishuang]

my review was already +1, you need another community +1 (maybe consider reviewing 1-2 PRs 😉). See comment from above regarding this process #17561 (comment)

I think @NicDevTV code review from above still needs an answer, afterwards he likely also will +1.

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.

Thanks, you're right. I addressed the indirect-borrow case by making automatic suggestions conservative: if the predicate references any local other than the predicate binding, no machine-applicable suggestion is emitted. This catches aliases such as let r = &vec; the lint is still emitted with a regular help message.

I also added regression coverage for the is_some_and, if let, let-chain, and map(...).unwrap_or(false) forms.

Please take another look. @NicDevTV @CommanderStorm

[NicDevTV]

I don't see any blocking issues from my side now. LGTM.

View changes since this review

[rustbot]

r? @Manishearth

rustbot has assigned @Manishearth for the project review.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 9 candidates
• 9 candidates expanded to 9 candidates
• Random selection from 6 candidates

[Manishearth]

Are you using an LLM here?

https://github.com/rust-lang/rust-clippy/blob/master/CONTRIBUTING.md#llm-policy

[cuishuang]

Are you using an LLM here?

https://github.com/rust-lang/rust-clippy/blob/master/CONTRIBUTING.md#llm-policy

I use CodeX to refine unit tests and perform a review with it before the final submission. Does this need to be mentioned in the PR detail?

[Manishearth]

Please actually read and follow the LLM policy

[Manishearth]

Currently we do not allow LLM generated content in rust-lang/rust-clippy. I believe I must close this PR, sorry.

[cuishuang]

Currently we do not allow LLM generated content in rust-lang/rust-clippy. I believe I must close this PR, sorry.

The vast majority of the content was not generated by an LLM; I primarily used the tool to review my own work. I have read the relevant information and understand your decision to close the matter, though I feel the current regulations are overly strict---perhaps even running counter to the times. I believe they will eventually change.

I will be more careful with future submissions and handle them all manually.

Thank you again for your guidance throughout our extensive exchange of over thirty messages.