bugfix(sierra-generator): represent user-defined phantom types as never - #10120
Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryMedium Risk Overview
New function-generator expectations cover Reviewed by Cursor Bugbot for commit 60c7d62. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 769bb15e72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ver` A `#[phantom]` struct/enum had no Sierra representation, so a type containing one (e.g. `Option<Ph>` built via `None`, including through a generic instantiation) failed to specialize and panicked the Sierra generator instead of compiling. Since a phantom type is uninhabited, represent user-defined phantom structs/enums as `never` (an empty, representable enum) in `get_concrete_type_id`, so containers of them specialize. Extern phantom types are left as-is (their `Phantom` long id): their identity is meaningful to the libfuncs that consume them, e.g. circuit gates. Fixes #10113.
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on eytan-starkware and TomerStarkware).
769bb15 to
60c7d62
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 60c7d62. Configure here.
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on eytan-starkware and TomerStarkware).
orizi
left a comment
There was a problem hiding this comment.
@orizi resolved 1 discussion.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on eytan-starkware and TomerStarkware).
TomerStarkware
left a comment
There was a problem hiding this comment.
@TomerStarkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on eytan-starkware).
Summary
User-defined phantom structs and enums are now represented as the
nevertype in Sierra, rather than using the dedicatedPhantomlong id. This allows containers such asOption<Ph>(wherePhis a#[phantom]enum) to specialize correctly without causing an ICE. Extern phantom types retain theirPhantomlong id, since their identity is required by the libfuncs that consume them (e.g. circuit gates).Type of change
Please check one:
Why is this change needed?
When a user-defined phantom type (struct or enum) appeared inside a generic container like
Option<Ph>, the Sierra generator would ICE because the phantom type could not be represented in a way that allowed the container to specialize. Since user-defined phantom types are uninhabited, mapping them toneveris semantically correct and allows the container to specialize without issues.What was the behavior or documentation before?
User-defined phantom structs and enums were given a dedicated
Phantomlong id, the same as extern phantom types. Placing them inside a generic container (e.g.Option<Ph>) caused an ICE during Sierra generation.What is the behavior or documentation after?
User-defined phantom structs and enums are mapped to the
nevertype in Sierra. A container such asOption<Ph>now generates valid Sierra code, with theSomebranch represented asenum_match<core::never>(an uninhabited match). Extern phantom types are unaffected and continue to use thePhantomlong id.Related issue or discussion (if any)
Fixes #10113
Additional context
A new test case (
Test a user-defined phantom type in a container specializes (represented as never)) was added togenericsto cover theOption<Ph>pattern and verify the generated Sierra output.