Get specific interfaces with correct specific from named constraints #6435
Conversation
danakj
force-pushed
the
specific-named-constraints
branch
from
13d2bed to
d44f699
Compare
danakj
requested review from
a team and
jonmeow
and removed request for
a team and
dwblaikie
toolchain/check/type_completion.cpp
Outdated
| auto require_specific_id = MakeCopyOfSpecificAndAppendSelf( | ||
| context, loc_id, impls.specific_id, require.generic_id, | ||
| GetRequireImplsSpecificSelf(context, require)); | ||
| auto facet_type_id = GetFacetTypeInSpecific( | ||
| context, require.facet_type_inst_id, require_specific_id); |
There was a problem hiding this comment.
The way these are composed, it seems like the APIs are intended to be generic, but it seems like (for example) MakeCopyOfSpecificAndAppendSelf and GetRequireImplsSpecificSelf are tightly associated since they're both assuming operation on the same structure. Had you considered a single function that took in require and returned facet_type_id?
e.g. auto facet_type_id = GetFacetTypeInRequireImpls(context, loc_id, impls.specific_id, require);
There was a problem hiding this comment.
Yeah I was just trying to break things into smaller pieces. I have been thinking about how I will need to do this same operation in more places. In particular, in an impl declaration, we will need to find all the required interfaces (in the impl's specific facet type) and ensure they have been implemented already. So I think this will need to become a single operation that is exposed elsewhere, probably from sem_ir/require_impls.h. But for now, leaving it here.
I can combine some of these steps.
toolchain/check/type_completion.cpp
Outdated
| auto require_specific_id = MakeCopyOfSpecificAndAppendSelf( | ||
| context, loc_id, impls.specific_id, require.generic_id, | ||
| GetRequireImplsSpecificSelf(context, require)); | ||
| auto facet_type_id = GetFacetTypeInSpecific( | ||
| context, require.facet_type_inst_id, require_specific_id); |
There was a problem hiding this comment.
Yeah I was just trying to break things into smaller pieces. I have been thinking about how I will need to do this same operation in more places. In particular, in an impl declaration, we will need to find all the required interfaces (in the impl's specific facet type) and ensure they have been implemented already. So I think this will need to become a single operation that is exposed elsewhere, probably from sem_ir/require_impls.h. But for now, leaving it here.
I can combine some of these steps.
2 participants
When forming an IdentifiedFacetType, we collect interfaces named by require decls in named constraints that the facet type refers to. These interfaces come with a specific, but the require decl is inside an named constraint which may be generic. So we need the specific being applied to the containing named constraint to also be applied to the require decl and its target interfaces.
This uncovered that the facet type in require decls was not being imported correctly, as it was not being attached to the require decl's generic. This is fixed by making import of RequireImplsDecl multiphase, so that the decl instruction exists before we resolve the facet type within it. And by pointing the generic importing machinery to the RequireImplsDecl, and from there to the RequireImpls structure to get the generic id.
Then
ImplStore::GetOrAddLookupBucketcan use an IdentifiedFacetType to correctly get the interface being impl'd, both in the local and the imported named constraint case. Which allows us to correctly diagnose redeclarations in the impl file of an impl of an interface through a named constraint. And to correctly not diagnose them when the specific in the generic named constraint differs from other decls.