New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

non-lexical lifetimes #2094

Merged
merged 2 commits into from Sep 29, 2017

Conversation

Projects
None yet
@nikomatsakis
Contributor

nikomatsakis commented Aug 2, 2017

Extend Rust's borrow system to support non-lexical lifetimes -- these are lifetimes that are based on the control-flow graph, rather than lexical scopes. The RFC describes in detail how to infer these new, more flexible regions, and also describes how to adjust our error messages. The RFC also describes a few other extensions to the borrow checker, the total effect of which is to eliminate many common cases where small, function-local requires would be required to pass the borrow check. (The appendix describes some of the remaining borrow-checker limitations that are not addressed by this RFC.)

Due to its size and complexity, this RFC is being run through an experimental process. The text of the RFC itself is not in this file -- rather, it can be found at the non-lexical lifetimes repository. Prior to merging, the final version of the text will be added to this PR directly; until then, hosting the RFC at the repository allows for us to track (using open issues or pending PRs) important points of conversation and so forth. Feel free to leave ordinary comments on the PR, or to open issues -- important points will be elevated to issues for further discussion.

The ideas in this RFC have been implemented in prototype form. This prototype includes a simplified control-flow graph that allows one to create the various kinds of region constraints that can arise and implements the region inference algorithm which then solves those constraints.

Rendered.

@nikomatsakis nikomatsakis added the T-lang label Aug 2, 2017

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Aug 2, 2017

🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊 🎊

(Sorry, reactions only let you do one, this RFC deserves many)

@ErichDonGubler

This comment has been minimized.

ErichDonGubler commented Aug 2, 2017

Verdict: let's do it!

@nicoburns

This comment has been minimized.

nicoburns commented Aug 2, 2017

I have a question about a slightly modified version of an example in the RFC. Under the proposal in the RFC, would the following be valid?

fn process_or_default() {
    let mut map = ...;
    let key = ...;
    match map.get_mut(&key) 
        Some(value) => {
            println!("{:?}", value);
            map.insert(key, V::default());
        },
        None => {
            map.insert(key, V::default());
        }
    }
}

Note that unlike the example in the RFC, the Some branch also mutates the map. In my mind, this ought to be valid, as even though the Some branch borrows the map to retrieve value, that borrow should expire after the println!, but it isn't completely clear to me whether this is the proposed system would allow or not.

@xfix

This comment has been minimized.

Contributor

xfix commented Aug 2, 2017

@nicoburns

If I understand correctly, in this case, lifetime of value ends just after println!("{:?}", value); statement as there are no uses of value after that point. So yes, this will handle this.

@Gankro

This comment has been minimized.

Contributor

Gankro commented Aug 3, 2017

Disclaimer: haven't read layers 2-5 yet, and have been skimming the end of layer 1 to try to understand the earlier parts. I probably need to understand a bunch of this stuff to update the nomicon, so understanding it well is pretty important to me.

First off, when describing the algorithm for solving constraints, you introduce the notion of "exiting" the lifetime 'p when handling a constraint like ('foo: 'p) @ A/1, without any clear explanation. Is exiting 'p just entering a region where 'p is "dead"?

Second off, I'm having trouble rectifying these statements:

In the case of example 4, therefore, we wish our algorithm to compute that 'foo is {A/1, B/0, C/0}, which notably excludes the points B/1 through B/4. 'bar should be inferred to the set {B/3, B/4, C/0}. The lifetime 'p will be the union of 'foo and 'bar, since it contains all the points where the variable p is valid.

So 'p must be considered to outlive 'foo and 'bar, since it has all of their points and more, right..?

But...

Whenever references are copied from one location to another, the Rust subtyping rules requires that the lifetime of the source reference outlives the lifetime of the target location.

So let p: &'p _ = &'foo foo;

Means 'foo outlives 'p..?

I think my issue might be trying to think of the lifetimes without the @ constraints? Is that now completely foolhardy?

@RReverser

This comment has been minimized.

RReverser commented Aug 3, 2017

