Repository navigation
Make @unboxed apply to a whole variant only - #9040
Merged
Merged
Conversation
Why:
`@unboxed` on a single constructor made that constructor untagged and
left the others tagged. Such mixed variants missed the checks of
untagged ones (#9020) and interface and coercion checks (#9038), and
their matches crashed or picked the wrong case. Public code uses the
attribute on every payload constructor or on the only one, which the
type-level `@unboxed` says, but no code mixes tagged and untagged cases.
`@unboxed` on a constructor is an error that says to write it on the type.
The layout of a variant is untagged exactly when its type is `@unboxed`, so
`check_invariant` no longer considers tagged and untagged blocks together,
the type's `@unboxed` is no longer copied onto constructors, and
`process_untagged`, a twin of `has_untagged`, is gone.
Diagnostic added (@unboxed on a constructor):
Example:
type t = | @unboxed A(int) | B({x: int}) | C
Before (diagnostic): none; `A(3)` is `3` and `B` is `{TAG: "B", x: 1}`
After (diagnostic):
@unboxed applies to a whole variant: write it on the type, as in `@unboxed type t = ...`, instead of on constructor A.
Diagnostic added (@unboxed on a single-field inline record constructor):
Example:
type t = | @unboxed A({x: int})
Before (diagnostic): none; `A({x: 1})` is `{x: 1}`
After (diagnostic):
@unboxed applies to a whole variant: write it on the type, as in `@unboxed type t = ...`, instead of on constructor A. For a constructor whose payload is an inline record with one field, `@unboxed type t = A({...})` makes the value that field. To keep the record object, give the record type a name: `@unboxed type t = A(r)`.
Breaking: `@unboxed` on a constructor no longer compiles. Moving it to the
type keeps the representation, except for a constructor whose payload is an
inline record with one field, whose value becomes the field; a named record
type keeps the object.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t
Signed-off-by: Christoph Knittel <ck@cca.io>
Changelog💥 Breaking Change
Semantic Diff2 commits · Full page Make
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9040 +/- ##
==========================================
- Coverage 81.33% 81.32% -0.01%
==========================================
Files 456 456
Lines 64042 64036 -6
==========================================
- Hits 52086 52077 -9
- Misses 11956 11959 +3
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
cknitt
marked this pull request as ready for review
October 25, 2026 08:16
Member
Author
|
@codex review |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-9040 |
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. |
|
Codex Review: Didn't find any major issues. Keep them coming! 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 error for a coercion between variants with different `@unboxed`
configurations said that the wrong one was unboxed.
Diagnostic modified (unboxed configurations):
Example:
@unboxed type x = One(bool) | Two
type y = One(bool) | Two
Before (diagnostic):
- Variant x is not unboxed.
- Variant y is unboxed.
After (diagnostic):
- Variant x is unboxed.
- Variant y is not unboxed.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t
Signed-off-by: Christoph Knittel <ck@cca.io>
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
cknitt
enabled auto-merge (squash)
October 25, 2026 10:37
cristianoc
approved these changes
Oct 25, 2026
cristianoc
left a comment
cristianoc
left a comment
Collaborator
There was a problem hiding this comment.
The commits' descriptions are so much better than the PR
This was referenced Oct 25, 2026
cknitt
added a commit
that referenced
this pull request
Oct 25, 2026
…#9015) * Match untagged variants with a literal switch and one typeof dispatch Why: The payload dispatch of an untagged variant ran its instance checks before the `typeof` checks, so strings and numbers reached `instanceof`: a class missing at run time threw for them, and a `Symbol.hasInstance` could claim them. Literals were tested by a size heuristic that chose between two equivalent expressions, the object case by a guard per instance case, and a subclass by negating it in its superclass's check. The literals come first, compared with `===` by one `switch (x)` (an `if` per body for at most two), whose default is the payload dispatch: one `switch (typeof x)` over the primitive kinds, whose default is an `if` chain over the instance cases (a subclass first), the function case, the tagged constructors and the object case. In a complete match the last arm is not checked. A literal without an arm skips the `switch` when no payload check would accept it, and a `null` that only the object check would accept adds `x !== null` to it. Checks leading to the default before the last checked arm fold into its check, so class checks run only on objects. `Dynamic_checks.is_a_literal_case` and the per-instance guards are gone. JS modified (primitives before classes): Example: @unboxed type t = Missing(missing) | Text(string) | Count(float) let kind = value => switch value { | Missing(_) => "missing" | Text(text) => text | Count(_) => "count" } Before (js): if (value instanceof UntaggedMissingClass) { return "missing"; } switch (typeof value) { case "string" : return value; case "number" : return "count"; } After (js): switch (typeof value) { case "string" : return value; case "number" : return "count"; default: return "missing"; } JS modified (instances without an arm): Example: @unboxed type t = D(Date.t) | R(RegExp.t) | O({x: int}) | S(string) | I(int) | F(int => int) let kind = value => switch value { | O(_) => "O" | S(_) => "S" | _ => "other" } Before (js): if (value instanceof RegExp || value instanceof Date) { return "other"; } switch (typeof value) { case "object" : return "O"; case "string" : return "S"; default: return "other"; } After (js): if (typeof value === "string") { return "S"; } else if (typeof value === "object" && !(value instanceof RegExp) && !(value instanceof Date)) { return "O"; } else { return "other"; } JS modified (a subclass first): Example: @unboxed type upload = Blob(Blob.t) | File(File.t) let kind = upload => switch upload { | Blob(_) => "blob" | File(_) => "file" } Before (js): if (upload instanceof Blob && !(upload instanceof File)) { return "blob"; } else { return "file"; } After (js): if (upload instanceof File) { return "file"; } else { return "blob"; } Review: Does `break` in an arm leave the generated `switch` instead of the loop? No: every arm placed in a generated `switch` is compiled in a switch context, where `break` and `continue` are labeled; `untagged_loop_control_test.res` checks arms of every part. Review: What changes for a value outside every case? It lands in the last arm of the chain: with `PPromise | PFloat` the float case is now checked, so a thenable lands in `PPromise` (rule 10). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> * Drop literal arms that a complete payload dispatch runs anyway A complete payload dispatch with one body runs it unchecked, so a literal arm with the same body needs no case of its own. JS modified (literal arm equal to the payload arm): Example: @unboxed type rec t = | @as(null) Null | @as(undefined) Undefined | WhyNotAnotherOne | Point({x: int, y: int}) let plus = (x, y) => switch y { | Null | Undefined => x | WhyNotAnotherOne => WhyNotAnotherOne | Point(_) => WhyNotAnotherOne } Before (js): switch (y) { case null : case undefined : return x; case "WhyNotAnotherOne" : return "WhyNotAnotherOne"; default: return "WhyNotAnotherOne"; } After (js): if (y == null) { return x; } else { return "WhyNotAnotherOne"; } Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> * Update the gentype example for the untagged dispatch The gentype example matches an untagged variant too; its output follows the new dispatch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> * Say precisely when literals use an if and what folds Rule 7 counts the literals that get a case, as the code does, and rule 11 names the values constant folding sees: literals and blocks of constants. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> * Emit untagged checks directly instead of through Dynamic_checks The check AST in Ast_untagged_variants only encoded the int-tag test of tagged variants and the instance checks. Js_exp_make now builds both directly (is_int_tag from the literal list, is_instance_of), and matching_facts no longer carries has_null, has_undefined and has_other_literal, which that test derived. A class is never a switch key, so block_type_name no longer pretends to emit one. No output changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> * Share which literal passes which payload check Code generation and constant folding each had a table of the literals a payload case accepts, and they disagreed on null and an object case. Variant_runtime.literal_passes is now the one table; folding follows the generated code, where `typeof null` is "object", so null without a null case folds into the object case as it is matched at run time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> * Keep the checks an empty arm or a tagged block needs Why: The payload dispatch decided that a match had no arm besides its primitive ones from the compiled chain of the others, which is empty when their bodies do nothing, and then ran the last primitive arm unchecked. A literal arm sharing the body of the payload dispatch was dropped, although two tagged blocks are told apart by reading their tag, which a literal such as `null` does not have. Whether there are other arms comes from the dispatch entries. A literal arm joins the payload dispatch only when at most one tagged block is matched, and the last tag of a complete match is not checked, as the last arm of every other complete match. JS modified (an arm that does nothing): Example: @unboxed type t = S(string) | O({x: int}) let f = v => { switch v { | S(_) => Console.log("s") | O(_) => () } Console.log("after") } Before (js): switch (typeof v) { default: console.log("s"); } console.log("after"); After (js): if (typeof v === "string") { console.log("s"); } console.log("after"); JS modified (a literal sharing the body of tagged blocks): Example: type t = | @unboxed MS(string) | MB({x: int}) | MC(int) | @as(null) MNull | @as(1) MOne let f = v => switch v { | MB(_) => "other" | MC(_) => "other" | MOne => "one" | MNull | MS(_) => "other" } Before (js): if (v === 1) { return "one"; } if (typeof v === "string") { return "other"; } switch (v.TAG) { case "MB" : case "MC" : return "other"; } After (js): if (v === null) { return "other"; } if (v === 1) { return "one"; } if (typeof v === "string") { return "other"; } switch (v.TAG) { case "MB" : default: return "other"; } Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> * Drop the tagged blocks of mixed variants from the dispatch Since #9040 a variant is untagged as a whole or not at all, so the payload dispatch of an untagged variant never meets a tagged block. The `switch` on the tag inside its `if` chain, the merging of tagged clauses, the count of tagged blocks before dropping literal arms and the tag-name parameter are gone, and rule 8 no longer lists tagged constructors. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> * Give a literal a case where skipping would add a check Why: A literal without an arm skipped the literal switch whenever the payload dispatch led it to the default, even where that made a complete dispatch check its last arm or added `x !== null`, although the switch existed for other literals anyway. When other literals get a case, a literal whose skipping would add such a check gets a case too. Literals that reach the default without a new check still skip. The fallback for tagged blocks is gone: an untagged variant has none since #9040. Rule 10 now says where a value outside every case lands when the match shares a default. Generated code changed (a bool literal beside string literals): Example: @unboxed type t = | @as("a") A | @as("b") B | @as(true) T | S(string) | O({x: int}) let f = (x: t) => switch x { | S(_) => 1 | O(_) => 2 | _ => 3 } Before (JS): switch (x) { case "a" : case "b" : return 3; default: if (typeof x === "string") { return 1; } else if (typeof x === "object") { return 2; } else { return 3; } } After (JS): switch (x) { case "a" : case "b" : case true : return 3; default: if (typeof x === "string") { return 1; } else { return 2; } } Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> * Let literals skip the switch whatever other literals do Why: Giving a literal a case where skipping would add a check saves one typeof or null check in a narrow situation, at the price of two more predicates in the most intricate part of the dispatch. This undoes that rule, keeping the removal of the tagged-block fallback and the rule 10 clause from the same commit. Generated code changed (a bool literal beside string literals): Example: @unboxed type t = | @as("a") A | @as("b") B | @as(true) T | S(string) | O({x: int}) let f = (x: t) => switch x { | S(_) => 1 | O(_) => 2 | _ => 3 } Before (JS): switch (x) { case "a" : case "b" : case true : return 3; default: if (typeof x === "string") { return 1; } else { return 2; } } After (JS): switch (x) { case "a" : case "b" : return 3; default: if (typeof x === "string") { return 1; } else if (typeof x === "object") { return 2; } else { return 3; } } Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> * Regenerate the callable-instance test for the typeof dispatch Why: The test added to the bottom of the stack is compiled by this pull request's dispatch, which checks the function arm against the classes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> --------- Signed-off-by: Christoph Knittel <ck@cca.io> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
cknitt
added a commit
that referenced
this pull request
Oct 26, 2026
* Deprecate @unboxed on a variant constructor Why: v13 rejects @unboxed on a constructor (#9040): @unboxed applies to a whole variant. 12.x users should hear about it before upgrading. `@unboxed` written on a constructor warns (warning 3), before the type's attributes are copied to its constructors, so a type-level `@unboxed` does not warn. Diagnostic added (@unboxed on a constructor): Example: type mixed = | @unboxed Text(string) | Count(int) @unboxed type whole = | @unboxed Name(string) | Age(int) Before (diagnostic): none After (diagnostic): Warning number 3 deprecated: @unboxed on constructor Text is deprecated and will be an error in v13, where @unboxed applies to a whole variant: write it on the type, as in `@unboxed type t = ...`. Warning number 3 deprecated: @unboxed on constructor Name is deprecated and will be an error in v13. The type is already @unboxed, so remove it from the constructor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> * Add PR link to changelog entry Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> * Skip spread constructors in the @unboxed constructor deprecation Why: A variant spread copies the source constructors with their attributes, including the source type's `@unboxed`, so the spread site warned about an annotation the user never wrote, or repeated the source's warning. The deprecation now skips constructors marked as coming from a spread; `@unboxed` written on a constructor is reported once, where it is written. Diagnostic removed (@unboxed reported at a spread): Example: @unboxed type a = A(string) @unboxed type b = | ...a | B(int) Before (diagnostic): Warning number 3 1:1-8 deprecated: @unboxed on constructor A is deprecated and will be an error in v13. The type is already @unboxed, so remove it from the constructor. After (diagnostic): none Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t Signed-off-by: Christoph Knittel <ck@cca.io> --------- Signed-off-by: Christoph Knittel <ck@cca.io> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9038.
@unboxedon a single constructor made that constructor untagged and left the others tagged. Variants mixing the two never really worked:Public code on GitHub uses the attribute either on every payload constructor or on the only constructor. Both mean the same as
@unboxedon the type. None of it actually mixes tagged and untagged cases.This PR makes
@unboxedapply to a whole variant only:@unboxedon a constructor is an error that says to write it on the type:@unboxed type t = A({x: int})makes the value1, the legacy unboxed representation, where the constructor form gave{x: 1}. The error says so and suggests a named record type to keep the object.@unboxed.check_invariantno longer considers tagged and untagged blocks together (the gate added in Check variants with some @unboxed constructors like untagged ones #9021).@unboxedis no longer copied onto constructors.process_untagged, a twin ofhas_untagged, is gone.compiler/ml/UntaggedVariants.mdis rewritten.A second commit fixes the error for a coercion between variants with different
@unboxedconfigurations, which named the wrong variant as unboxed (snapshotvariant_to_variant_coercion_unboxed). It was part of #9039, which this PR replaces.Breaking:
@unboxedon a constructor no longer compiles. A deprecation warning for 12.x will follow separately, so that libraries can move the attribute before upgrading.New tests: fixtures
unboxed_on_constructor.resandunboxed_on_single_field_record_constructor.res. The mixed-variant fixtures anduntagged_mixed_test.resare removed. Rule 1 of the spec test now uses a type-level@unboxed.🤖 Generated with Claude Code
https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t