Skip to content

Conversation

danakj
Copy link
Contributor

@danakj danakj commented Sep 19, 2025

If a BindSymbolicName is converted to type and then to its exact FacetType, we get a FacetValue wrapping the BindSymbolicName but providing no different information: it has the same witnesses and FacetType as the original BindSymbolicName. Yet it is a different constant value, creating multiple canonical forms with the same meaning. Now we make that FacetValue with the same FacetType as the BindSymbolicName it wraps evaluate back to the BindSymbolicName, making it the unique canonical form.

This makes the "shortcut" in convert for avoiding impl lookup when converting from FacetAccessType to FacetType in this exact scenario work the same as doing the full impl lookup.

@danakj danakj force-pushed the facetaccesstype-roundtrip branch from 24137ba to 66a28e9 Compare September 22, 2025 14:44
@danakj danakj requested a review from zygoloid September 22, 2025 21:41
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

New test looks good but I think theres'a bug to be fixed here too.

@danakj
Copy link
Contributor Author

danakj commented Sep 23, 2025

PTAL

@danakj danakj requested a review from zygoloid September 23, 2025 14:26
@danakj danakj changed the title Document the lossless round-trip through FacetAccessType back to the original facet value as being meaningful Make BindSymbolicName the canonical form of a FacetValue wrapping the BindSymbolicName Sep 23, 2025
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! I assume the new test also passes with the shortcut removed now? [Edit: I see you mentioned that it does in another comment]

Co-authored-by: Richard Smith <[email protected]>
@danakj danakj enabled auto-merge September 23, 2025 21:41
@danakj danakj added this pull request to the merge queue Sep 23, 2025
Merged via the queue into carbon-language:trunk with commit 82679e6 Sep 23, 2025
8 checks passed
@danakj danakj deleted the facetaccesstype-roundtrip branch September 23, 2025 22:40
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