MIR borrow check (under debug flag) #43108

pnkfelix · 2017-07-07T13:57:51Z

Here is the current state of MIR borrow check.

It consists of (1.) some refactoring, (2.) a dataflow analysis to identify the borrows themselves, and (3.) a mir "transform" that does the borrow check itself based on the aforementioned dataflow results.

(There's also a drive-by fix to dataflow that I can factor into a separate PR if necessary. Interestingly I could not find a way to observe the bug outside of MIR borrowck.)

To be clear, this branch is not ready to be used as the default borrow check. Thus the code is guarded: To get mir-borrowck to run, you need to either supply an attribute #[rustc_mir_borrowck] or a debug flag -Z borrowck-mir.

Here are the main issues with the current MIR borrowck as it stands in this PR:

No Notes emitted yet, just errors. (So the feedback is definitely inferior compared to AST borrowck today)
Lvalue rendering differs between Ast and Mir. (Mostly minor, but replacement of field names with indices is very bad; big priority for me to fix ASAP.)
Lots of ICEs (presumably because some MIR operations used here have well-formedness assumptions that are violated in borrowck-broken code)
Conflates lots of cases that are distinguished by AST-borrowck
Conflates "uninitialized" with "moved" (special case of previous bullet, one that I think should be fixed ASAP)

(I am hoping to fix as many of the above issues as I can in the near term, but I also would like to land this even if they are not all fixed, because the rebasing effort is getting to be a real drag.)

rust-highfive · 2017-07-07T13:57:55Z

r? @arielb1

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

pnkfelix · 2017-07-07T14:01:29Z

Here is a gist that summarizes how behavior differs on a number of test cases. I post this mostly to make it very clear that while some things are definitely working, there remains some serious work to do: MIR borrowck status

pnkfelix · 2017-07-07T14:04:54Z

Oh, one thing that I just realized I should double check, and change if I haven't already made this so.

Currently, to make it easy to compare the results, when you opt-in to MIR-borrowck, I actually run both MIR-borrowck and AST-borrowck.

I have tried to set things up so that errors from MIR-borrowck get emitted with a suffix "(Mir)" in the output, and errors from AST-borrowck get emitted with a suffix "(Ast)".

While I think the above is great for while this is in development, its no good to expose this distinction to end users. So I need to ensure that the "(Ast)" suffix is only emitted if one has MIR-borrowck turned on.

nikomatsakis

first round of comments

nikomatsakis · 2017-07-11T19:39:52Z

src/librustc_driver/driver.rs

@@ -934,6 +934,7 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(sess: &'tcx Session,
    passes.push_pass(MIR_CONST, mir::transform::rustc_peek::SanityCheck);

    // What we need to run borrowck etc.
+    passes.push_pass(MIR_CONST, mir::transform::borrow_check::BorrowckMir);


I think borrowck should be the last thing in the MIR_VALIDATED passes. It should come after we qualify constants (since that is needed to get an accurate picture of what data has 'static lifetime). (i.e., if we write &(3+4), that -- per the rvalue lifetimes RFC -- should be a &'static i32).

That said, I think I was expecting borrowck to be its own query -- but maybe it's better to just have it be a pass. (If we do make it its own query, we would have to make sure it gets forced by the "optimize" stuff, though I think that already happens...?)

So.. why would typeck be a pass while borrowck is a query?

Well, one reason is that borrowck produces output that other things consume. If nothing else, it produces data about which mut declarations are used that is later gathered up by the lints.

With respect to typeck, I expect it to be eventually subsumed, most likely, into the region inference code, which will be a part of borrowck, and hence to go away as a distinct entity.

But I think we can work al this out as we go. No need to block anything on it.

So.. why would typeck be a pass while borrowck is a query?

MIR typeck ATM is basically a hacky sanity check. I imagine MIR borrowck/regionck will subsume it.

nikomatsakis · 2017-07-11T19:41:05Z

src/librustc_mir/dataflow/impls/borrows.rs

+    mir: &'a Mir<'tcx>,
+    borrows: IndexVec<BorrowIndex, BorrowData<'tcx>>,
+    loc_map: FxHashMap<Location, BorrowIndex>,
+    rgn_map: FxHashMap<Region<'tcx>, FxHashSet<BorrowIndex>>,


Nit: I'd prefer region_map and location_map

nikomatsakis · 2017-07-11T19:43:01Z

src/librustc_mir/dataflow/impls/borrows.rs

+        });
+        match stmt.kind {
+            mir::StatementKind::EndRegion(extent) => {
+                for idx in self.rgn_map.get(&ReScope(extent)).unwrap_or_else(|| {


Nit: extract self.rgn_map.get(...) into a temporary, I think, this for loop is kind of hard to read

nikomatsakis · 2017-07-11T19:52:52Z

src/librustc_mir/dataflow/impls/borrows.rs

+            mir::StatementKind::Assign(_, ref rhs) => {
+                if let mir::Rvalue::Ref(region, _, _) = *rhs {
+                    let loc = location;
+                    let idx = self.loc_map.get(&loc).unwrap_or_else(|| {


Nit: location, index

nikomatsakis · 2017-07-11T19:53:08Z

src/librustc_mir/dataflow/impls/borrows.rs

+            mir::StatementKind::SetDiscriminant { .. } |
+            mir::StatementKind::StorageLive(..) |
+            mir::StatementKind::StorageDead(..) |
+            mir::StatementKind::Nop => {}


Yay, exhaustive!

carols10cents · 2017-07-17T12:30:20Z

Ping @nikomatsakis to get this in your queue for next time you're reviewing PRs (I know you're traveling) :)

nikomatsakis · 2017-07-17T13:48:46Z

Well, I am not "assigned" this PR, I was just doing some drive-by reviewing, but I will be happy to try and push it through. I expect @arielb1 will provide very useful reviews though =) he tends to have a keener eye than me for flaws.

alexcrichton · 2017-07-20T18:40:21Z

ping r? @arielb1

carols10cents · 2017-07-24T15:31:55Z

ping @arielb1 @rust-lang/compiler for review!

arielb1 · 2017-07-24T16:04:02Z

Back from vacation! I'll review this as soon as I'll finish catching up on stuff.

bors · 2017-07-24T23:02:28Z

☔ The latest upstream changes (presumably #43454) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2017-07-27T23:26:08Z

Just another traige ping for you @arielb1

aidanhs · 2017-08-03T19:06:10Z

@pnkfelix unfortunately it looks like this needs a rebase!

arielb1 · 2017-08-08T09:37:48Z

I'm done with a bunch of other stuff. Maybe I'll review it tonight.

arielb1 · 2017-08-10T18:33:41Z

src/librustc_mir/util/borrowck_errors.rs

+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+pub enum Origin { Ast, Mir }
+
+impl fmt::Display for Origin {


Don't you want AST borrowck to format as the empty string and MIR borrowck to format as (Mir), at least for the time being?

Hmm apparently I never pushed up the changes to render Origin::Ast as empty string. Will address pronto. (My plan had been to have the empty string rendering be conditional, so that when one passes -Z borrowck-mir, you get a symmetry between the error messages in terms of whether they say "... (Ast)" or "... (Mir)".)

(Update: well, I did push my intended change in 09f8040c8ddaddb257c6f652258d22a0a901a1a8 ... so now I'm trying to why the PR's diff is not showing the effects of that change...)

Or, maybe I misinterpreted the point of @arielb1's comment? Perhaps the idea is that errors from MIR borrowck should unconditionally include the "... (Mir)", regardless of what flags are set? That sounds good to me.

arielb1 · 2017-08-10T18:51:26Z

src/librustc_driver/driver.rs

@@ -934,6 +934,7 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(sess: &'tcx Session,
    passes.push_pass(MIR_CONST, mir::transform::rustc_peek::SanityCheck);

    // What we need to run borrowck etc.
+    passes.push_pass(MIR_CONST, mir::transform::borrow_check::BorrowckMir);
    passes.push_pass(MIR_VALIDATED, mir::transform::qualify_consts::QualifyAndPromoteConstants);
    passes.push_pass(MIR_VALIDATED,


I see a SimplifyBranches pass here within the MIR_VALIDATED pipeline. It's important that this won't run before borrowck, because otherwise code within an if false won't be checked.

I don't disagree with your assessment, but I am left somewhat confused as to how things end up in this state. Does this mean that SimplifyBranches should be in the MIR_OPTIMIZED pipeline instead of the MIR_VALIDATED one?

Hmm, I see. I guess niko's (and perhaps others' ) expectation was that borrowck would be injected in between MIR_VALIDATED and MIR_OPTIMIZED; but this PR as written is running its MIR borrowck right before the qualify_consts step, which does not match that expectation.

Still not 100% sure how we'll end up dealing with this. I'll add a FIXME regarding the conflict for now.

arielb1 · 2017-08-10T19:24:13Z

src/librustc_mir/dataflow/impls/borrows.rs

+                for idx in borrow_indexes { sets.kill(&idx); }
+            }
+
+            mir::StatementKind::Assign(_, ref rhs) => {


Any consideration of assign/borrow interactions? Or does current borrowck not deal with any of them, so that they have to wait for NLL?

My current thinking is to wait for NLL to address matters here.

(But even when we do, it more likely would go into transform::borrow_check, not dataflow::impls::borrows, right?)

The nice thing here is that everything is pretty much orthogonal - NLL determines where each borrow ends, 2φB when a borrow/lifetime starts - depending on the implementation, and borrows when they are active. We should probably place all of these features under the same feature gate, but there's no reason to implement them simultaneously.

The thing is, when we have something like

let mut t1 = 1; let mut t2 = 2; let mut x = &'α mut t1; let y = &'β mut *x; x = &mut y2; // this is an error today, but is actually sound // `*x` is no longer borrowed here (because of the assignment) u = (x, y); print(u); // - but α & β can't end until here (even with NLL)

The assignment to x "ends" all borrows to things pointed to by x - the borrows still exists, but the alias to them through x is inaccessible, which means there's no need to track accesses through it.

This is also required for the standard x = &mut (*x).next; pattern to work - if the borrow of (*x).next (actually for the old value of x) remained, that won't work the next time we go around the loop.

okay, i can see how dataflow::impls::borrows could be affected

arielb1 · 2017-08-10T19:25:58Z

src/librustc_mir/dataflow/impls/borrows.rs

+            }
+
+            mir::StatementKind::Assign(_, ref rhs) => {
+                if let mir::Rvalue::Ref(region, _, _) = *rhs {


Nice to see that this ended up so compact. I expected more... trouble.

arielb1 · 2017-08-13T16:31:27Z

src/librustc_mir/transform/borrow_check.rs

+                                    (rhs, span), location, flow_state);
+            }
+            StatementKind::SetDiscriminant { ref lvalue, variant_index: _ } => {
+                // FIXME: should this count as a mutate from borrowck POV?


Deaggregator always runs after borrowck, so this doesn't matter. But this should count as a mutate.

arielb1 · 2017-08-13T16:32:14Z

src/librustc_mir/transform/borrow_check.rs

+            StatementKind::Nop |
+            StatementKind::StorageLive(..) |
+            StatementKind::StorageDead(..) => {
+                // ignored by borrowck


StorageDead is not ignored by borrowck. It is what causes non-drop values to be dropped when they go out of scope.

Ah! That seems important.

arielb1 · 2017-08-13T16:41:43Z

src/librustc_mir/transform/borrow_check.rs

+                //
+                // FIXME: is above actually true? Do we want to track
+                // the fact that uninitialized data can be created via
+                // `NullOp::Box`?


That's done by gather_moves.

... especially with the changes added by #43772, right?

And even before that. Yeah.

arielb1 · 2017-08-13T16:57:59Z

src/librustc_mir/transform/borrow_check.rs

+            }
+        }
+
+        // does any loan generated here conflict with another loan also generated here?


Can this happen? If so, please comment.

My model is that a borrow is a "local" mutate_lvalue + a "global" loan. How is that incorrect?

Hmm. This loop was meant to be analogous to the one here in AST borrowck.

I was starting from an assumption that in general, a given statement in a control-flow graph could introduce multiple loans (and those can be mutually conflicting).

But now that you have questioned the presence of this code, I am experiencing some deja-vu, in that I am realizing once again that MIR is designed such that each borrow arises solely from an Rvalue::Ref, each of which in turn arises solely from a StatementKind::Assign (perhaps that is what you were getting at by pointing out that a borrow "is a mutate_lvalue" ...?), and thus there is at most one loan generated from any given point in the control flow graph.

So, that's good news; not only can this loop go away, but also the supporting code like the IdxSet::pairs method...

arielb1 · 2017-08-14T11:02:36Z

src/librustc_mir/dataflow/drop_flag_effects.rs

@@ -306,3 +237,36 @@ pub(crate) fn drop_flag_effects_for_location<'a, 'tcx, F>(
        }
    }
 }
+
+fn propagate_assignment<'a, 'tcx, F>(tcx: TyCtxt<'a, 'tcx, 'tcx>,


this will be killed by rebase.

(confirmed; that whole commit goes away)

arielb1 · 2017-08-14T11:13:12Z

The infrastructure looks ok. I'll r+ after a rebase and then we'll go to make it perfect.

kennytm · 2017-08-16T12:20:52Z

src/librustc_mir/transform/borrow_check.rs

+                            ty::TyRawPtr(_) => {
+                                return false;
+                            }
+                            ty::TyRef(_/*rgn*/, ty::TypeAndMut { ty: _, mutbl: hir::MutImmutable }) => {


[00:03:55] tidy error: /checkout/src/librustc_mir/transform/borrow_check.rs:771: line longer than 100 chars [00:03:55] tidy error: /checkout/src/librustc_mir/transform/borrow_check.rs:777: line longer than 100 chars [00:03:55] tidy error: /checkout/src/librustc_mir/transform/borrow_check.rs:835: line longer than 100 chars [00:03:55] tidy error: /checkout/src/librustc_mir/transform/borrow_check.rs:968: line longer than 100 chars [00:03:55] tidy error: /checkout/src/librustc_mir/dataflow/mod.rs:50: line longer than 100 chars [00:03:56] some tidy checks failed

…ter reuse by mir borrowck). post-rebase: Do not put "(Ast)" suffix in error msg unless passed `-Z borrowck-mir`. (But unconditionally include "(Mir)" suffix for mir-borrowck errors.)

…ock`/`usize` argument pairs.

post-rebase: addressed review comment: rename `loc_map`/`location_map` and `rgn_map`/`region_map`. post-rebase: remove now unnecessary `mut` decl. post-rebase: address comments: bind iterator expr, and alpha-rename `loc`/`location` and `idx`/`index`.

…parent module. (This code is more general purpose than just supporting drop flag elaboration.)

…parent module. Refactored `each_bit`, which traverses a `IdxSet`, so that the bulk of its implementation lives in `rustc_data_structures`.

One can either use `-Z borrowck-mir` or add the `#[rustc_mir_borrowck]` attribute to opt into MIR based borrow checking. Note that regardless of whether one opts in or not, AST-based borrow check will still run as well. The errors emitted from AST-based borrow check will include a "(Ast)" suffix in their error message, while the errors emitted from MIR-based borrow check will include a "(Mir)" suffix. post-rebase: removed check for intra-statement mutual conflict; replaced with assertion checking that at most one borrow is generated per statement. post-rebase: removed dead code: `IdxSet::pairs` and supporting stuff.

…taflow`.

… in. Post-rebase: ariel confirmed `SetDiscriminant` should indeed be a mutate.

…unning dtors) relevant to borrowck.

Added two fixmes: The `SimplifyBranches` pass cannot stay where it is, and `BorrowckMir` should be a query, not a pass. But I am going to leave those changes to a future PR.

pnkfelix · 2017-08-16T15:29:43Z

@bors r=arielb1

bors · 2017-08-16T15:29:44Z

📌 Commit 8738a08 has been approved by arielb1

bors · 2017-08-16T15:36:06Z

⌛ Testing commit 8738a08 with merge 00a6797...

MIR borrow check (under debug flag) Here is the current state of MIR borrow check. It consists of (1.) some refactoring, (2.) a dataflow analysis to identify the borrows themselves, and (3.) a mir "transform" that does the borrow check itself based on the aforementioned dataflow results. (There's also a drive-by fix to dataflow that I can factor into a separate PR if necessary. Interestingly I could not find a way to observe the bug outside of MIR borrowck.) To be clear, this branch is not ready to be used as the default borrow check. Thus the code is guarded: To get mir-borrowck to run, you need to either supply an attribute `#[rustc_mir_borrowck]` or a debug flag `-Z borrowck-mir`. Here are the main issues with the current MIR borrowck as it stands in this PR: * No Notes emitted yet, just errors. (So the feedback is definitely inferior compared to AST borrowck today) * Lvalue rendering differs between Ast and Mir. (Mostly minor, but replacement of field names with indices is very bad; big priority for me to fix ASAP.) * Lots of ICEs (presumably because some MIR operations used here have well-formedness assumptions that are violated in borrowck-broken code) * Conflates lots of cases that are distinguished by AST-borrowck * Conflates "uninitialized" with "moved" (special case of previous bullet, one that I think should be fixed ASAP) (I am hoping to fix as many of the above issues as I can in the near term, but I also would like to land this even if they are *not* all fixed, because the rebasing effort is getting to be a real drag.)

bors · 2017-08-16T18:24:54Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 00a6797 to master...

Mir borrowck as query Turn the `mir-borrowck` pass (aka "transform") into a query. (If I had realized how relatively easy this was going to be, I would have made it part of #43108. `let hindsight = 20/20;`)

rust-highfive assigned arielb1 Jul 7, 2017

shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 7, 2017

pnkfelix force-pushed the mir-borrowck3c branch from 58f9bb8 to b7b7003 Compare July 7, 2017 14:48

nikomatsakis reviewed Jul 12, 2017

View reviewed changes

pnkfelix force-pushed the mir-borrowck3c branch from b7b7003 to d4c368b Compare July 12, 2017 14:07

nikomatsakis self-assigned this Jul 17, 2017

rust-highfive unassigned nikomatsakis Jul 20, 2017

aidanhs 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 Aug 3, 2017

arielb1 reviewed Aug 10, 2017

View reviewed changes

arielb1 reviewed Aug 13, 2017

View reviewed changes

arielb1 reviewed Aug 14, 2017

View reviewed changes

pnkfelix force-pushed the mir-borrowck3c branch 2 times, most recently from ff07dec to 0aefd7d Compare August 16, 2017 10:46

kennytm reviewed Aug 16, 2017

View reviewed changes

pnkfelix added 12 commits August 16, 2017 15:52

Move borrowck error msg construction to module in rustc_mir (for la…

8e79fc7

…ter reuse by mir borrowck). post-rebase: Do not put "(Ast)" suffix in error msg unless passed `-Z borrowck-mir`. (But unconditionally include "(Mir)" suffix for mir-borrowck errors.)

Refactor trait BitDenotation to take Location instead of `BasicBl…

cff060b

…ock`/`usize` argument pairs.

Migrated some code out of dataflow::drop_flag_effects and into its …

e8ebc90

…parent module. (This code is more general purpose than just supporting drop flag elaboration.)

remove now unnecessary constraint on BitDenotation for fn do_dataflow.

3ef1afc

Move DataFlowState::{each_bit,interpret_set} method definitions to …

9b8b8c6

…parent module. Refactored `each_bit`, which traverses a `IdxSet`, so that the bulk of its implementation lives in `rustc_data_structures`.

Factored MoveData construction code into builder submodule.

869f05a

Added some documentation for the struct BlockSets in `rustc_mir::da…

15c411d

…taflow`.

Cleanup: Every BitDenotation is a DataflowOperator, so build that…

b6528f0

… in. Post-rebase: ariel confirmed `SetDiscriminant` should indeed be a mutate.

Address review comment: StatementKind::StorageDead has an effect (r…

6d6280e

…unning dtors) relevant to borrowck.

Moved mir-borrowck pass down to where comments say it should be.

8738a08

Added two fixmes: The `SimplifyBranches` pass cannot stay where it is, and `BorrowckMir` should be a query, not a pass. But I am going to leave those changes to a future PR.

pnkfelix force-pushed the mir-borrowck3c branch from 0aefd7d to 8738a08 Compare August 16, 2017 14:10

bors merged commit 8738a08 into rust-lang:master Aug 16, 2017

bors mentioned this pull request Aug 16, 2017

Generator support #43076

Merged

7 tasks

ishitatsuyuki mentioned this pull request Aug 17, 2017

Make unused-extern-crate warn-by-default #42588

Merged

pnkfelix mentioned this pull request Aug 21, 2017

Mir borrowck as query #44009

Merged

pnkfelix mentioned this pull request Sep 15, 2017

MIR-borrowck: error diagnostics need notes #44596

Closed

MIR borrow check (under debug flag) #43108

MIR borrow check (under debug flag) #43108

Conversation

pnkfelix commented Jul 7, 2017

rust-highfive commented Jul 7, 2017

pnkfelix commented Jul 7, 2017

pnkfelix commented Jul 7, 2017

nikomatsakis left a 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

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

carols10cents commented Jul 17, 2017

nikomatsakis commented Jul 17, 2017

alexcrichton commented Jul 20, 2017

carols10cents commented Jul 24, 2017

arielb1 commented Jul 24, 2017

bors commented Jul 24, 2017

alexcrichton commented Jul 27, 2017

aidanhs commented Aug 3, 2017

arielb1 commented Aug 8, 2017

Choose a reason for hiding this comment

pnkfelix Aug 14, 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 Aug 14, 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

arielb1 Aug 14, 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

arielb1 Aug 13, 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

Choose a reason for hiding this comment

arielb1 Aug 13, 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

arielb1 commented Aug 14, 2017

Choose a reason for hiding this comment

pnkfelix commented Aug 16, 2017

bors commented Aug 16, 2017

bors commented Aug 16, 2017

bors commented Aug 16, 2017

pnkfelix Aug 14, 2017 •

edited

Loading

pnkfelix Aug 14, 2017 •

edited

Loading

arielb1 Aug 14, 2017 •

edited

Loading

arielb1 Aug 13, 2017 •

edited

Loading

arielb1 Aug 13, 2017 •

edited

Loading