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

lqd · 2019-07-16T23:06:04Z

Since basic Polonius functionality was re-enabled by @matthewjasper in #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 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 · 2019-07-17T20:47:49Z

@bors r+
At some point we'll probably want to have CI running these tests cc @rust-lang/infra

bors · 2019-07-17T20:47:50Z

📌 Commit 3920403008067135b6255068f80c5f23ebeec816 has been approved by matthewjasper

bors · 2019-07-20T09:55:56Z

⌛ Testing commit 3920403008067135b6255068f80c5f23ebeec816 with merge 85d339d554a6ac0acc1bd8c49722045c09dac5b2...

bors · 2019-07-20T12:21:24Z

💔 Test failed - checks-azure

rust-highfive · 2019-07-20T12:21:25Z

The job x86_64-gnu-aux of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-07-20T09:57:21.3086437Z do so (now or later) by using -b with the checkout command again. Example:
2019-07-20T09:57:21.3086497Z 
2019-07-20T09:57:21.3086773Z   git checkout -b <new-branch-name>
2019-07-20T09:57:21.3086824Z 
2019-07-20T09:57:21.3087159Z HEAD is now at 85d339d55 Auto merge of #62736 - lqd:polonius_tests3, r=matthewjasper
2019-07-20T09:57:21.3223189Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-07-20T09:57:21.3226150Z ==============================================================================
2019-07-20T09:57:21.3226241Z Task         : Bash
2019-07-20T09:57:21.3226329Z Description  : Run a Bash script on macOS, Linux, or Windows

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

lqd · 2019-07-20T12:36:31Z

Looks like a network issue

2019-07-20T12:18:08.1142326Z error: failed to download from `https://crates.io/api/v1/crates/bitflags/1.0.3/download`
2019-07-20T12:18:08.1142895Z 
2019-07-20T12:18:08.1143134Z Caused by:
2019-07-20T12:18:08.1143339Z   [35] SSL connect error (OpenSSL SSL_connect: SSL_ERROR_SYSCALL in connection to crates.io:443 )
2019-07-20T12:18:08.1164594Z thread 'main' panicked at 'tests failed for https://github.com/servo/webrender', src/tools/cargotest/main.rs:88:9

@bors retry

This is test specific to the NLL migrate mode which is irrelevant to -Z polonius. It can't currently be encoded depending on migrate-mode and NLL/Polonius mode, so the NLL compare-mode also ignores it.

There is no difference between the NLL and Polonius outputs, and it manually tests NLLs.

…mpare mode This is just a difference from the test construction, it's ignore-compare-mode-nll and manually checks migrate/nll over edition2015/2018. This failure is because the `migrate2015` and `migrate2018` revisions are ran with `-Zpolonius`. There is no actual difference in the errors output by NLLs and Polonius.

This is a test about turning the NLL feature gate on, ignored by the NLL compare-mode.

…re mode Once again, the difference is in the test construction, it is ignored in compare-mode NLL and tested manually with revisions, and fails because the `migrate` revision is ran with `-Zpolonius`. There is no actual difference in the errors output by NLLs and Polonius.

2 of the 3 errors are "fixed by Polonius" 🎉

…Polonius

…r Polonius

Like "call-kills-loans", Polonius didn't know about some `killed` facts.

…ve-fn-1.rs for Polonius

…y loans

…ually check each outcome

Fixing assignments to projections made the Polonius output exactly the same as the NLL one.

lqd · 2019-07-22T10:51:22Z

Thanks to @oli-obk for the information about the Place 2.0 changes for the rebase.

@bors r=matthewjasper rollup

bors · 2019-07-22T10:51:23Z

📌 Commit e16bede has been approved by matthewjasper

bors · 2019-07-22T10:51:24Z

🌲 The tree is currently closed for pull requests below priority 1500, this pull request will be tested once the tree is reopened

Centril · 2019-07-22T13:28:39Z

@bors rollup-

@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

@ghost

