[NLL] E0389 error message uses the term immutable item inappropriately

[nikomatsakis]

The test compile-fail/E0389.rs gives the error message "cannot assign to immutable item fancy_ref.num". This is not particularly good because:

(a) "item" is probably not a very clear term for end-users but also
(b) even if it were, I would expect it to refer to "global things" like functions or statics, not fields of a local variable.

[nikomatsakis]

Mentoring instructions

The error messages in question seem to originate here:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 1404 to 1407 in da569fa

	let item_msg = match self.describe_place(place) {
	Some(name) => format!("immutable item `{}`", name),
	None => "immutable item".to_owned(),
	};

I think that to fix this, we will want to examine the Place more closely. A Place is a MIR data structure defined here

rust/src/librustc/mir/mod.rs

Lines 1231 to 1243 in da569fa

	/// A path to a value; something that can be evaluated without
	/// changing or disturbing program state.
	#[derive(Clone, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
	pub enum Place<'tcx> {
	/// local variable
	Local(Local),
	/// static or static mut variable
	Static(Box<Static<'tcx>>),
	/// projection out of a place (access a field, deref a pointer, etc)
	Projection(Box<PlaceProjection<'tcx>>),
	}

I think we could either print "immutable path" instead of "immutable item", or else (maybe better)
examine the variant and adjust our message accordingly:

If it's a local, print "immutable local variable".

For a static, "immutable static" or "immutable static variable".

For anything else, maybe just "path"?

In general, I think I would also prefer the message to be like

error: cannot assign to `x`
  >
  > let x = 22;
  >     ^ `x` not declared `mut`

It might take a bit of tweaking to get this right, but by walking up the Place we can basically figure out why x is not mutable etc. I think the AST borrow-checker already does this to some extent?

[gaurikholkar-zz]

Investigating this

[gaurikholkar-zz]

Looking at possible error messages for the similar cases
Message 1

let fancy_ref = &(&mut fancy);
                ^^^^^^^^^^^^^^^^^ consider changing this to `mut`

Message2

error[E0389]: cannot assign to `fancy_ref.num`
  --> src/test/compile-fail/E0389.rs:18:5
   |
18 |     let fancy_ref = &(&mut fancy);
   |                     ^not declared `mut`

cc @estebank @nikomatsakis

[estebank]

I think I would prefer:

error[E0389]: cannot assign to field `num` in immutable reference `fancy_ref`
 --> src/main.rs:8:5
  |
7 |    let fancy_ref = &(&mut fancy);
  |                    - help: consider changing this to be a mutable reference: `&mut `
8 |     fancy_ref.num = 6; //~ ERROR E0389
  |     ^^^^^^^^^^^^^^^^^ cannot assign to field of immutable reference

The "immutable/mutable reference" is jargony, but consistent with other errors. We need to make it clear that fancy_ref is immutable, not fancy_ref.num.

[nikomatsakis]

Overall, I am feeling torn here. I like @estebank's phrasing, but I have two concerns:

First, I personally don't like term 'immutable reference'. I know it is widely used in our error messages -- and even in the Rust book -- but I think it is misleading. In particular, an &T reference does not imply that T cannot be mutated, it just means you need to use special types to do it. For example, &Cell<u32> permits mutation of the u32 value (similarly &Mutex<u32>). Our mutation story is definitely something that confuses people, and I think terminology like "immutable reference" doesn't help. However, this is to some extent a larger question.
- That said, it is also somewhat jargon-y, and it'd be nice if we could avoid it.
- My personal term is shared reference, but I realize that is not in widespread use, and I suspect it too should be avoided. =)
- I wonder if we can just say &-reference or something?

Ignoring that, I think that @estebank's message is nice, though I wonder: I feel like there should be a principle of only mentioning the things that actually matter in the error, and in fact the precise field (num, here) doesn't matter. That is, it's not that you can't assign to num -- you can't assign to any field, or any other content.

Putting it all together, maybe something like this?

error[E0389]: cannot assign through `&`-reference `fancy_ref`
 --> src/main.rs:8:5
  |
7 |    let fancy_ref = &(&mut fancy);
  |                    - help: consider changing this to be a mutable reference: `&mut `
8 |     fancy_ref.num = 6; //~ ERROR E0389
  |     ^^^^^^^^^^^^^^^^^ cannot assign through `&`-reference

