`&mut &T` coerced to `&T` suggests `&mut mut x`

[cramertj]

This code (play):

struct X;
impl X {
    fn mutate(&mut self) {}
}

fn main() {
    let term = Some(X);
    let term = match &term {
        Some(term) => &mut term,
        None => term.as_ref().unwrap(),
    };
    term.mutate();
}

Produces these errors:

error[E0596]: cannot borrow `term` as mutable, as it is not declared as mutable
 --> src/main.rs:9:23
  |
9 |         Some(term) => &mut term,
  |                       ^^^^^^^^^
  |                       |
  |                       cannot borrow as mutable
  |                       try removing `&mut` here

error[E0596]: cannot borrow `*term` as mutable, as it is behind a `&` reference
  --> src/main.rs:12:5
   |
9  |         Some(term) => &mut term,
   |                       --------- help: consider changing this to be a mutable reference: `&mut mut term`
...
12 |     term.mutate();
   |     ^^^^ `term` is a `&` reference, so the data it refers to cannot be borrowed as mutable

error: aborting due to 2 previous errors

Note that the second error suggests changing &mut term to &mut mut term, which obviously won't fix the issue.

[cramertj]

(note that this isn't match_ergonomics specific-- the issue reproduces if the match is changed to match term { and Some(ref term))

[arielb1]

So the issue here is with the coercion, not with match ergonomics. (play)

struct X;
impl X {
    fn mutate(&mut self) {}
}

pub fn foo(x: bool) {
    let mut term = X;
    let ref_term = if x {
        &mut term
    } else {
        &X
    };
    ref_term.mutate();
}

Also, plz no parsing Rust with regular expressions:

struct X;
impl X {
    fn mutate(&mut self) {}
}

pub fn foo(x: bool) {
    let mut term = X;
    let ref_term = if x {
        & /* c/*om*/ment */ mut term
    } else {
        &X
    };
    ref_term.mutate();
}

[saleemjaffer]

@spastorino I would like to help with this.

[spastorino]

@saleemjaffer I know more or less how to fix the issue but never found time to tackle it. So yeah, go ahead.

[pnkfelix]

NLL triage. Assigning P-high priority, because actively misleading the user can impede the learning process about the language as a whole

[pnkfelix]

@spastorino would you be willing to provide mentorship for @saleemjaffer, outlining the approach you were thinking of for resolving it?

[spastorino]

@pnkfelix @saleemjaffer sure, ping me on Zulip. Until March 31 I'm very busy with Rust Latam though, still ping me and we can see how I can help.

[saleemjaffer]

Sure, I'll be picking this up in a day.

[saleemjaffer]

Just to be clear:

struct X;
impl X {
    fn mutate(&mut self) {}
}

pub fn foo(x: bool) {
    let mut term = X;
    let ref_term = if x {
        &mut term
    } else {
        &X
    };
    ref_term.mutate();
}

Instead of this error:

error[E0596]: cannot borrow `*ref_term` as mutable, as it is behind a `&` reference
  --> src/lib.rs:14:5
   |
10 |         & mut term
   |         ---------- help: consider changing this to be a mutable reference: `&mut  mut term`
...
14 |     ref_term.mutate();
   |     ^^^^^^^^ `ref_term` is a `&` reference, so the data it refers to cannot be borrowed as mutable

what exactly do we want to show? Do we want to not show this:

10 |         & mut term
   |         ---------- help: consider changing this to be a mutable reference: `&mut  mut term`
...

This way the user understands that &mut term has been cooerced to &term and is not mislead.

[spastorino]

@saleemjaffer exactly, we want to avoid the help suggestion.

[saleemjaffer]

https://github.com/rust-lang/rust/blob/dd363d14ae842ab5aae3ee81b337f26aabcb8875/src/librustc_mir/borrow_check/mutability_errors.rs#L374-L424

In particular this is where the issue occurs: https://github.com/rust-lang/rust/blob/dd363d14ae842ab5aae3ee81b337f26aabcb8875/src/librustc_mir/borrow_check/mutability_errors.rs#L385-L395

[spastorino]

hey @saleemjaffer how are you going with this?. Sorry for the absense but I was running Rust Latam and I had zero time. Let me know your progress, feel free to create a Zulip thread and start talking about this.

[saleemjaffer]

https://rust-lang.zulipchat.com/#narrow/stream/147480-t-compiler.2Fwg-diagnostics/topic/.2357431

[spastorino]

@rustbot release-assignment

Just figured that I was still assigned to this.

[rustbot]

Error: Cannot release unassigned issue

Please let @rust-lang/release know if you're having trouble with this bot.

[pnkfelix]

The problem as shown in @arielb1's reduction no longer reproduces.

Click for info about when the behavior on the reduction changed

That change happened between rustc 1.37.0-nightly (0dc9e9c10 2019-06-15) and rustc 1.37.0-nightly (4edff843d 2019-06-16). The following PR's landed in that time period: ``` 4edff843dd219cf19a5fede6c78c7ce95402e1f5 Auto merge of #61347 - Centril:stabilize-underscore_const_names, r=petrochenkov 799cf3f603b5809a68853efcd779671cb8e046c4 Auto merge of #61881 - glaubitz:sparc64-ffi-abi, r=petrochenkov e3175c34b4211d219f114d6dc608194ebaf03c44 Auto merge of #61754 - nikomatsakis:trait-caching-perf-3, r=pnkfelix 37b6a5e5e82497caf5353d9d856e4eb5d14cbe06 Auto merge of #61739 - chansuke:separate-unit-tests, r=petrochenkov 68655029d49d20cadcff849d14b362b6778ce86a Auto merge of #60730 - matthewjasper:optimize-false-edges, r=pnkfelix 374c63e0fc356eb61b1966cb6026a2a49fe9226d Auto merge of #61886 - Centril:rollup-3p3m2fu, r=Centril ``` And thanks to [rustup-toolchain-install-master](https://github.com/kennytm/rustup-toolchain-install-master), I was able to quickly install the most relevant of the above commits, in order to determine that yes, this bug was resolved by #60730: we now issue the following diagnostic here: ``` error[E0596]: cannot borrow `*ref_term` as mutable, as it is behind a `&` reference --> issue-57431-narrowed.rs:13:5 | 11 | &X | -- help: consider changing this to be a mutable reference: `&mut X` 12 | }; 13 | ref_term.mutate(); | ^^^^^^^^ `ref_term` is a `&` reference, so the data it refers to cannot be borrowed as mutable ```

However, I believe this is merely a masking of a still present bug; going back to @cramertj 's original example, we continue to see the following diagnostic:

error[E0596]: cannot borrow `term` as mutable, as it is not declared as mutable
 --> src/main.rs:9:23
  |
9 |         Some(term) => &mut term,
  |                       ^^^^^^^^^
  |                       |
  |                       cannot borrow as mutable
  |                       try removing `&mut` here

error[E0596]: cannot borrow `*term` as mutable, as it is behind a `&` reference
  --> src/main.rs:12:5
   |
9  |         Some(term) => &mut term,
   |                       --------- help: consider changing this to be a mutable reference: `&mut mut term`
...
12 |     term.mutate();
   |     ^^^^ `term` is a `&` reference, so the data it refers to cannot be borrowed as mutable

[pnkfelix]

Ah, here is a way to fix @arielb1 's reduction (and reduce it even further!) (play):

struct X;
impl X {
    fn mutate(&mut self) {}
}

pub fn foo() {
    let mut term = X;
    let ref_term: &X = &mut term;
    ref_term.mutate();
}

fn main() { }

which currently emits the diagnostic:

error[E0596]: cannot borrow `*ref_term` as mutable, as it is behind a `&` reference
 --> src/main.rs:9:5
  |
8 |     let ref_term: &X = &mut term;
  |                        --------- help: consider changing this to be a mutable reference: `&mut mut term`
9 |     ref_term.mutate();
  |     ^^^^^^^^ `ref_term` is a `&` reference, so the data it refers to cannot be borrowed as mutable

[pnkfelix]

As hinted at in discussions on PR #59808, there are two tacks we might take with this:

1. In the short term, we should attempt to stop emitting the diagnostic when its going to say something like &mut mut.

2. In the long term, we should attempt to provide the user with a better diagnostic for this scenario.

   • The issue is that there is a &mut &T-to-&T coercion inserted, and then we subsequently try to suggest to the user that they replace the (coerced from &mut &) & with &mut, so it ends up looking like &mut mut to the user.
   • So, can we do better? Not clear; at the very least, we need point to the actual source of why the compiler has "inferred" an &X type for ref_term
   • in my latest reduction, its not even an inference at all, its an outright type annotation (that the compiler should be highlighting, but is not).

[saleemjaffer]

@pnkfelix I was actually working on #59808. I have some time now and can pick this up again!

So, can we do better? Not clear; at the very least, we need point to the actual source of why the compiler has "inferred" an &X type for ref_term

I actually figured out where in the code the complier inferred a &X for ref_term. But again do we want to do the short term or the long term solution?

[pnkfelix]

Hi @saleemjaffer ; I was on leave, but I am back now.

Sometimes we decide a given short-term solution is not worth the effort, and jump straight to a long-term solution. I.e. if initial investigation of the short-term solution indicates that it is nearly as much effort as the long-term one would be.

My guess is that this is not one of those cases. I believe we could have people independently work on the short- and long-term solutions being suggested here.

(That's my way of saying: Feel free to work on whichever aspect you prefer. If you think you can make progress on the long-term fix, feel free.)

[saleemjaffer]

Yeah I can pick this up in a few days!

On Wed, 11 Sep 2019, 15:09 Felix S Klock II, notifications@github.com wrote:

Hi @saleemjaffer https://github.com/saleemjaffer ; I was on leave, but I am back now.

Sometimes we decide a given short-term solution is not worth the effort, and jump straight to a long-term solution. I.e. if initial investigation of the short-term solution indicates that it is nearly as much effort as the long-term one would be.

My guess is that this is not one of those cases. I believe we could have people independently work on the short- and long-term solutions being suggested here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/57431?email_source=notifications&email_token=AESUZVDC65O7JBL3HXUQHHLQJC4ETA5CNFSM4GORVH42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6N4YAQ#issuecomment-530304002, or mute the thread https://github.com/notifications/unsubscribe-auth/AESUZVAJ5VPOB6UIQFBEYXLQJC4ETANCNFSM4GORVH4Q .

[nellshamrell]

Looking into this one

[nellshamrell]

Here's what I'm thinking:

Step 1: Remove the help dialog entirely - it's point the user in the wrong direction entirely.

Step 2: Somehow identify that a coercion has taken place and indicate that to the user to give them a clue as to how to fix the issue.

The fix for:

struct X;
impl X {
    fn mutate(&mut self) {}
}

pub fn foo() {
    let mut term = X;
    let ref_term: &X = &mut term;
    ref_term.mutate();
}

fn main() { }

is

struct X;
impl X {
    fn mutate(&mut self) {}
}

pub fn foo() {
    let mut term = X;
    let ref_term: &mut X = &mut term;
    ref_term.mutate();
}

fn main() { }

Identifying every type of coercion would be quite difficult - but by indicating to the user that a coercion has likely occurred, we can hopefully point them in the correct direction to fix their code.

[nellshamrell]

I have a failing test in place on my fork:

Test File Expected STDERR

[nellshamrell]

On initial investigation, I think the place to fix this may be in the check_access_permissions method of the borrow checker.

[nellshamrell]

I did a pretty thorough debug trace through the borrow checker, trying to figure out what borrow rule I should focus on editing.

I first added some additional debug statements to the borrow checker here to allow me to better track the process with which the code in my test is executed by the borrow checker.

I then built the stage 1 compiler with

./x.py build --stage 1

Then I ran my test in debug mode with:

RUSTC_LOG=debug /home/nell/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /home/nell/rust/src/test/ui/borrowck/issue-57431-coerced-mut-reference.rs

This produced A LOT of debug output.

In my test, this line is the one that fails the borrow check, so I focused on the debug statements for when the borrow checker was processing line 11.

They are:

====================

Corresponds to

ref_term.mutate()

361821 [DEBUG rustc_mir::borrow_check] =========
361822 [DEBUG rustc_mir::borrow_check] place (*_2)
361823 [DEBUG rustc_span::source_map] byte pos BytePos(218) is on the line at byte pos BytePos(214)
361824 [DEBUG rustc_span::source_map] char pos CharPos(218) is on the line at char pos CharPos(214)
361825 [DEBUG rustc_span::source_map] byte is on line: 11
361826 [DEBUG rustc_span::source_map] byte pos BytePos(226) is on the line at byte pos BytePos(214)
361827 [DEBUG rustc_span::source_map] char pos CharPos(226) is on the line at char pos CharPos(214)
361828 [DEBUG rustc_span::source_map] byte is on line: 11
361829 [DEBUG rustc_mir::borrow_check] span /home/nell/rust/src/test/ui/borrowck/issue-57431-coerced-mut-reference.rs:11:5: 11:13
361830 [DEBUG rustc_mir::borrow_check] kind Reservation(MutableBorrow(Mut { allow_two_phase_borrow: true }))
361831 [DEBUG rustc_mir::borrow_check] is_local_mutation_allowedNo
361832 [DEBUG rustc_mir::borrow_check] flow_stateBorrowckAnalyses { borrows: [bw0], uninits: [mp0, mp3, mp4, mp5], ever_inits: [in0, in2] }
361833 [DEBUG rustc_mir::borrow_check] location bb0[12]
361834 [DEBUG rustc_mir::borrow_check] =========
361835 [DEBUG rustc_mir::borrow_check] =========

=======

Corresponds to

ref_term.mutate()

361903 [DEBUG rustc_mir::borrow_check] =========
361904 [DEBUG rustc_mir::borrow_check] place _5
361905 [DEBUG rustc_span::source_map] byte pos BytePos(218) is on the line at byte pos BytePos(214)
361906 [DEBUG rustc_span::source_map] char pos CharPos(218) is on the line at char pos CharPos(214)
361907 [DEBUG rustc_span::source_map] byte is on line: 11
361908 [DEBUG rustc_span::source_map] byte pos BytePos(226) is on the line at byte pos BytePos(214)
361909 [DEBUG rustc_span::source_map] char pos CharPos(226) is on the line at char pos CharPos(214)
361910 [DEBUG rustc_span::source_map] byte is on line: 11
361911 [DEBUG rustc_mir::borrow_check] span /home/nell/rust/src/test/ui/borrowck/issue-57431-coerced-mut-reference.rs:11:5: 11:13
361912 [DEBUG rustc_mir::borrow_check] kind Write(Mutate)
361913 [DEBUG rustc_mir::borrow_check] is_local_mutation_allowedNo
361914 [DEBUG rustc_mir::borrow_check] flow_stateBorrowckAnalyses { borrows: [bw0], uninits: [mp0, mp3, mp4, mp5], ever_inits: [in0, in2] }
361915 [DEBUG rustc_mir::borrow_check] location bb0[12]
361916 [DEBUG rustc_mir::borrow_check] =========
361917 [DEBUG rustc_mir::borrow_check] =========

========

Corresponds to

ref_term.mutate()

362059 [DEBUG rustc_mir::borrow_check] =========
362060 [DEBUG rustc_mir::borrow_check] place _5
362061 [DEBUG rustc_span::source_map] byte pos BytePos(218) is on the line at byte pos BytePos(214)
362062 [DEBUG rustc_span::source_map] char pos CharPos(218) is on the line at char pos CharPos(214)
362063 [DEBUG rustc_span::source_map] byte is on line: 11
362064 [DEBUG rustc_span::source_map] byte pos BytePos(235) is on the line at byte pos BytePos(214)
362065 [DEBUG rustc_span::source_map] char pos CharPos(235) is on the line at char pos CharPos(214)
362066 [DEBUG rustc_span::source_map] byte is on line: 11
362067 [DEBUG rustc_mir::borrow_check] span /home/nell/rust/src/test/ui/borrowck/issue-57431-coerced-mut-reference.rs:11:5: 11:22
362068 [DEBUG rustc_mir::borrow_check] kind Write(Move)
362069 [DEBUG rustc_mir::borrow_check] is_local_mutation_allowedYes
362070 [DEBUG rustc_mir::borrow_check] flow_stateBorrowckAnalyses { borrows: [bw0], uninits: [mp0, mp3, mp4], ever_inits: [in0, in2, in3] }
362071 [DEBUG rustc_mir::borrow_check] location bb0[13]
362072 [DEBUG rustc_mir::borrow_check] =========
362073 [DEBUG rustc_mir::borrow_check] =========
=================

Corresponds to

ref_term.mutate()

362236 [DEBUG rustc_mir::borrow_check] =========
362237 [DEBUG rustc_mir::borrow_check] place _5
362238 [DEBUG rustc_span::source_map] byte pos BytePos(234) is on the line at byte pos BytePos(214)
362239 [DEBUG rustc_span::source_map] char pos CharPos(234) is on the line at char pos CharPos(214)
362240 [DEBUG rustc_span::source_map] byte is on line: 11
362241 [DEBUG rustc_span::source_map] byte pos BytePos(235) is on the line at byte pos BytePos(214)
362242 [DEBUG rustc_span::source_map] char pos CharPos(235) is on the line at char pos CharPos(214)
362243 [DEBUG rustc_span::source_map] byte is on line: 11
362244 [DEBUG rustc_mir::borrow_check] span /home/nell/rust/src/test/ui/borrowck/issue-57431-coerced-mut-reference.rs:11:21: 11:22
362245 [DEBUG rustc_mir::borrow_check] kind Write(StorageDeadOrDrop)
362246 [DEBUG rustc_mir::borrow_check] is_local_mutation_allowedYes
362247 [DEBUG rustc_mir::borrow_check] flow_stateBorrowckAnalyses { borrows: [], uninits: [mp0, mp3, mp5], ever_inits: [in0, in2, in3, in4] }
362248 [DEBUG rustc_mir::borrow_check] location bb2[0]
362249 [DEBUG rustc_mir::borrow_check] =========
362250 [DEBUG rustc_mir::borrow_check] =========
========

Corresponds to ref_term.mutate()

362281 [DEBUG rustc_mir::borrow_check] =========
362282 [DEBUG rustc_mir::borrow_check] place _4
362283 [DEBUG rustc_span::source_map] byte pos BytePos(235) is on the line at byte pos BytePos(214)
362284 [DEBUG rustc_span::source_map] char pos CharPos(235) is on the line at char pos CharPos(214)
362285 [DEBUG rustc_span::source_map] byte is on line: 11
362286 [DEBUG rustc_span::source_map] byte pos BytePos(236) is on the line at byte pos BytePos(214)
362287 [DEBUG rustc_span::source_map] char pos CharPos(236) is on the line at char pos CharPos(214)
362288 [DEBUG rustc_span::source_map] byte is on line: 11
362289 [DEBUG rustc_mir::borrow_check] span /home/nell/rust/src/test/ui/borrowck/issue-57431-coerced-mut-reference.rs:11:22: 11:23
362290 [DEBUG rustc_mir::borrow_check] kind Write(StorageDeadOrDrop)
362291 [DEBUG rustc_mir::borrow_check] is_local_mutation_allowedYes
362292 [DEBUG rustc_mir::borrow_check] flow_stateBorrowckAnalyses { borrows: [], uninits: [mp0, mp3, mp5], ever_inits: [in0, in2, in4] }
362293 [DEBUG rustc_mir::borrow_check] location bb2[1]
362294 [DEBUG rustc_mir::borrow_check] =========
362295 [DEBUG rustc_mir::borrow_check] =========
=======

Based on this, I think that I need to modify one of these read or write kinds.

• Reservation(MutableBorrow(Mut { allow_two_phase_borrow: true }))
• Write(Mutate)
• Write(Move)
• Write(StorageDeadOrDrop)

And my instinct is to look further into Write(Mutate) first.

Edit: I think this section of the debug log is key:

361843 [DEBUG rustc_mir::borrow_check::diagnostics::mutability_errors] report_mutability_error(access_place=(*_2), span=/home/nell/rust/src/test/ui/borrowck/issue-57431-coerced-mut-reference.rs:11:5:        11:13, the_place_err=PlaceRef { local: _2, projection: [Deref] }, error_access=MutableBorrow, location=bb0[12],)
361844 [DEBUG rustc_mir::borrow_check::diagnostics::mutability_errors] report_mutability_error: access_place_desc=Some("*ref_term")
361845 [DEBUG rustc_mir::borrow_check::diagnostics] borrowed_content_source: mpi=mp2
361846 [DEBUG rustc_mir::borrow_check::diagnostics] borrowed_content_source: init=mp2@Statement(bb0[6]) (Deep)
361847 [DEBUG rustc_mir::borrow_check::diagnostics] borrowed_content_source: loc=bb0[6] is_terminator=false
361848 [DEBUG rustc_mir::borrow_check::diagnostics::mutability_errors] report_mutability_error: item_msg="`*ref_term`", reason=", as it is behind a `&` reference"
361849 [DEBUG rustc_span::source_map] byte pos BytePos(218) is on the line at byte pos BytePos(214)
361850 [DEBUG rustc_span::source_map] char pos CharPos(218) is on the line at char pos CharPos(214)
361851 [DEBUG rustc_span::source_map] byte is on line: 11
361852 [DEBUG rustc_span::source_map] byte pos BytePos(226) is on the line at byte pos BytePos(214)
361853 [DEBUG rustc_span::source_map] char pos CharPos(226) is on the line at char pos CharPos(214)
361854 [DEBUG rustc_span::source_map] byte is on line: 11
361855 [DEBUG rustc_mir::borrow_check::diagnostics] borrow_spans: use_span=/home/nell/rust/src/test/ui/borrowck/issue-57431-coerced-mut-reference.rs:11:5: 11:13 location=bb0[12]
361856 [DEBUG rustc_errors::diagnostic_builder] Created new diagnostic
361857 [DEBUG rustc_mir::borrow_check::diagnostics::mutability_errors] report_mutability_error: act="borrow as mutable", acted_on="borrowed as mutable"

[estebank]

@nellshamrell you can use more granular log filtering like RUSTC_LOG=rustc_mir::borrow_check=debug to avoid spamming stdout too much and you can use -Ztreat-err-as-bug=1 to make the first error being emitted cause a panic. This, along with RUST_BACKTRACE=1 should help you pinpoint where an error is being emitted (with some stacks sometimes elided or having to navigate the code backwards to find where the error is constructed, instead of where it is emitted).

[Aaron1011]

Removing the NLL-diagnostics label, since the output is the same both with and without #![feature(nll)]

[estebank]

The diagnostic is currently pointing you in the right direction, but we still don't correctly look at the as_ref() as the cause for the immutable borrow &X by the time we call mutate.

[estebank]

Current output:

error[E0596]: cannot borrow `term` as mutable, as it is not declared as mutable
 --> src/main.rs:9:23
  |
9 |         Some(term) => &mut term,
  |                       ^^^^^^^^^
  |                       |
  |                       cannot borrow as mutable
  |                       help: try removing `&mut` here

error[E0596]: cannot borrow `*term` as mutable, as it is behind a `&` reference
  --> src/main.rs:12:5
   |
8  |     let term = match &term {
   |         ---- consider changing this binding's type to be: `&mut X`
...
12 |     term.mutate();
   |     ^^^^^^^^^^^^^ `term` is a `&` reference, so the data it refers to cannot be borrowed as mutable

[estebank]

Current output, aesthetic changes only:

error[E0596]: cannot borrow `term` as mutable, as it is not declared as mutable
 --> src/main.rs:9:23
  |
9 |         Some(term) => &mut term,
  |                       ^^^^^^^^^
  |                       |
  |                       cannot borrow as mutable
  |                       help: try removing `&mut` here

error[E0596]: cannot borrow `*term` as mutable, as it is behind a `&` reference
  --> src/main.rs:12:5
   |
12 |     term.mutate();
   |     ^^^^ `term` is a `&` reference, so the data it refers to cannot be borrowed as mutable
   |
help: consider specifying this binding's type
   |
8  |     let term: &mut X = match &term {
   |             ++++++++