Skip to content

/rust

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

Remove the "leak check" in favor of "universes" #48407

Closed
wants to merge 22 commits into from

Conversation

Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
S-blocked
Projects
None yet
Milestone
No milestone
8 participants
@sgrif @nikomatsakis @bors @shepmaster @pietroalbini @aidanhs @TimNN @rust-highfive
@sgrif
Copy link
Contributor

sgrif commented Feb 21, 2018

See #48049 and https://gist.github.com/nikomatsakis/87822133b2d85ccac801e08bcb9702de for context. This branch is not yet expected to pass compile-fail or UI tests, nor does it work with NLL. However, it should be ready for a crater run. This PR requires #47861.

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis Feb 21, 2018

@rust-highfive rust-highfive added the S-waiting-on-review label Feb 21, 2018

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Feb 21, 2018

@bors try

As @sgrif said, we'd like to do a crater run first, and then fix the build failures etc.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 21, 2018

🔒 Merge conflict

@bors bors added S-waiting-on-author and removed S-waiting-on-review labels Feb 21, 2018

@sgrif

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

sgrif commented Feb 22, 2018

I'll fix the merge conflict in the morning
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 22, 2018

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

@nikomatsakis nikomatsakis referenced this pull request Feb 25, 2018

Closed

🔬 move to universe-based lifetimes #48536

@shepmaster

This comment has been minimized.

Sign in to view
Copy link
Member

shepmaster commented Mar 2, 2018

Ping from triage, @sgrif! We haven't heard from you in a week; perhaps you have moved to a very polar location and "morning" takes a few weeks to occur 😇?
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Mar 3, 2018

I think @sgrif was waiting to #47861 to land... which it has now.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Mar 3, 2018

@sgrif

As noted on gitter, we need to remove the universe from ParamEnv, now that we it is added into InferCtxt.

@nikomatsakis nikomatsakis referenced this pull request Mar 3, 2018

Open

tracking issue for "chalkification" #48049

3 of 4 tasks complete
@sgrif

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

sgrif commented Mar 4, 2018

Yup, was waiting on that (and just had a busy week at work which kept me from having time to work on this). Still on the radar, planning on just deleting all the compile-fail and ui tests that are failing in order to get a crater run going.

@sgrif sgrif force-pushed the sgrif:sg-universe-refactorings branch from 0d0ca02 to 123ff00 Mar 6, 2018

@sgrif

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

sgrif commented Mar 6, 2018

@rust-lang/release needs crater

@pietroalbini pietroalbini added S-waiting-on-crater and removed S-waiting-on-author labels Mar 6, 2018

@pietroalbini

This comment has been minimized.

Sign in to view
Copy link
Member

pietroalbini commented Mar 6, 2018 •
edited

Added to the queue (there are 3 other PRs waiting before this one).

Edit: a try run is also needed
@sgrif

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

sgrif commented Mar 6, 2018

@nikomatsakis Can you ask bors for me please :)
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Mar 7, 2018 •
edited

@sgrif I would but I see travis is sad
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 8, 2018

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

sgrif added some commits Mar 6, 2018

@sgrif
Revert "change skolemizations to use universe index" 
This reverts commit 755bdaa.
48df576
@sgrif
Revert "fix tests in `librustc_driver`" 
This reverts commit 17df455.
50bd71c
@sgrif
Revert "fix tidy error" 
This reverts commit a985634.
5dad682
@sgrif
Revert "change skolemizations to use universe index" 
This reverts commit 35e78b5.
2a94f30
@sgrif
introduce `UniverseIndex` into `InferCtxt` 
Always using root environment for now.
3d4791b
@sgrif
add universes to type inference variables 
This gives each type inference variable a notion of universe but doesn't
do anything with it. We can always get the "current universe" from
infer_ctxt. This relies on the property of type variables that they can
never interact with siblings.
ff17bd7
@sgrif
Revert "introduce `UniverseIndex` into `ParamEnv`" 
This reverts commit d4df52c.
f6e6d23
@sgrif
Revert "add universes to type inference variables" 
This reverts commit 13efaf0.
26d1fc7
@aidanhs

This comment has been minimized.

Sign in to view
Copy link
Member

aidanhs commented Mar 15, 2018