But I'm not sure if it's really an improvement on what @estebank wrote. In some ways, e.g. the use of the word "through" instead of a field name, it may be less clear. Curious what others think.

(I'm also not sure about &-reference vs "immutable reference". I'm willing to go with the latter for now since it is used broadly, though I do want to revisit it at some point.)

[gaurikholkar-zz]

@nikomatsakis I feel the term shared reference might be confusing. &-reference sounds better! I do agree that cannot assign through &-reference fancy_ref is short and clear and the mention of the field num might not be much needed.

[nikomatsakis]

@gaurikholkar ok, if @estebank is happy with that, let's try for that for now. This is going to require a bit of analysis of the Place which is being assigned.

[estebank]

I disagree slightly that num is irrelevant because it's the users intention to assign to it, but given that the span is already point to it might not need to be explicitly mentioned. Otherwise I'm fine with the proposed phrasing.

[nikomatsakis]

I disagree slightly that num is irrelevant because it's the users intention to assign to it

Yeah, irrelevant is too strong. But it's not that there is anything special about the num field. I suppose something like:

error[E0389]: cannot assign through `&`-reference `fancy_ref`
 --> src/main.rs:8:5
  |
7 |    let fancy_ref = &(&mut fancy);
  |                    - help: consider changing this to be a mutable reference: `&mut `
8 |     fancy_ref.num = 6; //~ ERROR E0389
  |     ^^^^^^^^^^^^^^^^^ `num` field is located through an `&`-reference, cannot be assigned

but that seems a bit verbose (and likely to run off the side of the screen).

[estebank]

<bikeshed>What about?

error[E0389]: cannot assign to `num` through `&`-reference `fancy_ref`
 --> src/main.rs:8:5
  |
7 |    let fancy_ref = &(&mut fancy);
  |                    - help: consider changing this to be a mutable reference: `&mut `
8 |     fancy_ref.num = 6; //~ ERROR E0389
  |     ^^^^^^^^^^^^^^^^^ cannot assign to field of `&`-reference

[nikomatsakis]

@estebank that seems really good. We should consider all the other cases that will arise I suppose. We can probably fallback to my simpler variant when it is not a field being assigned?

[estebank]

We can probably fallback to my simpler variant when it is not a field being assigned?

That would be my preference, yes.

[nikomatsakis]

@gaurikholkar ok so let me leave you a few more tips. I mentioned already that we will want to examine the Place that is being assigned, and how we can special case local variables and so forth. A path like foo.bar will typically be a projection:

rust/src/librustc/mir/mod.rs

Lines 1241 to 1242 in da569fa

	/// projection out of a place (access a field, deref a pointer, etc)
	Projection(Box<PlaceProjection<'tcx>>),

A projection consists of an "element" and a base Place:

rust/src/librustc/mir/mod.rs

Lines 1258 to 1266 in da569fa

	/// The `Projection` data structure defines things of the form `B.x`
	/// or `*B` or `B[index]`. Note that it is parameterized because it is
	/// shared between `Constant` and `Place`. See the aliases
	/// `PlaceProjection` etc below.
	#[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
	pub struct Projection<'tcx, B, V, T> {
	pub base: B,
	pub elem: ProjectionElem<'tcx, V, T>,
	}

Elements consist of things like fields or dereferences:

rust/src/librustc/mir/mod.rs

Lines 1268 to 1269 in da569fa

	#[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
	pub enum ProjectionElem<'tcx, V, T> {

In this case, where foo is a reference, foo.bar is short-hand for (*foo).bar. So the place would be like:

• Place::Projection
  • elem: ProjectionElem::Field for the field bar
  • base: (representing the path *foo)
    • Place::Projection
      • elem: ProjectionElem::Deref for the field *
      • base: (representing the path foo)
        • Place::Local

[nikomatsakis]

I think, for error messages purposes, we want to consider things like this:

• If the outermost layer of the Place is a Projection of a field, then we can extract the field name and use it in the error.
• If somewhere in the place there is a deref of a borrowed reference (&T type), this is the "through an &-reference" case.
  • However, there is already code that looks for that, it's called the function is_mutable

rust/src/librustc_mir/borrow_check/mod.rs

Lines 1471 to 1476 in da569fa

	/// Can this value be written or borrowed mutably
	fn is_mutable<'d>(
	&self,
	place: &'d Place<'tcx>,
	is_local_mutation_allowed: LocalMutationIsAllowed,
	) -> Result<(), &'d Place<'tcx>> {

and it seems like is_mutable already returns the path that indicates why the field is not mutable. So I guess most of what we need is there. You'll have to read into a bit to figure out just what to change though, or shoot me a few questions later on.

[gaurikholkar-zz]

Thanks @nikomatsakis

[nikomatsakis]

@gaurikholkar how goes ? Just checking in here =)

[gaurikholkar-zz]

This is the output for now.

error[E0594]: cannot assign to `&`-reference *fancy_ref
  --> src/test/compile-fail/E0389.rs:18:5
   |
18 |     fancy_ref.num = 6; //~ ERROR E0389
   |     ^^^^^^^^^^^^^^^^^ cannot assign through `&`-reference

error: aborting due to previous error

cc @nikomatsakis @estebank

[nikomatsakis]

@gaurikholkar making progress! One nit: should be "cannot assign through &-reference", and also *fancy_ref ought to be embedded in ticks. Also, we probably want to strip the *, I imagine. That leaves us with:

error[E0594]: cannot assign through `&`-reference `fancy_ref`

[gaurikholkar-zz]

Current status. The error message labels are inverted, will fix that and open a PR.

error[E0594]: cannot assign through `&`-reference fancy_ref
  --> src/test/compile-fail/E0389.rs:18:5
   |
18 |     fancy_ref.num = 6; //~ ERROR E0389
   |     ^^^^^^^^^^^^^^^^^ cannot assign to field of `&`-reference
   |
help: consider changing this to be a mutable reference: `&mut `
  --> src/test/compile-fail/E0389.rs:17:21
   |
17 |     let fancy_ref = &(&mut fancy);
   |                     ^^^^^^^^^^^^^

error: aborting due to previous error

[nikomatsakis]

@gaurikholkar this is awesome =)

A few notes:

• Obviously, I think we'd prefer to have the suggestion inline. I am not sure why the current one is not, I guess it makes a difference precisely which methods of DiagnosticBuilder you use -- maybe @estebank knows...
• Second, it seems like the span we are getting from MIR underlines the entire &(&mut fancy) expression, but we only want the & character. That is annoying. I think that -- to fix it -- we often do some kind of grungy "munging" of the span. Again, @estebank might know if there is a suitable helper. In this case, it's probably relatively easy, since we only want the first character, so maybe we can just do that.

[gaurikholkar-zz]

error[E0594]: cannot assign through `&`-reference fancy_ref
  --> src/test/compile-fail/E0389.rs:18:5
   |
17 |     let fancy_ref = &(&mut fancy);
   |                     ------------- help: consider changing this to be a mutable reference: `&mut`
18 |     fancy_ref.num = 6; //~ ERROR E0389
   |     ^^^^^^^^^^^^^^^^^ cannot assign to field of `&`-reference

@nikomatsakis the suggestion is inline now

[estebank]

I am not sure why the current one is not

I'm guessing that it wasn't a span_suggestion, but rather a span_help. Regardless, span suggestions do not appear inline if the text is too long (IIRC, over 10 words) or there're multiple suggestions in the same diagnostic.

In this case, it's probably relatively easy, since we only want the first character, so maybe we can just do that.

There's already a method to get a span pointing at the end of a given span (tcx.sess.codemap().end_point(span)), but the equivalent for the beginning was removed due to lack of use, it could be brought back in.

@gaurikholkar be careful with suggestions, everything underlined will be replaced with the content of the suggestion string, in this case the resulting code would be let fancy_ref = &mut;, which is not what we want. If you use a span pointing only at &, you don't need to change anything else. Really like the change!

[gaurikholkar-zz]

I'm guessing that it wasn't a span_suggestion, but rather a span_help

Yeah I was using span_help instead before

If you use a span pointing only at &, you don't need to change anything else. Really like the change!

@estebank are you saying we should alter the suggestion/error message or get back the old method?

[estebank]

@gaurikholkar I'm saying that the only change needed to fix the incorrect suggestion result is changing the span to only point to the &.

[nikomatsakis]

Clearing assignment since #48914 was closed for inactivity. It'd be good to dust off the PR and get those changes landed though! We were getting close.

[gaurikholkar-zz]

Reopening the PR @nikomatsakis @estebank