NLL suggests removing mutability from a variable, doing so causes a compiler error

[shepmaster]

UPDATE: Mentoring instructions below.

---

This code compiles, but with a warning:

#![feature(nll)]

#[derive(Debug)]
struct A {}

fn init_a() -> A {
    A {}
}

#[derive(Debug)]
struct B<'a> {
    ed: &'a mut A,
}

fn init_b<'a>(ed: &'a mut A) -> B<'a> {
    B { ed }
}

#[derive(Debug)]
struct C<'a> {
    pd: &'a mut B<'a>,
}

fn init_c<'a>(pd: &'a mut B<'a>) -> C<'a> {
    C { pd }
}

#[derive(Debug)]
struct D<'a> {
    sd: &'a mut C<'a>,
}

fn init_d<'a>(sd: &'a mut C<'a>) -> D<'a> {
    D { sd }
}

fn main() {
    let mut a = init_a();
    let mut b = init_b(&mut a);
    let mut c = init_c(&mut b);

    let d = init_d(&mut c);

    println!("{:?}", d)
}

warning: variable does not need to be mutable
  --> src/main.rs:40:9
   |
40 |     let mut c = init_c(&mut b);
   |         ---^^
   |         |
   |         help: remove this `mut`
   |
   = note: #[warn(unused_mut)] on by default

Removing the mut causes a compiler error:

error[E0596]: cannot borrow immutable item `c` as mutable
  --> src/main.rs:42:20
   |
42 |     let d = init_d(&mut c);
   |                    ^^^^^^ cannot borrow as mutable

[shepmaster]

Without NLL, the original code has both an error and the warning:

error[E0597]: `c` does not live long enough
  --> src/main.rs:40:25
   |
40 |     let d = init_d(&mut c);
   |                         ^ borrowed value does not live long enough
...
43 | }
   | - `c` dropped here while still borrowed
   |
   = note: values in a scope are dropped in the opposite order they are created

warning: variable does not need to be mutable
  --> src/main.rs:38:9
   |
38 |     let mut c = init_c(&mut b);
   |         ---^^
   |         |
   |         help: remove this `mut`
   |
   = note: #[warn(unused_mut)] on by default

[Pulkit07]

I am going to try fix this.

[Pulkit07]

Status update: Understanding the two error pastes, I don't think this a NLL specific problem as the warning is produced in non-NLL case also. I am looking into code how the used_mut is computed in borrowck.

[Pulkit07]

This is the part of code where the error message is coming from

rust/src/librustc_borrowck/borrowck/unused.rs

Lines 73 to 90 in 8d3e93b

	for (_name, ids) in mutables {
	// If any id for this name was used mutably then consider them all
	// ok, so move on to the next
	if ids.iter().any(|&(_, ref id, _)| self.used_mut.contains(id)) {
	continue
	}
	let mut_span = tcx.sess.codemap().span_until_char(ids[0].2, ' ');
	// Ok, every name wasn't used mutably, so issue a warning that this
	// didn't need to be mutable.
	tcx.struct_span_lint_node(UNUSED_MUT,
	ids[0].0,
	ids[0].2,
	"variable does not need to be mutable")
	.span_suggestion_short(mut_span, "remove this `mut`", "".to_owned())
	.emit();
	}

I am bit confused by the comment here

rust/src/librustc_borrowck/borrowck/unused.rs

