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.

Sign up for GitHub

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

Jump to bottom

enforce WF conditions after generalizing #41716

Merged
merged 3 commits into from
May 12, 2017
Merged

enforce WF conditions after generalizing #41716

merged 3 commits into from
May 12, 2017

Conversation

nikomatsakis
Copy link
Contributor

Add a WF(T') obligation after generalizing T to T', if T' contains an unconstrained type variable in a bivariant context.

Fixes #41677.

Beta nominating -- regression.

r? @arielb1

@nikomatsakis nikomatsakis added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 2, 2017
@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2017
@bors
Copy link
Contributor

bors commented May 6, 2017

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

@Mark-Simulacrum Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2017
@arielb1
Copy link
Contributor

arielb1 commented May 7, 2017

Got back from holidays. Had a fun week.

This looks like a rather brutal fix. As for concrete worries, I was worried that we might create wf obligations "inside" an higher-ranked binder, and that would cause trouble, but I don't think we ever do subtyping in a higher-ranked context right now?

@nikomatsakis
Copy link
Contributor Author

This looks like a rather brutal fix.

Brutal as in "brute force"?

As for concrete worries, I was worried that we might create wf obligations "inside" an higher-ranked binder, and that would cause trouble, but I don't think we ever do subtyping in a higher-ranked context right now?

I don't think that would be a problem. It's not that we don't do subtyping in a higher-ranked context. Rather, it's that we protect against skolemized regions "escaping" into instantiated type variables. If we did instantiate some type variable with something that includes skolemized regions --- or with region variables that wind up related to skolemized variables --- that would be detected during the leak-check. Since we only ever create the WF relations with the value from an instantiated variable, this implies that these relations cannot reference skolemized regions.

(As an aside, I am, though, actively planning how to overhaul that system right now to make it more robust, roughly in the manner that we talked about.)

@arielb1
Copy link
Contributor

arielb1 commented May 9, 2017

This is basically a hack around bivariance not being implemented properly, but let's go for it (r+ modulo conflict fix).

This was referenced May 9, 2017
Closed
Closed
@nikomatsakis
replace the type generalizer with one based on variance
a950c37
@nikomatsakis
add a WF obligation if a type variable appears in bivariant position
a4151ff
@nikomatsakis
correct various error messages that changed
2490ee5 
The new messages seem universally better. I think these result because
we recognize that we are in an invariant context more often.
@nikomatsakis
Copy link
Contributor Author

@bors r=arielb1

@bors
Copy link
Contributor

bors commented May 11, 2017

📌 Commit 2490ee5 has been approved by arielb1

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 12, 2017
@Mark-Simulacrum
Rollup merge of rust-lang#41716 - nikomatsakis:issue-41677, r=arielb1
5b5c045 
enforce WF conditions after generalizing

Add a `WF(T')` obligation after generalizing `T` to `T'`, if `T'` contains an unconstrained type variable in a bivariant context.

Fixes rust-lang#41677.

Beta nominating -- regression.

r? @arielb1
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request May 12, 2017
Closed
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 12, 2017
@Mark-Simulacrum
Rollup merge of rust-lang#41716 - nikomatsakis:issue-41677, r=arielb1
c8b1559 
enforce WF conditions after generalizing

Add a `WF(T')` obligation after generalizing `T` to `T'`, if `T'` contains an unconstrained type variable in a bivariant context.

Fixes rust-lang#41677.

Beta nominating -- regression.

r? @arielb1
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request May 12, 2017
Closed
@bors
Copy link
Contributor

bors commented May 12, 2017

⌛ Testing commit 2490ee5 with merge 141e8a6...

bors added a commit that referenced this pull request May 12, 2017
@bors
Auto merge of #41716 - nikomatsakis:issue-41677, r=arielb1
141e8a6 
enforce WF conditions after generalizing

Add a `WF(T')` obligation after generalizing `T` to `T'`, if `T'` contains an unconstrained type variable in a bivariant context.

Fixes #41677.

Beta nominating -- regression.

r? @arielb1
@bors
Copy link
Contributor

bors commented May 12, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 141e8a6 to master...

@bors bors merged commit 2490ee5 into rust-lang:master May 12, 2017
@brson
Copy link
Contributor

brson commented May 23, 2017

Doesn't cherry-pick cleanly cc @nikomatsakis

@nikomatsakis nikomatsakis mentioned this pull request May 23, 2017
Merged
@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 23, 2017
@nikomatsakis
Copy link
Contributor Author

Accepting for backport. cc @rust-lang/compiler

bors added a commit that referenced this pull request May 24, 2017
@bors
Auto merge of #42182 - nikomatsakis:beta, r=brson
2df18ed 
Beta backports

Backports of:

- #41913 (or some of it)
- #41937
- #41716
- #41563
@nikomatsakis nikomatsakis removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 25, 2017
This was referenced Jun 8, 2017
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@arielb1 arielb1

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nikomatsakis @bors @arielb1 @brson @aidanhs @Mark-Simulacrum