[NLL] Get Polonius borrow check to work in simple cases #54468

matthewjasper · 2018-09-22T13:16:29Z

Restores the generation of outlives facts from subtyping.
Restore liveness facts.
Generate invalidates facts at the start point of each location,
where we check for errors.
Add a small test for simple cases (previously these cases have compiled, and more recently ICEd).

Closes #54212
cc #53142 (will need test)

Known limitations

Two phase borrows aren't implemented for Polonius yet
Invalidation facts haven't been updated for some of the recent changes to make Drop terminators access fewer things.
Fact generation is not as optimized as it could be.
Around 30 tests fail in compare mode, often tests that are ignored in nll compare mode

bors · 2018-09-23T04:10:47Z

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

nikomatsakis

Nice! Left a few comments.

src/librustc_mir/borrow_check/nll/invalidation.rs

nikomatsakis · 2018-09-25T16:29:58Z

src/librustc_mir/borrow_check/nll/type_check/constraint_conversion.rs

+        // In the new analysis, all outlives relations etc
+        // "take effect" at the mid point of the statement
+        // that requires them, so ignore the `at_location`.
+        if let Some(all_facts) = &mut self.all_facts {


Maybe it would be better if we just iterated over the outlives_constraints that we build up at the end? I forget though if there are outlives constraints we don't need for polonius.

nikomatsakis · 2018-09-25T16:32:34Z

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

@@ -1112,6 +1112,17 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {
                            .constraints
                            .liveness_constraints
                            .add_element(region_vid, term_location);
+
+                        if let Some(facts) = borrowck_context.all_facts {


These facts I believe are not needed in polonius — pretty sure I excluded them intentionally. We only care about the "transitive relations" that are formed via these late-bound regions. Did you find they were needed in particular?

nikomatsakis · 2018-09-25T16:33:03Z

src/librustc_mir/borrow_check/nll/type_check/relate_tys.rs

@@ -266,7 +266,19 @@ impl<'cx, 'bccx, 'gcx, 'tcx> TypeRelating<'cx, 'bccx, 'gcx, 'tcx> {
                    locations: self.locations,
                });

-            // FIXME all facts!
+            if let Some(all_facts) = borrowck_context.all_facts {
+                if let Some(from_location) = self.locations.from_location() {


this would it seems be caught better by just inspecting the final outlives relations and convering them (as I mentioned earlier), then we wouldn't have to repeat this code

matthewjasper · 2018-09-25T21:34:44Z

Comments addressed

bors · 2018-09-25T22:56:27Z

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

matthewjasper · 2018-09-26T19:02:04Z

rebased

bors · 2018-09-26T20:51:27Z

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

* Restores the generation of outlives facts from subtyping. * Restore liveness facts. * Generate invalidates facts at the start point of each location, where we check for errors. * Add a small test for simple cases.

nikomatsakis · 2018-09-27T18:24:44Z

@bors r+

bors · 2018-09-27T18:24:44Z

📌 Commit 610903f has been approved by nikomatsakis

bors · 2018-09-27T19:27:05Z

⌛ Testing commit 610903f with merge 8876906...

@nikomatsakis

[NLL] Get Polonius borrow check to work in simple cases * Restores the generation of outlives facts from subtyping. * Restore liveness facts. * Generate invalidates facts at the start point of each location, where we check for errors. * Add a small test for simple cases (previously these cases have compiled, and more recently ICEd). Closes #54212 cc #53142 (will need test) ### Known limitations * Two phase borrows aren't implemented for Polonius yet * Invalidation facts haven't been updated for some of the recent changes to make `Drop` terminators access fewer things. * Fact generation is not as optimized as it could be. * Around 30 tests fail in compare mode, often tests that are ignored in nll compare mode r? @nikomatsakis

bors · 2018-09-27T22:07:54Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 8876906 to master...

@matthewjasper

Polonius: fix some cases of `killed` fact generation, and most of the `ui` test suite Since basic Polonius functionality was re-enabled by @matthewjasper in rust-lang#54468, some tests were still failing in the polonius compare-mode. This PR fixes all but one test in the `ui` suite by: - fixing some bugs in the fact generation code, related to the `killed` relation: Polonius would incorrectly reject some NLL-accepted code, because of these missing `killed` facts. - ignoring some tests in the polonius compare-mode: a lot of those manually test the NLL or migrate mode, and the failures were mostly artifacts of the test revisions, e.g. that `-Z polonius` requires full NLLs. Some others were also both failing with NLL and succeeding with Polonius, which we can't encode in tests at the moment. - blessing the output of some tests: whenever Polonius and NLL have basically the same errors, except for diagnostics differences, the Polonius output is blessed. Whenever we've advanced into a less experimental phase, we'll want to revisit these cases (much like we did on the NLL test suite last year) to specifically work on diagnostics. Fact generation changes: - we now kill loans on the destination place of `Call` terminators - we now kill loans on the locals destroyed by `StorageDead` - we now also handle assignments to projections: killing the loans on a either a deref-ed local, or the ones whose `borrowed_place` conflicts with the current place. One failing test remains: an overflow during fact generation, on a case of polymorphic recursion (and which I'll continue investigating later). This adds some tests for the fact generation changes, with some simple Polonius cases similar to the existing smoke tests, but also for some cases encountered in the wild (in the `rand` crate for example). A more detailed write-up is available [here](https://hackmd.io/CjYB0fs4Q9CweyeTdKWyEg?view) with an explanation for each test failure, the steps taken to resolve it (as a commit in the current PR), NLL and Polonius outputs (and diff), etc. Since they've worked on this before, and we've discussed some of these failures together: r? @matthewjasper

@matthewjasper

Polonius: fix some cases of `killed` fact generation, and most of the `ui` test suite Since basic Polonius functionality was re-enabled by @matthewjasper in rust-lang#54468, some tests were still failing in the polonius compare-mode. This PR fixes all but one test in the `ui` suite by: - fixing some bugs in the fact generation code, related to the `killed` relation: Polonius would incorrectly reject some NLL-accepted code, because of these missing `killed` facts. - ignoring some tests in the polonius compare-mode: a lot of those manually test the NLL or migrate mode, and the failures were mostly artifacts of the test revisions, e.g. that `-Z polonius` requires full NLLs. Some others were also both failing with NLL and succeeding with Polonius, which we can't encode in tests at the moment. - blessing the output of some tests: whenever Polonius and NLL have basically the same errors, except for diagnostics differences, the Polonius output is blessed. Whenever we've advanced into a less experimental phase, we'll want to revisit these cases (much like we did on the NLL test suite last year) to specifically work on diagnostics. Fact generation changes: - we now kill loans on the destination place of `Call` terminators - we now kill loans on the locals destroyed by `StorageDead` - we now also handle assignments to projections: killing the loans on a either a deref-ed local, or the ones whose `borrowed_place` conflicts with the current place. One failing test remains: an overflow during fact generation, on a case of polymorphic recursion (and which I'll continue investigating later). This adds some tests for the fact generation changes, with some simple Polonius cases similar to the existing smoke tests, but also for some cases encountered in the wild (in the `rand` crate for example). A more detailed write-up is available [here](https://hackmd.io/CjYB0fs4Q9CweyeTdKWyEg?view) with an explanation for each test failure, the steps taken to resolve it (as a commit in the current PR), NLL and Polonius outputs (and diff), etc. Since they've worked on this before, and we've discussed some of these failures together: r? @matthewjasper

rust-highfive assigned nikomatsakis Sep 22, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 22, 2018

matthewjasper force-pushed the fix-polonius branch from 6356c42 to 596fd1b Compare September 22, 2018 13:17

memoryruins added the A-NLL Area: Non-lexical lifetimes (NLL) label Sep 22, 2018

This comment has been minimized.

Sign in to view

matthewjasper force-pushed the fix-polonius branch 2 times, most recently from 6cb00f7 to 2489b50 Compare September 22, 2018 16:04

This comment has been minimized.

Sign in to view

matthewjasper force-pushed the fix-polonius branch from 2489b50 to 68ed229 Compare September 22, 2018 17:19

nikomatsakis requested changes Sep 25, 2018

View reviewed changes

matthewjasper force-pushed the fix-polonius branch from 68ed229 to 7a8c001 Compare September 25, 2018 21:24

matthewjasper force-pushed the fix-polonius branch from 7a8c001 to 8e71e5d Compare September 26, 2018 19:00

This comment has been minimized.

Sign in to view

matthewjasper force-pushed the fix-polonius branch 2 times, most recently from 0c9d1fb to 3f568b2 Compare September 26, 2018 19:41

Get Polonius borrow check to work in simple cases

610903f

* Restores the generation of outlives facts from subtyping. * Restore liveness facts. * Generate invalidates facts at the start point of each location, where we check for errors. * Add a small test for simple cases.

matthewjasper force-pushed the fix-polonius branch from 3f568b2 to 610903f Compare September 26, 2018 21:04

bors 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 Sep 27, 2018

nikomatsakis approved these changes Sep 27, 2018

View reviewed changes

bors merged commit 610903f into rust-lang:master Sep 27, 2018

matthewjasper deleted the fix-polonius branch October 2, 2018 18:16

lqd mentioned this pull request Jul 16, 2019

Polonius: fix some cases of killed fact generation, and most of the ui test suite #62736

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NLL] Get Polonius borrow check to work in simple cases #54468

[NLL] Get Polonius borrow check to work in simple cases #54468

matthewjasper commented Sep 22, 2018 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

bors commented Sep 23, 2018

nikomatsakis left a comment

nikomatsakis Sep 25, 2018

nikomatsakis Sep 25, 2018

nikomatsakis Sep 25, 2018

matthewjasper commented Sep 25, 2018

bors commented Sep 25, 2018

matthewjasper commented Sep 26, 2018

This comment has been minimized.

bors commented Sep 26, 2018

nikomatsakis commented Sep 27, 2018

bors commented Sep 27, 2018

bors commented Sep 27, 2018

bors commented Sep 27, 2018

[NLL] Get Polonius borrow check to work in simple cases #54468

[NLL] Get Polonius borrow check to work in simple cases #54468

Conversation

matthewjasper commented Sep 22, 2018 • edited Loading

Known limitations

This comment has been minimized.

This comment has been minimized.

bors commented Sep 23, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Sep 25, 2018

Choose a reason for hiding this comment

nikomatsakis Sep 25, 2018

Choose a reason for hiding this comment

nikomatsakis Sep 25, 2018

Choose a reason for hiding this comment

matthewjasper commented Sep 25, 2018

bors commented Sep 25, 2018

matthewjasper commented Sep 26, 2018

This comment has been minimized.

bors commented Sep 26, 2018

nikomatsakis commented Sep 27, 2018

bors commented Sep 27, 2018

bors commented Sep 27, 2018

bors commented Sep 27, 2018

matthewjasper commented Sep 22, 2018 •

edited

Loading