NLL diagnostics miss opportunities to suggest "change this to mut"

[pnkfelix]

The following tests rightly suggest under AST borrowck to consider adding mut to the binding site, but there is no such suggestion under NLL.

Tests:

• augmented-assignments.rs
• codemap_tests/huge_multispan_highlight.rs
• did_you_mean/issue-31424.rs
• did_you_mean/issue-39544.rs
• macros/span-covering-argument-1.rs
• span/borrowck-borrow-overloaded-auto-deref-mut.rs
• span/borrowck-borrow-overloaded-deref-mut.rs
• span/borrowck-object-mutability.rs

(This list of tests is drawn from an informal paper document that I have been using to keep notes for myself as I work on this...)

[nikomatsakis]

Here are some tips on where this might need to get fixed. For things like X = ..., where a variable is being directly assigned, I think we wind up in this path of the code:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 1375 to 1380 in 61f35e5

	fn check_if_reassignment_to_immutable_state(
	&mut self,
	context: Context,
	(place, span): (&Place<'tcx>, Span),
	flow_state: &Flows<'cx, 'gcx, 'tcx>,
	) {

Notably, this call to is_mutable — when it returns Err — also returns some information about why this path is not mutable:

rust/src/librustc_mir/borrow_check/mod.rs

Line 1855 in 61f35e5

) -> Result<RootPlace<'d, 'tcx>, &'d Place<'tcx>> {

(You can see that is_mutable returns a Place in the event of error here.)

I think that in the case we want, the "not mutable cause" will be a Place::Local; we could thread that Local through to the following function to use for making suggestions:

rust/src/librustc_mir/borrow_check/mod.rs

Line 1395 in 61f35e5

self.report_illegal_reassignment(context, (place, span), init.span);

(Given a mir::Local, we have to ask the Mir for the LocalDecl, which will tell us if this is a user_variable and also where it was declared etc for use when making the suggestion.)

[nikomatsakis]

If you'd like to help out with this, come join the NLL working group on Zulip.

[nikomatsakis]

It turns out that we have this code already:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 1708 to 1709 in 61f35e5

	err.note(&format!("the value which is causing this path not to be \
	mutable is...: `{}`", name));

I think we ought to be replacing that code with a better version. After all, that code doesn't look like it gives particularly useful errors today:

error[E0594]: cannot assign to immutable item `x`
  --> $DIR/issue-45983.rs:17:18
   |
LL |     give_any(|y| x = Some(y));
   |                  ^^^^^^^^^^^ cannot mutate
   |
   = note: the value which is causing this path not to be mutable is...: `x`

What we would need to do:

• Remove the if place != place_err; instead, test if place_err matches Place::Local(l)
• If so, find the local declaration for l (self.mir.local_decls[l])
  • if is_user_variable is true, continue
  • mutability should be immutable
  • the source_info can give you the span
• Construct a suggestion based on this
  • the API to add a suggestion is span_suggestion

[pnkfelix]

I think the same code that is doing this check (fn check_access_permissions) will also end up being responsible for properly checking #51032. So it might be good to combine some effort there, and/or think about both issues when looking at replacing this code with a better version.

(I do agree that the code here needs revision.)

[pnkfelix]

I believe this will be eventually addressed by PR #51275 once that is finalized.