Replace HIR's ItemId structs with aliases #67999
Conversation
Dylan-DPC-zz
added
the
S-waiting-on-review
|
cc @Zoxc |
| // the HIR, since they just signify a HIR nodes own path. But `ItemId` et al | ||
| // are used when another item in the HIR is *referenced* and we certainly | ||
| // want to pick up on a reference changing its target, so we hash the NodeIds | ||
| // in "DefPath Mode". |
There was a problem hiding this comment.
These types do have different impls than HirId. Why can this now be changed?
There was a problem hiding this comment.
Seeing that comment I thought about how much the NodeIds have become deprecated after the lowering and wondered if we could get away with it. If there's a reason we should stick to this solution, though, we can probably still merge all these 3 types into 1, because their impls are identical.
There was a problem hiding this comment.
Yes, I think we should either merge them, or keep them separate if there's a good reason for them to remain separate. I'm not confident about the reason for their being separate in the first place, though, so I'd prefer if someone else clarified this.
There was a problem hiding this comment.
There was a problem hiding this comment.
Merging the 3 types into 1 should be fine. Using an alias would be a change in semantics, leading to things being ignored during hashing.
There was a problem hiding this comment.
Any suggestion on the name? Would ItemId be fine?
|
☔ The latest upstream changes (presumably #68034) made this pull request unmergeable. Please resolve the merge conflicts. |
JohnCSimon
added
S-waiting-on-author
|
Ping from triage: @ljedrz can you please address the merge conflicts? |
|
Pinging again from triage: @ljedrz can you please address the merge conflicts? |
|
@JohnCSimon I'm still waiting for further instructions from the compiler team; see varkor's comment. |
|
I actually don't think this change is an improvement. New-typing things in order to avoid accidental mixups is a common technique for avoiding bugs. I personally would leave the types as they are unless there is a strong reason to change them. You could collapse the |
|
Closing in favor of #68858. |
…=michaelwoerister Merge item id stable hashing functions Supersedes rust-lang#67999 splitting out the pure cleanup bits, i.e. merging `hash_item_id`, `hash_impl_item_id` and `hash_trait_item_id` into a common `hash_reference_to_item`. r? @michaelwoerister
I'm not 100% sure this is a valid approach (and if not, then perhaps all 3 could be merged into a single
ItemId), but it passes the tests locally.It appears that we can "downgrade" HIR's
ImplItemId,ItemIdandTraitItemIdto just aliases ofHirIdwhich is a cleanup on its own and opens up further cleanup opportunities.r? @varkor