Repository navigation
Conversation
|
@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. |
![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: 24e441242e
ℹ️ 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".
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
24e4412 to
4154702
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## not-undefined-inclusion #8967 +/- ##
========================================================
Coverage 81.29% 81.29%
========================================================
Files 456 456
Lines 64009 64063 +54
========================================================
+ Hits 52034 52080 +46
- Misses 11975 11983 +8
🚀 New features to boost your workflow:
|
|
@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". |
2e15e8c to
6b964d9
Compare
|
@codex review |
6b964d9 to
2d98edf
Compare
Changelog
Semantic Diff3 commits · Full page Test primitive untagged cases before class checks4827415 · cknitt + Claude · Oct 22, 2026, 19:14 UTC
When a variant has class cases, the arms of primitive kinds (string, number, bigint, boolean, symbol) are now tested first, in an outer JS modified (a class case beside primitive cases) @instanceOf("Nope") type nope
@unboxed type t = N(nope) | S(string) | F(float) | A(array<int>)
-if (v instanceof Nope) {
- return "N";
-}
-if (Array.isArray(v)) {
- return "A";
-}
switch (typeof v) {
case "string" :
return v;
case "number" :
return "F";
+ default:
+ if (v instanceof Nope) {
+ return "N";
+ }
+ if (Array.isArray(v)) {
+ return "A";
+ }
}Review
Trust Symbol.hasInstance only for objects and functionsb59197c · cknitt + Claude · Oct 23, 2026, 05:18 UTC
Rule 8 now says that Representation modified (a primitive accepted by Symbol.hasInstance) %%raw(`globalThis.Even = class { static [Symbol.hasInstance](x) { return x % 2 === 0 } }`)
@instanceOf("Even") type even
@unboxed type t = E(even) | N(float) | S(string)
-4 ↦ E
+4 ↦ NKeep class checks away from primitives and default-arm cases9dea0b9 · cknitt + Claude · Oct 23, 2026, 11:40 UTC
Of a class arm and an arm of another kind, the two-arm form checks the other one. A variant with class cases tests its literals by comparison. The JS modified (an instance handled by the default) @unboxed type t = D(Date.t) | R(RegExp.t) | O({x: int}) | S(string) | I(int) | F(int => int)
let f = v => switch v { | O(_) => "O" | S(_) => "S" | _ => "other" }
switch (typeof v) {
- case "object" :
- return "O";
case "string" :
return "S";
default:
- return "other";
+ if (v instanceof RegExp || v instanceof Date) {
+ return "other";
+ }
+ switch (typeof v) {
+ case "object" :
+ return "O";
+ default:
+ return "other";
+ }
}JS modified (a literal handled by the default) @unboxed type t = | @as("lit") Lit | S(string) | I(int) | B(bool) | O({x: int}) | F(int => int)
let f = v => switch v { | S(_) => "S" | O(_) => "O" | _ => "other" }
+if (v === "lit") {
+ return "other";
+}
switch (typeof v) {
case "string" :
return "S";JS modified (two arms) @unboxed type t = W(weird) | S(string)
-if (v instanceof Weird) { return "W"; } else { return "S"; }
+if (typeof v === "string") { return "S"; } else { return "W"; } |
2d98edf to
edd9350
Compare
Why:
A `switch` on an untagged variant ran its class checks before the
`typeof` switch, so strings and numbers reached `v instanceof C` too. A
class missing where the code runs, such as a user class or a newer
`Intl` class, then threw for every value, even for the primitive cases.
When a variant has class cases, the arms of primitive kinds (string,
number, bigint, boolean, symbol) are now tested first, in an outer
`switch (typeof v)` whose default holds the class checks and the remaining
`object`/`function` arms. A class instance is never a primitive, so no value
changes arm. Primitive and other arms no longer share a body through
fall-through, since they sit in different switches. Variants with only an
array case keep the old form, since `Array.isArray` cannot throw.
JS modified (a class case beside primitive cases):
Example:
@instanceof("Nope") type nope
@unboxed type t = N(nope) | S(string) | F(float) | A(array<int>)
Before (js):
if (v instanceof Nope) {
return "N";
}
if (Array.isArray(v)) {
return "A";
}
switch (typeof v) {
case "string" :
return v;
case "number" :
return "F";
}
After (js):
switch (typeof v) {
case "string" :
return v;
case "number" :
return "F";
default:
if (v instanceof Nope) {
return "N";
}
if (Array.isArray(v)) {
return "A";
}
}
Review: What does it cost? A second `switch (typeof v)` when there are
object or function arms too, and a body shared by a primitive and an object
arm is emitted twice (`awaitUser` in `UntaggedVariants.mjs`).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fc7LEuyEfynjaKCVEBPmTF
Signed-off-by: Christoph Knittel <ck@cca.io>
Why:
With the primitive cases tested first, a class whose
`Symbol.hasInstance` accepts a primitive no longer wins over a case of
that primitive kind, while the spec still said `Symbol.hasInstance` is
trusted without qualification.
Rule 8 now says that `Symbol.hasInstance` is trusted for objects and
functions, and that a primitive whose kind is a case lands in that case
first. The values of an `@instanceOf` type are instances, so a primitive of
that type already contradicts its annotation.
Representation modified (a primitive accepted by Symbol.hasInstance):
Example:
%%raw(`globalThis.Even = class { static [Symbol.hasInstance](x) { return x % 2 === 0 } }`)
@instanceof("Even") type even
@unboxed type t = E(even) | N(float) | S(string)
Before (representation): 4 ↦ E
After (representation): 4 ↦ N
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fc7LEuyEfynjaKCVEBPmTF
Signed-off-by: Christoph Knittel <ck@cca.io>
Why:
Two forms still ran class checks on primitives: the two-arm `if`, with
the class arm declared first, and the literal test, in its form of
negated payload checks. And a `switch` with a default arm let the cases
it does not name reach the arms it names: a `Date` landed in the object
arm, and an `@as("lit")` literal in the string arm (#8995).
Of a class arm and an arm of another kind, the two-arm form checks the
other one. A variant with class cases tests its literals by comparison. The
`switch (typeof x)` form is preceded by a guard sending instances of cases
without an arm to the default, as it already did for arrays; when that guard
has class checks, the primitive arms are tested first. The literal test is
skipped only when no literal would pass the check of a payload arm. The
spec's rule 10 example of an unchecked branch changes accordingly: with
`PPromise(promise<int>) | PFloat(float)`, the float case is checked, so a
thenable that is not a `Promise` lands in `PPromise`.
JS modified (an instance handled by the default):
Example:
@unboxed type t = D(Date.t) | R(RegExp.t) | O({x: int}) | S(string) | I(int) | F(int => int)
let f = v => switch v { | O(_) => "O" | S(_) => "S" | _ => "other" }
Before (js):
switch (typeof v) {
case "object" :
return "O";
case "string" :
return "S";
default:
return "other";
}
After (js):
switch (typeof v) {
case "string" :
return "S";
default:
if (v instanceof RegExp || v instanceof Date) {
return "other";
}
switch (typeof v) {
case "object" :
return "O";
default:
return "other";
}
}
JS modified (a literal handled by the default):
Example:
@unboxed type t = | @as("lit") Lit | S(string) | I(int) | B(bool) | O({x: int}) | F(int => int)
let f = v => switch v { | S(_) => "S" | O(_) => "O" | _ => "other" }
Before (js):
switch (typeof v) {
case "string" :
return "S";
After (js):
if (v === "lit") {
return "other";
}
switch (typeof v) {
case "string" :
return "S";
JS modified (two arms):
Example:
@unboxed type t = W(weird) | S(string)
Before (js): if (v instanceof Weird) { return "W"; } else { return "S"; }
After (js): if (typeof v === "string") { return "S"; } else { return "W"; }
Fixes: #8995
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>
edd9350 to
9dea0b9
Compare
Fixes #8995. Stacked on #8966. Follow-up from the risk assessment of #8950 (section 4).
A
switchon an untagged variant ran its class checks before itstypeofswitch, so strings and numbers reachedv instanceof Ctoo. A class missing where the code runs, such as a user class, orIntl.Segmenteron an older engine, then threwReferenceError/TypeErrorfor every value, the primitive cases included.When a variant has class cases, the arms of primitive kinds (string, number, bigint, boolean, symbol) are now tested first, in an outer
switch (typeof v). Its default holds the class checks and the remainingobject/functionarms:A class instance is never a primitive, so no value changes arm. Two details:
Array.isArraycannot throw.Symbol.hasInstance: a class whose hook accepts a primitive no longer wins over a case of that primitive kind. The spec now trustsSymbol.hasInstanceonly for objects and functions. The values of an@instanceOftype are instances, so such a primitive already contradicts its annotation.The other forms: of a class arm and an arm of another kind, the two-arm form now checks the other one, and a variant with class cases tests its literals by comparison, so primitives never reach a class check there either. With one class arm and a default, every value still reaches it.
#8995 (pre-existing on master): a
switchwith a_arm let cases it does not name reach the arms it names: aDatelanded in the object arm, and an@as("lit")literal in the string arm. Thetypeofswitch is now preceded by a guard sending instances of cases without an arm to the default (generalizing the existing array guard), and the literal test is skipped only when no literal can pass a payload arm's check. The spec's rule 10 example changes accordingly:PPromise(promise<int>) | PFloat(float)now checks the float arm, so a thenable that is not aPromiselands inPPromise.Cost: variants with class cases and object or function arms get a second
switch (typeof v), and a body shared by a primitive and an object arm is emitted twice, as inawaitUserinUntaggedVariants.mjs. The two-arm form is unchanged: it still checks the arm declared first.UntaggedVariants.mdrules 8 and 10 describe the new order, and the item is off the "still open" list.New tests in
untagged_default_arm_test.res(#8995,Symbol.hasInstancein the two-arm form and the literal test) and inuntagged_runtime_annotations_test.res: a missing class beside string and number cases, and a default shared by a primitive and an object case.🤖 Generated with Claude Code
https://claude.ai/code/session_01Fc7LEuyEfynjaKCVEBPmTF