Just as a passing note - this has no try run so can't be crater run. I've marked it back as waiting on author (merge conflicts will need resolving, tests don't need to pass but it's usually a good idea).
@sgrif
Fix rebase issues
Loading status checks…
3f599ad

@sgrif sgrif force-pushed the sgrif:sg-universe-refactorings branch from c6b875d to 3f599ad Mar 19, 2018

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 20, 2018

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

@sgrif sgrif referenced this pull request Mar 23, 2018

Merged

Remove universes from `ty::ParamEnv` #49313

@sgrif

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

sgrif commented Mar 23, 2018

This is essentially blocked on #48696. I'm working on a fix

@shepmaster shepmaster added S-blocked and removed S-waiting-on-author labels Mar 23, 2018

bors added a commit that referenced this pull request Mar 29, 2018

@bors
Auto merge of #49313 - sgrif:sg-revert-stuff, r=nikomatsakis 
Remove universes from `ty::ParamEnv`

This change was never meant to land. #48407 takes an alternate approach. However, that PR is now blocked on some issues with canonicalization, and rebasing these reverts gets harder each time, so let's just get this bit out of the way now.

r? @nikomatsakis
Loading status checks…
dca1470

@shepmaster shepmaster added S-blocked and removed S-blocked labels Mar 30, 2018

sgrif added a commit to sgrif/rust that referenced this pull request Apr 2, 2018

@sgrif
Always canonicalize skolemized regions 
Since `ReSkolemized` has the flag `KEEP_IN_LOCAL_TCX`, and
canonicalization is meant to ensure that flag is no longer present, it
implicitly is depending on the existence of the leak check. However,
since we want to remove that in rust-lang#48407, we can no longer make that
assumption.

This is related to, but does not resolve rust-lang#48696. This does unblock that
PR, however. It could have been tacked on there, but I wanted to make
sure this is a reasonable short term approach separately from that.
7cd2749

@sgrif sgrif referenced this pull request Apr 2, 2018

Closed

Always canonicalize skolemized regions #49598

sgrif added a commit to sgrif/rust that referenced this pull request Apr 2, 2018

@sgrif
Always canonicalize skolemized regions 
Since `ReSkolemized` has the flag `KEEP_IN_LOCAL_TCX`, and
canonicalization is meant to ensure that flag is no longer present, it
implicitly is depending on the existence of the leak check. However,
since we want to remove that in rust-lang#48407, we can no longer make that
assumption.

This is related to rust-lang#48696, but does not resolve it. This does unblock that
PR, however. It could have been tacked on there, but I wanted to make
sure this is a reasonable short term approach separately from that.
0c49d63

@shepmaster shepmaster added S-blocked and removed S-blocked labels Apr 7, 2018

@TimNN TimNN added S-blocked and removed S-blocked labels Apr 17, 2018

@sgrif sgrif referenced this pull request Apr 23, 2018

Closed

handle universes in canonicalization #48696

@TimNN TimNN added S-blocked and removed S-blocked labels Apr 24, 2018

@sgrif sgrif referenced this pull request May 2, 2018

Merged

Refactorings in preparation for the removal of the leak check #50397

bors added a commit that referenced this pull request May 4, 2018

@bors
Auto merge of #50397 - sgrif:sg-smaller-universe-refactorings, r=niko… 
…matsakis

Refactorings in preparation for the removal of the leak check

This contains all of the commits from #48407 that I was able to pull out on their own. This has most of the refactoring/ground work to unblock other work, but without the behavior changes that still need a crater run and NLL changes.

r? @nikomatsakis
Loading status checks…
1eab9c5

@shepmaster shepmaster added S-blocked and removed S-blocked labels May 6, 2018

@shepmaster

This comment has been minimized.

Sign in to view
Copy link
Member

shepmaster commented May 6, 2018

This has been blocked for a long time and seemingly on a chain of dependencies. There's quite a lot of merge conflicts. Does this PR have any value in its current state?
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented May 7, 2018

@shepmaster in its current state no but I think we are more or less at the point where @sgrif can rebase
@sgrif

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

sgrif commented May 7, 2018

Yeah, I'm hoping to spend some time on it this week now that #50397 has been pulled out as a subset.
@sgrif

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

sgrif commented May 13, 2018

I'm on vacation for a while, so I'm going to close this until I can get it rebased

@sgrif sgrif closed this May 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.