RefCell::borrow does not pass borrow-check without a seemingly no-op let binding

[SimonSapin]

When upgrading Rust in Servo, many usages of RefCell::borrow that were previously fine now cause borrow-check errors like this:

components/script/dom/xmlhttprequest.rs:678:9: 678:13 error: `self` does not live long enough
components/script/dom/xmlhttprequest.rs:678         self.status_text.borrow().clone()
                                                                               ^~~~
components/script/dom/xmlhttprequest.rs:677:39: 679:6 note: reference must be valid for the destruction scope surrounding block at 677:38...
components/script/dom/xmlhttprequest.rs:677     fn StatusText(self) -> ByteString {
components/script/dom/xmlhttprequest.rs:678         self.status_text.borrow().clone()
components/script/dom/xmlhttprequest.rs:679     }
components/script/dom/xmlhttprequest.rs:677:39: 679:6 note: ...but borrowed value is only valid for the block at 677:38
components/script/dom/xmlhttprequest.rs:677     fn StatusText(self) -> ByteString {
components/script/dom/xmlhttprequest.rs:678         self.status_text.borrow().clone()
components/script/dom/xmlhttprequest.rs:679     }

This error message makes no sense to me, the two blocks it talks about are the same.

This can be worked around by binding the result of .borrow() with let before using it, enough though such a change looks like a no-op:

 fn StatusText(self) -> ByteString {
-     self.status_text.borrow().clone()
+     let status_text = self.status_text.borrow();
+     status_text.clone()
}

[alexcrichton]

cc @pnkfelix

[larsbergstrom]

@SimonSapin has also mentioned that there are some cases (e.g., in our "script" crate) where even this transformation does not work.

cc @nikomatsakis

[SimonSapin]

I got stuck at some point but @jdm managed to find work arounds servo/servo@0c12dd9

[pnkfelix]

This may be a consequence of the new scoping rules introduced in #21657. We may attempt to address coding patterns like this by implementing #22323; but implementing that is not a 1.0-blocking issue (i.e. it may wait until after 1.0 is released, depending on other priorities).

[pnkfelix]

cc #22321

[nikomatsakis]

Standalone test case:

use std::cell::RefCell;

fn foo(x: RefCell<String>) -> String {
  x.borrow().clone()
}

fn main() { }

[pnkfelix]

leaving nominated tag to talk again next week, assigning to self to investigate.

[pnkfelix]

Another work-around worth noting (if only because it provides a hint at the problem here):

fn foo(x: RefCell<String>) -> String {
  let t = x.borrow().clone();
  t // this works!
}

I think this is an artifact of our temporary r-value rules, in particular the one that says that all temporaries for the tail expression in a block are assigned the lifetime of the parent of the block (that is, they can outlive the block), and thus the Ref returned by x.borrow() is assigned a lifetime that is too long.

[pnkfelix]

(Having said that, I would like to see if we can still fix this in some way. But I no longer see this as a potential smoking gun pointing at some fundamental flaw in the system. So I've removed the I-nominated tag.)

---

More details for those interested: the thing that comes up is this:

1. borrow has this signature: fn borrow<'a>(&'a self) -> Ref<'a>, and the Ref has a destructor.
2. So dropck wants the 'a to outlive the parent of the scope of the returned Ref.
3. The returned Ref, due to our tail expression rule for expression-blocks, is assigned the lifetime of the parent of the block.
4. So 'a in the call to borrow ends up being assigned the lifetime of the destruction scope surrounding the function body.
5. But the parameter x does not live that long.

We could hack in special support for this case (i.e. some hack in terms of how fn bodies are treated).

• For example maybe we could change things so that fn parameters are torn down after the fn body is torn down (in that case the parent of the block would be the destruction scope for the fn body, and the fn parameters would be defined to outlive that. But then again that might be too much of a breaking-change, not sure.) But I don't know if that's a great option; e.g. then people will still run into problems with code that isn't a fn-body, like this revision of the example above:

use std::cell::RefCell;

fn foo(x: RefCell<String>) -> String {
    let result = {
        let t = x;
        t.borrow().clone()
    };
    result
}

fn main() { }

(but maybe that is acceptable...)

[pnkfelix]

Actually, according to the current dynamic semantics, fn parameters already are torn down after the fn body is torn down.

So it should be entirely sound (perhaps even a legitimate bug fix, not sure) to make the code-extents reflect the fact that the temporaries from the tail expression for the fn-body are destroyed before the parameters are dropped (i.e., that the parameters strictly outlive all of the r-value temporaries of the fn body).

Here is some demo code illustrating this (note also that the behavior differs if one passes --cfg nested, but in either case the parameters still outlive the r-value temporaries).

struct D(&'static str, u32);

impl Drop for D {
    fn drop(&mut self) {
        println!("Dropping D({}, {})", self.0, self.1);
    }
}

impl D {
    fn incr(&self, name: &'static str) -> D {
        D(name, self.1 + 1)
    }
}

#[cfg(not(nested))]
fn foo(a1: D, b1: D) -> (&'static str, D) {
    let _b2 = b1.incr("b");

        let _a2 = a1.incr("a");
        let b3 = b1.incr("temp1").incr("b");
        println!("made b2 a2 and b3");
        ("foo_direct", b3.incr("temp2").incr("b"))

}

#[cfg(nested)]
fn foo(a1: D, b1: D) -> (&'static str, D) {
    let _b2 = b1.incr("b");
    {
        let _a2 = a1.incr("a");
        let b3 = b1.incr("temp1").incr("b");
        println!("made b2 a2 and b3");
        ("foo_nested", b3.incr("temp2").incr("b"))
    }
}

fn main() {
    let (name, result) = foo(D("param_a", 1), D("param_b", 1));
    println!("called {}, got result D({}, {})",
             name, result.0, result.1);
}

(The reason that its interesting that the behavior differs with and without cfg nested is that it even differs if you comment out the let _b2 = ...; line at the top ... i.e. the program transformation:

fn function(args ...) -> result { stmts ...; expr } 

to

fn function(args ...) -> result { { stmts ...; expr } }

is not semantics preserving; it changes the time at which the temporaries within expr are dropped with respect to the let-bindings within stmts ...)

[pnkfelix]

Okay, I'm pretty happy with the solution given in #24021.

[agrover]

@pnkfelix this still is present w.r.t expression blocks, though, which are pretty useful when attempting to control the length of borrows. Using workaround in #23338 (comment) for now but it would be great for this to work nicely at some point, it led me to think I was going crazy 😄