-
-
Notifications
You must be signed in to change notification settings - Fork 14.9k
Use special DefIds for aliases #155981
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
base: main
Are you sure you want to change the base?
Use special DefIds for aliases #155981
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,14 +22,14 @@ where | |
| let cx = self.cx(); | ||
| let inherent = goal.predicate.alias; | ||
|
|
||
| let impl_def_id = cx.parent(inherent.def_id()); | ||
| let impl_args = self.fresh_args_for_item(impl_def_id); | ||
| let impl_def_id = cx.impl_assoc_term_parent(inherent.def_id().try_into().unwrap()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see #155392 (comment), we should open an issue for this unless you're up to update this in this PR
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what I meant by:
Do you want me to try that and check perf?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like that, we match on ht e
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit difficult to do on the |
||
| let impl_args = self.fresh_args_for_item(impl_def_id.into()); | ||
|
|
||
| // Equate impl header and add impl where clauses | ||
| self.eq( | ||
| goal.param_env, | ||
| inherent.self_ty(), | ||
| cx.type_of(impl_def_id).instantiate(cx, impl_args).skip_norm_wip(), | ||
| cx.type_of(impl_def_id.into()).instantiate(cx, impl_args).skip_norm_wip(), | ||
| )?; | ||
|
|
||
| // Equate IAT with the RHS of the project goal | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is surprising to me in the face of associated type/const defaults
this should fail in r-a because
fetch_eligible_assoc_itemwould ry to return aDefinitionAssocTermId?View changes since the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r-a does not really have a distinction between trait assoc types and impl assoc types, all type aliases are the same. I considered reflecting that in the solver but decided to put more type safety at almost no cost.
Although... now that you bring it up, we expect the parent of an
ImplAssocTermIdto be an impl (impl_assoc_term_parent()), which could cause problems. How does rustc handle that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's mainly that we very rarely refer to impl associated types and just never care about whether its parent is an impl or trait 🤔
if that distinction doesn't exist in r-a either, i'd prefer to keep em as the same for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the separation completely will be a bit unfortunate since while we don't know whether the parent of an
ImplAssocTermIdis an impl or trait, we do know that the parent ofDefinitionAssocTermIdis a trait, and we rely on this inInterner::projection_parent()to get aTraitId.But maybe, we can call it with a more generic name (e.g.
AssocTermIdinstead ofImplAssocTermId) and return a genericDefIdas its parent, while not touchingDefinitionAssocTermId?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can call it something like
ConcreteAssocTermId, since we don't want trait assoc types without default to end there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ImplOrTraitAssocTermId🤔 😅 I thinkConcreteAssocTermIdis also not quite what you want asfetch_eligible_assoc_itemdoes just look at assoc terms regardless of whether they have a valueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong objection to
ImplOrTraitAssocTermId, except that it's long 😅