RFC: Enable nested method calls

[nikomatsakis]

Enable "nested method calls" where the outer call is an &mut self borrow, such as vec.push(vec.len()) (where vec: Vec<usize>). This is done by extending MIR with the concept of a two-phase borrow; in this model, select &mut borrows are modified so that they begin with a "reservation" phase and can later be "activated" into a full mutable borrow. During the reservation phase, reads and shared borrows of the borrowed data are permitted (but not mutation), as long as they are confined to the reservation period. Once the mutable borrow is activated, it acts like an ordinary mutable borrow.

Two-phase borrows in this RFC are only used when desugaring method calls; this is intended as a conservative step. In the future, if desired, the scheme could be extended to other syntactic forms, or else subsumed as part of non-lexical lifetimes or some other generalization of the lifetime system.

Rendered.
Rendered text in rfcs repo

[est31]

I can remember struggling with the vec.len() issue, back when I have been new with Rust. In the optimal case we'd have non-lexical lifetimes or something similar, but if this improves the status quo and can be implemented and stabilized faster, I surely welcome it!

[Ixrec]

Missing blog post link?

[cramertj]

Probably this one.

[Ericson2314]

From the internals thread, I think all two-phase borrows can be desguared into &mut + &-reborrowing, but I'm not sure. Could this be cleared up? If so it might be nice to actually do that with &mut2 just a temporary type used midway in the HIR->MIR desugar. Even if an extra pipeline stage is gratuitous, knowing the desugaring is valid is nice for teaching / mental models.

[nikomatsakis]

@Ericson2314 ah thanks for bringing that up. There are some corner cases that probably merit discussion in the RFC text that I think are interesting (places where my text "a reservation acts like a shared borrow" is probably worth clarifying).

For example, if you are using shared borrows, there are cases where you would be able to do reassignments that you cannot with a mutable borrow. In particular, when you borrow the referent of a shared reference:

let mut x: &i32 = &22;
let y: &i32 = &*x;
x = &44; // allowed even though `*x` was borrowed above

We don't at present allow similar assignments for &mut reborrows, but I think we should. There's a rust issue about this from a long-time back but I can't find it just now.

Anyway, my key point is that if we allowed such assignments for &mut reborrows, then there might be cases where you could observe a reservation. Something like this:

let mut x: &mut i32 = &mut 22;
x.some_method({ x = something_else; ... })

which might desugar to:

x: &mut i32 = &mut 22;
tmp = &mut2 *x;
x = something_else; // allowed because it doesn't inferfere with `tmp`
some_method(tmp, ...)

[oli-obk]

Bikeshed: you mention several times, that the borrow means the value is reserved, why not mirror this and call it a &reserve for simplicity's sake? Otherwise the words "reserved" and "two-phase" will end up being used interchangeably which probably won't help teaching the concept.

[nikomatsakis]

I debated about this. I thought it was strange, because &reserve foo suggested that it only reserved foo (rather than later mutating it), but perhaps it's clearer to say that it produces a reserved reference that (upon first use) is "upgraded" to a regular mutable reference.

[cramertj]

&will_mut foo?

[oli-obk]

Typo: note

[arielb1]

That's not entirely correct:

fn main() {
    let v = &mut vec![];
    v.push(0);
    v.push(1);
}

ATM we do not emit a reborrow:

        _1 = &mut _2; // _1 is `v`
        StorageLive(_4);
        _4 = _1;
        _3 = const <std::vec::Vec<T>>::push(_4, const 0i32) -> [return: bb4, unwind: bb3];

That is a bug, however, because if we don't emit a reborrow the code will not borrow-check (because of the multiple use of v).

[nikomatsakis]

@arielb1 actually, we do emit it, but we optimize it out later (post borrowck).

[arielb1]

Given the current signature for IndexMut, this case isn't so obviously harmless - IndexMut for something like a wrapper of RefCell<Vec<T>> can e.g. call RefCell::get_mut, which can then be invalidated by someone calling .borrow_mut().push(..) from an immutable borrow.

[nikomatsakis]

Indeed. I meant that it's "obviously" harmless in the sense that we know what those routines do (I can add a note to this effect).

[RalfJung]

The two-phase borrow proposal that I describe here should be able to fit into that system in a fairly straightforward way.

I like your optimism here. ;)

[RalfJung]

