[NLL] prohibit "two-phase borrows" with existing borrows?

[nikomatsakis]

@RalfJung raised this example in which the "two-phase" borrow of x is compatible with a pre-existing share:

fn two_phase_overlapping1() {
    let mut x = vec![];
    let p = &x;
    x.push(p.len());
}

This poses a problem for stacked borrows, as well as for the potential refactoring of moving stacked borrows into MIR lowering (#53198) -- roughly for the same reason. It might be nice to change this, but -- if so -- we've got to move quick!

cc @arielb1 @pnkfelix

[nikomatsakis]

(It's actually not clear if we would want to backport this -- ideally we would, but it's probably a corner case.)

[Centril]

Nominated for discussion on the next T-lang meeting since this seems to a affect the type system in observable ways and because I'd like to understand this better... provided that we can wait until Thursday... ;)

[nagisa]

I only have theoretical knowledge of NLL’s implementation but it seems extremely hard to forbid this…?

[RalfJung]

From what I hear it's actually easy, we just have an additional constraint that such that when the two-phase borrow starts, all existing loans for that ref get killed (like they usually would for a mutable ref).

---

The problem is the "fake read" desugaring we do to make sure that match arms cannot mutate the discriminee:

fn foo(x: Option<String>) {
  match x {
    Some(mut ref s) if s.starts_with("hello") => s.push_str(" world!"),
    _ => {},
  }
}

Becomes something like

_fake1 = &shallow x;
_fake2 = &(x as Some).0;
// switch on discriminant
s_for_guard = &mut2phase (x as Some).0;
s_for_guard_ref = &s_for_guard;
// guard, using *s_for_guard_ref instead of s
FakeRead(_fake1);
FakeRead (_fake2);
s = s_for_guard;
// Arm as usual

When s_for_guard is created, we create a new mutable ref to something that has outstanding shared refs.

[RalfJung]

I once proposed an alternative to this desugaring that avoids 2-phase-borrows. I was told (by @arielb1 and probably others) it doesn't work because it doesn't preserve pointer identity (if the guard compares addresses it'd notice), but I actually don't see why: I think all pointers are the same as in the desugaring above. Namely, we should do:

_fake1 = &shallow x;
_fake2 = &(x as Some).0;
// switch on discriminant
s_for_guard = &(x as Some).0;
s_for_guard_ref = fake_mut(&s_for_guard);
// guard, using *s_for_guard_ref instead of s
FakeRead(_fake1);
FakeRead (_fake2);
s = &mut (x as Some).0;
// Arm as usual

where fake_mut is

fn fake_mut<'a, 'b, T>(x: &'a &'b T) -> &'a &'b mut T {
  std::mem::transmute(x)
}

fake_mut is actually safe to call with any possible x. And the pointers are exactly the same as in the desugaring above. So why does this not work?

[arielb1]

@RalfJung

In this translation, addr_of(s_for_guard) != addr_of(s), while in the previous translation it can be. However, I'm not sure how important this property is, and in any case, addr_of(s_for_guard) != addr(s) today.

And if we really wanted to preserve this property, we could have s be a union between &T and &mut T.

[RalfJung]

However, I'm not sure how important this property is, and in any case, addr_of(s_for_guard) != addr(s) today.

Okay, so we agree that my proposal doesn't break more than what we currently do -- but it might be harder to fix (if we care).

And if we really wanted to preserve this property, we could have s be a union between &T and &mut T

It would however still be the case that the mutable reference was created after the guard runs, which could be observable in terms of Stacked Borrows / LLVM noalias.

[arielb1]

Okay, so we agree that my proposal doesn't break more than what we currently do -- but it might be harder to fix (if we care).

Sure enough. So I think @RalfJung's solution (having an &&mut T -> &&T transmute, 2 addresses for ref/ref mut bindings in guards, and 2-phase borrows rejecting existing borrows) is actually fine.