make generalization code create new variables in correct universe #58056
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
|
(Still running tests locally) |
rust-highfive
added
the
S-waiting-on-review
nikomatsakis
force-pushed
the
issue-57843-universe-leak
branch
from
82070f0
to
02465f1
Compare
|
ping from triage @nikomatsakis @pnkfelix what's the update on this? |
|
I'll prioritize the review here. |
Ideally, we'd probably print the closure substs themselves actually.
In our type inference system, when we "generalize" a type T to become a suitable value for a type variable V, we sometimes wind up creating new inference variables. So, for example, if we are making V be some subtype of `&'X u32`, then we might instantiate V with `&'Y u32`. This generalized type is then related `&'Y u32 <: &'X u32`, resulting in a region constriant `'Y: 'X`. Previously, however, we were making these fresh variables like `'Y` in the "current universe", but they should be created in the universe of V. Moreover, we sometimes cheat in an invariant context and avoid creating fresh variables if we know the result must be equal -- we can only do that when the universes work out.
|
r=me |
pnkfelix
force-pushed
the
issue-57843-universe-leak
branch
from
02465f1
to
9661ee6
Compare
|
@bors r+ |
|
📌 Commit 9661ee6 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 9661ee6 with merge 0a71da25710b5fdfd90a31c35e8d2313986243c1... |
|
💔 Test failed - checks-travis |
|
Your PR failed on Travis (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 |
bors
added
S-waiting-on-review
|
@bors retry -- apparently we can't clone the repo anymore on macOS |
bors
added
S-waiting-on-bors
|
@bors p=1 Soundness bug, urgent-ish to get this stuff landed. |
|
⌛ Testing commit 9661ee6 with merge 08adf37b53626653cef443887b55132e715926a0... |
|
💔 Test failed - status-appveyor |
|
@bors retry p=1 rust-lang/rls#1265 |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Feb 20, 2019
[WIP] Re-implement leak check in terms of universes This PR temporarily restores the leak-check, but implemented in terms of universes. This is not because the leak check behavior was necessarily **correct**, but because (a) we may want to have a transition period and because (b) we want to have more breathing room to work through the full implications of handling higher-ranked types correctly. Note that this PR builds atop #58056. Fixes #58451 Fixes #46989 Fixes #57639 r? @aturon cc @arielb1, @lqd ~~Temporary note: I've not finished running `./x.py test` locally -- I'm confident a lot of error messages in tests will need updating. I sort of expect them to revert to the older, (imo) less good error messages, which is mildly unfortunate. There might be a way to preserve the new error messages, not sure.~~
Mark-Simulacrum
added
I-nominated
beta-nominated
|
#58592 depends on this so this will also need to be backported to beta (to be released in one week). Nominating for compiler team to approve. |
|
Thanks @Mark-Simulacrum. I was actually just coming to nominate it regardless -- it is an important soundness fix. |
bors
added a commit
that referenced
this pull request
Feb 20, 2019
…elix make generalization code create new variables in correct universe In our type inference system, when we "generalize" a type T to become a suitable value for a type variable V, we sometimes wind up creating new inference variables. So, for example, if we are making V be some subtype of `&'X u32`, then we might instantiate V with `&'Y u32`. This generalized type is then related `&'Y u32 <: &'X u32`, resulting in a region constriant `'Y: 'X`. Previously, however, we were making these fresh variables like `'Y` in the "current universe", but they should be created in the universe of V. Moreover, we sometimes cheat in an invariant context and avoid creating fresh variables if we know the result must be equal -- we can only do that when the universes work out. Fixes #57843 r? @pnkfelix
|
☀️ Test successful - checks-travis, status-appveyor |
bors
added a commit
that referenced
this pull request
Feb 21, 2019
Re-implement leak check in terms of universes This PR temporarily restores the leak-check, but implemented in terms of universes. This is not because the leak check behavior was necessarily **correct**, but because (a) we may want to have a transition period and because (b) we want to have more breathing room to work through the full implications of handling higher-ranked types correctly. Note that this PR builds atop #58056. Fixes #58451 Fixes #46989 Fixes #57639 r? @aturon cc @arielb1, @lqd ~~Temporary note: I've not finished running `./x.py test` locally -- I'm confident a lot of error messages in tests will need updating. I sort of expect them to revert to the older, (imo) less good error messages, which is mildly unfortunate. There might be a way to preserve the new error messages, not sure.~~
|
We failed to discuss this beta-nomination separately from PR #58592 in yesterday's T-compiler meeting. But as I understand it, PR #58592 depends on this PR. Therefore I am going to mark this as beta-accepted. |
pnkfelix
added
beta-accepted
Merged
pietroalbini
removed
the
beta-nominated
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.
In our type inference system, when we "generalize" a type T to become
a suitable value for a type variable V, we sometimes wind up creating
new inference variables. So, for example, if we are making V be some
subtype of
&'X u32, then we might instantiate V with&'Y u32.This generalized type is then related
&'Y u32 <: &'X u32, resultingin a region constriant
'Y: 'X. Previously, however, we were makingthese fresh variables like
'Yin the "current universe", but theyshould be created in the universe of V. Moreover, we sometimes cheat
in an invariant context and avoid creating fresh variables if we know
the result must be equal -- we can only do that when the universes
work out.
Fixes #57843
r? @pnkfelix