@Ericson2314 ah thanks for bringing that up. There are some corner cases that probably merit discussion in the RFC text that I think are interesting (places where my text "a reservation acts like a shared borrow" is probably worth clarifying).

This discussion is very interesting to me because modelling the reservation phase as a shared reborrow of the &mut is the most direct idea I have to account for these two-phase borrows in my formal model.

@nikomatsakis, does your comment say that this is or is not a valid way to think about two-phase borrows? You brought up an example, but it seems to me that (assuming we permitted the kind of overwriting of mutably borrowed variables you mention), the example should desugar just fine?
I guess I don't understand what you mean by "making reservations observable"; after all, the entire point of this PR is to accept more programs, so of course that will be observable.

I feel like this desugaring also gives a nicer explanation to why programs such as the one you use to demonstrate why the new check is needed should be rejected. (Yay for recursive grammars in natural languages. ;) Specifically, the lifetime of the borrow &v is larger than the lifetime of the implicit shared-reborrow.

Another reason I like this is that I found it very useful in my formal model to inherently link sharing and lifetimes -- in the sense that when a variable of a type is shared (i.e., when it is accessible through a shared borrow), then it is always shared for some particular lifetime. This approach has gotten us really far, and even properly handles subtleties like RefMut::map any why the closure there needs to be lifetime-polymorphic. So with these two-phase borrows being "like shared borrows" in the reservation phase, I feel like that "reservation phase" should also be a lifetime. (It may be a non-lexical lifetime though.) That lifetime then naturally upper-bounds the shared borrows that can be taken during the reservation phase, and it is when that lifetime ends that the "mutable phase" of the two-phase borrows starts. This also has the added benefit that the borrow checker doesn't have to reason about when the variable that a mutable borrow is stored into gets used (something that will need some special casing, as @nikomatsakis pointed out -- consider the case where the LHS of the assignment is not just a local, but some field). Instead, it just has to do what it can do best, i.e., reason about lifetimes (and, in particular, where the end).

I hope this makes any sense... I want to think more about this and see whether I can come up with a "formal definition" of this desugaring so that we can see whether the set of accepted programs differs.

[RalfJung]

All right so here's a proposal for handling this with "shared reborrow of a mutable borrow". I think this is effectively equivalent to doing "old-style" borrow checking after some source-to-source translation, which I will also try to describe. Either way, this proposal involves non-lexical lifetimes -- it doesn't need the full machinery, but it involves lifetimes that are not scopes or expressions.

The idea is for mut2 to take two lifetimes:

tmp = &'r 'w mut2 lv

(This looks somewhat like the Ref2 model, but the lifetimes here are somewhat different.)
'w is the "normal" lifetime of this mutable borrow, and 'r is the lifetime where the borrow is still "read-only" or "reserved" or "not yet upgraded". 'w has to outlive 'r.