Rollup of 9 pull requests Successful merges: - rust-lang#61727 (Add binary dependencies to dep-info files) - rust-lang#62736 (Polonius: fix some cases of `killed` fact generation, and most of the `ui` test suite) - rust-lang#62758 (ci: Install clang on Windows through tarballs) - rust-lang#62784 (Add riscv32i-unknown-none-elf target) - rust-lang#62814 (add support for hexagon-unknown-linux-musl) - rust-lang#62827 (Don't link mcjit/interpreter LLVM components) - rust-lang#62901 (cleanup: Remove `extern crate serialize as rustc_serialize`s) - rust-lang#62903 (Support SDKROOT env var on iOS) - rust-lang#62906 (Require a value for configure --debuginfo-level) Failed merges: - rust-lang#62910 (cleanup: Remove lint annotations in specific crates that are already enforced by rustbuild) r? @ghost

@ghost

Rollup of 9 pull requests Successful merges: - #61727 (Add binary dependencies to dep-info files) - #62736 (Polonius: fix some cases of `killed` fact generation, and most of the `ui` test suite) - #62758 (ci: Install clang on Windows through tarballs) - #62784 (Add riscv32i-unknown-none-elf target) - #62814 (add support for hexagon-unknown-linux-musl) - #62827 (Don't link mcjit/interpreter LLVM components) - #62901 (cleanup: Remove `extern crate serialize as rustc_serialize`s) - #62903 (Support SDKROOT env var on iOS) - #62906 (Require a value for configure --debuginfo-level) Failed merges: - #62910 (cleanup: Remove lint annotations in specific crates that are already enforced by rustbuild) r? @ghost

@nikomatsakis

Polonius: more `ui` test suite fixes Since rust-lang#62736, new tests have been added, and the `run-pass` suite was merged into the `ui` suite. This PR adds the missing tests expectations for Polonius, and updates the existing ones where the NLL output has changed in some manner (e.g. ordering of notes) Those are the trivial cases, but a more-detailed explanation is available [in this write-up](https://hackmd.io/CjYB0fs4Q9CweyeTdKWyEg?both#26-async-awaitasync-borrowck-escaping-closure-errorrs-outputs-from-NLL-Polonius-diff) starting at test case 26: they are only differing in diagnostics and instances of other existing test cases differences. Only 3 of the 9020 tests are still "failing" at the moment (1 failure, 2 OOMs). r? @nikomatsakis

@nikomatsakis

Polonius: more `ui` test suite fixes Since rust-lang#62736, new tests have been added, and the `run-pass` suite was merged into the `ui` suite. This PR adds the missing tests expectations for Polonius, and updates the existing ones where the NLL output has changed in some manner (e.g. ordering of notes) Those are the trivial cases, but a more-detailed explanation is available [in this write-up](https://hackmd.io/CjYB0fs4Q9CweyeTdKWyEg?both#26-async-awaitasync-borrowck-escaping-closure-errorrs-outputs-from-NLL-Polonius-diff) starting at test case 26: they are only differing in diagnostics and instances of other existing test cases differences. Only 3 of the 9020 tests are still "failing" at the moment (1 failure, 2 OOMs). r? @nikomatsakis

@nikomatsakis

Polonius: more `ui` test suite fixes Since rust-lang#62736, new tests have been added, and the `run-pass` suite was merged into the `ui` suite. This PR adds the missing tests expectations for Polonius, and updates the existing ones where the NLL output has changed in some manner (e.g. ordering of notes) Those are the trivial cases, but a more-detailed explanation is available [in this write-up](https://hackmd.io/CjYB0fs4Q9CweyeTdKWyEg?both#26-async-awaitasync-borrowck-escaping-closure-errorrs-outputs-from-NLL-Polonius-diff) starting at test case 26: they are only differing in diagnostics and instances of other existing test cases differences. Only 3 of the 9020 tests are still "failing" at the moment (1 failure, 2 OOMs). r? @nikomatsakis

rust-highfive assigned matthewjasper Jul 16, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 16, 2019

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 Jul 17, 2019

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 20, 2019

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 Jul 20, 2019

This comment has been minimized.

