Tweak error message when reborrowing &mut self as mut

[estebank]

• Point out the binding being reborrowed when trying to reborrow within a closure.
• When reborrowing &mut self inside a closure, the error message doesn't point at the reborrow statement but at the closure signature (Confusing error message with closures #47774). While we should fix the span itself, in the meantime mention the binding being reborrowed in the span label.

[rust-highfive]

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

[eddyb]

Why do I keep getting these?! Can we blacklist "error" and "suggest" for me in @rust-highfive?

r? @nikomatsakis

[estebank]

Sorry @eddyb, I'll make sure to set a reviewer going forward. I believe you were picked because you have substantial contributions to borrowck/mod.rs.

[nikomatsakis]

I agree the existing message is confusing, but this doesn't seem quite right. The problem is that &mut self desugars to self: &mut Self, and hence the variable self itself is immutable and can't be borrowed mutably. I find pointing out the "existing mutable borrow" somewhat misleading, since it suggests that this is a problem of a borrow conflicting with another action, but that's not right right.

For example, this code compiles:

struct Foo {
    u :u32,
}
impl Foo {
    fn foo(mut self: &mut Self) {
        || {
            let r = &mut self;
            r.u += 1;
        };
    }
    //~^^^^^ ERROR closure cannot assign to immutable argument `self` [E0595]
}

fn main() {}

I have to wonder -- rather than improve the diagnostics here, maybe we should make &mut self desugar to mut self: &mut Self so that these code snippets work?

[estebank]

I agree that &mut self should desugar to mut self and should probably be accepted. That being said, I would like to keep the label to other cases like TestEnum::Item(ref mut x).

[nikomatsakis]

I wonder if they should all desugar the same way? It seems like the error message is equally misleading here (or at least I find it misleading). The problem is that the binding is not mutable -- but why isn't it, anyway? I feel like we've been moving towards a place where it's ok to &mut borrow an &mut -- thanks to deref coercions etc -- and I wonder if we should just embrace it.

Not sure how to make this decision. Feels like a thin RFC, but still a highly visible change in some sense, certainly would want at least FCP!

cc @rust-lang/lang -- thoughts? The context is that &mut self and ref mut foo both desugar to an immutable binding, but people sometimes stub their toe writing &mut self or &mut foo, which results in an error (where &mut *self and &mut *foo would work).

@estebank -- that reminds me, the suggestion to remove &mut (instead of adding *) is not necessarily a good one. Removing &mut can result in moves, where adding * would work more often and be closer to the existing "intent" of the code, I think.

[estebank]

Changing the semantics to accept this in all cases sounds eminently reasonable to me. I know I found it confusing at first, even though I'm now used to it.

[nikomatsakis]

Interestingly, this also resolved something with match desugarings. I kind of forget the details now, but there was a minor inconsistency that centered about ref mut not being mut... maybe @cramertj remembers?

[nikomatsakis]

@estebank you want to try implementing those changes? Maybe we should open up an issue on it.

[estebank]

@nikomatsakis can you create the ticket with any pitfalls on the new semantic that we should avoid? I believe I can take this up probably during the weekend.

[estebank]

Closing in light of the conversation.