NLL: improve inference with flow results, represent regions with bitsets, and more #46319

nikomatsakis · 2017-11-27T22:37:57Z

This PR begins with a number of edits to the NLL code and then includes a large number of smaller refactorings (these refactorings ought not to change behavior). There are a lot of commits here, but each is individually simple. The goal is to land everything up to but not including the changes to how we handle closures, which are conceptually more complex.

The NLL specific changes are as follows (in order of appearance):

Modify the region inferencer's approach to free regions. Previously, for each free region (lifetime parameter) 'a, it would compute the set of other free regions that 'a outlives (e.g., if we have where 'a: 'b, then this set would be {'a, 'b}). Then it would mark those free regions as "constants" and report an error if inference tried to extend 'a to include any other region (e.g., 'c) that is not in that outlives set. In this way, the value of 'a would never grow beyond the maximum that could type check. The new approach is to allow 'a to grow larger. Then, after the fact, we check over the value of 'a and see what other free regions it is required to outlive, and we check that those outlives relationships are justified by the where clauses in scope etc.

Modify constraint generation to consider maybe-init. When we have a "drop-live" variable x (i.e., a variable that will be dropped but will not be otherwise used), we now consider whether x is "maybe initialized" at that point. If not, then we know the drop is a no-op, and we can allow its regions to be dead. Due to limitations in the fragment code, this currently only works at the level of entire variables.

Change representation of regions to use a BitMatrix. We used to use a BTreeSet, which was rather silly. We now use a MxN matrix of bits, where M is the number of variables and N is the number of possible elements in each set (size of the CFG + number of free regions).

The remaining commits (starting from
extract the implied_bounds code into a helper function ") are all "no-op" refactorings, I believe.

One concern I have is with the commit "with -Zverbose, print all details of closure substs"; this commit seems to include some "internal" stuff in the mir-dump files, such as internal interner numbers, that I fear may vary by platform. Annoying. I guess we will see. (I removed this commit.)

As for reviewer, @arielb1 has been reviewing the PRs, and they are certainly welcome to review this one too. But I figured it'd maybe be good to have more people taking a look and being familiar with this code, so I'll "nominate" @pnkfelix .

r? @pnkfelix

bors · 2017-11-28T02:12:57Z

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

bors · 2017-12-01T16:03:21Z

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

bors · 2017-12-03T15:34:56Z

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

pnkfelix · 2017-12-04T11:30:37Z

src/librustc/middle/free_region.rs

            self.relation.add(sub, sup)
        }
    }

-    /// True if `r_a <= r_b` is known to hold. Both `r_a` and `r_b`
-    /// must be free regions from the function header.
+    /// Tests whether `r_a <= sup`. Both must be free regions or


... did you mean to alpha-rename r_b to sup in the signature below? Why change the name in the comment?

pnkfelix · 2017-12-04T11:43:19Z

src/librustc_mir/transform/nll/region_infer.rs

+        // and hence we establish (transitively) a constraint that
+        // `'a: 'b`. The `propagate_constraints` code above will
+        // therefore add `end('a)` into the region for `'b` -- but we
+        // have no evidence that `'b` outlives `'a`, so we want to report


I think you mean to write either:

"'a outlives 'b" or

"'b is a subregion of 'a"

pnkfelix · 2017-12-04T11:51:36Z

src/librustc_mir/transform/nll/region_infer.rs