Lines 57 to 60 in 8d3e93b

	// Skip anything that looks like `&foo` or `&mut foo`, only look
	// for by-value bindings
	let hir_id = tcx.hir.node_to_hir_id(id);
	let bm = match self.bccx.tables.pat_binding_modes().get(hir_id) {

@nikomatsakis can you guide me a bit here. Also is there any way I can debug into rust source code and see what's happening there?

[nikomatsakis]

@Pulkit07 ok, a few things

First, you are correct that the code which issues mut warnings is currently tied to the "old" borrow check and hence independent of NLL. That said, we probably ought to port it to the "new" borrowck. This is perhaps a separate issue from this one.

Ah, well, I bet I see what is happening. I suspect that the old AST borrowck is reporting an error (which gets silenced, because we are in NLL mode). As a result, it exits early, which then in turn causes the set of "used mut declarations" to not be fully updated, and hence we get a lint warning.

(That is, the way this code works, the borrowck creates a set of which mut declarations are used, and then any mut declarations that are not in the set get linted. So if the borrowck stops early, the set will be incomplete.)

I feel like the best way to handle this may indeed be to update the MIR-based borrowck to compute this set, and to have the lint (when in NLL mode) use the set produced by the MIR-based borrowck.

[nikomatsakis]

The MIR-based code is probably somewhat simpler. I think we would want to modify this function:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 1378 to 1383 in 8d3e93b

	fn check_access_permissions(
	&self,
	(place, span): (&Place<'tcx>, Span),
	kind: ReadOrWrite,
	is_local_mutation_allowed: LocalMutationIsAllowed,
	) -> bool {

The idea would be that whenever we are doing a mutation, and the mutation is allowed because the local variable is declared mut, we add that local variable to a set. Note that local variables that the user declared are translated to a MIR LocalDecl:

rust/src/librustc/mir/mod.rs

Lines 441 to 446 in 8d3e93b

	/// A MIR local.
	///
	/// This can be a binding declared by the user, a temporary inserted by the compiler, a function
	/// argument, or the return place.
	#[derive(Clone, Debug, RustcEncodable, RustcDecodable)]
	pub struct LocalDecl<'tcx> {

and in particular this field will be true:

rust/src/librustc/mir/mod.rs

Line 453 in 8d3e93b

pub is_user_variable: bool,

We would then have to use the source_info somehow to decide to issue the lint:

rust/src/librustc/mir/mod.rs

Lines 483 to 484 in 8d3e93b

	/// Source info of the local.
	pub source_info: SourceInfo,

We might need to thread a bit more information here, I'm not sure.

[Pulkit07]

I am trying to implement a function for MirBorrowckCtxt which can return us a set of user defined mutable variables. The set can then be used to check whether all the variables are used mutably or not.

I am trying to implement the first step right now which is a function to return the set of user defined mutable variables. Here is the paste of the diff: https://gist.github.com/Pulkit07/e6254a3c0f37f82bfec48a512071d1f2

How can I convert a LocalDecl to a Place type? The Place enum is used everywhere but here I have LocalDecl type.

cc: @nikomatsakis

[nikomatsakis]

@Pulkit07

I am trying to implement a function for MirBorrowckCtxt which can return us a set of user defined mutable variables. The set can then be used to check whether all the variables are used mutably or not.

As a nit, I'm not sure it's worth actually building a set -- maybe return a impl Iterator<Item = Local>? A Local is a newtype'd index -- basically a struct wrapping an index -- that identifies a local variable. You can for example do something like:

fn user_declared_mutable_variables(&self) -> impl Iterator<Item = Local> + 'cx {
    // the 'cx here is the lifetime of the `mir`

    self.mir.local_decls.iter_enumerated()
      .filter_map(|(local, local_decl)| {
           if local_decl.is_user_declared && /* is mutable */ { Some(local) } else { None }
       })
}

If you want to construct the corresponding Place, you can do Place::Local(local):

rust/src/librustc/mir/mod.rs

Line 1236 in da569fa

Local(Local),

[nikomatsakis]

@Pulkit07 how goes? just checking in here =)

[nikomatsakis]

I'm clearing @spastorino because he's busy working on other things just now and I think this would be a good issue for someone to take up. The goal here is to rework the way that the mut lints work. Right now, the code which issues mut warnings is currently tied to the "old" borrow check and hence independent of NLL. The particular issue in this bug arises when the old borrow reports an error, but that error is silenced -- but more generally we should refactor how the mut lint code so that -- when feature(nll) is enabled -- it reads its results from the MIR borrowck.

I added some mentoring instructions here -- basically instructions on how to gather up the set of mut annotations that are actually used. Then we need some code that walks back down that list and reports warnings for any user-declared variables that are (a) declared mut but (b) not in the set.

cc @rust-lang/wg-compiler-nll

[KiChjang]

I will pick this issue up and work on it.