Coercing &mut to *const should not create a shared reference

[RalfJung]

View all comments

It has long been a rule in Rust that you must not mutate through a shared reference, or a raw pointer obtained from a shared reference.

Unfortunately, that rule currently forbids the following code:

fn direct_mut_to_const_raw() {
    let x = &mut 0;
    let y: *const i32 = x;
    unsafe { *(y as *mut i32) = 1; }
    assert_eq!(*x, 1);
}

The reason for this is that coercing &mut T to *const T implicitly first creates a shared reference and then coerces that to *const T, meaning y in the example above is technically a raw pointer obtained from a shared reference.

We should fix our coercion logic to no longer create this intermediate shared reference.

See #56161 for how we uncovered this problem.

Cc @eddyb @nikomatsakis

[RalfJung]

I believe the relevant code might be

rust/src/librustc_typeck/check/coercion.rs

Lines 734 to 752 in f504d3f

	coerce_mutbls(mt_a.mutbl, mutbl_b)?;
	// Although references and unsafe ptrs have the same
	// representation, we still register an Adjust::DerefRef so that
	// regionck knows that the region for `a` must be valid here.
	if is_ref {
	self.unify_and(a_unsafe, b, |target| {
	vec![Adjustment {
	kind: Adjust::Deref(None),
	target: mt_a.ty
	}, Adjustment {
	kind: Adjust::Borrow(AutoBorrow::RawPtr(mutbl_b)),
	target
	}]
	})
	} else if mt_a.mutbl != mutbl_b {
	self.unify_and(a_unsafe, b, simple(Adjust::MutToConstPointer))
	} else {
	self.unify_and(a_unsafe, b, identity)
	}

but I am not sure because this is type checking. I have no idea where the code that decides about lowering of coercions to reborrows/casts lives.

[eddyb]

I have no idea where the code that decides about lowering of coercions to reborrows/casts lives.

What do you mean? That code is generating a deref and a borrow, each of those gets turned into the equivalent MIR later, and the result is as if the user wrote &* explicitly.

[eddyb]

@nikomatsakis I believe the comment above if is_ref { has been outdated for quite a while (and makes no sense for NLL), can you confirm?

[RalfJung]

@eddyb that code generates "adjustments". I have no idea what that means. I found no use of AutoBorrow::RawPtr that would turn this into casts or reborrows or so.

[eddyb]

Oh I got confused as to what this is doing, I missed the AutoBorrow::RawPtr bit, looks like "borrow" and "cast reference to raw pointer" are fused into one adjustment. The lowering is here, btw:

rust/src/librustc_mir/hair/cx/expr.rs

Lines 152 to 180 in 1c3236a

	Adjust::Borrow(AutoBorrow::RawPtr(m)) => {
	// Convert this to a suitable `&foo` and
	// then an unsafe coercion. Limit the region to be just this
	// expression.
	let region = ty::ReScope(region::Scope {
	id: hir_expr.hir_id.local_id,
	data: region::ScopeData::Node
	});
	let region = cx.tcx.mk_region(region);
	expr = Expr {
	temp_lifetime,
	ty: cx.tcx.mk_ref(region,
	ty::TypeAndMut {
	ty: expr.ty,
	mutbl: m,
	}),
	span,
	kind: ExprKind::Borrow {
	region,
	borrow_kind: m.to_borrow_kind(),
	arg: expr.to_ref(),
	},
	};
	let cast_expr = Expr {
	temp_lifetime,
	ty: adjustment.target,
	span,
	kind: ExprKind::Cast { source: expr.to_ref() }
	};

So looking again at the testcase, y as *mut i32 still goes through the reference->raw pointer coercion logic, but it does so for coercing &mut T to *mut T which does a mutable reborrow instead of an immutable reborrow (and then there's a separate *mut T to *const T coercion).

Changing AutoBorrow::RawPtr(mutbl_b) to AutoBorrow::RawPtr(mt_a.mutbl) would work, but you then also need to push Adjust::MutToConstPointer to that vec![...], if mt_a.mutbl != mutbl_b.

EDIT: this may be backwards incompatible if the mutable reference being coerced was also already immutably borrowed, since you'd be introducing a mutable reborrow.

Maybe we can avoid reborrows altogether here? But I'd leave that to @nikomatsakis.

[RalfJung]

Changing AutoBorrow::RawPtr(mutbl_b) to AutoBorrow::RawPtr(mt_a.mutbl) would work, but you then also need to push Adjust::MutToConstPointer to that vec![...], if mt_a.mutbl != mutbl_b.

I did the first but not the last and tests seem to still pass...^^

But yeah, there is a compatibility problem with outstanding shared references. Ouch.

[eddyb]

Without Adjust::MutToConstPointer the MIR may end up malformed (using *mut T where *const T is expected), I'm surprised you don't get any errors!

[RalfJung]

using *mut T where *const T is expected

Yeah I figured. run-pass and ui tests all pass, so either the change did nothing or nothing checks the MIR^^

For the backwards compatibility issue:
I think my inclination is that we don't want to reborrow on a cast-to-raw. That would also make testing Stacked Borrows in miri easier, these implicit unavoidable reborrows are a pain. :P We'd have to make sure though that the borrow checker understands that after a (x: &mut T) as *mut T, x is still there -- it hasn't been moved away.

[eddyb]

I guess the tricky bit with making it a cast is that what we have to work with is Operand::{Move,Copy}.

Instead, we could just add to the MIR a sort of "borrow to raw pointer" (a lot like AutoBorrow::RawPtr, really), and then we just need to make sure the old borrowck doesn't treat even a mutable AutoBorrow::RawPtr like a real borrow.

[RalfJung]

"borrow to raw pointer"

You mean like rust-lang/rfcs#2582? :D

[eddyb]

@RalfJung Yupp, that's what I mean, that seems like the perfect solution here.

[RalfJung]

(Sorry, that was the wrong button.)

Notice that the lint discussed in rust-lang/rfcs#2582, at least when implemented on the MIR, would actually flag these reborrows that are in our way now: The new reference is created just to turn it into a raw pointer, it has no other use.