Perhaps I missed it from a quick glance, but are there mechanics that will allow opt-out from non-lexical lifetimes to plain old blocks for FFI interactions? Currently one can expect that values in the block will be deallocated in the end of the block, so it's "safe" to pass a pointer to such values to FFI code that needs to interact with these pointers in other FFI calls till the end of the block, even though Rust can't see or know whether the first FFI call just used a reference/pointer temporarily or stored somewhere else. (This is a bit vague description, I can provide an example a bit later when not from the phone, if it will be needed)

@xfix

This comment has been minimized.

Contributor

xfix commented Aug 3, 2017

@RReverser

An example I feel would be interesting, because I'm not exactly sure what the issue (but considering it's unsafe, there as well may be one). Note that this RFC doesn't affect destruction of variables, they still get destructed when they would have been normally, and all old code will work exactly as it should, this only affects code that wouldn't be compiled by current Rust.

@eddyb

This comment has been minimized.

Member

eddyb commented Aug 3, 2017

Not only what @xfix said, but lifetime inference cannot affect code generation.

@RReverser

This comment has been minimized.

RReverser commented Aug 3, 2017

doesn't affect destruction of variables, they still get destructed when they would have been normally, and all old code will work exactly as it should

Ah okay, that was the confusing bit - for some reason, I thought that non-lexical lifetimes would also bring earlier destruction for objects that otherwise would live till the end of the block.

@burdges

This comment has been minimized.

burdges commented Aug 3, 2017

If I understand, this code would now compile where currently the 2nd print gives an error, but the 1st and 2nd print() do the same thing here, assuming Foo has no interior mutability, while the 3rd does something different.

...
impl Foo {
    fn print(&self) { .. }
    fn mutate(&mut self) { .. }
}

struct Wrapper<'a>(Foo);

impl<'a> Drop for Wrapper<'a> {
    fn drop(&mut self) { self.0.mutate(); }
}

fn something(f: Foo) {
    {
        let x = Wrapper(&mutf);
        x.0.print();  // 1st
        f.print();  // 2nd
    }
    f.print();  // 3rd
}

It seemingly follows that non-lexical lifetimes limit the ability of wrapper types to impose temporary restrictions on data structures you apply them to, meaning this is not only a concern for FFIs. It'd be easy to fix this with some attribute, say #[lexical_lifetime], applied to the wrapper type though.

@eddyb

This comment has been minimized.

Member

eddyb commented Aug 3, 2017

@burdges A drop counts as an use so impl Drop is #[lexical_lifetime].
As I said before, this RFC cannot change what code executes.

@aturon aturon referenced this pull request Aug 3, 2017

Open

Language ergonomic/learnability improvements #17

10 of 31 tasks complete
@shepmaster

This comment has been minimized.

Member

shepmaster commented Aug 3, 2017

Thank you very much for discussing how error messages will be improved by this proposal! I believe that the new errors, with the borrow / invalidation / usage sites clearly highlighted, will go a long way to helping people understand what is wrong and how to fix them.

I've opted to continue using the term "lifetime" to refer to the portion of the program in which a reference is in active use (or, alternatively, to the "duration of a borrow"). As the intro to the RFC makes clear, this terminology somewhat conflicts with an alternative usage, in which lifetime refers to the dynamic extent of a value (what we call the "scope"). I think that -- if we were starting over -- it might have been preferable to find an alternative term that is more specific. However, it would be rather difficult to try and change the term "lifetime" at this point, and hence this RFC does not attempt do so.

I fully agree that the usage of the term "lifetime" is overloaded, and in a clear restart we would want to avoid using it. This feels like one of the trickiest aspects of Rust for people coming from "closely related" languages where they have to think about lifetimes. When people "get" that a lifetime is actually "the time period in which the address of a variable remains constant", that's when a lot of other things start to click for them.

To avoid confusion, however, it seems best if the error messages result from the region and borrow check avoid the term lifetime where possible

