Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In which we implement illegal subset relations errors using Polonius #67016

Merged
merged 10 commits into from Dec 9, 2019

Conversation

@lqd
Copy link
Contributor

lqd commented Dec 4, 2019

This PR is the rustc side of implementing subset errors using Polonius. That is, in

fn foo<'a, 'b>(x: &'a u32, y: &'b u32) -> &'a u32 {
    y
}

returning y requires that 'b: 'a but we have no evidence of that, so this is an error. (Evidence that the relation holds could come from explicit bounds, or via implied bounds).

Polonius outputs one such error per CFG point where the free region's placeholder loan unexpectedly flowed into another free region. While all these CFG locations could be useful in diagnostics in the future, rustc does not do that (and the duplication is only partially handled in the rest of the errors/diagnostics infrastructure, e.g. duplicate suggestions will be shown by the "outlives suggestions" or some of the #[rustc_*] NLL/MIR debug dumps), so I deduplicated the errors.

(The ordering also matters, otherwise some of the elided lifetime naming would change behaviour).

I've blessed a couple of tests, where the output is currently suboptimal:

  • the hrtb-perfect-forwarding tests mix subset errors with higher-ranked subtyping, however the plan is for chalk to eventually take care of some of this to generate polonius constraints (i.e. it's not polonius' job). Until that happens, polonius will not see the error that NLL sees.
  • some other tests have errors and diagnostics specific to 'static, I believe this to be because of it being treated as more "special" than in polonius. I believe the output is not wrong, but could be better, and appears elsewhere (I feel we'll need to look at polonius' handling of 'static at some point in the future, maybe to match a bit more what NLL does when it produces errors)

I'll create a tracking issue in the polonius repo to record these 2 points (and a general "we'll need to go over the blessed output" issue, much like we did for NLLs)

The last blessed test is because it's an improvement: in this case, more errors/suggestions were computed, instead of the existing code path where this case apparently stops at the first error.

The Naive variant in Polonius computes those errors, so this PR also switches the default variant to that, as we're also in the process of temporarily deactivating all other variants (which exist mostly for performance considerations) until we have completed more work on completeness and correctness, before focusing on efficiency once again.

While most of the correctness in this PR is hidden in the polonius compare-mode (which of course passes locally), I've added a couple of smoke-tests to the existing ones, so that we have some confidence that it works (and keeps working) until we're in a position where we can run them on CI.

As mentioned during yesterday's wg-polonius meeting, @nikomatsakis has already read through most of this PR (and which is matching what they thought needed to be done during the recent Polonius sprint), but Matthew was hopefully going to review (again, not urgent), so:

r? @matthewjasper

(This updates to the latest polonius-engine release, and I'm not sure whether Cargo.lock updates can easily be rolled up, but apart from that: this changes little that's tested on CI, so seems safe-ish to rollup ?)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 5, 2019

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

lqd added 10 commits Nov 18, 2019
- adapt to the new polonius `FactTypes` API
- reorganize the type aliases referring to polonius to avoid referencing the inner atom or fact types multiple times: only one input and output types should be enough for everyone. They could equally be in `borrow_check` as `nll` though.
- switches to using the Naive variant by default
- emits subset errors or propagates unsatisfied obligations
  to the caller
The plan is to use chalk and not have polonius deal with this.
The polonius output has one more error which should be displayed
in the regular case, but error reporting in the regular case stopped
at the first error.

Admittedly it would be nice to combine suggestions for the same source
lifetime so that `'a: 'b` and `'a: 'c` are not bothsuggested, but instead
a single `'a: 'b + 'c` is.
It's a relatively simple smoke-test for subset errors, executed outside
of the polonius compare-mode.
@lqd lqd force-pushed the lqd:placeholder_loans branch from 87e3cd9 to 1314ba3 Dec 6, 2019
@matthewjasper

This comment has been minimized.

Copy link
Contributor

matthewjasper commented Dec 7, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 7, 2019

📌 Commit 1314ba3 has been approved by matthewjasper

Centril added a commit to Centril/rust that referenced this pull request Dec 8, 2019
In which we implement illegal subset relations errors using Polonius

This PR is the rustc side of implementing subset errors using Polonius. That is, in
```rust
fn foo<'a, 'b>(x: &'a u32, y: &'b u32) -> &'a u32 {
    y
}
```
returning `y` requires that `'b: 'a` but we have no evidence of that, so this is an error. (Evidence that the relation holds could come from explicit bounds, or via implied bounds).

Polonius outputs one such error per CFG point where the free region's placeholder loan unexpectedly flowed into another free region. While all these CFG locations could be useful in diagnostics in the future, rustc does not do that (and the duplication is only partially handled in the rest of the errors/diagnostics infrastructure, e.g. duplicate suggestions will be shown by the "outlives suggestions" or some of the `#[rustc_*]` NLL/MIR debug dumps), so I deduplicated the errors.

(The ordering also matters, otherwise some of the elided lifetime naming would change behaviour).

I've blessed a couple of tests, where the output is currently suboptimal:
- the `hrtb-perfect-forwarding` tests mix subset errors with higher-ranked subtyping, however the plan is for chalk to eventually take care of some of this to generate polonius constraints (i.e. it's not polonius' job). Until that happens, polonius will not see the error that NLL sees.
- some other tests have errors and diagnostics specific to `'static`, I _believe_ this to be because of it being treated as more "special" than in polonius. I believe the output is not wrong, but could be better, and appears elsewhere (I feel we'll need to look at polonius' handling of `'static` at some point in the future, maybe to match a bit more what NLL does when it produces errors)

I'll create a tracking issue in the polonius repo to record these 2 points (and a general "we'll need to go over the blessed output" issue, much like we did for NLLs)

The last blessed test is because it's an improvement: in this case, more errors/suggestions were computed, instead of the existing code path where this case apparently stops at the first error.

The `Naive` variant in Polonius computes those errors, so this PR also switches the default variant to that, as we're also in the process of temporarily deactivating all other variants (which exist mostly for performance considerations) until we have completed more work on completeness and correctness, before focusing on efficiency once again.

While most of the correctness in this PR is hidden in the polonius compare-mode (which of course passes locally), I've added a couple of smoke-tests to the existing ones, so that we have some confidence that it works (and keeps working) until we're in a position where we can run them on CI.

As mentioned during yesterday's wg-polonius meeting, @nikomatsakis has already read through most of this PR (and which is matching  what they thought needed to be done [during the recent Polonius sprint](https://hackmd.io/CGMNjt1hR_qYtsR9hgdGmw#Compiler-notes-on-generating-the-placeholder-loans-support)), but Matthew was hopefully going to review (again, not urgent), so:

r? @matthewjasper

(This updates to the latest `polonius-engine` release, and I'm not sure whether `Cargo.lock` updates can easily be rolled up, but apart from that: this changes little that's tested on CI, so seems safe-ish to rollup ?)
bors added a commit that referenced this pull request Dec 8, 2019
Rollup of 6 pull requests

Successful merges:

 - #66892 (Format libcore with rustfmt (including tests and benches))
 - #67003 (Fix TypedArena returning wrong pointers for recursive allocations)
 - #67004 (Do not ICE on async fn with non-Copy infered type arg)
 - #67016 (In which we implement illegal subset relations errors using Polonius)
 - #67031 (Update tokio crates to latest versions)
 - #67138 (Simplify `Layout::extend_packed`)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 9, 2019

⌛️ Testing commit 1314ba3 with merge 3ff17e7...

bors added a commit that referenced this pull request Dec 9, 2019
In which we implement illegal subset relations errors using Polonius

This PR is the rustc side of implementing subset errors using Polonius. That is, in
```rust
fn foo<'a, 'b>(x: &'a u32, y: &'b u32) -> &'a u32 {
    y
}
```
returning `y` requires that `'b: 'a` but we have no evidence of that, so this is an error. (Evidence that the relation holds could come from explicit bounds, or via implied bounds).

Polonius outputs one such error per CFG point where the free region's placeholder loan unexpectedly flowed into another free region. While all these CFG locations could be useful in diagnostics in the future, rustc does not do that (and the duplication is only partially handled in the rest of the errors/diagnostics infrastructure, e.g. duplicate suggestions will be shown by the "outlives suggestions" or some of the `#[rustc_*]` NLL/MIR debug dumps), so I deduplicated the errors.

(The ordering also matters, otherwise some of the elided lifetime naming would change behaviour).

I've blessed a couple of tests, where the output is currently suboptimal:
- the `hrtb-perfect-forwarding` tests mix subset errors with higher-ranked subtyping, however the plan is for chalk to eventually take care of some of this to generate polonius constraints (i.e. it's not polonius' job). Until that happens, polonius will not see the error that NLL sees.
- some other tests have errors and diagnostics specific to `'static`, I _believe_ this to be because of it being treated as more "special" than in polonius. I believe the output is not wrong, but could be better, and appears elsewhere (I feel we'll need to look at polonius' handling of `'static` at some point in the future, maybe to match a bit more what NLL does when it produces errors)

I'll create a tracking issue in the polonius repo to record these 2 points (and a general "we'll need to go over the blessed output" issue, much like we did for NLLs)

The last blessed test is because it's an improvement: in this case, more errors/suggestions were computed, instead of the existing code path where this case apparently stops at the first error.

The `Naive` variant in Polonius computes those errors, so this PR also switches the default variant to that, as we're also in the process of temporarily deactivating all other variants (which exist mostly for performance considerations) until we have completed more work on completeness and correctness, before focusing on efficiency once again.

While most of the correctness in this PR is hidden in the polonius compare-mode (which of course passes locally), I've added a couple of smoke-tests to the existing ones, so that we have some confidence that it works (and keeps working) until we're in a position where we can run them on CI.

As mentioned during yesterday's wg-polonius meeting, @nikomatsakis has already read through most of this PR (and which is matching  what they thought needed to be done [during the recent Polonius sprint](https://hackmd.io/CGMNjt1hR_qYtsR9hgdGmw#Compiler-notes-on-generating-the-placeholder-loans-support)), but Matthew was hopefully going to review (again, not urgent), so:

r? @matthewjasper

(This updates to the latest `polonius-engine` release, and I'm not sure whether `Cargo.lock` updates can easily be rolled up, but apart from that: this changes little that's tested on CI, so seems safe-ish to rollup ?)
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 9, 2019

☀️ Test successful - checks-azure
Approved by: matthewjasper
Pushing 3ff17e7 to master...

@bors bors added the merged-by-bors label Dec 9, 2019
@bors bors merged commit 1314ba3 into rust-lang:master Dec 9, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr #20191206.15 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@lqd lqd deleted the lqd:placeholder_loans branch Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.