manual_option_as_slice: ignore by-reference bindings

[cuishuang]

changelog: [manual_option_as_slice]: avoid incorrect suggestions for by-reference Some bindings

extract_ident_from_some_pat previously ignored the effective binding mode of the inner Some pattern.

For Some(ref value) matched against option.as_ref(), value has an additional reference layer. As a result, slice::from_ref(value) can produce a &[&T], while the suggested option.as_slice() produces a &[T]. Applying the machine-applicable suggestion can therefore cause a type error.

Check the binding mode computed by type checking and only lint by-value bindings. Add regression tests for ref and ref mut 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

[cuishuang]

@blyxyas Would you mind taking a look at this when you have a chance? This fixes an incorrect suggestion in manual_option_as_slice. Thanks!

[CommanderStorm]

Community review:
The actual lint change makes sense to me as far as I can see.

🤔 I don't know if is_empty_slice should be moved above perf wise due to this change though. Have you checked this?

View changes since this review

[cuishuang]

Community review: The actual lint change makes sense to me as far as I can see.

🤔 I don't know if is_empty_slice should be moved above perf wise due to this change though. Have you checked this?

View changes since this review

Yes, is_empty_slice is already checked before extract_ident_from_some_pat. Since the && chain short-circuits, the new extract_binding_mode call is only made after the None arm is confirmed to be an empty slice, so I think the current ordering is appropriate.

[CommanderStorm]

(make sure to remember to squash+force push your commits 😉)

[cuishuang]

could you also add test coverage for the second path to this code (if-let)?

Squshed.

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

no need. my review is already an +1.
See #17560 (comment) for our process regarding reviews if you want to also help other people move their PRs move forward 😉