Fix deref field pattern suggestions and improve error messages

[chenyukang]

View all comments

Fixes #146995

[rustbot]

r? @fmease

rustbot has assigned @fmease.
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: compiler
• compiler expanded to 69 candidates
• Random selection from 11 candidates

[chenyukang]

it's perfect if we suggest ref mut here, but current diagnostic is better than &arg.field, because it's at least a right direction, since user will get this if apply ref fix here:

help: consider changing this to be a mutable reference
  |
7 |         Some(ref mut s) => s.push('a'), //~ ERROR cannot borrow `s` as mutable
  |                  +++

[fmease]

I wonder how other parts of the crate handle this 🤔 Surely, there's some infrastructure for this already and it's not just people hoping that the user already wrote a mut ^^.

[fmease]

I wonder how other parts of the crate handle this 🤔 Surely, there's some infrastructure for this already and it's not just people hoping that the user already wrote a mut ^^.

[chenyukang]

@rustbot ready

[fmease]

Two final questions; then we should be good to go

View changes since this review

[fmease]

Is this for disqualifying e.g., (&wrap)[0] / wrap[0] where wrap: &_? Do we still suggest &(&wrap)[0] / &wrap[0] as on main? If so, I don't understand the need for the extra check if the resulting suggestion is wrong either way. Did you add this check for a different reason I'm not thinking of?

In any case, this would also disqualify (&mut wrap)[0], wouldn't it, while the mut → ref mut would actually work out perfectly fine; if you keep this check you should probably check that ty::Ref(.., ty::Mutability::Not).

fn take(mut wrap: Wrap<[Option<NonCopy>; 1]>) {
    if let Some(mut val) = (&mut wrap)[0] { //~ ERROR cannot move out of dereference of `Wrap<Struct>`
        val.0 = ();
    }
}

[fmease]

If you're verifying the base type of indexing exprs (about which I have questions), then there should be no reason not to also check the base type of field exprs.

In user code, that situation would arise given e.g., (&wrap).field (❌ disqualify | don't suggest adding &¹, don't suggest adding ref); (&mut wrap).field (✅ qualify | don't suggest adding &, do suggest adding ref)

Footnotes

1. I think your PR still suggests that? not sure ↩

[chenyukang]

Thanks, this should be addressed now. The check now distinguishes &T from &mut T, so (&mut wrap)[0] still qualifies for the ref mut suggestion.

I also applied the same base-type check to field expr, so shared-ref bases like (&wrap).field are disqualified while (&mut wrap).field still gets the pattern-binding suggestion.

Done some trivial code refactor and sqush into one commit by the way.

[rustbot]

This PR was rebased onto a different main 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.

[chenyukang]

@rustbot ready