Fix constant folding of overlapping unboxed variant matches - #8631
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Signed-off-by: Christoph Knittel <ck@cca.io>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b0d0ba91e
ℹ️ 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".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8631 +/- ##
==========================================
+ Coverage 77.55% 77.58% +0.02%
==========================================
Files 475 475
Lines 63904 64051 +147
==========================================
+ Hits 49562 49693 +131
- Misses 14342 14358 +16
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8631 |
cristianoc
left a comment
cristianoc
left a comment
There was a problem hiding this comment.
Thanks for digging this one out — the diagnosis is right and the fix is sound. I spent some time on it and have data that I think argues for a different shape, plus some findings about the bug that go beyond #6950. Sharing it all rather than just an opinion.
Everything below is on branch repr-untagged-canonical-constants so you can read the actual diff and run the tests — the interesting file is tests/tests/src/unboxed_variant_fold_test.res and its checked-in .mjs, where the left of each comparison is a folded literal and the right is a real call.
The bug is pre-existing, not from the recent lambda.ml work
git log -L on the fold points at four commits from the last few days, which makes it look recent. It isn't. The same fold was in Lam.switch at v11.1.3:
| Lconst (Const_block (i, _, _)) ->
(try Ext_list.assoc_by_int lam_switch.sw_blocks i lam_switch.sw_failaction
with _ -> Lswitch(lam, lam_switch))Keyed by integer tag then, by block_runtime now (108aa8f06), but both select the branch from the source constructor and never consult the untagged runtime representation. The recent commits moved and re-keyed it; they didn't introduce it. #6950's playground repro on v11.1.3 confirms this independently.
It's six bugs, not one
I built a differential harness: compute each value twice, once folded and once through an @inline(never) boundary so the generated dispatch computes it, then compare at runtime. 27 cases. On master:
MISMATCH 1: folded=primary runtime=not Color <- #6950
MISMATCH 2: folded=secondary runtime=not Color
MISMATCH 5: folded=number runtime=one <- int overlap
MISMATCH 15: folded=float runtime=one <- @as(1) vs a float payload
MISMATCH 20: folded=a runtime=lit <- literal reachable only via a default arm
MISMATCH 24: folded=w runtime=x <- nested unboxed variant
6 MISMATCHES
Two of these are mechanisms worth calling out. #15: @as(1) is Int 1 and a float payload of 1.0 is Float "1." — different literal tags, one JS number. Any fix keyed on tag equality rather than runtime equality gets this wrong. #24 requires seeing through a nested unboxed constant. Your fix handles both (it bails on everything), but neither is in the test file, and #15 in particular is the kind of thing that would come back if the bail-out is ever narrowed.
I'd suggest landing this harness as a fixture whatever else happens — it's what turned one known bug into six, and it pins the fold against the actual generated dispatch rather than against a hand-written expectation.
Where the fix over-reaches
The new arm bails for every constant untagged block, but the unsound set is much narrower: it needs (a) literal-tagged constant constructors to exist at all, and (b) the payload's value to coincide with one. Both are decidable from matching_facts.literal_tags, which is already in sw_dispatch.
The clearest casualty is the canonical untagged variant:
@unboxed type t = A(int) | B(string)literal_tags = [], so is_a_literal_case is false and dispatch is pure typeof. Overlap is impossible; folding is disabled anyway. Same for @as(null) Nothing | Value(string) — null can never be a string. And in the overlapping type itself, only the overlapping values are affected: the PR's own snapshot has let foldedBlue = colorName("blue") where "blue" folded correctly before.
One knock-on that isn't obvious: lam_analysis.ml:239 uses Lambda.switch's folding power as an inlining heuristic —
| Lswitch (Lvar v, switch) ->
size (Lambda.switch lam switch) < small_inline_sizeso a fold that always returns the full switch also stops these functions being inlined. The cost isn't only the missed constant.
Why the fold couldn't have been right
Worth stating because it decides what "fixing" means here. lambda_switch carries two discriminants at different scopes:
- the case keys (
sw_consts/sw_blocks) come from the patterns in this match (matching.ml:2097) sw_dispatchcomes from the type declaration (matching.ml:2108,matching_facts layout)
In colorName, the keys are sw_blocks = [Color -> name] and a failaction. Nothing in the keys mentions "primary" or "secondary" — those constructors aren't in the patterns. Only literal_tags knows they exist, which is exactly why codegen needs it to emit value === "primary" || value === "secondary".
So the fold was reading a source that provably does not contain the information required. That's a representation defect rather than a coding slip, and it's why I'd rather correct the decision than remove it.
An alternative that keeps the folding
Two steps, built and passing. Diff is +135/-5 across lambda.ml, lambda.mli, translcore.ml.
1. Canonical constants. An untagged constructor has no runtime existence — js_dump.ml:974 erases it to its payload anyway. So erase it at the single construction site (translcore.ml:1196) via a normalizing smart constructor:
let const_block (tag_info : tag_info) (args : structured_constant list) =
match (tag_info, args) with
| Blk_constructor {runtime = {untagged = true}}, [payload] -> payload
| _ -> Const_block (tag_info, args)This isn't new here — const_constructor already does exactly this for integers ("a constructor represented as a number is a genuine number at runtime, so folding sees it as an ordinary integer"). It's the same principle applied to the case it was never extended to. After it, Color("primary") and Primary are the same constant and the wrong answer is unrepresentable.
Nice confirmation that this is the right layer: step 1 alone fixed the int case and kept it folded (numberName(Number(1)) -> "one"), because the erased constant landed in the existing Const_int arm, which was already value-based. The correct algorithm was already in the file, on the path where canonicalization had already happened.
2. Value-based fold. With no constructor left to consult, Lambda.switch has to classify the runtime value against sw_dispatch — literals first, then the payload's shape, the same order Dynamic_checks emits. Three-way, because soundness needs "cannot tell" kept apart from "no case matches":
type value_kind =
| Is_literal of Variant_runtime.literal_tag (* may be a declared literal *)
| Not_a_literal (* an object; no literal is one *)
| Unknown_value (* cannot say: the switch stays *)Unknown_value covers bigint, Const_char, Const_some — conservative, not complete.
Results. All 27 differential cases agree. Every fold preserved, including foldedBlue, A(int) | B(string), and the nested case. make test (276 ounit + 547 + 744 mocha, 629 modules), make test-syntax and make checkformat pass. The stdlib's generated JS is unchanged.
One test snapshot moves, and it's an improvement rather than a regression — VariantCoercion.mjs drops a redundant binding, because an untagged constant is now an ordinary constant and gets inlined rather than bound:
-let c = 100;
- c: c,
+ c: 100,That one snapshot aside, nothing else in the suite changed, which says the existing tests never exercised these paths — and is why this survived two years.
On the tests in this PR
Two suggestions, offered as such:
-
The ounit test. Its real value is the tagged cases (folding didn't become over-broad) and that it pins
Lambda.switchindependently of whether the inliner still hands it this shape. The untagged assertions are close to a restatement of the new match arm —switch arg sw = Lswitch (arg, sw)— against hand-builtVariant_runtimevalues, and they never check the actual symptom, which is a wrong runtime value. The differential test does check that. -
The ast-mapping fixture.
tests/syntax_tests/data/ast-mapping/guards the frozen v0 AST bridge, and AGENTS.md asks for a fixture there when changingparsetree.ml. This PR changes onlylambda.ml.@unboxedwith@as("...")/@as(1)tags is also already covered byVariantConstructorTags.resin that same directory. The only thing the new snapshot records is the printer adding a leading bar. I'd drop it.
Suggested way forward
- Canonical constants + value-based fold, instead of the bail-out. Keeps every fold the bail-out gives up, and the inlining heuristic with it.
- Land the differential harness as the regression fixture — it's the thing that found the other five.
- Follow-up, independent:
Variant_runtime.block = {runtime; block_type: block_type option}still makesuntagged = true, block_type = Nonerepresentable. Turning it intoTagged of tag | Untagged of block_typeremoves bothassert falses that guard it (lam_compile.ml:187,js_dump.ml:975— the latter carries a TODO already saying the type should have ruled it out). - Unrelated but cheap:
lam_analysis.mlrecognizesLswitchandLifthenelsefor the fold-on-inline heuristic but notLstringswitch. I tried adding it and constant string switches start folding — a missed opportunity on every string switch, six lines, independent of all of the above.
Happy to turn the branch into a PR against yours, or into an alternative for side-by-side comparison — whichever is more useful to you. And if you'd rather keep this PR's shape and just narrow the bail-out, the differential test stands on its own and I'd still suggest taking it.
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Adapt canonical constants and value-based dispatch from c32a837. Preserve safe folds while distinguishing arrays, the empty list, bigint spellings, and 32-bit integer tags. Replace the bailout tests with differential runtime coverage and value-based Lambda assertions. Co-authored-by: Cristiano Calcagno <cristianoc@users.noreply.github.com> Signed-off-by: Christoph Knittel <ck@cca.io>
25015f8 to
7403342
Compare
Signed-off-by: Christoph Knittel <ck@cca.io>
Rename the matching-only tag_type constructor to Payload_shape, leaving Tagged/Untagged for constructor representations. Remove type annotations that were only needed to distinguish the two Untagged constructors, and document the matching contract. Assert that nullary constructors have already been handled before computing a payload shape. This completes the representation cleanup: ordinary unboxed payload wrappers are erased during typedtree translation, while inline records remain objects. The preceding commit bumped CMI/CMT format versions for the changed serialized representation and rejects multi-argument unboxed constructors before lowering. This rename preserves the serialized layout. Validation: make test, including formatting, compiler unit tests, runtime regressions, build tests, and docstring tests. Lstringswitch is unchanged. Signed-off-by: Christoph Knittel <ck@cca.io>
|
Thanks for the detailed feedback and the implementation! I’ve incorporated your canonical-payload approach and included the representation cleanup here.
I also renamed the matching-only The differential tests cover literal/payload overlap and the additional edge cases we discussed. The full test suite passes. I’ve left the |
|
@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". |
2 participants
Unboxed variant payloads can share their runtime representation with literal constructors. Previously, folding
f(Color("primary"))could select theColorbranch even though runtime matching selectedPrimary.Preserve runtime dispatch when the scrutinee is a constant untagged block. Ordinary boxed variants retain constructor-based folding.
Adds Lambda, syntax, and runtime regression coverage for overlapping strings and numbers, default branches, and non-overlapping values.
Validation: full
make testand syntax suite pass, including formatting checks.Fixes #6950.