Support impl lookup for multiple interfaces in a facet type #5047
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If the query facet type has more than one interface, we must find an impl that provides that interface for the query type for each interface. This just looks like a for loop over the interfaces and ensuring we found one impl witness for every one. However the impl matching must change since it can't look at the constant value of the entire query facet type for comparison with the impl, as that query facet type may be for multiple interfaces and we are looking to match an impl of a single interface. To do this we break the query facet type up into each interface and make sure the interface ids match. Then ensure that the impl was able to deduce any generic parameters using the specific of the single query interface. There are some TODOs left here: 1. If the facet type for the query or the impl constraint has "other_requirements" then we can't verify that they match since they are lost. We fall back to comparing the constant id of the query to the impl's constraint (after deducing generics in the impl). This correctly eliminates mismatches but eagerly eliminates impls that could match the query interface as well when there's more than one interface in the query. 2. We don't return a witness for every interface in the query facet type. Since we can't demonstrate any use of the witness there yet, for cases that can have more than one interface in the query facet type, this doesn't break anything that was previously working. The return value is currently treated as a bool for cases with multiple interfaces in the facet type (as a test for "can this be converted") but the converted-to facet value's witnesses are unused.
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
josh11b
reviewed
Mar 4, 2025
josh11b
reviewed
Mar 4, 2025
| // `impl T as ...` for some other type `T` and should not be considered. | ||
| auto deduced_self_const_id = SemIR::GetConstantValueInSpecific( | ||
| context.sem_ir(), specific_id, impl.self_id); | ||
| if (type_const_id != deduced_self_const_id) { |
There was a problem hiding this comment.
Was this addressed? I think this would be a useful test.
| auto deduced_constraint_id = | ||
| context.constant_values().GetInstId(SemIR::GetConstantValueInSpecific( | ||
| context.sem_ir(), specific_id, impl.constraint_id)); | ||
| if (deduced_constraint_id == SemIR::ErrorInst::SingletonInstId) { |
There was a problem hiding this comment.
I think I don't understand the comment and an example would help me figure out what I was missing.
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
|
The conversation tab seems to not be including my replies to the last 2 comments, idk what's going on with github. So just linking them here: https://github.com/carbon-language/carbon-lang/pull/5047/files#r1980184279 https://github.com/carbon-language/carbon-lang/pull/5047/files#r1980207616 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the query facet type has more than one interface, we must find an impl that provides that interface for the query type for each interface. This just looks like a for loop over the interfaces and ensuring we found one impl witness for every one.
However the impl matching must change since it can't look at the constant value of the entire query facet type for comparison with the impl, as that query facet type may be for multiple interfaces and we are looking to match an impl of a single interface.
To do this we break the query facet type up into each interface and make sure the interface ids match. Then ensure that the impl was able to deduce any generic parameters using the specific of the single query interface.
There are some TODOs left here:
If the facet type for the query or the impl constraint has "other_requirements" then we can't verify that they match since they are lost. We fall back to comparing the constant id of the query to the impl's constraint (after deducing generics in the impl). This correctly eliminates mismatches but eagerly eliminates impls that could match the query interface as well when there's more than one interface in the query.
We don't return a witness for every interface in the query facet type. Since we can't demonstrate any use of the witness there yet, for cases that can have more than one interface in the query facet type, this doesn't break anything that was previously working. The return value is currently treated as a bool for cases with multiple interfaces in the facet type (as a test for "can this be converted") but the converted-to facet value's witnesses are unused.