When borrowing from lv while the mut2 borrow lasts (i.e., during 'w), where the compiler checks that the referee outlives the borrow, it considers 'r to also be an upper bound. This takes care of the special check that is the RFC adds to ensure that shared borrows taken in the reservation phase do not extend into the mutable phase.

When accessing a variable of type &'r 'w mut2 T, the compiler checks that 'r has ended. Only if that is the case, the variable may be used. The type is then equivalent to &'w mut2 T.

Effectively, the difference between this model and the RFC seems to be that the "reserved region" is explicit in my model, and merely checked by borrowck. In the RFC, on the other hand, the region is implicit and inferred by borrowck.

---

So, I promised a source-to-source translation. I think it could go somewhat as follows:

• Translate tmp = &'r 'w mut2 lv to tmp = &'w mut lv; lv_during_reserve = &'r tmp
• Change every use of lv during the reservation to use lv_during_reserve instead.
  I think the effect of this should be exactly what I described above.

[Ericson2314]

Yes, that reborrow was what I was thinking of. @nikomatsakis, I was also confused but gathered you were thinking of a different desugaring or something.

[frewsxcv]

s/a blog/this blog/ https://github.com/rust-lang/rfcs/pull/2025/files#r121198786

[antoyo]

What about interior mutability?
Can this cause some issues?
For instance, if you reserve something that is mutated later in a &self method call using interior mutability.

[briansmith]

Treating all mutable borrows as potentially deferred would make them something that everyday users would encounter, and we didn't feel satisfied with the "mental model" that resulted.

I think these subtleties are already encountered on a regular basis by people, and I don't think that making method calls special simplifies the mental model at all. In particular, it is common to think of functions as something that could be inlined into the caller at the point of the call, and to think of inlining as literally doing that inlining as a source-to-source transformation, instead of as a transformation of an intermediate form. In other words, I think people expect that any function call should be able to be rewritten as copy-pasta of the function's source code and so there's nothing special about functions. As an example of this idea, I think people expect all four of these to work and do the same thing:

fn len_then_push(tmp0: &mut Vec<usize>) {
    let tmp1 = tmp0.len(); // Used as shared.
    Vec::push(tmp0, tmp1); // Used as mutable.
}

fn main() {
   // Works fine today.
   let mut vec = vec![0];
   len_then_push(&mut vec);

   // Works fine today as only one reference to `vec` is used.
   {
     let tmp0 = &mut vec;   // `vec` is mutably borrowed.
     let tmp1 = tmp0.len(); // Used as shared.
     Vec::push(tmp0, tmp1); // Used as mutable.
   }

   // Now: Fails to compile. Future: OK.
   vec.push(vec.len());

   // Now: Fails to compile. Future: OK.
   {
     let tmp0 = &mut vec; // Now: Mutable borrow. Future: reserve `vec`.
     let tmp1 = vec.len();  // shared borrow of vec. Now: fails. Future: OK.
     Vec::push(tmp0, tmp1); // Future: mutable borrow of `vec` is activated.
   }
}

So, I do think it is fine to do some special case for function calls today, but I think there should be a more concrete plan to generalize the mechanism so that it isn't specific to function calls forever.

[burdges]

I frequently notice situations where a call to mem::drop would be more clear than a { } block, sometimes meaning I annotate the closing } // mem::drop(f); It's no biggie in that simple case, but if you find two overlapping blocks like that then the { } could be annoying enough that you'd reach for RefCell. This is especially likely when you create multiple closures to help mutate different objects. It'll be cool if NLL eventually understands mutable borrows being consumed by value like that.

[RalfJung]

@briansmith The problem here is that you are not just doing inlining of a function call. You are changing which variable is used for a particular function call. So you are asking for much more here than just "there should be nothing special about a function call".

Personally, I think it is perfectly reasonable for this code not to compile:

   // Now: Fails to compile. Future: OK.
   {
     let tmp0 = &mut vec; // Now: Mutable borrow. Future: reserve `vec`.
     let tmp1 = vec.len();  // shared borrow of vec. Now: fails. Future: OK.
     Vec::push(tmp0, tmp1); // Future: mutable borrow of `vec` is activated.
   }

After all, there's overlapping shared and mutable borrows here! And if you write the code like this, it is trivial to just swap the two let around, and everything works. (Of course, maybe that's just my bias for "I know the current model, why change it?")
The situation is somewhat different with vec.push(vec.len()); because the order of borrowing is totally not clear, and the fix in the current situation is actually making the code longer. To me, this justifies treating the function call different from the case where the user explicitly wrote the desugared form.

[KamilaBorowska]

