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

refactor ProjectionTy to store def-id of associated item + substs #42171

Closed
nikomatsakis opened this issue May 23, 2017 · 5 comments
Closed

refactor ProjectionTy to store def-id of associated item + substs #42171

nikomatsakis opened this issue May 23, 2017 · 5 comments
Labels
A-trait-system Area: Trait system C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

In preparation for ATC, we should refactor ProjectionTy to store the DefId of the trait associated item along with a set of substs for that item. (Currently, it stores a trait-ref (trait def-id + substs) and item name.) Currently, these substs will always be the same as the substs for the trait, but once ATC lands they will include additional items.

cc @tschottdorf, who has started in on this
cc @eddyb, who has a vague interest in this :)
@nikomatsakis nikomatsakis added A-trait-system Area: Trait system C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 23, 2017
@tbg
Copy link
Contributor

tbg commented May 27, 2017

Quick question - for storing substs, would a ProjectionTy start out with a copy of the substs of the corresponding TraitRef at creation time, or is it sharing the substs with it in general?

@eddyb
Copy link
Member

eddyb commented May 27, 2017

@tschottdorf How would the two options differ? What do you mean by "sharing"?

@tbg
Copy link
Contributor

tbg commented May 27, 2017

@eddyb Niko mentioned above that for now, the substs in the TraitRef and those in ProjectionTy will be identical, so making a copy isn't strictly necessary. He also mentioned that they're bound to diverge with ATC, so it seems reasonable not to tie the two together.

@eddyb
Copy link
Member

eddyb commented May 27, 2017

Maybe he wasn't clear on the fact that TraitRef in ProjectionTy would be replaced, so the end result only has DefId for the associated type and Substs for it (which currently would be the same that you would find in the TraitRef).

@tbg
Copy link
Contributor

tbg commented May 28, 2017

Sorry, didn't phrase that well - I'm aware that trait_ref will be dropped, what I'm really asking is: is the intended behavior instead of storing the trait_ref, it will store a copy of the substs that are in the trait_ref at the time the ProjectionTy is constructed. Your last sentence answers it though, so my question has been addressed. Thanks @eddyb!

@tbg tbg mentioned this issue May 29, 2017
Merged
tbg added a commit to tbg/rust that referenced this issue May 31, 2017
@tbg
Upgrade ProjectionTy's Name to a DefId
e5e664f 
Part of rust-lang#42171, in preparation for downgrading the contained `TraitRef` to
only its `substs`.
@nikomatsakis nikomatsakis mentioned this issue May 31, 2017
Open
4 tasks
frewsxcv added a commit to frewsxcv/rust that referenced this issue Jun 1, 2017
@frewsxcv
Rollup merge of rust-lang#42297 - tschottdorf:proj-ty, r=nikomatsakis
b3947f8 
Upgrade ProjectionTy's Name to a DefId

Part of rust-lang#42171, in preparation for downgrading the contained `TraitRef` to
only its `substs`.

Some inline questions in the diff. Look for `FIXME(tschottdorf)`. These comments
should be addressed before merging.
frewsxcv added a commit to frewsxcv/rust that referenced this issue Jun 1, 2017
@frewsxcv
Rollup merge of rust-lang#42297 - tschottdorf:proj-ty, r=nikomatsakis
b03ed42 
Upgrade ProjectionTy's Name to a DefId

Part of rust-lang#42171, in preparation for downgrading the contained `TraitRef` to
only its `substs`.

Some inline questions in the diff. Look for `FIXME(tschottdorf)`. These comments
should be addressed before merging.
@tbg tbg mentioned this issue Jun 2, 2017
Merged
tbg added a commit to tbg/rust that referenced this issue Jun 30, 2017
@tbg
Downgrade ProjectionTy's TraitRef to its substs
37610c6 
WIP: please review the FIXME(tschottdorf) code comments, in particular the last
remaining use of `deprecated_trait_ref`.

----

Addresses the second part of rust-lang#42171 by removing the `TraitRef` from
`ProjectionTy`, and directly storing its `Substs`.

Closes rust-lang#42171.
tbg added a commit to tbg/rust that referenced this issue Jul 11, 2017
@tbg
Downgrade ProjectionTy's TraitRef to its substs
fcdc7c3 
Addresses the second part of rust-lang#42171 by removing the `TraitRef` from
`ProjectionTy`, and directly storing its `Substs`.

Closes rust-lang#42171.
tbg added a commit to tbg/rust that referenced this issue Jul 11, 2017
@tbg
Downgrade ProjectionTy's TraitRef to its substs
687ee7f 
Addresses the second part of rust-lang#42171 by removing the `TraitRef` from
`ProjectionTy`, and directly storing its `Substs`.

Closes rust-lang#42171.
bors added a commit that referenced this issue Jul 11, 2017
@bors
Auto merge of #42388 - tschottdorf:projty-substs, r=nikomatsakis
1475e2c 
Downgrade ProjectionTy's TraitRef to its substs

Addresses the second part of #42171 by removing the `TraitRef` from
`ProjectionTy`, and directly storing its `Substs`.

Closes #42171.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-trait-system Area: Trait system C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@eddyb @nikomatsakis @tbg