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

nikomatsakis · 2017-10-25T22:41:42Z

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 · 2017-10-25T22:41:54Z

r? @pnkfelix

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

aturon · 2017-10-26T02:31:41Z

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

kennytm · 2017-10-26T05:54:53Z

src/librustc_mir/transform/nll/subtype.rs

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


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

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

kennytm · 2017-10-26T05:56:47Z

src/test/mir-opt/nll/region-liveness-drop-no-may-dangle.rs

+
+// 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 needs // ignore-tidy-linelength.

pnkfelix · 2017-10-26T16:54:54Z

src/librustc_mir/util/liveness.rs

            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


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

pnkfelix · 2017-10-26T17:26:34Z

src/librustc_mir/util/liveness.rs

+            }
+
+            ///////////////////////////////////////////////////////////////////////////
+            // DROP USES

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


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 ...?

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.

pnkfelix · 2017-10-26T17:32:19Z

src/test/mir-opt/nll/region-liveness-two-disjoint-uses.rs

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


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?

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

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

pnkfelix · 2017-10-26T17:40:46Z

src/test/mir-opt/nll/region-subtyping-basic.rs

+
+// 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]}


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

pnkfelix · 2017-10-26T17:48:17Z

src/librustc_mir/borrow_check.rs

 }

 #[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>,


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...

pnkfelix · 2017-10-26T17:50:24Z

This is epic!

🥇

pnkfelix · 2017-10-26T17:51:54Z

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

nikomatsakis · 2017-10-30T13:47:18Z

@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.

spastorino · 2017-10-30T13:57:05Z

@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 · 2017-10-30T14:01:33Z

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

nikomatsakis · 2017-10-30T17:15:54Z

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

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.

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.

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 · 2017-10-31T19:54:08Z

@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 · 2017-10-31T19:54:08Z

📌 Commit aae3e74 has been approved by pnkfelix

badboy · 2017-10-31T20:06:12Z

@spastorino Wuhu! Congrats on pushing this through!

bors · 2017-11-01T18:14:19Z

⌛ Testing commit aae3e74 with merge 2be4cc0...

@spastorino

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 · 2017-11-01T21:09:55Z

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

mgattozzi · 2017-11-02T02:57:07Z

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

@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

rust-highfive assigned pnkfelix Oct 25, 2017

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 26, 2017

kennytm reviewed Oct 26, 2017

View reviewed changes

Manishearth mentioned this pull request Oct 26, 2017

Tracking issue for RFC #2094: non-lexical lifetimes #44928

Closed

nikomatsakis force-pushed the nll-liveness branch 3 times, most recently from 708476a to 5d0992c Compare October 26, 2017 14:32

pnkfelix reviewed Oct 26, 2017

View reviewed changes

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 26, 2017

pnkfelix reviewed Oct 26, 2017

View reviewed changes

pnkfelix added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 26, 2017

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 27, 2017

spastorino force-pushed the nll-liveness branch from 738180d to 14a078d Compare October 27, 2017 16:35

nikomatsakis force-pushed the nll-liveness branch from 4573191 to 34ebd74 Compare October 30, 2017 13:04

nikomatsakis force-pushed the nll-liveness branch from 34ebd74 to fd2bdeb Compare October 30, 2017 13:48

nikomatsakis mentioned this pull request Oct 30, 2017

Add derive and doc comment capabilities to newtype_index macro #45605

Merged

nikomatsakis added 12 commits October 31, 2017 12:41

make MirSource::from_local_def_id invokable with 'gcx and 'tcx

5aa1cbf

make closure_self_ty invokable with 'gcx and 'tcx

29cdd73

reorder 'gcx and 'tcx in BorrowckErrors impl

b2c248e

test "needs drop" on region-erased, lifted types

82b287a

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

8144917

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.

connect MIR borrowck with NLL

a94b01a

change region display to '_#Nr, update the newtype_index! macro

f27eb1e

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`.

WIP patch compile-fail/nll/region-ends-after-if-condition.rs

bf57a23

remove the NLL pass (it is now invoked by mir borrowck)

7675ea9

runtest: fix mir directory and delete outdated MIR files

99aa313

treat -Znll as implying -Zborrowck-mir

d9fb792

rewrite write_mir_fn_graphviz to cope with non-global tcx

9b3af6c

nikomatsakis force-pushed the nll-liveness branch from bf7e83d to 9b3af6c Compare October 31, 2017 16:56

patch mir-opt reference files

aae3e74

nikomatsakis mentioned this pull request Oct 31, 2017

NLL: refine liveness with "maybe initialized" analysis #45665

Closed

nikomatsakis mentioned this pull request Oct 31, 2017

extend NLL with preliminary support for free regions on functions #45668

Merged

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 1, 2017

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

This was referenced Nov 1, 2017

Immovable types prototype where the Move trait is builtin #44917

Closed

MIR-borrowck: add permisson checks to fn access_lvalue #45436

Merged

pnkfelix mentioned this pull request Jul 18, 2018

Problem with borrow checker and AddAssign #52126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nikomatsakis commented Oct 25, 2017 •

edited

Loading

rust-highfive commented Oct 25, 2017

aturon commented Oct 26, 2017

kennytm Oct 26, 2017

spastorino Oct 26, 2017

kennytm Oct 26, 2017

spastorino Oct 26, 2017

pnkfelix Oct 26, 2017

pnkfelix Oct 26, 2017

nikomatsakis Oct 26, 2017

pnkfelix Oct 26, 2017 •

edited

Loading

spastorino Oct 26, 2017

nikomatsakis Oct 26, 2017

pnkfelix Oct 26, 2017 •

edited

Loading

pnkfelix Oct 26, 2017

nikomatsakis Oct 26, 2017

nikomatsakis Oct 31, 2017

pnkfelix commented Oct 26, 2017

pnkfelix commented Oct 26, 2017

nikomatsakis commented Oct 30, 2017

spastorino commented Oct 30, 2017

nikomatsakis commented Oct 30, 2017

nikomatsakis commented Oct 30, 2017

nikomatsakis commented Oct 31, 2017

bors commented Oct 31, 2017

badboy commented Oct 31, 2017

bors commented Nov 1, 2017

bors commented Nov 1, 2017

mgattozzi commented Nov 2, 2017

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

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

Conversation

nikomatsakis commented Oct 25, 2017 • edited Loading

rust-highfive commented Oct 25, 2017

aturon commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Oct 26, 2017

pnkfelix commented Oct 26, 2017

nikomatsakis commented Oct 30, 2017

spastorino commented Oct 30, 2017

nikomatsakis commented Oct 30, 2017

nikomatsakis commented Oct 30, 2017

nikomatsakis commented Oct 31, 2017

bors commented Oct 31, 2017

badboy commented Oct 31, 2017

bors commented Nov 1, 2017

bors commented Nov 1, 2017

mgattozzi commented Nov 2, 2017

nikomatsakis commented Oct 25, 2017 •

edited

Loading

pnkfelix Oct 26, 2017 •

edited

Loading

pnkfelix Oct 26, 2017 •

edited

Loading