That said, I disagree with this aspect. This sounds like we are trying to stop using the term lifetime. I worry that such a direction will make things worse. In such a world, we'd have existing literature (blog posts, the book, Stack Overflow, etc.) that uses the term "lifetime" but the error doesn't. There'd be no obvious way for a new user to connect the two.

or use qualification to make the meaning more clear

This is much more palatable to me. ❤️

@Florob

This comment has been minimized.

Florob commented Aug 4, 2017

I'm still a bit concerned about NLL, because to me it actually makes reasoning about references a lot harder. Specifically where a reference can be used without invalidating the program is no longer straight forward to answer. In particular the fact that introducing a use of a reference can invalidate a previously valid use of a referent feels like unfortunate error-at-a-distance behavior.

That said, the benefits probably still outweigh this concern by far.

@vorner

This comment has been minimized.

vorner commented Aug 4, 2017

Hello

I like it, these annoying things bite me from time to time (I guess it's less often now, because I avoid the pitfalls unconsciously already). I'm not an expert in the area, and I didn't follow every argument to the ground, but I think there are two small places that could be extended a bit:

  • The rules in section 5 (the borrow checker) could use some arguments about why they are sound. While I see why the NLLs themselves are sound, in this area I have just a vague feeling that it is probably OK, some hints to help me actually see it would be great.
  • I think the relation with explicit drop implementation might be a little confusing to the user. As I understand the proposal, a drop of type with a destructor counts as a use at the end of the block, but that use is invisible. This invisible use will „extend“ the lifetime until the end of the block (which will be the same as we have now). I believe the 3-points error message will explain it, it probably just needs a special case for the „invisible“ drop use.

Eg, this'll compile:

struct X<'a> {
  val: &'a usize,
}

{
  let x = 42;
  let y = X { val: &x };
  x = 50;
}

However, it'll stop compiling if a drop implementation is added to X.

@Ericson2314

This comment has been minimized.

Contributor

Ericson2314 commented Aug 7, 2017

fn main() -> ! {
  let x: usize;
  let y: &'static x = &x;
  loop { }
}

Actually, makes perfect sense to me based on the CFG-node contextuality of of the subtyping rules. Furthermore, this would be potentially very useful for embedded programming CC @japaric.

Yes it is unsound in the presence of concurrency right now, but I'd love to delve into trying to make it work. IMO just automatically throwing it out would be like just throwing out ! as a type in original Rust---convenient and not necessarily a mistake given limited resources, but "ex falso quodlibet oddities" usually have something profound to teach.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Aug 7, 2017

Yes it is unsound in the presence of concurrency right now,

Is it?

@Ericson2314

This comment has been minimized.

Contributor

Ericson2314 commented Aug 7, 2017

@RalfJung well the scoped threads example below---which I missed at first :). But actually maybe not; I dislike the premise of "the guard must be dead because its destructor is never run"---I think we should have a Guard<'static>, and then the loop in the forker is invalid.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Aug 7, 2017

This thing doesn't terminate, so I don't see a conflict with scoped threads. In fact I think I would prove this function safe in the RustBelt formal model.

EDIT: I assume you mean this? Not sure what the problem is with that example; there's too much left implicit for me to follow @nikomatsakis here. Where is 'static even coming up?

@xfix

This comment has been minimized.

Contributor

xfix commented Aug 7, 2017

@Ericson2314

I honestly wouldn't mind myself if this example would work, however there is an issue with unwinds which can be used as alternative means of escaping a function - not in this particular example, of course, but in practical examples of this. Even a simple integer addition can unwind in Rust, which is problematic for examples more complex than just loop {}.

This seems safe without unwind, but having borrowck depend on whether panic is unwind or not doesn't sound like a good idea - whether code would compile would depend on global compilation options.

@Ericson2314

This comment has been minimized.

Contributor

Ericson2314 commented Aug 7, 2017

I think we should take this to a separate issue---I do really like that the RFC-repo idea to avoid a mega thread.

@xfix Well, I've spilled much ink on features that benefit from no-unwind-specific borrow checking https://internals.rust-lang.org/t/a-stateful-mir-for-rust/3596 so it wouldn't be the first time I crossed that bridge :D.

