Regression with NLL and RefCell

[abusch]

Hi,

I have some code similar to the one below that compiled fine up until nightly-2018-01-03, but stopped compiling starting with nightly-2018-01-04:

#![feature(nll)]
use std::cell::RefCell;

struct Foo {
    pub v: Vec<u32>,
    pub b: u32,
}

fn f(foo: RefCell<Foo>) {
    let mut foo = foo.borrow_mut();
    
    foo.v.push(foo.b);
    
}

fn main() {
    let foo: RefCell<Foo> = RefCell::new(Foo {v: Vec::new(), b: 0});
    f(foo);
}

The error I now get is

Compiling playground v0.0.1 (file:///playground)
error[E0502]: cannot borrow `foo` as immutable because it is also borrowed as mutable
  --> src/main.rs:12:16
   |
12 |     foo.v.push(foo.b);
   |     ---        ^^^ immutable borrow occurs here
   |     |
   |     mutable borrow occurs here

error: aborting due to previous error

error: Could not compile `playground`.

To learn more, run the command again with --verbose.

I was able to work around it by coercing RefMut<T> to a &mut T using let foo = &mut *foo.borrow_mut(), and normal nll rules work as expected. But since it used to work with RefMut, I suppose it still should?

[matthewjasper]

I think this is expected due to #46984 fixing #46974.

[vitalyd]

@matthewjasper, is there a fundamental reason the OP example shouldn’t work? Is the issue that the fixes also make this case not work, but not necessarily intentionally so to speak?

[bluss]

the RefMut value bound in foo implements Deref/Mut so we should think of the line foo.v.push(foo.b); as:

foo.deref_mut().v.push(foo.deref().b);

Unless the deref method has any other treatment than a regular method call, it's not possible to allow this this code, because .deref_mut() borrows foo exclusively, and is allowed to modify foo.

Reading foo.b first is therefore something that changes the meaning of the program (reading it before the possible modification).

One class of programs NLL wants to fix is the (with v a vector) example v.push(v.pop().unwrap() + 1); the difference between the code in this issue and that, is that here the method call foo.deref_mut() is finished before we continue with the .push(..) method call.

[ghost]

It seems like the problem could be solved by inlining the call to deref_mut at the MIR level. Then the MIR borrowck would see that the mutable reference to v is actually not used before loading b. With two-phase borrowing, it all works out.

Is that a feasible solution to the problem?

[daboross]

@stjepang I don't think it's feasible to depend on inlined inner behavior of other functions for borrow checking of an outside function. We might know RefMut's behavior is "sound" and won't change, but that's not necessarily true of other things with Deref/DerefMut.

I mean, a type is literally allowed to do anything inside Deref or DerefMut, and it is not a breaking change to change what is done inside a function. If the borrow checker depends on the behavior of functions defined in separate libraries independently of their signature, it means changing the behavior of functions could be a breaking change!

I don't think it's reasonable to introduce this to rust now. So far, we've had everything that type / borrow checking can rely on encoded in function signatures. It would be nice if things stayed this way.

[comex]

Maybe there could be some sort of annotation on deref and deref_mut allowing the compiler to reorder calls to them - since sane implementations of them will be 'observationally pure'.

[matthewjasper]

DerefPure has been proposed before as part of rust-lang/rfcs#1646

[vitalyd]

Given how prevalent RefCell is, it’d be somewhat of a shame if we can’t make it work well with NLL. I understand the argument of why it doesn’t work today (ie what @bluss said), but it’s yet another papercut in usability. The DerefPure proposal is a good one. Even this NLL issue aside, it’d be nice to encode the purity of deref (which as @comex points out is the same impl everyone expects).

[daboross]

@vitalyd it's worth noting that doing these kind of things is possible with RefCell, and the restrictions aren't new to NLL. It just can't be made into a nice example as other NLL examples can.

For instance, this does work (replacing f in the original code snippet):

fn f(foo: RefCell<Foo>) {
    let mut foo = &mut *foo.borrow_mut();
    
    foo.v.push(foo.b);
}

I agree that "pure" deref may be desirable, maybe as an unsafe trait, or maybe as some kind of function and/or trait modifier, but I don't think it should be tied to NLL. It would be useful outside of using non-lexical lifetimes, and NLL is useful without it.

[arielb1]

This is indeed expected in the absence of Deref2Phi - DerefMut requires full mutable access, which is invalid when there is also an immutable borrow.

[nikomatsakis]

Seems like this is "not a bug — behaving as designed", so I'm going to close the issue. Thanks all.