cgen: fix option reference string interpolation - #27760
Conversation
|
This looks good for the direct ?int case, but one case mentioned in #27752 is still missing, an alias such as Could you also handle the aliased option type and add a regression case for it? After that, the issue should be fully covered. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
lots of ci errors |
|
Updated this to cover the aliased option case from the review. What changed:
Verification run locally:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
The red CI jobs appear to share the same root cause, the chained Can you fix it plz? |
|
Updated to avoid the Verification run locally:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 878448e175
ℹ️ 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".
| exp_typ = | ||
| g.table.fully_unaliased_type(expr.right_type).clear_option_and_result().ref().set_flag(.option) |
There was a problem hiding this comment.
Preserve option payload aliases when building the temp
When val has type ?Alias and Alias defines its own str(), this rebuilds the interpolation temporary from fully_unaliased_type(expr.right_type), turning the semantic ?&Alias into ?&<parent>. The g.get_str_fn(exp_typ) call below then generates/calls the parent type's stringifier instead of the alias override, so ${&val} silently loses the alias-specific formatting even though plain ${val} preserves it; only the option detection needs unaliasing, while the temporary type should keep the original payload alias when the option flag is already present.
Useful? React with 👍 / 👎.
| mut option_payload_ref_tmp := false | ||
| if expr is ast.PrefixExpr { | ||
| right_expr := expr.right.remove_par() | ||
| right_type := g.table.fully_unaliased_type(expr.right_type) | ||
| if expr.op == .amp && right_type.has_flag(.option) { |
There was a problem hiding this comment.
Hoist option-ref lowering before enum stringification
Because this option-reference lowering is added only inside the later auto-string branch, option references whose payload symbol is handled by an earlier branch never use it. For example, with enum Color { red } and val := ?Color(.red), ${&val} enters the sym.kind == .enum path above before this block and still passes the original ?Color storage to the ?&Color stringifier, leaving this fix incomplete for option enum payloads.
Useful? React with 👍 / 👎.
| right_type := g.table.fully_unaliased_type(expr.right_type) | ||
| if expr.op == .amp && right_type.has_flag(.option) { |
There was a problem hiding this comment.
Resolve generic right types before detecting option refs
In generic instantiations where T is an option, the prefix expression's right_type can still be the generic parameter while the interpolation expression type has already been resolved for the concrete call. Testing expr.right_type directly leaves option_payload_ref_tmp false for ${&val} inside fn f[T](val T) called as f[?int](...), so it falls back to the old address-of emission instead of the new option-payload temporary; resolve/unwrap the right type before checking the option flag.
Useful? React with 👍 / 👎.
Fixes #27752
This fixes C generation for string interpolation of references to option values, such as
${&val}wherevalis?int.The interpolation stringification path now materializes the existing option-payload-reference lowering into a temporary before adding the generated nil check/string call, avoiding invalid generated C such as
&&val.data.A regression test covers both a payload option and
none.Tests