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: |
5f2e4de to
df024f4
Compare
24e4412 to
4154702
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## instanceof-typeof-annotations #8967 +/- ##
==============================================================
Coverage 81.31% 81.32%
==============================================================
Files 456 456
Lines 64145 64182 +37
==============================================================
+ Hits 52161 52194 +33
- Misses 11984 11988 +4
🚀 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". |
df024f4 to
b19d240
Compare
2e15e8c to
6b964d9
Compare
|
@codex review |
b19d240 to
f8ee511
Compare
6b964d9 to
2d98edf
Compare
Changelog💅 Polish
Semantic Diff3 commits · Full page Test primitive untagged cases before class checks55b7297 · 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 functions8891e84 · 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 cases3604ed8 · 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 guard for instance cases without an arm also protects a function arm from class instances, and when the guard has class checks, the primitive arms are tested first. The spec's rule 10 example of an unchecked branch changes accordingly: with 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" }
-if (v instanceof RegExp || v instanceof Date) {
- return "other";
-}
switch (typeof v) {
- case "object" :
- return "O";
case "string" :
return "S";
+ default:
+ if (v instanceof RegExp || v instanceof Date) {
+ return "other";
+ }
+ switch (typeof v) {
+ case "object" :
+ return "O";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
f8ee511 to
a1622ab
Compare
edd9350 to
9dea0b9
Compare
a1622ab to
25fc167
Compare
25fc167 to
ecc1ff2
Compare
9dea0b9 to
4245a93
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 the guard that sends instance cases without
an arm to the default (#9010) knew only object instances, while a
declared class can have callable instances, and its class checks ran
before the primitive arms.
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
guard for instance cases without an arm also protects a function arm from
class instances, and when the guard has class checks, the primitive arms are
tested first. 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):
if (v instanceof RegExp || v instanceof Date) {
return "other";
}
switch (typeof v) {
case "object" :
return "O";
case "string" :
return "S";
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";
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"; }
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>
ecc1ff2 to
743b80b
Compare
4245a93 to
3604ed8
Compare
|
Superseded by #9015, which replaces the dispatch as a whole: literals first, then one |
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.
The default-arm guard (#9010) and declared classes: the guard that sends instance cases without an arm to the default now also protects a function arm, since a declared class can have callable instances, and when it has class checks, the primitive arms are tested first. 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(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