Use special DefIds for aliases#155981
Conversation
Like we do for other things for better experience in rust-analyzer. It's possible now that the `AliasTyKind` and `AliasTermKind` contains the DefId. It does require a few `try_into().unwrap()`s since in the solver's `consider_X_candidate()` only get an untyped `DefId`. It's possible to reduce that considerably if we'd pass them the typed def id as a parameter, but I don't know what will be the impact on perf.
rustbot
added
S-waiting-on-review
|
|
||
| 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()); |
There was a problem hiding this comment.
see #155392 (comment), we should open an issue for this unless you're up to update this in this PR
There was a problem hiding this comment.
That's what I meant by:
It does require a few try_into().unwrap()s since in the solver's consider_X_candidate() only get an untyped DefId. It's possible to reduce that considerably if we'd pass them the typed def id as a parameter, but I don't know what will be the impact on perf. Should I try to pursue that?
Do you want me to try that and check perf?
There was a problem hiding this comment.
I'd like that, we match on ht eAliasTermKind anyways, so passing in some (SpecificDefId, Args) as the goal to the different functions seems better to me?
There was a problem hiding this comment.
It's a bit difficult to do on the consider_X_candidate() method (not the method in the submodules but in mod.rs), since they are called from the generic assemble_and_merge_candidates(). Should I do that for them too?
| Err(TypeError::ProjectionMismatched(ExpectedFound::new( | ||
| a.def_id.into(), | ||
| b.def_id.into(), | ||
| ))) |
There was a problem hiding this comment.
can you update TypeError instead?
| impl_def_id: <Self::Interner as Interner>::ImplId, | ||
| ) -> Result< | ||
| Option<<Self::Interner as Interner>::DefId>, | ||
| Option<<Self::Interner as Interner>::ImplAssocTermId>, |
There was a problem hiding this comment.
this is surprising to me in the face of associated type/const defaults
trait Trait {
type Assoc = u32;
}
impl Trait for () {}this should fail in r-a because fetch_eligible_assoc_item would ry to return a DefinitionAssocTermId?
There was a problem hiding this comment.
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 ImplAssocTermId to be an impl (impl_assoc_term_parent()), which could cause problems. How does rustc handle that?
There was a problem hiding this comment.
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.
Removing the separation completely will be a bit unfortunate since while we don't know whether the parent of an ImplAssocTermId is an impl or trait, we do know that the parent of DefinitionAssocTermId is a trait, and we rely on this in Interner::projection_parent() to get a TraitId.
But maybe, we can call it with a more generic name (e.g. AssocTermId instead of ImplAssocTermId) and return a generic DefId as its parent, while not touching DefinitionAssocTermId?
There was a problem hiding this comment.
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.
ImplOrTraitAssocTermId 🤔 😅 I think ConcreteAssocTermId is also not quite what you want as fetch_eligible_assoc_item does just look at assoc terms regardless of whether they have a value
There was a problem hiding this comment.
I don't have a strong objection to ImplOrTraitAssocTermId, except that it's long 😅
3 participants
Renewal of #155025, after
AliasTermKindwas also ported.Like we do for other things for better experience in rust-analyzer.
It's possible now that the
AliasTyKindandAliasTermKindcontains the DefId.It does require a few
try_into().unwrap()s since in the solver'sconsider_X_candidate()only get an untypedDefId. It's possible to reduce that considerably if we'd pass them the typed def id as a parameter, but I don't know what will be the impact on perf. Should I try to pursue that?r? lcnr