Sign in to view

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 21, 2019

lqd added 12 commits July 22, 2019 10:32

Ignore NLL migrate mode test in the Polonius compare-mode

c442dae

This is test specific to the NLL migrate mode which is irrelevant to -Z polonius. It can't currently be encoded depending on migrate-mode and NLL/Polonius mode, so the NLL compare-mode also ignores it.

Ignore test issue-45983 in the polonius compare mode

d4ca9a3

There is no difference between the NLL and Polonius outputs, and it manually tests NLLs.

Ignore feature-gate-nll.rs in Polonius compare mode

63c837e

This is a test about turning the NLL feature gate on, ignored by the NLL compare-mode.

Bless output of test ui/emit-artifact-notifications.rs for Polonius

ff350f8

Bless output of test save-analysis/emit-notifications.rs for Polonius

2824db1

Bless output of test nll/loan_ends_mid_block_pair.rs for Polonius

9410104

Bless output of test consts/promote_const_let.rs for Polonius

6a7c15e

Bless output of test nll/get_default.rs for Polonius

c5a1bc1

2 of the 3 errors are "fixed by Polonius" 🎉

Bless output of test generator/ref-escapes-but-not-over-yield.rs for …

292d5c1

…Polonius

Bless output of test borrowck/borrowck-escaping-closure-error-2.rs fo…

6fe5292

…r Polonius

lqd added 11 commits July 22, 2019 11:36

Bless output of test nll/return-ref-mut-issue-46557.rs for Polonius

40e6b02

Add test extracted from rand, checking that StorageDead kills loans

9bd9b0d

Like "call-kills-loans", Polonius didn't know about some `killed` facts.

Rename test so that both "kills-loans" tests match names

606f798

Make both polonius loans tests check-pass

9e0fb6f

Bless output of test unboxed-closures/unboxed-closures-failed-recursi…

823ab42

…ve-fn-1.rs for Polonius

Polonius: generate killed facts for assignments to projections

2f3e36f

Add test checking various assignments are accepted in Polonius

d41e002

Add test to check that assignments to projections do not kill too man…

770129c

…y loans

issue-46589 passes in Polonius and fails in NLL, duplicate it and man…

c7f9a71

…ually check each outcome

Bless output of test nll/loan_ends_mid_block_pair.rs for Polonius, again

c0eab36

Fixing assignments to projections made the Polonius output exactly the same as the NLL one.

fix tidy

e16bede

lqd force-pushed the polonius_tests3 branch from 3920403 to e16bede Compare July 22, 2019 10:47

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 22, 2019

Centril mentioned this pull request Jul 24, 2019

Rollup of 8 pull requests #62937

Closed

Centril mentioned this pull request Jul 24, 2019

Rollup of 9 pull requests #62961

Merged

bors merged commit e16bede into rust-lang:master Jul 25, 2019

lqd deleted the polonius_tests3 branch July 25, 2019 08:15

lqd mentioned this pull request Sep 17, 2019

Polonius: more ui test suite fixes #64554

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lqd commented Jul 16, 2019 •

edited

Loading

matthewjasper commented Jul 17, 2019

bors commented Jul 17, 2019

bors commented Jul 20, 2019

bors commented Jul 20, 2019

rust-highfive commented Jul 20, 2019

lqd commented Jul 20, 2019

This comment has been minimized.

lqd commented Jul 22, 2019

bors commented Jul 22, 2019

bors commented Jul 22, 2019

Centril commented Jul 22, 2019

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

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

Conversation

lqd commented Jul 16, 2019 • edited Loading

matthewjasper commented Jul 17, 2019

bors commented Jul 17, 2019

bors commented Jul 20, 2019

bors commented Jul 20, 2019

rust-highfive commented Jul 20, 2019

lqd commented Jul 20, 2019

This comment has been minimized.

lqd commented Jul 22, 2019

bors commented Jul 22, 2019

bors commented Jul 22, 2019

Centril commented Jul 22, 2019

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

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

lqd commented Jul 16, 2019 •

edited

Loading