removing new_from_def_id and alias_ty_kind_from_def_id methods, small refactor to TypeRelativePath::AssocItem#155323
removing new_from_def_id and alias_ty_kind_from_def_id methods, small refactor to TypeRelativePath::AssocItem#155323josetorrs wants to merge 2 commits intorust-lang:mainfrom
new_from_def_id and alias_ty_kind_from_def_id methods, small refactor to TypeRelativePath::AssocItem#155323Conversation
rustbot
added
S-waiting-on-author
|
|
| #[derive(Debug, Clone, Copy)] | ||
| enum TypeRelativePath<'tcx> { | ||
| AssocItem(DefId, GenericArgsRef<'tcx>), | ||
| AssocItem(ty::AliasTyKind<'tcx>, GenericArgsRef<'tcx>), |
There was a problem hiding this comment.
This is misleading, AssocItem can represent associated constants under mGCA.
You can see yourself how in lower_type_relative_const_path you merely extract the DefId and ignore the actual kind.
Conceptually this (DefId, GenericArgRef) is an AliasTerm.
If anything, the entire payload should either be changed to AliasTerm or to (AliasTermKind, GenericArgsRef) if we don't want to intern it too early or it should just remain as is if the other options end up disimproving the lowering code.
There was a problem hiding this comment.
yeah I think the AliasTerm change makes sense.
this may be a dumb question but when you mention the other option: (AliasTermKind, GenericArgsRef). I looked at it here and it doesn't seem to have a DefId though it appears to be the long term goal based on the last task in the linked issue:
remove
def_idparam ofopt_alias_variancesafterAliasTermKindcontainsdef_idwithin
did you mean (AliasTermKind, DefId, GenericArgsRef)?
There was a problem hiding this comment.
Ah, not a dumb question at all! I'm only loosely following #154941, so I didn't realize that AliasTermKind wasn't updated yet and just assumed it was.
Right, (AliasTermKind, DefId, GenericArgsRef) would be an option or we wait until AliasTermKind has a DefId. I don't want to call the shots here though since WaffleLapkin guides this initiative.
I'm just the self-proclaimed janitor of HIR ty lowering :)
There was a problem hiding this comment.
updated to AliasTerm here: dc59a2a
though now that I'm looking at it, it's still using the methods new_from_def_id and alias_ty_kind_from_def_id
There was a problem hiding this comment.
i raised #155371 as an alternative for the (AliasTermKind, DefId, GenericArgsRef) option. lmk what you think
| mode.assoc_tag(), | ||
| )? { | ||
| return Ok(TypeRelativePath::AssocItem(did, args)); | ||
| return Ok(TypeRelativePath::AssocItem(ty::Inherent { def_id }, args)); |
There was a problem hiding this comment.
No, def_id can just as well refer to a type-level inherent associated constant.
| } | ||
|
|
||
| Ok(TypeRelativePath::AssocItem(item_def_id, args)) | ||
| Ok(TypeRelativePath::AssocItem(ty::Projection { def_id: item_def_id }, args)) |
There was a problem hiding this comment.
No, item_def_id can just as well refer to a type-level associated constant.
|
Reminder, once the PR becomes ready for a review, use |
|
HIR ty lowering was modified cc @fmease |
rustbot
added
S-waiting-on-review
4 participants
r? @WaffleLapkin
related issue: #154941
tackling the task:
new_from_def_idandalias_ty_kind_from_def_idand replace them with calls to e.g.new_projectiondirectlyin
probe_inherent_assoc_itemonly searches inherent impls here, so the result is always Inherent. inresolve_type_relative_paththe result is always Projection.