Rename struct types to ADT types and introduce new interned ADT ID #454
Conversation
| /// TODO: Currently only handles structs, may need more work for enums & unions | ||
| pub fn verify_adt_decl(&self, adt_id: AdtId<I>) -> Result<(), WfError<I>> { |
There was a problem hiding this comment.
I'm not sure if this function generalizes to all ADTs in its current form, so I flagged it with this TODO.
| type DefId: Debug + Copy + Eq + Ord + Hash; | ||
|
|
||
| /// The ID type for ADTs | ||
| type InternedAdtId: Debug + Copy + Eq + Ord + Hash; |
There was a problem hiding this comment.
So, I'm not actually sure why we need this. @nikomatsakis on your issue, you said that for rustc this would be &'tcx AtdDef. But we can access that with tcx.adt_def(DefId). So...I'm not really sure why Adts need their own special id.
There was a problem hiding this comment.
The reason is that, eventually, we want rustc's types to be equal to chalk's types. We could potentially convert rustc to use a DefId and not a &'tcx AdtDef, and I considered that, but it would mean doing a query every time we want to access field information, so it might imply extra overhead.
To be honest, I suspect that we may want to take this same approach for all ids, to give Interner implementations more flexibility in how they manage their ids, but it seems ok to start here.
We could always remove it later if converting rustc to use DefId is better, or we could go further, or leave it as is.
There was a problem hiding this comment.
Hmm. I can understand the sentiment towards wanting to not have to query every time we want to access field information. But I don't think we should do that here, for a couple reasons:
- It's different than what we do for everything else (right now). I would rather things be consistent than trying to optimize for less overhead. (This is, in part, because we don't exactly know how much overhead there would be. And everything else isn't optimized at all.)
- I think this is possibly just wrong. If we want to store the
AdtDef(or any other info for other types), then I think it should be stored inInternedType, not in the equivalent ofDefId.
There was a problem hiding this comment.
I can see the argument for uniformity, but I don't understand the argument for it being wrong. From the perspective of chalk, it is an id -- an id is just something opaque that we give to the RustIrDatabase in exchange for information.
The only difference here is that we've given the Interner (and RustIrDatabase) more license to decide how to represent ids (as I noted above, I suspect we ought to do this for all ids).
There was a problem hiding this comment.
As for performance, it's true that we haven't (in rustc) measured the impact of replacing AdtDef with a DefId. It's also true that when we added AdtDef (instead of def_id), it was a noticeable win. But the compiler was a very different beast back then.
There was a problem hiding this comment.
Well, to be clear, I do think you have a point about performance and uniformity. And I resisted suggesting we introduce this indirection for some time because I wanted to try converting rustc to use a DefId instead for precisely this reason. And I still sort of do. But I also think we can do that work independently by adding the indirection, which is why I wound up suggesting it.
Closes #453
The only functional changes are due to the introduction of the ADT ID, and they are minimal.