Repository navigation
Conversation
Changelog💅 Polish
Semantic Diff8 commits · Full page ✨ Say in rule 6 that variants with some
|
|
@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. Chef's kiss. 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". |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## instanceof-typeof-annotations #9015 +/- ##
=================================================================
- Coverage 81.32% 81.31% -0.02%
=================================================================
Files 456 456
Lines 64180 64193 +13
=================================================================
+ Hits 52196 52197 +1
- Misses 11984 11996 +12
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
85c7fd6 to
77e7004
Compare
|
@codex review |
|
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". |
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>
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>
77e7004 to
66880b8
Compare
|
@codex review |
![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: 66880b8db8
ℹ️ 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".
| | _ :: _, [] -> ( | ||
| (* No other arm: in a complete match the last primitive arm is not | ||
| checked *) | ||
| match List.rev primitive_clauses with | ||
| | (_, {J.switch_body}) :: rest when complete -> |
There was a problem hiding this comment.
When a complete match is evaluated for effects and its final non-primitive arm does nothing, other is empty even though that arm exists. For example, with @unboxed type t = S(string) | O({x: int}), evaluate switch v { | S(_) => Console.log("s") | O(_) => () } before another statement. The object arm compiles to [], so this branch removes the string check and makes its logging body the unconditional default; passing O({x: 1}) then incorrectly logs "s". Determine whether non-primitive arms exist from the dispatch entries rather than from whether their compiled bodies contain statements.
Useful? React with 👍 / 👎.
Stacked on #8950. Replaces #8967 with the dispatch structure the architecture review proposed.
An untagged variant now matches in two steps:
===, by oneswitch (x), or by anifper body for at most two literals. Its default is the payload dispatch.switch (typeof x)over the primitive kinds, or aniffor one kind. Its default is anifchain, in this order: the instance cases (a subclass before its superclasses, otherwise declaration order), the function case, the tagged constructors (oneswitchon the tag) and the object case.In a complete match the last arm is never checked, whichever form it is in.
What this removes:
Dynamic_checks.is_a_literal_case, and with it the rest ofAst_untagged_variants.Dynamic_checksandJs_exp_make.emit_check:Js_exp_makebuilds the int-tag test of tagged variants and the instance checks directly, andmatching_factsdropshas_null,has_undefinedandhas_other_literal;null/array/instance guards in front of thetypeofswitch;!(x instanceof Sub)negations in superclass checks.Code generation and constant folding share one table of which literal passes which payload check (
Variant_runtime.literal_passes). The two copies disagreed onnulland an object case; folding now follows the generated code, wheretypeof nullis"object".What follows from the new structure:
instanceof. A class missing at run time therefore throws only for objects and functions, andSymbol.hasInstancecan't claim strings.switchwhen no payload check would accept it.nullthat only the object check would accept becomes&& x !== nullon that check. JSON decoders keep their singletypeof x === "object" && x !== null && !Array.isArray(x).typeof x === "object" && !(x instanceof Date). Class checks then run only on objects.Spec rules 7–10 in
compiler/ml/UntaggedVariants.mddescribe the new structure, and rule 11 names the values constant folding sees. Rule 10 changes one outcome: withPPromise | PFloat, the float case is now checked, so a thenable lands inPPromise.New tests:
untagged_loop_control_test.res:breakandcontinuein literal, primitive, class, object and default arms leave or continue the loop, not the generatedswitch.untagged_default_arm_test.res:Symbol.hasInstanceaccepts everything doesn't take primitives or literals (carried over from Test primitive untagged cases before class checks #8967);untagged_runtime_annotations_test.res: a class missing at run time doesn't affect primitive cases, with every arm present or with a default (partly from Test primitive untagged cases before class checks #8967). Also a default shared by primitive and object cases.ounit_lambda_constant_tests.ml:nullwithout a null case folds into the object case.🤖 Generated with Claude Code
https://claude.ai/code/session_01CMkrYaktctdKc4M41WF36t