MIR-borrowck: immutable unique closure upvars can be mutated

[arielb1]

MIR borrowck allows for immutable unique closure upvars to be mutated, e.g.

fn main() {
    let x = 0;

    (move || {
        x = 1; // mutating immutable local, ok on MIR, bad on AST
    })()
}

[nikomatsakis]

There are two ways to fix this. We can update the TyRef variant in TypeVariants to include a 3-way mutability -- this is the Right Fix, no question. Or we can hack is_mutable to understand that a deref of a &mut which comes from an upvar of unique kind is not mutable. That is the "contained fix", but I worry about it.

[nikomatsakis]

UPDATE: You can probably ignore these instructions. I think we will use the simpler approach described below for now.

So for more context:

The problem here is that the closure, when it captures x, is actually using an &uniq borrow (explanation as to why). This is something that is only used in closure desugaring. However, our type that represents references only includes two options, & or &mut:

rust/src/librustc/ty/sty.rs

Lines 122 to 124 in aabfed5

	/// A reference; a pointer with an associated lifetime. Written as
	/// `&'a mut T` or `&'a T`.
	TyRef(Region<'tcx>, TypeAndMut<'tcx>),

You'll note that the actual & operator in MIR supports three mutabilities:

rust/src/librustc/mir/mod.rs

Lines 1326 to 1327 in aabfed5

	/// &x or &mut x
	Ref(Region<'tcx>, BorrowKind, Lvalue<'tcx>),

Where BorrowKind is defined here, and in particular has three variants.

If we want to do this the right way, then the first thing we should do I think is to refactor TyRef to use BorrowKind instead of TypeAndMut. I would do this in two steps, I suspect:

1. Refactor TyRef(Region, TypeAndMut) into TyRef(Region, Type, Mutability).
2. Refactor TyRef(Region, Type, Mutability) to replace Mutability with BorrowKind.

Once we get that done, we can continue.

[arielb1]

@nikomatsakis

Having another kind of "semi-secret" reference in the type-system that is used almost nowhere scares me - e.g., upvar OIBITs can be implemented for &T and &mut T, missing &uniq T.

I would prefer a spot change to permission checking - at least error reporting already has to detect dereferences of upvars, so it can check whether an upvar is supposed to be immutable, and in that case treat it as immutable.

[nikomatsakis]

@arielb1 I'm of two minds. On the one hand, I think that trying to make the type system more honest would reveal a lot of interesting things, and I still think it's something we ought to do at some point, precisely because it will reveal interactions like the OIBIT one that you mentioned, which I think are important for us to understand and work through. However, now may not be that time.

That said, certainly, a spot change will be easier. =) If we want to do that, this is what I think we have to do:

We need to extend the UpvarDecl struct to record whether this was a &uniq borrow or what. I don't see that this information is readily available now. I would change the by_ref: bool field to by_ref: Option<BorrowKind>, with a comment that None means by-value.

Then we have to alter MIR construction to update the field for its new type. This is the code in question:

rust/src/librustc_mir/build/mod.rs

Lines 430 to 437 in 5f1c37a

	let by_ref = match capture {
	ty::UpvarCapture::ByValue => false,
	ty::UpvarCapture::ByRef(..) => true
	};
	let mut decl = UpvarDecl {
	debug_name: keywords::Invalid.name(),
	by_ref,
	};

It should be fairly easy to extract the mode from there, it's found in the UpvarCapture data (along with some other stuff we don't want to put in the MIR, which is why we have a new type).

Finally, we have to modify the is_mutable function in the borrow checker (the "permissions check"). Currently, for a deref like *x, it examines only the type of x:

rust/src/librustc_mir/borrow_check.rs

Lines 989 to 997 in 5f1c37a

	ty::TyRef(_, tnm) => {
	match tnm.mutbl {
	// Shared borrowed data is never mutable
	hir::MutImmutable => Err(lvalue),
	// Mutably borrowed data is mutable, but only if we have a
	// unique path to the `&mut`
	hir::MutMutable => self.is_unique(&proj.base),
	}
	},

It needs to also have a check whether this is the field of an upvar. The helper function introduced in #46087 would be most useful here, so maybe we wait for that to land or else build on it. Then you could do is_upvar_field_projection(&proj.base) and -- if so -- check the upvar decl (self.mir.upvar_decls[var_index].by_ref) to get the reference mode.

[arielb1]

@nikomatsakis

My model used to be that immutable locals are basically a lint - i.e. we already know they have no soundness implication, instead we just check them for the user's convenience, and for that we do not need to add another type. A closure captures an immutable local by &mut - it's just that the lint can "see through" closure captures (while the lint can't "see through" normal captures into structs).

I just think that adding a new kind of reference to the type-system itself would raise questions, and if we are actually adding new kinds of references, it's for the better if they are user-visible instead of being a hidden compiler feature.

[davidtwco]

I've started work on this locally, will open a WIP PR when I've got something.