fix: Test all generic args for trait when finding matching impl #13475
Conversation
|
@bors r+ |
|
I think you should be able to unify the substitutions directly instead of looping manually? That would also take care of consts. |
|
Also, I think we have some bug report(s) that this fixes as well. |
fix: Test all type args for trait when finding matching impl Addresses #13463 (comment) When finding matching impl for a trait method, we've been testing the unifiability of self type. However, there can be multiple impl of a trait for the same type with different generic arguments for the trait. This patch takes it into account and tests the unifiability of all type arguments for the trait (the first being the self type) thus enables rust-analyzer to find the correct impl even in such cases.
|
@flodiebold I was pretty sure there's a way to do that! Are you talking about Could someone r-? |
|
bors r- |
|
@bors r- |
|
☀️ Try build successful - checks-actions |
1 similar comment
|
☀️ Try build successful - checks-actions |
lowr
force-pushed
the
fix/lookup-impl-method-trait-ref
branch
2 times, most recently
from
037208e to
324151d
Compare
lowr
changed the title
lowr
force-pushed
the
fix/lookup-impl-method-trait-ref
branch
from
324151d to
6410bb0
Compare
It seems to me we could just make |
lowr
force-pushed
the
fix/lookup-impl-method-trait-ref
branch
from
6410bb0 to
ed7481a
Compare
|
|
||
| let goal = crate::Goal::all(Interner, wh_goals); | ||
| if !table | ||
| .unify(trait_ref.substitution.as_slice(Interner), expected_subst.as_slice(Interner)) |
There was a problem hiding this comment.
maybe we should just have actual TraitRefs here, then we could just unify those
| db: &dyn HirDatabase, | ||
| env: Arc<TraitEnvironment>, | ||
| trait_: TraitId, | ||
| name: &Name, | ||
| fn_subst: Substitution, |
There was a problem hiding this comment.
kind of weird to be passing the fn subst, but not the actual fn ID, right? we could just pass the trait subst, and then merge that with the trait_ parameter into a TraitRef
There was a problem hiding this comment.
Yeah I feel weird too. This function is called from hir and I didn't want to let hir manipulate Substitutions (besides passing them around as-is). It's just my preference so can we just make new Substitution for the trait out of fn subst in hir layer?
There was a problem hiding this comment.
Hmm I think ideally we'd have some nice functions to get the trait ref from a function subst and so on. I think for now either would be fine.
There was a problem hiding this comment.
Ah, that would be useful and an extension method would be good place for it I guess? Will try adding that.
lowr
force-pushed
the
fix/lookup-impl-method-trait-ref
branch
from
ed7481a to
67f1d8f
Compare
|
So I ended up restructuring the related methods.
@flodiebold I'd appreciate if you could take a look again. I think the weirdness is somewhat alleviated and it's more organized than before. |
|
@bors r+ |
|
☀️ Test successful - checks-actions |
Addresses #13463 (comment)
When finding matching impl for a trait method, we've been testing the unifiability of self type. However, there can be multiple impl of a trait for the same type with different generic arguments for the trait. This patch takes it into account and tests the unifiability of all type arguments for the trait (the first being the self type) thus enables rust-analyzer to find the correct impl even in such cases.