Stream: t-compiler/wg-nll

Topic: #57431 suggests `&mut mut x`


nikomatsakis (Jan 10 2019 at 19:30, on Zulip):

This looks like an "easy fix" potentially -- I was chatting with @Santiago Pastorino about maybe picking it up

Santiago Pastorino (Jan 10 2019 at 19:32, on Zulip):

:+1:

nikomatsakis (Jan 10 2019 at 19:34, on Zulip):

I haven't really looked at all here, I admit

nikomatsakis (Jan 10 2019 at 19:34, on Zulip):

I guess first thing would be to find where that suggestion is being generated

Santiago Pastorino (Jan 10 2019 at 19:35, on Zulip):

:+1:

Santiago Pastorino (Jan 10 2019 at 19:35, on Zulip):

going to check this tomorrow

Santiago Pastorino (Jan 11 2019 at 18:37, on Zulip):
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();
}
Santiago Pastorino (Jan 11 2019 at 18:38, on Zulip):

@nikomatsakis what are the new semantics exactly of matching on something that's &Option and having the arms being Options?

Santiago Pastorino (Jan 11 2019 at 18:38, on Zulip):

does something other than that happens?

Santiago Pastorino (Jan 11 2019 at 18:38, on Zulip):

I mean, inside the arms after the => do I have something special?

Santiago Pastorino (Jan 11 2019 at 18:39, on Zulip):

is the behavior written somewhere? :)

Santiago Pastorino (Jan 11 2019 at 18:39, on Zulip):

I guess there's an RFC, going to look for it

Santiago Pastorino (Jan 11 2019 at 19:16, on Zulip):

@nikomatsakishow much of https://github.com/rust-lang/rfcs/blob/master/text/2005-match-ergonomics.md is implemented on 2018?

Santiago Pastorino (Jan 11 2019 at 20:08, on Zulip):

@nikomatsakis anyway, besides from the question I've asked ...

Santiago Pastorino (Jan 11 2019 at 20:09, on Zulip):

I think we should just remove help: consider changing this to be a mutable reference: &mut mut term entirely in this case

Santiago Pastorino (Jan 11 2019 at 20:10, on Zulip):

I guess the code is checking the local but is not considering that there's &mut before the local already

Santiago Pastorino (Jan 11 2019 at 20:10, on Zulip):

I wonder how in the code can I check if there's a &mut before the local

nikomatsakis (Jan 11 2019 at 20:16, on Zulip):

how much of https://github.com/rust-lang/rfcs/blob/master/text/2005-match-ergonomics.md is implemented on 2018?

all of it

nikomatsakis (Jan 11 2019 at 20:16, on Zulip):

also in 2015 I think

nikomatsakis (Jan 11 2019 at 20:16, on Zulip):

so basically there is an implicit ref added to the bindings

Santiago Pastorino (Jan 11 2019 at 20:19, on Zulip):

oh ok, I read that differently given that the issue associated https://github.com/rust-lang/rust/issues/42640 is open

Santiago Pastorino (Jan 11 2019 at 20:20, on Zulip):

I think we should just remove help: consider changing this to be a mutable reference: &mut mut term entirely in this case
@nikomatsakis we need to do this, right?

Santiago Pastorino (Jan 11 2019 at 20:21, on Zulip):

also ...

Santiago Pastorino (Jan 11 2019 at 20:21, on Zulip):

I guess the code is checking the local but is not considering that there's &mut before the local already
I wonder how in the code can I check if there's a &mut before the local

:point_up:

nikomatsakis (Jan 11 2019 at 21:26, on Zulip):

oh ok, I read that differently given that the issue associated https://github.com/rust-lang/rust/issues/42640 is open

that...should probably be closed

nikomatsakis (Jan 11 2019 at 21:27, on Zulip):

well, @Santiago Pastorino, looking more closely, I think the "span" we are using here is a bit odd

Santiago Pastorino (Jan 11 2019 at 21:28, on Zulip):

I think I've figured this out

Santiago Pastorino (Jan 11 2019 at 21:28, on Zulip):

anyway, what do you refer by "span" is a bit odd?

nikomatsakis (Jan 11 2019 at 21:29, on Zulip):

I guess what we really want to be saying is

error[E0596]: cannot borrow `*term` as mutable, as it is behind a `&` reference
  --> src/main.rs:12:5
   |
