Find the builtin TypeCanAggregateDestroy in the FacetType for facet values #6119
Conversation
|
Based on #6107, first commit is |
| // here. | ||
| // | ||
| // See also test: | ||
| // facet_access_type_converts_back_to_original_facet_value.carbon |
There was a problem hiding this comment.
Which directory should I look in for this test? I'm not seeing it in this PR or at trunk.
Maybe include the full path to make it easy to find?
There was a problem hiding this comment.
It's a split in another file, so you have to do a search to find it. I didn't want to put both, it felt like a lot...
There was a problem hiding this comment.
Maybe only put the filename, or just remove the mention? I don't know how I'm supposed to find this, also the name could be duplicated in multiple files.
| auto bind = context.insts().Get(bind_id); | ||
| if (bind.Is<SemIR::BindSymbolicName>()) { | ||
| // If the FacetTypes are the same, then the FacetValue didn't add/remove | ||
| // any witnesses. | ||
| if (bind.type_id() == inst.type_id) { |
There was a problem hiding this comment.
Maybe bundle this together and reduce nesting, such as:
| auto bind = context.insts().Get(bind_id); | |
| if (bind.Is<SemIR::BindSymbolicName>()) { | |
| // If the FacetTypes are the same, then the FacetValue didn't add/remove | |
| // any witnesses. | |
| if (bind.type_id() == inst.type_id) { | |
| // If the FacetTypes are the same, then the FacetValue didn't add/remove | |
| // any witnesses. | |
| if (auto bind = context.insts().TryGetAs<SemIR::BindSymbolicName>(bind_id); bind && bind.type_id() == inst.type_id) { |
There was a problem hiding this comment.
The reduced nesting is nice but I am kind of averse to using the if (.. ; ..) syntax just cuz it's so easy to forget to test the thing on the lhs of the ; which is automatically tested without the ;. So idk if it's better overall..
There was a problem hiding this comment.
You could also move the auto bind out of the if -- mainly I'm suggesting the TryGetAs, and use of && instead of nested if
When doing impl lookup with a constraint facet type including the builtin
TypeCanAggregateDestroy, we look at the type to see if it satisfies it. However if the type is a facet value, we need to look at the FacetType to see if the eventual concrete type is going to satisfy it.Note that we can do this check up front in the
LookupImplWitness()function without creating a symbolic instruction to be modified by future specifics with a more precise type for the facet value, because the result ofTypeCanAggregateDestroydoes not actually provide a witness, so we don't need the final specific type.This was noticed by removing the "shortcut" in convert for converting a
FacetAccessType(<symbolic binding>)totypeof(<symbolic binding>). By removing the shortcut, we go into impl lookup when checkingimpldecls containingTypeCanAggregateDestroyvia deduce.