Repository navigation
Conversation
cknitt
added this pull request to stack #9016
October 24, 2026 16:28
Changelog💅 Polish
Semantic Diff8 commits · Full page Say in rule 6 that variants with some
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## untagged-literal-overlap-warnings #9023 +/- ##
=====================================================================
+ Coverage 81.32% 81.33% +0.01%
=====================================================================
Files 456 456
Lines 64327 64297 -30
=====================================================================
- Hits 52313 52296 -17
+ Misses 12014 12001 -13
🚀 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 24, 2026 16:44
Member
Author
|
@codex review |
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. 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". |
cknitt
force-pushed
the
untagged-literal-overlap-warnings
branch
from
October 24, 2026 16:47
8ded3c5 to
9d1eebb
Compare
cknitt
force-pushed
the
untagged-review-fixes
branch
from
October 24, 2026 16:47
f802bdf to
1b4ef25
Compare
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>
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>
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>
Why:
Payload kinds had two sources: the shapes declarations state, and a list
of builtin paths in get_block_type_from_typ. Builtins were outside the
generic checks, so an annotation could not restate a builtin's kind and
an interface could not state it over a builtin implementation.
The declarations of `int`, `float`, `bigint`, `string`, `bool`, `dict`,
`array` and `promise` in `Predef` state their shapes, and only tuples and
function types are classified structurally. `@typeOf("number") type count =
int` is a restatement, `@typeOf("string") type t = int` a contradiction, and
an interface's `@typeOf("number") type t` accepts `type t = int`.
Diagnostic modified (annotation over a builtin):
Example:
@typeof("string") type t = int
Before (diagnostic):
@typeof on a type abbreviation must describe an abstract type, or a type with the same annotation. The abbreviated type already has a runtime representation the compiler knows, or can be undefined.
After (diagnostic):
This annotation, @typeof("string"), contradicts the abbreviated type, whose values are recognized by @typeof("number").
Diagnostic modified (contradicting annotation):
Example:
@typeof("symbol") type sym
@typeof("number") type t = sym
Before (diagnostic):
This annotation, @typeof("number"), contradicts the abbreviated type's annotation, @typeof("symbol").
After (diagnostic):
This annotation, @typeof("number"), contradicts the abbreviated type, whose values are recognized by @typeof("symbol").
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>
Why:
"A constructor without @as is its name" was re-derived wherever a tag
was read: in the matching facts, constant folding, code generation, the
duplicate-literal check, coercions and gentype, each with its own
`None` case, and `[]` special-cased in several of them.
The layout stores every constructor's literal, the name when there is no
`@as` and `0` for `[]`. Readers take it as is, and matching compares tags
without the widening to an optional literal. Messages that name the declared
`@as` read the constructor's annotation. Since the literal is what the value
is, `A` and `@as("A") A` now agree across an interface and in a coercion, and
constant folding compares any two constructors by their runtime values.
Representation modified (constructor tag in the layout):
Example:
type t = A | @as("b") B
Before (layout):
A -> {name: "A", literal: None}
B -> {name: "B", literal: Some (String "b")}
After (layout):
A -> {name: "A", literal: String "A"}
B -> {name: "B", literal: String "b"}
Review: Does this change generated JavaScript? No snapshot changed: the
resolved literal is the value every reader computed before.
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>
Why:
`@instanceOf` and `@typeOf` can refine or restate the type an
abbreviation stands for, while `@notUndefined` was rejected on every
abbreviation, even of an abstract type it could refine.
`@notUndefined` on an abbreviation follows the rule of the other
annotations: it can refine a type whose representation the compiler does
not see, or restate that the type cannot be undefined, keeping a payload
kind the type has. The check whether a type can be undefined is shared with
interface inclusion (`Typeopt.undefinedness`); `get_unboxed_type_representation`
moves to `Ctype` so that `Typedecl` can call it.
Diagnostic modified (@notundefined on an abbreviation):
Example:
@notundefined
type t = option<int>
Before (diagnostic):
@notundefined can only be used on abstract types
After (diagnostic):
@notundefined on a type abbreviation must describe an abstract type, or a type with the same annotation. The abbreviated type already has a runtime representation the compiler knows, or can be undefined.
Diagnostic modified (@notundefined on a record):
Example:
@notundefined
type t = {x: int}
Before (diagnostic):
@notundefined can only be used on abstract types
After (diagnostic):
@notundefined can only be used on abstract types and type abbreviations, whose runtime representation it describes.
JS modified (refined abstract type):
Example:
type opaque
@notundefined type refined = opaque
let wrap = (x: refined) => Some(x)
Before (js): error
After (js):
function wrap(x) {
return x;
}
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>
Why:
A coercion accepts constructors whose literals are spelled differently
but are the same value, such as `@as(1.)` for `@as(1.0)`, while a match
on a constant constructor was folded by comparing tags structurally, so
the coerced constant missed its arm.
Constant constructors and tagged blocks are now found by the runtime value
of their literal, as the untagged dispatch already does.
JS modified (folded match of a coerced constant):
Example:
type source = | @as(1.) One
type target = | @as(1.0) One | Two | Three | Four
let isOne = (x: target) =>
switch x {
| One => true
| _ => false
}
let r = isOne((One: source :> target))
Before (js): let r = false;
After (js): let r = true;
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>
Why:
A constant constructor represented by an int is lowered to an int, and
a match on it was folded by comparing with the int tags of the matched
variant only. A coercion accepts `@as(1)` for `@as(1.)`, so the coerced
constant missed its arm.
The int is compared with every constant's literal by value.
JS modified (folded match of a coerced int constant):
Example:
type source = | @as(1) One
type target = | @as(1.) One | Two | Three | Four
let isOne = (x: target) =>
switch x {
| One => true
| _ => false
}
let r = isOne((One: source) :> target)
Before (js): let r = false;
After (js): let r = true;
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>
A payload pattern whose values are all literal-case values can never match (warning 11/12), and a literal case covers the equal bool payload value in the exhaustiveness check. Fixes #8806. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kr4cjoUi2f5VpMvE6wT9bo Signed-off-by: Christoph Knittel <ck@cca.io>
The check for untagged payload patterns shadowed by literal cases ran only
when warning 11 was active, so with `-w -11+12` or `@warning("-11")` a dead
or-pattern alternative got no warning 12. It now runs when either warning is
active; the usefulness analysis is still gated on warning 11.
Example (rescript):
@unboxed type t = | @as("a") A | S(string)
@warning("-11")
let g = x => switch x { | S("a") => 1 | S("a" | "c") => 2 | S(_) | A => 3 }
Diagnostic (text):
Before: no warnings
After: Warning 12 on the "a" in S("a" | "c"); the dead S("a") case stays
silent because warning 11 is off.
Area: typechecker tests
Output: unchanged
Formats: none
PPX: unchanged
Editor: unchanged
Tests: ~tests/build_tests/super_errors/fixtures/warning_12_untagged_literal_shadows_or_pattern.res
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kr4cjoUi2f5VpMvE6wT9bo
Signed-off-by: Christoph Knittel <ck@cca.io>
Rule 7 says which payload patterns a literal case shadows, which warnings report them, and how a literal case counts in the exhaustiveness check. 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>
#9021 checks a variant with some `@unboxed` constructors like an untagged one: tagged blocks count as one object case, and an untagged case's `@as` is not a tag. Rule 6 now says so and names its tests. 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>
cknitt
force-pushed
the
untagged-literal-overlap-warnings
branch
from
October 24, 2026 20:20
9d1eebb to
c49c11e
Compare
cknitt
force-pushed
the
untagged-review-fixes
branch
from
October 24, 2026 20:20
1b4ef25 to
5f36699
Compare
Why:
A public abbreviation could declare a subclass when its arguments were
not all type variables, meant for phantom type hierarchies. The test
was syntactic, so `@instanceOf("Dog") type dog = animal<int>` passed,
and every `animal<int>` was then a dog without a coercion.
A subclass is always a private abbreviation, phantom hierarchies included;
upcasts use `:>`.
Diagnostic added (public subclass of an instantiated class):
Example:
@instanceof("Animal") type animal<'a>
@instanceof("Dog") type dog = animal<int>
Before (diagnostic): none
After (diagnostic):
A subclass must be a private abbreviation: write `= private ...` so that instances of Animal cannot be used as instances of Dog without a coercion.
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>
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>
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>
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>
check_invariant checks a layout, and Variant_layout, which derives the layout, is its only caller; Ast_untagged_variants keeps the attributes and errors. Its doc comment, which described another function, now says what it checks. 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>
Interface inclusion compared classes and kinds in Includecore and @notundefined in Includemod, the two levels of one declared shape. Includecore.compare_runtime_annotations now checks both; the messages are unchanged. 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>
cknitt
force-pushed
the
untagged-review-fixes
branch
from
October 24, 2026 20:28
5f36699 to
19cd7e9
Compare
`@notUndefined`, `@typeOf` and `@instanceOf` declare levels of one stored shape, checked on abbreviations by one entry point and across interfaces by one comparer. Rule 5 now describes the three together, with their errors and interface checks; rule 13 is gone and the rule on `@tag` and interfaces becomes rule 13. 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. 🎉 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
force-pushed
the
untagged-literal-overlap-warnings
branch
from
October 25, 2026 05:34
c49c11e to
da12cf0
Compare
Member
Author
|
Folded into the PRs whose code it changed, so no PR in the stack introduces something a later one rewrites:
|
cknitt
removed this pull request from stack #9016
October 25, 2026 05:37
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.
Stacked on #8914. These are the findings of an architecture and simplicity review of the untagged variant stack as a whole, fixed or cleaned up before merging. One commit each.
Fixes
@unboxedconstructors are checked like untagged ones (Variants with some @unboxed constructors skip the untagged variant checks #9020; the same fix as Check variants with some @unboxed constructors like untagged ones #9021 on master). Without it, such a variant could have two number cases, an unknown payload beside another, or an untagged object beside a tagged block, and its matches picked the wrong case. Now tagged blocks count as one object case. The@asof an untagged case no longer clashes with a tagged case's tag. Rule 6 says so.privateabbreviation. The phantom-type exception was a syntactic test that also accepted@instanceOf("Dog") type dog = animal<int>, which made everyanimal<int>a dog without a coercion. Phantom hierarchies now write= private …and upcast with:>.case, as the code does. Rule 11 names the values constant folding sees.Cleanups (no output changes)
Ast_untagged_variants.Dynamic_checksandJs_exp_make.emit_checkare gone.Js_exp_makebuildsis_int_tagfrom the literal list, andis_instance_ofdirectly.matching_factsdropshas_null,has_undefinedandhas_other_literal. A class is never aswitchkey, soblock_type_nameno longer pretends to emit one.Variant_runtime.literal_passesis the one table of which literal passes which payload check, shared by code generation and constant folding. The two copies disagreed onnulland an object case. Folding now follows the generated code and putsnullwithout a null case into the object case, as happens at run time.check_invariantmoves fromAst_untagged_variantstoVariant_layout, its only caller.@instanceOf/@typeOf, and@notUndefined) inIncludecore.compare_runtime_annotations.Includemod.not_undefined_mismatchis gone, and the messages are unchanged.I tried and dropped stating the true literals of the predefined constructors (
()andNoneasundefined): re-exports such astype t<'a> = option<'a> = None | Some('a)derive their own layout, which then no longer matches.New tests:
untagged_mixed_two_numbers.res,untagged_mixed_unknown_payload.res,untagged_mixed_object_and_tagged.resandinstance_of_public_subclass_instance.res;untagged_mixed_test.res: an untagged case's@asequal to a tag;ounit_lambda_constant_tests.ml:nullwithout a null case folds into the object case.🤖 Generated with Claude Code
https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t