@arielb1

This comment has been minimized.

Contributor

arielb1 commented Aug 9, 2017

@RalfJung

The infinite loop example here:

let mut foo = 22;

unsafe {
  // dtor joins the thread
  let _guard = thread::spawn(|foo| {
    loop {
      // in other thread: continuously read from `foo`
      println!("{}", foo);
    }
  );
  // we assume `_guard` is not live here, because why not?
  loop {
    foo += 1;
  }
  // unreachable drop of `_guard` joins the thread
}

Is unsound because both the main and the spawned thread can access foo - note that guard is never dropped.

However, that API is unsafe anyway, because of mem::forget:

let scope = Scope::new();
let mut foo = 22;

unsafe {
  // dtor joins the thread
  mem::forget(thread::spawn(|foo| {
    loop {
      // in other thread: continuously read from `foo`
      println!("{}", *foo);
    }
  ));
  
  // there's no guard, the gates are open, the threads are spinning
  loop {
    foo += 1;
  }
}
@Ericson2314

This comment has been minimized.

Contributor

Ericson2314 commented Aug 9, 2017

@arielb1 nikomatsakis/nll-rfc#35 I forgot to link it back here.

@Nashenas88

This comment has been minimized.

Nashenas88 commented Aug 23, 2017

@Florob, what if there was a way to actively visualize the lifetime while you were editing? My original visualizer idea is somewhat invalidated by this RFC, but this seems like only a small deviation from what I was working on. Imagine the following workflow:

  1. write code until you hit a lifetime inference/borrow error
  2. turn on the visualization of scopes/lifetimes/borrows (for selected items of course, turning it on for everything would overload the visuals)
  3. modify the code to immediately see how it affects the spans of the regions

I imagine such a model would make it easier (not necessarily easy) for someone to learn to understand the new system.

@vorner, you reminded me of an idea I've had a few times. What if we showed the user the drop statements that get injected, and point to those when explaining the issues with outlives (doesn't help with named lifetimes). Quite a few times I've gotten confused when the error description seems to point to the exact same spot, the final closing brace of the block. E.g.:

struct X<'a> {
    val: &'a usize,
}

{
    let x = 42;
    let y = X { val: &x };
    //               ^~ borrowed here, etc.
    x = 50;
    //  ^~ modified here, etc.
    // Some output to distinguish that this is compiler generated and not user input
    drop(y);
    //   ^ but must live until here, etc.
    // Some output to distinguish that this is compiler generated and not user input
}
@xfix

This comment has been minimized.

Contributor

xfix commented Aug 31, 2017

@agalakhov