+
+        // Find every region `o` such that `fr: o`
+        // (because `fr` includes `end(o)`).
+        for &outlived_fr in &fr_value.free_regions {


I dislike the name outlived_fr (at least in this context, where there is another variable named fr).

I'm spending a lot of time staring at the name, wondering: does this name mean "I outlive(d?) the region named fr"? Or does the name mean "I am a free region (abbreviated "fr") that is outlived (by another free region that happens to be named "fr")?"

(You would think that the comment would clarify this, but since I know from 1:1 discussions and also comments elsewhere in this PR that niko sometimes mixes up the argument order for 'a: 'b.)

alternative name suggestions:

must_be_outlived

lower_bound

subregion

pnkfelix · 2017-12-04T11:57:20Z

hmm apparently my strategy of giving comments commit-by-commit may be suboptimal for this particular commit series, since the points I had in earliest commits have been completely subsumed?

Update: no, my strategy was sound; but github's not smart enough to recognize some kinds of code migrations. So I needed to do a second pass over my review and transcribe most of the "outdated" comments onto the diff, where they are attached to the up-to-date locations in the code.

pnkfelix · 2017-12-04T12:09:39Z

src/librustc_mir/borrow_check/mod.rs

-    );
-    let flow_uninits = do_dataflow(
+    ));
+    let flow_uninits = FlowInProgress::new(do_dataflow(


sort of a shame that some of these changes which strike me as refactorings, like pulling the FlowInProgress::new calls out of InProgress::new (which I assume were necessitated by lifetime issues...) were not factored into a separate commit, to ease identification of the specific semantic changes being made in this commit...

(but since I took forever to get around to the review, I won't block this PR on requesting such changes...)

It will be useful later for diagnostics to be able to remember where things were live.

Rather than declaring some region variables to be constant, and reporting errors when they would have to change, we instead populate each free region X with a minimal set of points (the CFG plus end(X)), and then we let inference do its thing. This may add other `end(Y)` points into X; we can then check after the fact that indeed `X: Y` holds. This requires a bit of "blame" detection to find where the bad constraint came from: we are currently using a pretty dumb algorithm. Good place for later expansion.

In particular, if we see a variable is DROP-LIVE, but it is not MAYBE-INIT, then we can ignore the drop. This leavess attempt to use more complex refinements of the idea (e.g., for subpaths or subfields) to future work.

This should be more efficient than allocating two BTreeSets for every region variable?—as it is written in rust-lang#45670.

This revealed some shortcomings, one of which is fixed. Fixes rust-lang#45937.

Now it can be reused by the NLL code.

We now visit just the stuff in the CFG, and we add liveness constraints for all the random types, regions etc that appear within rvalues and statements.

pnkfelix · 2017-12-04T14:26:19Z

src/librustc_mir/borrow_check/nll/region_infer.rs

+    /// A map from each MIR Location to its column index in
+    /// `liveness_constraints`/`inferred_values`. (The first N columns are
+    /// the free regions.)
+    point_indices: BTreeMap<Location, usize>,


... hmm, maybe at some point we'll want an IndexBitMatrix analogous to our IndexVec (where I suppose it would have two type parameters instead of just one), so that we could use an index type here instead of usize... But, that need not be part of this PR.

I have done a big refactoring of this (in particular, encapsulating the BitMatrix) on nll-master, but I'm not inclined to bother rebasing it back earlier at this point.

pnkfelix · 2017-12-04T14:30:07Z

src/librustc/infer/outlives/env.rs

+    ) where
+        I: IntoIterator<Item = OutlivesBound<'tcx>>,
+    {
+        // But also record other relationships, such as `T:'x`,


This comment as written doesn't make sense in this new context.

In the old code, it came after let implied_bounds = infcx.implied_bounds(...), so it made sense to open with "But also record ..."; here, we are just jumping directly into this loop...

pnkfelix · 2017-12-04T14:31:25Z

src/librustc/infer/outlives/env.rs

+    /// Processes outlives bounds that are known to hold, whether from implied or other sources.
+    ///
+    /// The `infcx` parameter is optional; if the implied bounds may
+    /// contain inference variables, it should be supplied, in which


I personally would say "it must be supplied" (rather than "should"), since the code below isn't going to just ignore the variables in that case, but rather, it will panic when the implied bounds contain inference variables.

pnkfelix · 2017-12-04T14:36:39Z

src/librustc_data_structures/transitive_relation.rs

+    /// Given an element A, returns the maximal set {B} of elements B
+    /// such that
+    ///
+    /// - A != A


you probably want to write "A != B" here. 😉

pnkfelix · 2017-12-04T15:08:52Z

src/librustc/infer/outlives/free_region_map.rs

+        }
+    }
+
+    /// Tests whether `r_a <= sup`. Both must be free regions or


Transcribed from bfc696a : ... did you mean to alpha-rename r_b to sup in the signature below? Why change the name in the comment?

pnkfelix · 2017-12-04T15:11:11Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

+        // and hence we establish (transitively) a constraint that
+        // `'a: 'b`. The `propagate_constraints` code above will
+        // therefore add `end('a)` into the region for `'b` -- but we
+        // have no evidence that `'b` outlives `'a`, so we want to report


Transcribed from bfc696a :

I think you mean to write either:

"have no evidence that 'a outlives 'b" or

"have no evidence that 'b is a subregion of 'a"

(after all, in the code itself, we are in a situation where we need the region on the input parameter to outlive the region on the output return type.)

pnkfelix · 2017-12-04T15:12:15Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

+
+        // Find every region `o` such that `fr: o`
+        // (because `fr` includes `end(o)`).
+        for outlived_fr in fr_value.take_while(|&i| i < self.num_universal_regions) {


Transcribed from bfc696a :

I dislike the name outlived_fr (at least in this context, where there is another variable named fr).

I'm spending a lot of time staring at the name, wondering: does this name mean "I outlive(d?) the region named fr"? Or does the name mean "I am a free region (abbreviated "fr") that is outlived (by another free region that happens to be named "fr")?"

(You would think that the comment would clarify this, but since I know from 1:1 discussions and also comments elsewhere in this PR that niko sometimes mixes up the argument order for 'a: 'b.)

alternative name suggestions:

must_be_outlived

lower_bound

subregion

pnkfelix · 2017-12-04T15:15:20Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

+    /// A map from each MIR Location to its column index in
+    /// `liveness_constraints`/`inferred_values`. (The first N columns are
+    /// the free regions.)
+    point_indices: BTreeMap<Location, usize>,


Transcribed from 22b3175 :

.. hmmm, maybe at some point we'll want an IndexBitMatrix analogous to our IndexVec (where I suppose it would have two type parameters instead of just one), so that we could use an index type here instead of usize... But, that need not be part of this PR.

pnkfelix · 2017-12-04T15:18:12Z

src/librustc/infer/outlives/env.rs

+    /// Processes outlives bounds that are known to hold, whether from implied or other sources.
+    ///
+    /// The `infcx` parameter is optional; if the implied bounds may
+    /// contain inference variables, it should be supplied, in which


Transcribed from #46319 (comment) since github's algorithm for marking comments "outdated" is not quite smart enough to recognize when code has just been moved around...

I personally would say "it must be supplied" (rather than "should"), since the code below isn't going to just ignore the variables in that case, but rather, it will panic when the implied bounds contain inference variables.

pnkfelix · 2017-12-04T15:19:51Z

Okay I left various notes with nits on comments.

But the code itself seems fine. r=me once you've addressed my comments (or decided to disregard them ...)

nikomatsakis · 2017-12-04T17:27:42Z

@bors r=pnkfelix p=1

Tests pass locally. I'm giving p=1 because NLL is a top priority and we need to clear way for next PR.

bors · 2017-12-04T17:27:43Z

📌 Commit a6adc74 has been approved by pnkfelix

bors · 2017-12-04T19:07:58Z

⌛ Testing commit a6adc74 with merge 8503b3f...

@arielb1

…kfelix NLL: improve inference with flow results, represent regions with bitsets, and more This PR begins with a number of edits to the NLL code and then includes a large number of smaller refactorings (these refactorings ought not to change behavior). There are a lot of commits here, but each is individually simple. The goal is to land everything up to but not including the changes to how we handle closures, which are conceptually more complex. The NLL specific changes are as follows (in order of appearance): **Modify the region inferencer's approach to free regions.** Previously, for each free region (lifetime parameter) `'a`, it would compute the set of other free regions that `'a` outlives (e.g., if we have `where 'a: 'b`, then this set would be `{'a, 'b}`). Then it would mark those free regions as "constants" and report an error if inference tried to extend `'a` to include any other region (e.g., `'c`) that is not in that outlives set. In this way, the value of `'a` would never grow beyond the maximum that could type check. The new approach is to allow `'a` to grow larger. Then, after the fact, we check over the value of `'a` and see what other free regions it is required to outlive, and we check that those outlives relationships are justified by the where clauses in scope etc. **Modify constraint generation to consider maybe-init.** When we have a "drop-live" variable `x` (i.e., a variable that will be dropped but will not be otherwise used), we now consider whether `x` is "maybe initialized" at that point. If not, then we know the drop is a no-op, and we can allow its regions to be dead. Due to limitations in the fragment code, this currently only works at the level of entire variables. **Change representation of regions to use a `BitMatrix`.** We used to use a `BTreeSet`, which was rather silly. We now use a MxN matrix of bits, where `M` is the number of variables and `N` is the number of possible elements in each set (size of the CFG + number of free regions). The remaining commits (starting from extract the `implied_bounds` code into a helper function ") are all "no-op" refactorings, I believe. ~~One concern I have is with the commit "with -Zverbose, print all details of closure substs"; this commit seems to include some "internal" stuff in the mir-dump files, such as internal interner numbers, that I fear may vary by platform. Annoying. I guess we will see.~~ (I removed this commit.) As for reviewer, @arielb1 has been reviewing the PRs, and they are certainly welcome to review this one too. But I figured it'd maybe be good to have more people taking a look and being familiar with this code, so I'll "nominate" @pnkfelix . r? @pnkfelix

bors · 2017-12-04T21:46:09Z

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

rust-highfive assigned pnkfelix Nov 27, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 28, 2017

nikomatsakis force-pushed the nll-master-to-rust-master-2 branch 2 times, most recently from 2160a5a to 1f8cc00 Compare December 1, 2017 10:13

nikomatsakis force-pushed the nll-master-to-rust-master-2 branch from 1f8cc00 to 105e8a3 Compare December 2, 2017 02:24

pnkfelix referenced this pull request in nikomatsakis/rust Dec 4, 2017

add comments to region_infer, restructure a bit

bfc696a

pnkfelix reviewed Dec 4, 2017

View reviewed changes

nikomatsakis and others added 16 commits December 4, 2017 08:25

separate out the liveness constraints from the final value

932452e

It will be useful later for diagnostics to be able to remember where things were live.

borrow_check.rs: rustfmt

08c8d7e

inform constraint generation using maybe-init

a9cb25b

In particular, if we see a variable is DROP-LIVE, but it is not MAYBE-INIT, then we can ignore the drop. This leavess attempt to use more complex refinements of the idea (e.g., for subpaths or subfields) to future work.

region_infer: BitMatrix representation of region values

22b3175

This should be more efficient than allocating two BTreeSets for every region variable?—as it is written in rust-lang#45670.

extract the implied_bounds code into a helper function

aa51603

move free_regions_map into infer::outlives

abd7d88

make resolve_regions_and_report_errors take an OutlivesEnv

5562663

This revealed some shortcomings, one of which is fixed. Fixes rust-lang#45937.

make no_late_bound_regions a method on Binder<T>

1f33145

extract the code to create OutlivesBounds into its own module

c45307f

Now it can be reused by the NLL code.

rename implied_bounds module to bounds

31d61f1

break type-checking of aggregate-kind out into helper function

45f6027

rename greater_than to reachable_from

fb4b06a

promote region_infer into its own module

663e7a4

extend TransitiveRelation with parents function

4de7336

constraint_generation: create liveness constraints more thoroughly

49d2274

We now visit just the stuff in the CFG, and we add liveness constraints for all the random types, regions etc that appear within rvalues and statements.

type_check.rs: rustfmt

c1708ab

pnkfelix reviewed Dec 4, 2017

View reviewed changes

nikomatsakis force-pushed the nll-master-to-rust-master-2 branch from 105e8a3 to c1708ab Compare December 4, 2017 14:37

pnkfelix reviewed Dec 4, 2017

View reviewed changes

nikomatsakis added 6 commits December 4, 2017 10:36

free_region_map: Fix typo in comment: r_a <= r_b

14480b4

region_infer: Fix typo in comment

a393596

outlives/env: Fix comment that lost surrounding context.

7409ffd

transtive_relation: fix typo in comment for parents

2716160

outlives/env: fix comment, say must and not should

ab2bc9f

adopt longer and shorter rather than fr and outlived_fr

a6adc74

nikomatsakis force-pushed the nll-master-to-rust-master-2 branch from 4a1bc10 to a6adc74 Compare December 4, 2017 16:30

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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 4, 2017

bors merged commit a6adc74 into rust-lang:master Dec 4, 2017

bors mentioned this pull request Dec 4, 2017

MIR borrowck: implement union-and-array-compatible semantics #46268

Merged

nikomatsakis deleted the nll-master-to-rust-master-2 branch December 5, 2017 09:34

eddyb mentioned this pull request Mar 25, 2020

MIR dump prints the location of each statement as a comment #70400

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NLL: improve inference with flow results, represent regions with bitsets, and more #46319

NLL: improve inference with flow results, represent regions with bitsets, and more #46319

nikomatsakis commented Nov 27, 2017 •

edited

Loading

bors commented Nov 28, 2017

bors commented Dec 1, 2017

bors commented Dec 3, 2017

pnkfelix Dec 4, 2017

pnkfelix Dec 4, 2017

pnkfelix Dec 4, 2017 •

edited

Loading

pnkfelix Dec 4, 2017

pnkfelix commented Dec 4, 2017 •

edited

Loading

pnkfelix Dec 4, 2017

pnkfelix Dec 4, 2017

pnkfelix Dec 4, 2017

nikomatsakis Dec 4, 2017

pnkfelix Dec 4, 2017

pnkfelix Dec 4, 2017 •

edited

Loading

pnkfelix Dec 4, 2017

pnkfelix Dec 4, 2017

pnkfelix Dec 4, 2017

pnkfelix Dec 4, 2017

pnkfelix Dec 4, 2017 •

edited

Loading

pnkfelix Dec 4, 2017 •

edited

Loading

pnkfelix commented Dec 4, 2017

nikomatsakis commented Dec 4, 2017

bors commented Dec 4, 2017

bors commented Dec 4, 2017

bors commented Dec 4, 2017

NLL: improve inference with flow results, represent regions with bitsets, and more #46319

NLL: improve inference with flow results, represent regions with bitsets, and more #46319

Conversation

nikomatsakis commented Nov 27, 2017 • edited Loading

bors commented Nov 28, 2017

bors commented Dec 1, 2017

bors commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Dec 4, 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

pnkfelix Dec 4, 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

pnkfelix Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

pnkfelix Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

pnkfelix commented Dec 4, 2017

nikomatsakis commented Dec 4, 2017

bors commented Dec 4, 2017

bors commented Dec 4, 2017

bors commented Dec 4, 2017

nikomatsakis commented Nov 27, 2017 •

edited

Loading

pnkfelix Dec 4, 2017 •

edited

Loading

pnkfelix commented Dec 4, 2017 •

edited

Loading

pnkfelix Dec 4, 2017 •

edited

Loading

pnkfelix Dec 4, 2017 •

edited

Loading

pnkfelix Dec 4, 2017 •

edited

Loading