Just curious, how does that compare to Assign traits. Even today, you can write code like vec.push(vec.len()), but only when using Assign traits. For instance (this is ugly, nobody should write code like this, but it's valid to borrowck).

use std::ops::AddAssign;

#[derive(Debug)]
struct MyVec<T>(Vec<T>);

impl<T> AddAssign<T> for MyVec<T> {
    fn add_assign(&mut self, elem: T) {
        self.0.push(elem);
    }
}

fn main() {
    let mut vec = MyVec(Vec::new());

    vec += 3;
    vec += vec.0.len();

    println!("{:?}", vec);
}

[nikomatsakis]

@RalfJung

The idea is for mut2 to take two lifetimes [...]

Yes, I like this formulation. As far as whether "borrowck" infers the lifetime or what, I suppose it's all a matter of perspective. That is, one way that I had imagined implementing this in my head was roughly like so:

• Before doing anything else, borrowck does a pass over the MIR and identifies each x = &mut2... borrow. For each such x, it identifies the NLL-style lifetime between the borrow and the first use of x. This is the 'r borrow in your model. It stores this in a table.
• Then later in borrowck, when we encounter the x = &mut2... borrow, we can indicate that we have a reservation for 'r.

So you can view just the second part of it as "borrowck-proper". However, I hadn't considered the idea of limiting shared borrows that occur with a mut2 in scope to be a "sublifetime" of 'r, rather than considering the "transition" to be an event. I like that idea. I think the main reason that I hadn't thought about it was that, in the current state of the compiler, the "regions" of a borrow are still going to be tied to the HIR, and thus it would be a bit awkward to compare them.

I can rewrite the RFC to describe your alternate description (or maybe add it as an alternative way of looking at things).

So, I promised a source-to-source translation. [...]

This translation seems reasonable. I was assuming that you and @Ericson2314 meant a translation where you did not rewrite all accesses to lv during the reservation to this new lv_during_reserve variable. Specifically, I thought maybe the idea was to try and encode the same semantics as x = &mut2 lv by starting out with a shared borrow and then doing a mutable borrow afterwards, so that vec.push(vec.len()) would be something like:

let tmp1;
{
    let _tmp = &vec; // the "reservation"
    let tmp2 = &vec; // desugarging vec.len();
    tmp1 = Vec::len(tmp2);
}
let tmp3 = &mut vec; // the actual borrow
Vec::push(tmp3, tmp1);

I think it's plausible to imagine something along these lines (though it'd be quite complex), but in that scenario, you would want to account for those cases where evaluating the arguments could mutate the original path (vec, here). That is, the code above assumes that the &mut vec will borrow the same value as the original &vec -- but that is not true for all paths, if we allow base parts to be "overwritten" (i.e., following the original path may lead somewhere new, even if the original contents are unperturbed).

[nikomatsakis]

@burdges

It'll be cool if NLL eventually understands mutable borrows being consumed by value like that.

It ought to, yes.

UPDATE: Except that you wouldn't have to explicitly drop, I wouldn't think.

[nikomatsakis]

Just curious, how does that compare to Assign traits. Even today, you can write code like vec.push(vec.len()), but only when using Assign traits.

Yeah, to be honest, I think that's a bug in the current borrow checker. I was noticing that earlier when preparing this RFC and wanting to dig into it. This RFC would hopefully make actual code that does this still type-check.

[RalfJung]

Just curious, how does that compare to Assign traits. Even today, you can write code like vec.push(vec.len()), but only when using Assign traits.

Yeah, to be honest, I think that's a bug in the current borrow checker. I was noticing that earlier when preparing this RFC and wanting to dig into it. This RFC would hopefully make actual code that does this still type-check.

Looks very much like a bug. Replacing "+=" by vec.add_assign(vec.0.len()); results in the code being rejected.

EDIT: Yep, it's a bug. A soundness bug: rust-lang/rust#27868 (comment)

[briansmith]

Personally, I think it is perfectly reasonable for this code not to compile:

Just to be clear, that fragment is from the RFC's description of one of the proposed extensions, with only the comments modified. Also, I do agree the current behavior is reasonable. However, I think it would be closer to ideal if that kind of code fragment compiled, as it is unambiguously safe. My point is that I disagree with the idea that a more powerful borrow checker should be rejected because of concerns about the difficulty of the "mental model." In this case, it isn't even clear that the more powerful proposal would even be more mentally taxing, as what it additionally allows isn't significantly different from what is already allowed.

[nikomatsakis]

My point is that I disagree with the idea that a more powerful borrow checker should be rejected because of concerns about the difficulty of the "mental model."

I definitely would like to see a more powerful borrow checker, in many respects, but I still think it should be predictable. I'm concerned that the analysis I am proposing here, if scaled up, would not be sufficiently predictable for people.

For example, if I write:

let x = &mut v;
let y = Vec::len(&v);
use(x, y);

and I explain to you that the &mut v doesn't start until x is used, that's fine. But then maybe you try this:

let x;
x = &mut v;
let y = Vec::len(&v);
use(x, y);

and now you get an error (because of how MIR desugaring works; it would introduce a temporary, and hence x = &mut v winds up as let tmp = &mut v; x = tmp, and so there is a use). We could maybe change how desugaring works to handle that case, but then maybe you hit this one:

let x = &mut v;
let t = x; // is this a use?
let y = Vec::len(&v);
use(t, y);

and so on. In contrast, a "borrowing for the future" model (or Ref2) would handle all of those cases equally. So it seemed to me better to adopt such a more complete model, as part of the NLL work, and just do something limited to start.

[briansmith]

We could maybe change how desugaring works to handle that case,

Right, I'd see it as a bug in the translation to MIR, or more likely a limitation of the borrow checker's interpretation of copies of mutable references that is best removed.

but then maybe you hit this one:

let x = &mut v;
let t = x; // is this a use?
let y = Vec::len(&v);
use(t, y);

That code is clearly OK insofar as the reasons for which we have borrow checking are concerned. (How to formulate a valid borrow checking rule that allows it isn't clear, but we can see that the code is safe, so there's probably a way to formulate a rule that allows it.) What matters is whether or not a data race or reference invalidation occurred or whether references escaped the current scope such that we have to assume that races/invalidations would occur since we have to limit ourselves to just local analysis (AFAICT).

and just do something limited to start.

Yep. In my origin message up above, I did say that I thought it was fine to do something more limited first. I'm just saying that the assertion that the other alternatives are too complicated for users to understand seems unwarranted, and so such alternatives shouldn't be dismissed for that reason.

[pnkfelix]

(Just want to throw in my support for @RalfJung's alternative formulation that uses two distinct lifetimes. But that's probably because I prefer how I see that factoring the compiler changes involved here.)

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[ViktorHavrysh]

I'm a beginner in Rust, so please correct me if I'm wrong, but my understanding is that currently methods are just syntactic sugar for free functions. That is, this

struct Foo {
    frobbed: bool,
}
fn frob(foo: &mut Foo) -> i32 { foo.frobbed = true; 42 }

is equivalent to

struct Foo {
    frobbed: bool,
}
impl Foo {
    fn frob(&mut self) -> i32 { self.frobbed = true; 42 }
}

The only difference is the way it is being called. So these should be equivalent:

let foo = Foo { frobbed: false };
foo.some_method(foo.frob()); // first call
foo.some_method(frob(&mut foo)); // second call

This RFC makes the first call magically work in some cases where the second wouldn't.

If everything that I have said so far is correct, I find that I dislike this RFC. It introduces a special case that is bound to trip someone up. Suppose someone new to rust is experimenting with a design, and decides that something would work better as a free function. Suddenly they find that something that used to work breaks for no apparent reason. They google stuff, and find out that it should not have worked in the first place. If they are lucky, they find this RFC and understand what's going on. If they're unlucky, they just assume that methods are somehow special (and with this RFC, they really are) and come out with a flawed understanding of Rust. So in my opinion, this RFC runs counter to the goal of lowering Rust's learning curve.

I think that either both the first call and the second call should be made to work using some more general solution, or both should continue to fail. This baby step seems to leave the language in a non-self-consistent state until further steps are taken that make methods not magical again.

[golddranks]

@VictorGavrish Your fear is fortunately unfounded: first of all, neither of the examples there wouldn't compile, because frob takes a mut receiver, and nesting that isn't allowed even by the rules of this RFC. Secondly, the point is that shared references are allowed during the "inactive" phase of the mutable borrow that some_method is called with: unless I missed something, all kind of shared references would be allowed during that inactive phase, regardless whether the surface syntax is a method call or taking a reference explicitly, so if frob took a shared reference, both of the examples would be allowed.

[Ericson2314]

@nikomatsakis Anything based on #2025 (comment) that should go in this?

However, I hadn't considered the idea of limiting shared borrows that occur with a mut2 in scope to be a "sublifetime" of 'r, rather than considering the "transition" to be an event. I like that idea. I think the main reason that I hadn't thought about it was that, in the current state of the compiler, the "regions" of a borrow are still going to be tied to the HIR, and thus it would be a bit awkward to compare them.

I'd like to think the such FIFO-respecting (FIFO along control flow paths, not surface syntax scopes) events and lifetimes are always in correspondence---that's what I did in my stateful MIR at least. And now we have LifetimeEnd, it seems rustc is taking the that approach too?

[rfcbot]

The final comment period is now complete.

[aturon]

Huzzah! This RFC has been merged!

Tracking issue

[vitalyd]

What’s the reason @VictorGavrish’s example with nested mutable calls isn’t included in NLL? This came up in https://users.rust-lang.org/t/possibly-stupid-question-about-borrows-created-in-function-call-arguments/.

Is there a fundamental problem with this or just a temporary limitation?