Not really in the provided fn do<'t>(&mut self) -> SomeEnum<'t> case (although I think it could be more cleanly written as fn do(&mut self) -> SomeEnum<'static>). There is nothing saying that SomeEnum<'t> depends on self here, so it doesn't.

Of course, if a result can possibly borrow a value, Rust compiler has to assume that it... uh... possibly can borrow a value. A function whose return value lifetime doesn't depend on input type lifetime can explicitly specify 'static lifetime. Also, it's otherwise possible to prove that if match entered a particular branch, there is no way a value was actually borrowed within this branch, for instance in a following program. I don't think this RFC handles this particular issue however.

extern crate rand;

use std::borrow::Cow;

fn main() {
    let mut input = String::from("Example");
    // Cow<'input, str>, where 'input is lifetime of input variable
    let mut cow = if rand::random() {
        input.as_str().into()
    } else {
        String::from("Unrelated owned string").into()
    };
    match cow {
        Cow::Borrowed(input) => println!("Borrowed: {}", input),
        Cow::Owned(ref mut owned) => {
            input += " is safe, as it's known now that Cow doesn't borrow";
            println!("{}", input);

            *owned += " that was modified";
            println!("Anyway, owned: {}", owned);
        }
    }
    println!(
        "Cow is still accessible (owned or borrowed) and holds this value: {}",
        cow
    );
}
@agalakhov

This comment has been minimized.

agalakhov commented Aug 31, 2017

To be more precise: consider a result of &mut and code like that:

let result: Result<&mut Something, Error> = do(&mut obj); // internally Ok(&mut obj.something)
match result {
    Ok(reference) => {
        // `obj` is (and is expected to be) borrowed as `reference`
    }
    Err(error) => {
        // I don't expect `obj` to be borrowed here
    }
}
@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 6, 2017

@rfcbot fcp merge

I move that we merge this RFC. There are still some outstanding questions to be resolved, but I think that nobody has raised an objection to the core ideas at play. I think we can work through the remaining unresolved questions as we implement and before stabilization:

  • Which borrows are valid during a drop? nikomatsakis/nll-rfc#40
    • @pnkfelix and I have an idea that he is supposed to be writing up and/or prototyping (he can fill us in with his status) that builds on the #[may_dangle] attribute
    • but, in any case, we'll work something out
  • How to integrate liveness with moves? nikomatsakis/nll-rfc#42
    • we can make the analysis more precise by disregarding drops of a variable that is guaranteed to not be initialized
    • this doesn't seem to be particularly tricky, we just haven't written up the analysis in question
  • Best way to describe free regions nikomatsakis/nll-rfc#36
    • I revised the rfc with a new approach and did a proof-of-concept implementation in prototype
    • I didn't close the existing issue yet -- I wanted to polish up the prototype impl a bit -- but it seemed like there was general agreement.
  • Integrating two-phase borrows nikomatsakis/nll-rfc#37
    • This is not described in the RFC, but does not seem to be tricky
    • (The whole idea of two-phase borrows was to be easily integrated)
  • Whether to "embrace infinite loops" nikomatsakis/nll-rfc#35
    • Also a relatively minor question that doesn't drastically affect the RFC.

Prior to stabilizing, clearly, we should resolve these questions (and I will add them to the Unresolved Questions section).

@rfcbot

This comment has been minimized.

rfcbot commented Sep 6, 2017

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@alexanderjsummers

This comment has been minimized.

alexanderjsummers commented Sep 8, 2017

I've been reading the proposal (which generally sounds like an excellent thing for the language, and is mostly very clear). After some discussions with @vakaras here, I have some smaller suggestions about various parts the document (I'll make PRs/issues) but the main queries we have are about Layer 5 (how this fits into the borrow checker). In particular, the last of the three rules for the "transfer function" doesn't seem to be correct, based on our understanding so far.

This rule states that:

  • if this is an assignment lv = <rvalue>, then any loan for some path P
    of which lv is a prefix is killed.

For example, considering the following code:

struct Cell {
  value: u32,
}

fn f(x : Cell, y : Cell)
  let c: &mut Cell = &x;   // (0) borrow on x
  let v = &mut (*c).value; // (1) borrow on x's field
  c = &y; // (2) reassign c to borrow on y
  use(x); // (3) not allowed; v is still in scope
  use(v);
  use(c);

I expect (and get, from the prototype) an error telling me that using x is not OK. But the third rule above suggests that the loan created on line (1) goes out of scope on line (2) (where we assign to c, which is a prefix of the path borrowed on line (1)). If the loan indeed goes out of scope here (as would the loan created on line (0), according to the same rule), how is the borrow checker meant to generate the appropriate error on line (3)?

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 8, 2017

@alexanderjsummers

First, some nits. I am adjusting the example to get the mut matching correct. =)

struct Cell {
  value: u32,
}

fn f(x : Cell, y : Cell)
  let c: &mut Cell = &mut x;   // (0) borrow on x
  let v = &mut (*c).value; // (1) borrow on x's field
  c = &mut y; // (2) reassign c to borrow on y
  use(x); // (3) not allowed; v is still in scope
  use(v);
  use(c);
}

Now, onto your question:

But the third rule above suggests that the loan created on line (1) goes out of scope on line (2)

