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

Move ids to type family #309

Merged
merged 4 commits into from Dec 17, 2019

Conversation

@nikomatsakis
Copy link
Collaborator

nikomatsakis commented Dec 13, 2019

This PR moves the RawId type into the type family. It is presented as TF::DefId, mirroring the name that rustc uses. The goal here is to allow rustc to map this to DefId.

For now, I kept the various newtypes (e.g., StructId and so forth) exactly as they are. At some point we are going to move to move things like TypeName so that we have an "interned" TypeName, but I'm not 100% sure about the various newtypes, they may want to stay like this, or else move into the TypeFamily, or maybe we just get rid of them (but I think it's nice to have the distinctions between various kinds of ids).

I think a good rule though would be that TypeName should be opaque to most of the chalk logic (apart perhaps from the clause generation logic, which would interact with it through methods on RustIrDatabase), which implies that we may want to extract some of its variants out into TypeName.

Nicholas Matsakis added 3 commits Dec 13, 2019
Motivation: Avoid manually inspecting the variants of `TypeName` since
it will eventually become opaque.
This will be used for interconverting ids between type families.  It
may be easier, of course, to just give up on the idea of having
generic type families (that map to the same underlying family at
runtime) but I've not given up on the idea *quite yet*.
@nikomatsakis

This comment has been minimized.

Copy link
Collaborator Author

nikomatsakis commented Dec 13, 2019

@detrumi I think you were poking at making a change like this, maybe you'd care to review?

Nicholas Matsakis
@nikomatsakis nikomatsakis force-pushed the nikomatsakis:move-ids-to-type-family branch from 6275a07 to 24ed1ee Dec 14, 2019
Copy link
Contributor

jackh726 left a comment

Like I said on Zulip, I don't really know what the direction for these changes is, but I looked over this anyways. Just the one comment, but everything else looks good.

}

impl SpecializationPriorities {
impl<TF: TypeFamily> SpecializationPriorities<TF> {
pub fn new() -> Self {

This comment has been minimized.

Copy link
@jackh726

jackh726 Dec 16, 2019

Contributor

If SpecializationPriorities derives Default, why is this needed?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Dec 17, 2019

Author Collaborator

True, we don't need this, we can write ::default...

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Dec 17, 2019

Author Collaborator

Ah, I remember. The problem is that TF doesn't necessarily implement Default. The fix would be either:

  • to manually implement Default for SpecializationPriorities<TF> (for any TF, not just TF: Default)
  • to require that all TypeFamily: Default, as we did with Clone and various other traits

The former is more typing that new which I think is why I didn't do it. The latter is actually a fine strategy so long as type families remain "dummy" types that just pull together a bunch of other logic. But I think that eventually we will want a TypeFamily to actually be an arena, in which case it may not want to implement Default. (i.e., in rustc, I expect a type family to be the tcx). That said, we could also make it so that you have TF::Context to represent the arena value -- but I think that may be more annoying in the end. I thought it'd be useful if we actually make a TF be the context itself, since that will help with type inference.

(I contemplated renaming TF to TCX... but I'm not sure we plan to keep that name in the compiler.)

@nikomatsakis

This comment has been minimized.

Copy link
Collaborator Author

nikomatsakis commented Dec 17, 2019

I'm going to go ahead and merge this as is for now. Seems like the question of Default is not a major problem.

@nikomatsakis nikomatsakis merged commit 522d4d2 into rust-lang:master Dec 17, 2019
6 checks passed
6 checks passed
Test (stable)
Details
Test (stable)
Details
Test (nightly)
Details
Test (nightly)
Details
Format
Details
Format
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.