replace the leak check with universes, take 2 #65232
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonas-schievink
added
A-lazy-normalization
Centril
reviewed
Oct 9, 2019
This was referenced Oct 9, 2019
|
When this PR is complete (particularly the "I didn't finish ripping out the leak-check completely." bullet point), it will also fix #59490. |
It definitely considers regions that are not live anywhere and don't outlive any universal regions as rust/src/librustc_mir/borrow_check/nll/region_infer/mod.rs Lines 1524 to 1533 in 9c5a5c4 |
|
It occurs to me that updating NLL to match is not (presently) a strict requirement, but it would be something we have to do before we disable the AST-based regionck. |
|
Can you bless the NLL compare mode output and check how this handles #57936 as well? |
|
@matthewjasper I will bless compare-mode output, yes. As far as #57936, I get two errors: one in the This error is very bad -- I think we have an open issue on it, actually? I should fix that. The problem here is that the real cause has to do with the full trait ref, but for some reason we're not printing all the details. |
|
(I had not noticed #57936 at the time, or else I forgot about it, I'm not sure. :) |
|
Commit just pushed. Message: --bless --compare-mode=nll -- but note that we get two errors Both are related to the NLL region checker not handling the |
We used to have #57362 for example, but it was fixed. At the time we mentioned there might still be some cases missing from
|
|
☔ The latest upstream changes (presumably #64939) made this pull request unmergeable. Please resolve the merge conflicts. |
| = help: the following implementations were found: | ||
| <&'static u32 as Bar> | ||
| = note: `Bar` would have to be implemented for the type `&'0 u32`, for any lifetime `'0`... | ||
| = note: ...but `Bar` is actually implemented for the type `&'1 u32`, for some specific lifetime `'1` |
There was a problem hiding this comment.
It would be nice for this to use 'static instead of '1.
| | | | ||
| | expected signature of `fn(&(u32, u32)) -> _` | ||
|
|
||
| error: aborting due to 3 previous errors |
There was a problem hiding this comment.
The other two errors are missing here because typeck errors stop compilation before borrow checking, right?
| = help: the following implementations were found: | ||
| <SomeStruct as Foo<(&'a isize, &'a isize)>> | ||
| = note: `SomeStruct` must implement `Foo<(&'0 isize, &'1 isize)>`, for any two lifetimes `'0` and `'1`... | ||
| = note: ...but `SomeStruct` actually implements `Foo<(&'2 isize, &'2 isize)>`, for some specific lifetime `'2` |
There was a problem hiding this comment.
This wording is a little odd.
| = note: expected type `&isize` | ||
| found type `&usize` | ||
|
|
||
| error: aborting due to 2 previous errors |
There was a problem hiding this comment.
Typeck errors are hiding borrowck errors
| | | | ||
| | expected signature of `fn(for<'r> fn(&'r u32), &i32) -> _` | ||
| LL | with_closure_expecting_fn_with_free_region(|x: fn(&'x u32), y| {}); | ||
| | ^ requires that `'x` must outlive `'static` |
There was a problem hiding this comment.
Does replacing 'x with 'static make this compile?
edit: it doesn't. It would be nice for the error messages here to be better.
| ... | ||
| LL | foo((), drop) | ||
| | ^^^ expected bound lifetime parameter 'a, found concrete lifetime | ||
| | expected signature of `fn(<() as Trait<'a>>::Item) -> _` |
There was a problem hiding this comment.
This should be compiling but isn't because we don't normalize types with escaping bound regions, right?
nikomatsakis
changed the title
|
I looked a bit at the region errors managing to avoid the nice_region_errors mechanism, and left a couple notes in this Zulip thread. |
JohnCSimon
added
S-waiting-on-author
|
Ping from triage. |
nikomatsakis
force-pushed
the
lazy-norm-anon-const-push-2
branch
from
1b4cc3c
to
049dafd
Compare
|
The job Click to expand the log.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 |
|
The job Click to expand the log.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 |
If this is referring to this, I guess this can be done in a follow up issue I think that this should be good to go with follow up issues raised and a crater run. |
|
Ping from triage: |
|
@bors retry |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 4b3c66d with merge 92e5173d86cfea6987c917c74cb611fd330f959d... |
|
Your PR failed (pretty log, 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.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 |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
@bors retry |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Feb 7, 2020
…tthewjasper
replace the leak check with universes, take 2
This PR is an attempt to revive the "universe-based region check", which is an important step towards lazy normalization. Unlike before, we also modify the definition of `'empty` so that it is indexed by a universe. This sidesteps some of the surprising effects we saw before -- at the core, we no longer think that `exists<'a> { forall<'b> { 'b: 'a } }` is solveable. The new region lattice looks like this:
```
static ----------+-----...------+ (greatest)
| | |
early-bound and | |
free regions | |
| | |
scope regions | |
| | |
empty(root) placeholder(U1) |
| / |
| / placeholder(Un)
empty(U1) -- /
| /
... /
| /
empty(Un) -------- (smallest)
```
This PR has three effects:
* It changes a fair number of error messages, I think for the better.
* It fixes a number of bugs. The old algorithm was too conservative and caused us to reject legal subtypings.
* It also causes two regressions (things that used to compile, but now do not).
* `coherence-subtyping.rs` gets an additional error. This is expected.
* `issue-57639.rs` regresses as before, for the reasons covered in #57639.
Both of the regressions stem from the same underlying property: without the leak check, the instantaneous "subtype" check is not able to tell whether higher-ranked subtyping will succeed or not. In both cases, we might be able to fix the problem by doing a 'leak-check like change' at some later point (e.g., as part of coherence).
This is a draft PR because:
* I didn't finish ripping out the leak-check completely.
* We might want to consider a crater run before landing this.
* We might want some kind of design meeting to cover the overall strategy.
* I just remembered I never finished 100% integrating this into the canonicalization code.
* I should also review what happens in NLL region checking -- it probably still has a notion of bottom (empty set).
r? @matthewjasper
|
☀️ Test successful - checks-azure |
|
This regressed the EDIT: All svd2rust generated crates are similarly affected (they have similar structure and compiler load to |
bors
added a commit
to rust-lang/rust-clippy
that referenced
this pull request
Feb 9, 2020
Rustup "index ReEmpty by universe" cc rust-lang/rust#65232 changelog: none
|
I have opened a fix at #69044 |
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Feb 11, 2020
…twice, r=davidtwco Don't run coherence twice for future-compat lints This fixes the regression introduced by rust-lang#65232 (which I mentioned in rust-lang#65232 (comment)). Old algorithm: * Run coherence with all future-incompatible checks off, reporting errors on any overlap. * If there's no overlap (common case), run it *again*, with the future-incompatible checks on. Report warnings for any overlap found. New algorithm: * Run coherence with all additional future-incompatible checks *on*, which means that we'll find *all* potentially overlapping impls immediately. * If this found overlap, run coherence again, with the future-incompatible checks off. If that *still* gives an error, we report it. If not, it ought to be a warning. This reduces time spent in coherence checking for the nrf52810-pac by roughly 50% relative to current master.
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is an attempt to revive the "universe-based region check", which is an important step towards lazy normalization. Unlike before, we also modify the definition of
'emptyso that it is indexed by a universe. This sidesteps some of the surprising effects we saw before -- at the core, we no longer think thatexists<'a> { forall<'b> { 'b: 'a } }is solveable. The new region lattice looks like this:This PR has three effects:
coherence-subtyping.rsgets an additional error. This is expected.issue-57639.rsregresses as before, for the reasons covered in Regression in trait bounds that are redundant with associated type's HRTB #57639.Both of the regressions stem from the same underlying property: without the leak check, the instantaneous "subtype" check is not able to tell whether higher-ranked subtyping will succeed or not. In both cases, we might be able to fix the problem by doing a 'leak-check like change' at some later point (e.g., as part of coherence).
This is a draft PR because:
r? @matthewjasper