8  |        let term = match &term {
   |                         ----- help: consider changing this to be a mutable reference: `&mut term`
...
12 |     term.mutate();
   |     ^^^^ `term` is a `&` reference, so the data it refers to cannot be borrowed as mutable
nikomatsakis (Jan 11 2019 at 21:29, on Zulip):

note that this is a totally different spot we are underlining

nikomatsakis (Jan 11 2019 at 21:29, on Zulip):

however I don't know that we have the logic in place to track that sort of thing back

nikomatsakis (Jan 11 2019 at 21:30, on Zulip):

in any case the code seems to be getting confused, and probably that "help" should just not be printing

nikomatsakis (Jan 11 2019 at 21:30, on Zulip):

as we are not smart enough to track it back to the &term we really want

nikomatsakis (Jan 11 2019 at 21:30, on Zulip):

I am guessing that somehow the problem has to do with the MIR desugaring

nikomatsakis (Jan 11 2019 at 21:31, on Zulip):

i.e., the error message maybe sort of makes sense if you look at the desugaring somehow

nikomatsakis (Jan 11 2019 at 21:31, on Zulip):

I think I've figured this out

oh?

nikomatsakis (Jan 11 2019 at 21:31, on Zulip):

however I don't know that we have the logic in place to track that sort of thing back

I don't want to confuse you: I don't expect us to correctly underline line 8. That would be the ideal thing, but I don't know that the MIR contains enough info for us to be that smart right now.

Santiago Pastorino (Jan 11 2019 at 21:45, on Zulip):

however I don't know that we have the logic in place to track that sort of thing back

I don't want to confuse you: I don't expect us to correctly underline line 8. That would be the ideal thing, but I don't know that the MIR contains enough info for us to be that smart right now.

don't worry, I guess we can go step by step, first I'd just remove that note

Santiago Pastorino (Jan 11 2019 at 21:47, on Zulip):

I was wondering if I could just check if the thing is a reference and you are mutably borrowing and in that case avoid doing it

Santiago Pastorino (Jan 11 2019 at 21:48, on Zulip):

but yeah, I should take a look at the mir to see what's going on

Santiago Pastorino (Jan 11 2019 at 21:48, on Zulip):

by mir desugaring you meant, to see the mir dump and see the statements generated?

Santiago Pastorino (Jan 11 2019 at 21:52, on Zulip):

@nikomatsakis specifically I was checking if I could add some logic here https://github.com/rust-lang/rust/blob/b43986184b8f4e0d633e8ae1704f0e19aec30cb2/src/librustc_mir/borrow_check/mutability_errors.rs#L392 and figure that you're mutably borrowing a reference

nikomatsakis (Jan 11 2019 at 22:04, on Zulip):

@Santiago Pastorino ok -- is that the path the code goes down, in this example?

Santiago Pastorino (Jan 11 2019 at 22:35, on Zulip):

unsure, I need to properly check

Santiago Pastorino (Jan 11 2019 at 22:49, on Zulip):

but according to the error message it should go through that match

Santiago Pastorino (Jan 11 2019 at 22:50, on Zulip):

don’t remember now if it definitely goes through that arm or may be a different one

Santiago Pastorino (Jan 12 2019 at 12:42, on Zulip):

@nikomatsakis it executes this branch https://github.com/rust-lang/rust/blob/b43986184b8f4e0d633e8ae1704f0e19aec30cb2/src/librustc_mir/borrow_check/mutability_errors.rs#L380 and in particular binding_mode is ty::BindingMode::BindByValue(hir::Mutability::MutImmutable)

Santiago Pastorino (Jan 12 2019 at 12:42, on Zulip):

and I figured that I have no idea what that is

Santiago Pastorino (Jan 12 2019 at 12:42, on Zulip):

unsure why ByValue and why MutImmutable, I'd have said that is ByReference and MutMutable, hehehe

Santiago Pastorino (Jan 12 2019 at 12:43, on Zulip):

it's probably talking about a different thing that the one I'm assuming

Santiago Pastorino (Jan 12 2019 at 12:43, on Zulip):

I'm assuming &mut term

Matthew Jasper (Jan 12 2019 at 12:48, on Zulip):

The binding mode is referring to the term in Some(term)

Santiago Pastorino (Jan 12 2019 at 13:10, on Zulip):

@Matthew Jasper yeah, that was the other possibility but still that should be of type &X so I'd assume we get a ty::BindingMode::BindByReference(hir::Mutability::MutImmutable instead of BindByValue

Santiago Pastorino (Jan 12 2019 at 13:10, on Zulip):

and I also wonder how to get the &mut info

Matthew Jasper (Jan 12 2019 at 14:25, on Zulip):

Actually, I'm wrong, it's the term in the second let term = . (having the same name for all of the variables isn't great...)

Santiago Pastorino (Jan 12 2019 at 20:32, on Zulip):

agreed that confuses a lot :P

Santiago Pastorino (Jan 12 2019 at 20:32, on Zulip):

but, I think it's ...

Santiago Pastorino (Jan 12 2019 at 20:32, on Zulip):
Some(term) => &mut term,
   |          --------- help: consider changing this to be a mutable reference: &mut mut term
Santiago Pastorino (Jan 12 2019 at 20:33, on Zulip):

it's the term that's comes from that arm

Santiago Pastorino (Jan 12 2019 at 20:33, on Zulip):

Some(term)

Santiago Pastorino (Jan 12 2019 at 20:33, on Zulip):

and given match ergonomics, I believe that's a & + the supposed type

Santiago Pastorino (Jan 12 2019 at 20:34, on Zulip):

which is X

Santiago Pastorino (Jan 12 2019 at 20:34, on Zulip):

that's why I believe that should be a &X

Santiago Pastorino (Jan 12 2019 at 20:34, on Zulip):

@Matthew Jasper ^^^

Matthew Jasper (Jan 12 2019 at 20:53, on Zulip):

It might be better to look at a case without a match. This results in the same diagnostics issue:

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

fn foo(c: bool) {
    let mut term = X;
    let ref_term = if c {
        &mut term
    } else {
        &X
    };
    ref_term.mutate();
}
Santiago Pastorino (Jan 14 2019 at 18:42, on Zulip):

@Matthew Jasper back with this

Santiago Pastorino (Jan 14 2019 at 18:42, on Zulip):

yes, your example is simpler

Santiago Pastorino (Jan 14 2019 at 18:42, on Zulip):

checking it now

Santiago Pastorino (Jan 14 2019 at 18:42, on Zulip):

I think what I've said is still valid though

Santiago Pastorino (Jan 14 2019 at 18:46, on Zulip):

so ...

Santiago Pastorino (Jan 14 2019 at 18:46, on Zulip):
struct X;
impl X {
    fn mutate(&mut self) {}
}

fn foo(c: bool) {
    let mut term = X;
    let ref_term = if c {
        &mut term
    } else {
        &X
    };
    ref_term.mutate();
}
Santiago Pastorino (Jan 14 2019 at 18:46, on Zulip):

the error is ...

Santiago Pastorino (Jan 14 2019 at 18:46, on Zulip):
error[E0596]: cannot borrow `*ref_term` as mutable, as it is behind a `&` reference
  --> src/test/ui/nll/issue-57431.rs:15:5
   |
11 |         &mut term
   |         --------- help: consider changing this to be a mutable reference: `&mut mut term`
...
15 |     ref_term.mutate();
   |     ^^^^^^^^ `ref_term` is a `&` reference, so the data it refers to cannot be borrowed as mutable

error: aborting due to previous error

For more information about this error, try `rustc --explain E0596`.
Santiago Pastorino (Jan 14 2019 at 18:48, on Zulip):

so here https://github.com/rust-lang/rust/blob/b43986184b8f4e0d633e8ae1704f0e19aec30cb2/src/librustc_mir/borrow_check/mutability_errors.rs#L380 local_decl.is_user_variable.as_ref() is returning a thing which it'sbinding_mode is ty::BindingMode::BindByValue(hir::Mutability::MutImmutable)

Santiago Pastorino (Jan 14 2019 at 18:50, on Zulip):

so I guess that's just talking about term and not &mut term

Santiago Pastorino (Jan 14 2019 at 18:50, on Zulip):

I wonder if I can get somehow that &mut applied to term?

Santiago Pastorino (Jan 14 2019 at 19:54, on Zulip):

@nikomatsakis any thought on this?

Santiago Pastorino (Jan 14 2019 at 19:54, on Zulip):
DEBUG 2019-01-14T19:43:40Z: rustc_mir::borrow_check::mutability_errors: report_mutability_error(access_place=(*_2), span=src/test/ui/nll/issue-57431.rs:15:5: 15:13, the_place_err=(*_2), error_access=MutableBorrow, location=bb4[2],)
DEBUG 2019-01-14T19:43:40Z: rustc_mir::borrow_check::mutability_errors: report_mutability_error: access_place_desc=Some("*ref_term")
DEBUG 2019-01-14T19:43:40Z: rustc_mir::borrow_check::mutability_errors: report_mutability_error: item_msg="`*ref_term`", reason=", as it is behind a `&` reference"
DEBUG 2019-01-14T19:43:40Z: rustc_mir::borrow_check::error_reporting: borrow_spans: use_span=src/test/ui/nll/issue-57431.rs:15:5: 15:13 location=bb4[2]
DEBUG 2019-01-14T19:43:40Z: rustc_mir::borrow_check::mutability_errors: report_mutability_error: act="borrow as mutable", acted_on="borrowed as mutable"
DEBUG 2019-01-14T19:43:40Z: rustc_mir::borrow_check::mutability_errors: report_mutability_error:local=_2, local_decl=LocalDecl { mutability: Not, is_user_variable: Some(Set(Var(VarBindingForm { binding_mode: BindByValue(MutImmutable), opt_ty_info: None, opt_match_place: Some((None, src/test/ui/nll/issue-57431.rs:10:20: 14:6)), pat_span: src/test/ui/nll/issue-57431.rs:10:9: 10:17 }))), internal: false, is_block_tail: None, ty: &'_#6r X, user_ty: UserTypeProjections { contents: [] }, name: Some(ref_term), source_info: SourceInfo { span: src/test/ui/nll/issue-57431.rs:10:9: 10:17, scope: scope[4] }, visibility_scope: scope[3] },)
Santiago Pastorino (Jan 14 2019 at 19:54, on Zulip):

this is the logs I'm getting

Santiago Pastorino (Jan 14 2019 at 19:55, on Zulip):

unsure for instance why access_place is Some("*ref_term")

Santiago Pastorino (Jan 14 2019 at 19:58, on Zulip):

I see that the local this is referring to is _2

Santiago Pastorino (Jan 14 2019 at 19:58, on Zulip):

from mir output that is ...

Santiago Pastorino (Jan 14 2019 at 19:59, on Zulip):
        StorageLive(_3);                 // bb2[0]: scope 1 at src/test/ui/nll/issue-57431.rs:11:9: 11:18
        _3 = &'_#2r mut _1;              // bb2[1]: scope 1 at src/test/ui/nll/issue-57431.rs:11:9: 11:18
        _2 = &'_#3r (*_3);               // bb2[2]: scope 1 at src/test/ui/nll/issue-57431.rs:11:9: 11:18
        StorageDead(_3);                 // bb2[3]: scope 1 at src/test/ui/nll/issue-57431.rs:12:5: 12:6
Santiago Pastorino (Jan 14 2019 at 19:59, on Zulip):

line 11 is ...

Santiago Pastorino (Jan 14 2019 at 19:59, on Zulip):

&mut term

Santiago Pastorino (Jan 14 2019 at 20:00, on Zulip):

from there I guess _3 is &mut term and _1 is term

Santiago Pastorino (Jan 14 2019 at 20:01, on Zulip):

_2 is &*(&mut term)?

Santiago Pastorino (Jan 14 2019 at 20:01, on Zulip):

if so ... why?

Matthew Jasper (Jan 14 2019 at 20:02, on Zulip):

The else branch has type &X, so &mut term is being coerced to match

Santiago Pastorino (Jan 14 2019 at 20:03, on Zulip):

you're right :)

Santiago Pastorino (Jan 14 2019 at 20:05, on Zulip):

so then why is _2 which is &*(&mut term) a ty::BindingMode::BindByValue and hir::Mutability::MutImmutable thing?

Matthew Jasper (Jan 14 2019 at 20:11, on Zulip):

It's declared let ref_temp, which is immutable since there's no mut and by value because there's no ref

Santiago Pastorino (Jan 15 2019 at 03:14, on Zulip):

ahh so my understanding about what's BindByValue and what's MutImmutable is completely different from the one it has

Santiago Pastorino (Jan 15 2019 at 03:14, on Zulip):

you mean that that's solely based on the presence of a mut in the binding and a ref?

nikomatsakis (Jan 15 2019 at 21:35, on Zulip):

OK, that second example is interesting

nikomatsakis (Jan 15 2019 at 21:35, on Zulip):

Hmm

nikomatsakis (Jan 15 2019 at 21:36, on Zulip):

so the problem here is that the & we see in the source is one that was autogenerated from a coercion

nikomatsakis (Jan 15 2019 at 21:36, on Zulip):

it feels like something we could track

nikomatsakis (Jan 15 2019 at 21:36, on Zulip):

in which case we could say "if the borrow in question comes from a coercion, don't print this suggestion" or something

Santiago Pastorino (Jan 15 2019 at 22:01, on Zulip):

ahh you're right

Santiago Pastorino (Jan 15 2019 at 22:08, on Zulip):

any pointer or idea on where that could be tracked ... :)

Santiago Pastorino (Jan 15 2019 at 22:09, on Zulip):

going to check out tomorrow

Santiago Pastorino (Jan 17 2019 at 14:22, on Zulip):

so the problem here is that the & we see in the source is one that was autogenerated from a coercion

ok, so this makes sense but I wonder if my approach was correct

Santiago Pastorino (Jan 17 2019 at 14:22, on Zulip):

so ...

Santiago Pastorino (Jan 17 2019 at 14:22, on Zulip):

I could track those autogenerated & from a coercion, need to investigate exactly where

Santiago Pastorino (Jan 17 2019 at 14:23, on Zulip):

the question is ...

Santiago Pastorino (Jan 17 2019 at 14:23, on Zulip):

https://github.com/rust-lang/rust/blob/b43986184b8f4e0d633e8ae1704f0e19aec30cb2/src/librustc_mir/borrow_check/mutability_errors.rs#L380

Santiago Pastorino (Jan 17 2019 at 14:24, on Zulip):

there I was checking for a BindByValue binding so no occurrence of the borrow

Santiago Pastorino (Jan 17 2019 at 14:24, on Zulip):

as @Matthew Jasper said that's related with ref or mut in a binding so that's not what I should look for

Santiago Pastorino (Jan 17 2019 at 14:25, on Zulip):

@nikomatsakis the question is how can I figure out there if the binding is related to a borrow or a mutable borrow?

Santiago Pastorino (Jan 17 2019 at 19:17, on Zulip):

@nikomatsakis back to this, I figured that just by adding some kind of information that says if the ref is autogenerated or not is enough

Santiago Pastorino (Jan 17 2019 at 19:17, on Zulip):

my previous question was wrong :)

Santiago Pastorino (Jan 17 2019 at 19:18, on Zulip):

I wonder where could I track this ... any idea on where to look?

Santiago Pastorino (Jan 17 2019 at 19:18, on Zulip):

I guess we want Projection to have an autogenerated boolean?

nikomatsakis (Jan 17 2019 at 22:10, on Zulip):

this is not about the projection

nikomatsakis (Jan 17 2019 at 22:10, on Zulip):

at least I don't think so

nikomatsakis (Jan 17 2019 at 22:10, on Zulip):

it's about the actual borrow, I think?

nikomatsakis (Jan 17 2019 at 22:13, on Zulip):

that is, there is some statement like X = &Y.Z, and I think it is this statement that might carry a flag about it being synthetic

nikomatsakis (Jan 17 2019 at 22:14, on Zulip):

I think if we wanted to trace this, we would extend the MIR and HAIR with such a flag -- maybe in the source-info somewhere? -- and then we would modify the HAIR so that when we are generating "auto-generated" refs, we set this flag, and same for HAIR->MIR lowering

nikomatsakis (Jan 17 2019 at 22:24, on Zulip):

as an example, here is a point in creating the HAIR where we know that the borrow is synthetic:

https://github.com/rust-lang/rust/blob/daa53a52a2667533d5fe59bfcc5b8614b79c3d31/src/librustc_mir/hair/cx/expr.rs#L134-L139

Santiago Pastorino (Jan 21 2019 at 16:54, on Zulip):

that is, there is some statement like X = &Y.Z, and I think it is this statement that might carry a flag about it being synthetic

@nikomatsakis what's the meaning of being synthetic to start with? :)

Santiago Pastorino (Jan 21 2019 at 16:55, on Zulip):

I guess I understand given the context but just in case clarification won't hurt

Santiago Pastorino (Jan 21 2019 at 16:55, on Zulip):

as an example, here is a point in creating the HAIR where we know that the borrow is synthetic:

https://github.com/rust-lang/rust/blob/daa53a52a2667533d5fe59bfcc5b8614b79c3d31/src/librustc_mir/hair/cx/expr.rs#L134-L139

I guess we may add a flag to ExprKind::Borrow to handle the "auto-generated" fact?

nikomatsakis (Jan 22 2019 at 20:26, on Zulip):

@Santiago Pastorino

what's the meaning of being synthetic to start with? :)

by synthetic, I meant "does not corespond to a & that the user typed"

I guess we may add a flag to ExprKind::Borrow to handle the "auto-generated" fact?

yeah basically

Last update: Nov 21 2019 at 23:25UTC