I'm not sure if the bug is in the text or your interpretation of it, but let me explain what I expect to happen and we can decide. =) First off, the assignment c = &y does indeed clear the loan of (*c).value. However, the loan of x from line 0 is still in scope and is not cleared. This results in an error on line (2), because we are attempting to use x but it is still borrowed (from line 0). However, we would be able to use (*c).value, because that loan was cleared. Is this the behavior you saw from the prototype? (It would be helpful, btw, to see the .nll file that you created, presuming you did create one.)

Note also that region inference comes as a first phase, before this rule is applied. This means that the lifetime of v (and hence, indirectly, the lifetime of c) will extend down below.

(Something about this setup bugs me a bit; I have wondered if it would make sense to integration the borrowck with the region inference step. But for now, that's not how it is specified.)

@Ericson2314

This comment has been minimized.

Contributor

Ericson2314 commented Sep 8, 2017

One belated bike-shed on terminology: We've used "non-lexical" for a while now, but I worry that people will take it as a synonym for "dynamic", influenced by "lexical vs dynamic scope". I think it's important for learning to understand that this a still a static system, in that a given CFG node is always in the same unchanging, known set of lifetimes. That staticness in my view makes both learning NLL and debugging code with it far far easier.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 8, 2017

@Ericson2314

@arielb1 also raised the question of finding another name for this proposal, though I tend to think that the current name is "ok", since this isn't a feature that people will "interact with" for the most part (i.e., once its in, it'll just be "lifetimes" -- a term with which I have some issues anyhow, but that's another story!).

@alexanderjsummers

This comment has been minimized.

alexanderjsummers commented Sep 8, 2017

@nikomatsakis

Thanks a lot for the speedy clarifications. As you might have guessed, the example had gone through a little by-hand massaging, which obviously wasn't totally error-proof.. I've copied our original file at the end of this post.

I think we mixed up the lvalue and rvalue for the first loan, which is why we had the idea that it gets cancelled too. Now it seems clear how we can get the error we do, with respect to the given rules. What I find a bit counter-intuitive is that the notion of loans considered in scope (with respect to the discussion in the RFC) doesn't correspond with the notion of borrows which are still active, if I understand correctly. For example, at point (2), as you've explained, the second loan gets cleared. But the borrowed reference v which it (at least in my head) corresponds to is still in scope, and indeed will get used later. It's this borrow which ultimately causes the error, because x can't be used until it actually expires. Of course, this is still achieved via the fact that the first loan's lifetime gets extended, but it seems conceptually a bit confusing to me that the scope of the borrow and the scope of the corresponding loan are different. Not sure if that makes any sense, but I hope you see what I mean.

Here's the original code and interaction with the prototype. By the way, is the information as to which loans get cancelled reported in some way? The regions reported seem to correspond to the scope of the actual borrowed references, and not the loans.

struct List<+> {
  value: 0,
}

let x: List<()>;
let y: List<()>;
let list: &'list mut List<()>;
let v: &'v mut ();

block START {
    x = use();
    y = use();
    list = &'b1 mut x;
    v = &'b2 mut (*list).value;
    list = &'b3 mut y;
    use(x);
    use(v);
    use(list);
}

// With use(x):

// Message: ../test/example.nll: point START/5 cannot read Var(Variable { name: "x" }) because Var(Variable { name: "x" }) is mutably borrowed (at point `START/2`)

//  Loan {
//      point: START/2,
//      path: Var(Variable { name: "x" }),
//      kind: Mut,
//      region: {START/3, START/4, START/5, START/6, START/7} }
//  Loan {
//      point: START/3,
//      path: Extension(Extension(Var(Variable { name: "list" }), FieldName { name: "*" }), FieldName { name: "value" }),
//      kind: Mut,
//      region: {START/4, START/5, START/6} }
//  Loan {
//      point: START/4,
//      path: Var(Variable { name: "y" }),
//      kind: Mut,
//      region: {START/5, START/6, START/7} }

// If we re-run with use(x) commented out (which shifts some line numbers):

//  Loan {
//      point: START/2,
//      path: Var(Variable { name: "x" }),
//      kind: Mut,
//      region: {START/3, START/4, START/5, START/6} }
//  Loan {
//      point: START/3,
//      path: Extension(Extension(Var(Variable { name: "list" }), FieldName { name: "*" }), FieldName { name: "value" }),
//      kind: Mut,
//      region: {START/4, START/5} }
//  Loan {
//      point: START/4,
//      path: Var(Variable { name: "y" }),
//      kind: Mut,
//      region: {START/5, START/6} }

//  assert 'list == {START/3, START/4, START/5, START/6};
//  assert 'v == {START/4, START/5};
//  assert 'b1 == {START/3, START/4, START/5, START/6};
//  assert 'b2 == {START/4, START/5};
//  assert 'b3 == {START/5, START/6};

Also, as a (possibly-unrelated) side-question, I'm not sure why the region for the first loan contains START/7 - it seems that that value of list is dead, and the lifetime of that borrow shouldn't extend beyond the penultimate statement.

@arielb1

This comment has been minimized.

Contributor

arielb1 commented Sep 10, 2017

By the way, is the information as to which loans get cancelled reported in some way?

What gets cancelled on an assignment is not loans that the overwritten variable refer to, its loans that refer to the overwritten value (and are now "orphaned"). e.g. in your code:

struct List<+> {
  value: 0,
}

let x: List<()>;
let y: List<()>;
let list: &'list mut List<()>;
let v: &'v mut ();

block START {
    x = use();
    y = use();
    list = &'b1 mut x;
//  This creates a lifetime constraint where `list` (and therefore the borrow
//  of `x`) must live as long as `v` is.
    v = &'b2 mut (*list).value;
//  (*list).value is now borrowed (and "dynamically" refers to x.value), so e.g.
//  use(&list); //~ ERROR cannot use `list` while `(*list).value` is borrowed
    list = &'b3 mut y;
//  (*list).value now refers to a completely different thing (y.value), and
//  is therefore no longer borrowed. The old borrow for `(*list).value` is
//  now "orphaned" - it is not accessible by any path derived from the old
//  path. The lifetime constraint still remains.

//  use(x); // ignore this, I don't want an error
    use(&list); // non-consuming use, in any order
    use(v);
    use(list);
}
@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 11, 2017

@alexanderjsummers

Of course, this is still achieved via the fact that the first loan's lifetime gets extended, but it seems conceptually a bit confusing to me that the scope of the borrow and the scope of the corresponding loan are different. Not sure if that makes any sense, but I hope you see what I mean.

Indeed, I do understand what you meant, and I too find this interaction somewhat surprising, though it seems to by and large "do the trick" (i.e., it mostly accepts the programs I want it to, and seems valid). I feel like there may be a more elegant way to express it. This is what I was trying to refer to when I wrote:

(Something about this setup bugs me a bit; I have wondered if it would make sense to integration the borrowck with the region inference step. But for now, that's not how it is specified.)

As @arielb1 points out, though, what's really happening here is that we are eliminated a kind of "false sharing" (to abuse a term). That is, we have on record a loan of *v, but since v now points somewhere else, that loan of course doesn't make any sense.

One could imagine instead using a kind of SSA-like-rewriting to achieve a similar effect, at least for simple cases like local variables; but it'd be more work to do the same for paths like v.foo, and I'm not sure just what the net effect would look like. In general, I've preferred not to use SSA, so that the analysis is closer to the original program. SSA is also less ideal for cases where you can update a variable via indirect write (e.g., let p = &mut x; *p += 1; *p += 1; changes x twice, but how do we account for that in the numbering?), though I think the strictness of the borrow checker kind of makes that largely a non-issue, since you can't access x while it is being changed in this way.

Anyway, I'd be not be opposed to exploring such alternate formulations, though I think we need not block on it. To my mind, what's being RFC'd here are the high-level concepts: the details of the checks we perform will be tweaked over time no doubt.

(Though I would like the RFC and prototype to serve as the basis for a reference document and implementation that we can keep up to date.)

@alexanderjsummers

This comment has been minimized.

alexanderjsummers commented Sep 11, 2017

@arielb1 and @nikomatsakis

Thanks a lot for the further details and clarifications. I'm no longer worried that there's a technical issue here - the rules indeed seem to do the "right thing". If the idea is to evolve this document into documentation later, my suggestion would be to pick a different name from "loan" if possible, for what is tracked (but maybe this terminology is fixed already).

My conceptual understanding of borrowed references has always been that the references take some capabilities from each other, and that these are for a particular location, and a particular lifetime.
As I understand it now (I think much better!) the notion of borrow and the notion of loan are two different things with different durations - the borrow is (or can be consistently thought of as) on the location itself, and it persists for the chosen lifetime of the borrow (this is consistent with what happens in the example). The loan struct, on the other hand, tracks something which is dependent on a particular path to that location, and can persist for a different (I suppose, only shorter) length of time. To me, the two words themselves mean the same thing, but it seems that here they describe subtly different concepts. If the loan concept is also more "internal", in the sense that it only shows up when understanding the borrow checker, maybe it could get a less similar (but arguably less pretty) name, such as PathLoan? It seems to me that this is more of an auxiliary concept which is used as a medium for enforcing the desired invariants about the borrowed references themselves. Of course, this intuition is probably highly subjective, and if it seems inaccurate then please let me know.

I guess the conversion to SSA would make the distinction between the borrows and loans more explicit, since the rules for cancelling loans would be tied to distinct syntactic paths in the program, but I guess the distinction (which was what I was missing in my first comment) would still be the same in the end. Maybe converting to SSA for the sake of an example could be a(n alternative) way of explaining how the borrow checker rules can be thought about in terms of why they make sense, but I guess it would just be for illustration if so, and as you say, for general paths it might not be all that pretty.

@nikomatsakis I also find it interesting to think about whether these phases could somehow feedback to each other meaningfully - it doesn't yet seem clear to me why this kind of phased checking naturally arises, but it indeed seems to achieve the right results, too! I might ponder this a bit and ping you an email if I think I have any sensible thoughts.

I have a few smaller suggestions about the document, in terms of things I would find helpful to clarify. Is it best to comment here, make pull requests (but in some cases I don't actually know what I would propose yet), or add issues etc.? Or indeed, keep them to myself? :)

@PhilipDaniels

This comment has been minimized.

PhilipDaniels commented Sep 11, 2017

If you are still looking for a name for the concept, perhaps "control flow lifetimes" would be apt?

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 11, 2017

@alexanderjsummers

I have a few smaller suggestions about the document, in terms of things I would find helpful to clarify. Is it best to comment here, make pull requests (but in some cases I don't actually know what I would propose yet), or add issues etc.? Or indeed, keep them to myself? :)

I'd say open PRs or open issues on the nll-rfc repo.

@rfcbot

This comment has been minimized.

rfcbot commented Sep 18, 2017

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

@rfcbot

This comment has been minimized.

rfcbot commented Sep 28, 2017

The final comment period is now complete.

@aturon

This comment has been minimized.

Member

aturon commented Sep 29, 2017

🔔 THIS RFC HAS BEEN MERGED! 🔔

Tracking issue.

@aturon aturon merged commit aa80b68 into rust-lang:master Sep 29, 2017

@mgattozzi

This comment has been minimized.

Member

mgattozzi commented Sep 29, 2017

This has been a long time coming! :D Congrats to Niko and the compiler team for all their hardwork on getting this right!

@00imvj00

This comment has been minimized.

00imvj00 commented Oct 31, 2017

so, when it will be available in stable rust ? or nightly rust to play around with? @nikomatsakis

@golddranks

This comment has been minimized.

golddranks commented Oct 31, 2017

@0freeman00 The approved RFCs have always a link to a tracking issue that has information about the implementation. Check this out: rust-lang/rust#44928

@pravic

This comment has been minimized.

pravic commented Nov 9, 2017

The feature name is not filled which broke the RFC book a bit: https://rust-lang.github.io/rfcs/2094-nll.html (note nil there).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment