[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).