Skip to content

Conversation

danakj
Copy link
Contributor

@danakj danakj commented Sep 23, 2025

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 of TypeCanAggregateDestroy does 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>) to typeof(<symbolic binding>). By removing the shortcut, we go into impl lookup when checking impl decls containing TypeCanAggregateDestroy via deduce.

@github-actions github-actions bot requested a review from zygoloid September 23, 2025 14:36
@danakj danakj requested a review from jonmeow September 23, 2025 14:37
@danakj
Copy link
Contributor Author

danakj commented Sep 23, 2025

Based on #6107, first commit is mask-facet-value

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG, thanks!

@danakj danakj requested a review from a team as a code owner October 2, 2025 18:09
@danakj danakj requested review from geoffromer and removed request for a team and geoffromer October 2, 2025 18:09
@danakj danakj enabled auto-merge October 2, 2025 18:20
@danakj danakj added this pull request to the merge queue Oct 2, 2025
Merged via the queue into carbon-language:trunk with commit 0c761a9 Oct 2, 2025
8 checks passed
@danakj danakj deleted the mask-facet-value branch October 2, 2025 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants