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

enable non-lexical lifetimes in the MIR borrow checker #45538

Merged
merged 33 commits into from Nov 1, 2017

Conversation

Projects
None yet
9 participants
@nikomatsakis
Contributor

nikomatsakis commented Oct 25, 2017

This PR, joint work with @spastorino, fills out the NLL infrastructure and integrates it with the borrow checker. Don't get too excited: it includes still a number of hacks (the subtyping code is particularly hacky). However, it does kinda' work. =)

The final commit demonstrates this by including a test that -- with both the AST borrowck and MIR borrowck -- reports an error by default. But if you pass -Znll, you only get an error from the AST borrowck, demonstrating that the integration succeeds:

struct MyStruct {
    field: String
}

fn main() {
    let mut my_struct = MyStruct { field: format!("Hello") };

    let value = &my_struct.field;
    if value.is_empty() {
        my_struct.field.push_str("Hello, world!");
        //~^ ERROR cannot borrow (Ast)
    }
}
@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 25, 2017

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@aturon

This comment has been minimized.

Member

aturon commented Oct 26, 2017

Congrats, both of you! Been awesome seeing you power through this.

@@ -0,0 +1,89 @@
use super::RegionIndex;

This comment has been minimized.

@kennytm

kennytm Oct 26, 2017

Member

Tidy error 😏

[00:03:02] tidy error: /checkout/src/librustc_mir/transform/nll/subtype.rs: incorrect license
[00:03:02] tidy error: /checkout/src/test/mir-opt/nll/region-liveness-drop-no-may-dangle.rs:48: line longer than 100 chars
[00:03:03] some tidy checks failed

This comment has been minimized.

@spastorino

spastorino Oct 26, 2017

Member

Thanks @kennytm I've already fixed this in my local machine, will push in a while :)

// END RUST SOURCE
// START rustc.node12.nll.0.mir
// | R4: {bb1[3], bb1[4], bb1[5], bb2[0], bb2[1], bb2[2], bb3[0], bb4[0], bb4[1], bb4[2], bb6[0], bb7[0], bb7[1], bb8[0]}

This comment has been minimized.

@kennytm

kennytm Oct 26, 2017

Member

This needs // ignore-tidy-linelength.

This comment has been minimized.

@spastorino

spastorino Oct 26, 2017

Member

Also this.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:nll-liveness branch 3 times, most recently from 708476a to 5d0992c Oct 26, 2017

LvalueContext::Store |
// We let Call defined the result in both the success and unwind cases.
// This may not be right.
// We let Call defined the result in both the success and

This comment has been minimized.

@pnkfelix

pnkfelix Oct 26, 2017

Member

typo: should be "we let Call define the result"

}
///////////////////////////////////////////////////////////////////////////
// DROP USES
// We consider drops to always be uses of locals.
// Drop eloboration should be run before this analysis otherwise

This comment has been minimized.

@pnkfelix

pnkfelix Oct 26, 2017

Member

This note about when drop "eloboration" should run (which came from cbdb186) ... it applied to @Zoxc's move analysis, but does it apply to our uses here for NLL? I ask because I am pretty sure we want borrowck to run before drop elaboration ...?

This comment has been minimized.

@nikomatsakis

nikomatsakis Oct 26, 2017

Contributor

The comment is really kind of outdated. We do not want to run drop elaboration before the check -- and we are ok with drops being considered uses.

// END RUST SOURCE
// START rustc.node12.nll.0.mir
// | R0: {bb1[1], bb2[0], bb2[1]}

This comment has been minimized.

@pnkfelix

pnkfelix Oct 26, 2017

Member

would it be better to print the region identifiers here as: _#0r:, _#2r:, _#3r: (or some uniform suffix of those) to make more clear the correspondence with their appearances in the borrows?

This comment has been minimized.

@spastorino

spastorino Oct 26, 2017

Member

To be honest, at the beginning I have a hard time figuring this output out, so ... maybe going through this and making it more clear is a good things to do. /cc @nikomatsakis

This comment has been minimized.

@nikomatsakis

nikomatsakis Oct 26, 2017

Contributor

I thought about that. I guess we might as well adopt the existing _#0r, even though it's horrible.

// END RUST SOURCE
// START rustc.node12.nll.0.mir
// | R0: {bb1[1], bb1[2], bb1[3], bb1[4], bb1[5], bb1[6], bb2[0], bb2[1]}

This comment has been minimized.

@pnkfelix

pnkfelix Oct 26, 2017

Member

hmm, do we not have an explicit notation for where R0 (aka '_#0r, right?) arises, somewhere in the MIR dump?

}
#[allow(dead_code)]
pub struct MirBorrowckCtxt<'c, 'b, 'a: 'b+'c, 'gcx: 'a+'tcx, 'tcx: 'a> {
tcx: TyCtxt<'a, 'gcx, 'gcx>,
mir: &'b Mir<'gcx>,
tcx: TyCtxt<'a, 'gcx, 'tcx>,

This comment has been minimized.

@pnkfelix

pnkfelix Oct 26, 2017

Member

niko can i bribe you with some halloween candy to break out this semi-mechanical lifetime-parameter refactoring (or generalization, if you prefer) into a separate commit? This commt is pretty hard to digest the way it is currently set up...

This comment has been minimized.

@nikomatsakis

nikomatsakis Oct 26, 2017

Contributor

Twix.

This comment has been minimized.

@nikomatsakis

nikomatsakis Oct 31, 2017

Contributor

Done btw.

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Oct 26, 2017

This is epic!

🥇

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Oct 26, 2017

my questions and nits are all pretty minor.

It might be a good idea to see if @arielb1 wants to take a look too, but he might be on vacation

@spastorino spastorino force-pushed the nikomatsakis:nll-liveness branch from 738180d to 14a078d Oct 27, 2017

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:nll-liveness branch from 4573191 to 34ebd74 Oct 30, 2017

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Oct 30, 2017

@pnkfelix addressed feedback. I also rebased and cleaned up the history, incorporating a number of changes from @spastorino. Have to figure out those travis failures though.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:nll-liveness branch from 34ebd74 to fd2bdeb Oct 30, 2017

@spastorino

This comment has been minimized.

Member

spastorino commented Oct 30, 2017

@nikomatsakis some of my commits are lost, related to the tests and fixes we were doing last night. Is that included in the code you've pushed? should I push that again?.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Oct 30, 2017

@spastorino I don't believe they were lost, rather I merged them into the appropriate places.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Oct 30, 2017

Those travis failures are curious. I don't see them locally, on mac or linux. :/

nikomatsakis added some commits Oct 26, 2017

test "needs drop" on region-erased, lifted types
This will be important in next commit, since the input types will be
tagged not with `'gcx` but rather `'tcx`. Also, using the region-erased,
lifted types enables better caching.
make the dataflow / mir-borrowck types carry a `'tcx` lifetime
Also, factor out `do_mir_borrowck`, which is the code that actually
performs the MIR borrowck from within the scope of an inference context.

This change should be a pure refactoring.
change region display to `'_#Nr`, update the `newtype_index!` macro
The macro now takes a format string. It no longer defaults to using the
type name. Didn't seem worth going through contortions to maintain.  I
also changed most of the debug formats to be `foo[N]` instead of `fooN`.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:nll-liveness branch from bf7e83d to 9b3af6c Oct 31, 2017

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Oct 31, 2017

@bors r=pnkfelix

I'm going to go ahead and land this so as to unblock further work. It ought not to deeply interact with any "enabled by default" code, so it's low risk.

@bors

This comment has been minimized.

Contributor

bors commented Oct 31, 2017

📌 Commit aae3e74 has been approved by pnkfelix

@badboy

This comment has been minimized.

Member

badboy commented Oct 31, 2017

@spastorino Wuhu! Congrats on pushing this through!

@bors

This comment has been minimized.

Contributor

bors commented Nov 1, 2017

⌛️ Testing commit aae3e74 with merge 2be4cc0...

bors added a commit that referenced this pull request Nov 1, 2017

Auto merge of #45538 - nikomatsakis:nll-liveness, r=pnkfelix
enable non-lexical lifetimes in the MIR borrow checker

This PR, joint work with @spastorino, fills out the NLL infrastructure and integrates it with the borrow checker. **Don't get too excited:** it includes still a number of hacks (the subtyping code is particularly hacky). However, it *does* kinda' work. =)

The final commit demonstrates this by including a test that -- with both the AST borrowck and MIR borrowck -- reports an error by default. But if you pass `-Znll`, you only get an error from the AST borrowck, demonstrating that the integration succeeds:

```
struct MyStruct {
    field: String
}

fn main() {
    let mut my_struct = MyStruct { field: format!("Hello") };

    let value = &my_struct.field;
    if value.is_empty() {
        my_struct.field.push_str("Hello, world!");
        //~^ ERROR cannot borrow (Ast)
    }
}
```
@bors

This comment has been minimized.

Contributor

bors commented Nov 1, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing 2be4cc0 to master...

@bors bors merged commit aae3e74 into rust-lang:master Nov 1, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@mgattozzi

This comment has been minimized.

Member

mgattozzi commented Nov 2, 2017

It's finally here! :D Well part of it but still! :D

bors added a commit that referenced this pull request Nov 6, 2017

Auto merge of #45668 - nikomatsakis:nll-free-region, r=arielb1
extend NLL with preliminary support for free regions on functions

This PR extends #45538 with support for free regions. This is pretty preliminary and will no doubt want to change in various ways, particularly as we add support for closures, but it's enough to get the basic idea in place:

- We now create specific regions to represent each named lifetime declared on the function.
- Region values can contain references to these regions (represented for now as a `BTreeSet<RegionIndex>`).
- If we wind up trying to infer that `'a: 'b` must hold, but no such relationship was declared, we report an error.

It also does a number of drive-by refactorings.

r? @arielb1

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