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

chalk lowering rule: ProjectionEq-Normalize #52153

Merged
merged 1 commit into from
Nov 14, 2018
Merged

Conversation

csmoe
Copy link
Member

@csmoe csmoe commented Jul 8, 2018

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 8, 2018
@csmoe csmoe 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 Jul 8, 2018
@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@TimNN

This comment has been minimized.

let trait_ref = tcx.impl_trait_ref(trait_id).unwrap();

// `U`
let ty = tcx.type_of(item_id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think this will work, what we want to do is to introduce a new type parameter U. Not sure how to do that (probably through tcx.mk_ty_param), I’ll try and get back to you for more guidance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so as you can see in the description of the rule, we want the output to be something like:

ProjectionEq(<Self as Trait>::Item = U) :- Normalize(<Self as Trait>::Item = U)

where U is a new type variable. This could be done naively by having:

// add a type param at depth `0`
let ty = tcx.mk_ty_param(0, Symbol::intern(U).as_interned_str());

however as you can see, rustc refers to a type param with a depth index and an explicit name (here “U”), instead of referring to all parameters by using indices only. Hence if the name U is already taken, as in:

trait Foo<U> {
    #[rustc_dump_program_clauses]
    type Item;
}

then the output clause will be wrong as it will use the same parameter U for two unrelated things.

A possible fix would be to choose U whenever this name is not already in use, and in case it is used just choose a random name which is not already in use. Another possible fix would be two Binder layers instead of only one, as I think type parameters can have the same name if they are not at the same depth. I’ll confer with niko.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm so the problem here is that rustc doesn't really support this yet

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can leave this as a kind of FIXME for now

src/librustc_traits/lowering.rs Outdated Show resolved Hide resolved
src/librustc_traits/lowering.rs Outdated Show resolved Hide resolved
src/librustc_traits/lowering.rs Outdated Show resolved Hide resolved
src/librustc_traits/lowering.rs Outdated Show resolved Hide resolved
let trait_ref = tcx.impl_trait_ref(trait_id).unwrap();

// `U`
let ty = tcx.type_of(item_id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so as you can see in the description of the rule, we want the output to be something like:

ProjectionEq(<Self as Trait>::Item = U) :- Normalize(<Self as Trait>::Item = U)

where U is a new type variable. This could be done naively by having:

// add a type param at depth `0`
let ty = tcx.mk_ty_param(0, Symbol::intern(U).as_interned_str());

however as you can see, rustc refers to a type param with a depth index and an explicit name (here “U”), instead of referring to all parameters by using indices only. Hence if the name U is already taken, as in:

trait Foo<U> {
    #[rustc_dump_program_clauses]
    type Item;
}

then the output clause will be wrong as it will use the same parameter U for two unrelated things.

A possible fix would be to choose U whenever this name is not already in use, and in case it is used just choose a random name which is not already in use. Another possible fix would be two Binder layers instead of only one, as I think type parameters can have the same name if they are not at the same depth. I’ll confer with niko.

@csmoe
Copy link
Member Author

csmoe commented Jul 27, 2018

@scalexm thank you, Your instructions will be addressed soon.

@TimNN

This comment has been minimized.

@csmoe

This comment has been minimized.

@csmoe csmoe added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 16, 2018
@scalexm
Copy link
Member

scalexm commented Aug 17, 2018

@csmoe I think that’s good now! cc @nikomatsakis, what do you think of the FIXME?

@pietroalbini
Copy link
Member

Ping from triage @nikomatsakis! This PR needs your review.

@TimNN
Copy link
Contributor

TimNN commented Sep 4, 2018

Ping from triage @nikomatsakis / @rust-lang/compiler: This PR requires your review.

@TimNN
Copy link
Contributor

TimNN commented Sep 11, 2018

Ping from triage @rust-lang/compiler / @nikomatsakis: This PR requires your review.

@XAMPPRocky XAMPPRocky added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 26, 2018
@scalexm scalexm added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Oct 2, 2018
@scalexm
Copy link
Member

scalexm commented Oct 2, 2018

I propose that we wait for rustc to handle quantification over types, once this is done it should be almost trivial to finish this PR.

@bors
Copy link
Contributor

bors commented Oct 11, 2018

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

@scalexm
Copy link
Member

scalexm commented Nov 13, 2018

@bors r+
Unblocked by #55330

@bors
Copy link
Contributor

bors commented Nov 13, 2018

📌 Commit e853d6c has been approved by scalexm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 13, 2018
@bors
Copy link
Contributor

bors commented Nov 14, 2018

⌛ Testing commit e853d6c with merge 126a0e2...

bors added a commit that referenced this pull request Nov 14, 2018
chalk lowering rule: ProjectionEq-Normalize

cc #49177
r? @scalexm
@bors
Copy link
Contributor

bors commented Nov 14, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: scalexm
Pushing 126a0e2 to master...

@bors bors merged commit e853d6c into rust-lang:master Nov 14, 2018
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #52153!

Tested on commit 126a0e2.
Direct link to PR: #52153

🎉 rls on windows: test-fail → test-pass (cc @nrc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Nov 14, 2018
Tested on commit rust-lang/rust@126a0e2.
Direct link to PR: <rust-lang/rust#52153>

🎉 rls on windows: test-fail → test-pass (cc @nrc @Xanewok, @rust-lang/infra).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants