Repository navigation
Conversation
Why:
The translation of a polymorphic variant argument of an external read
an argument other than a literal twice, as `arg.NAME` and `arg.VAL` of
an event-style `@string`, or inside the arrow function that switches
on it for `@int` and `@as`. So `on(t, next())` called `next()` twice,
and an `await` in the argument was a syntax error in the arrow
function. The functions in `Js_of_lam_variant` return an expression
and cannot bind the argument, so `Lam_compile` now binds it to a
temporary among the statements of the call's arguments, unless reading
it twice or in the function is the same as reading it once.
JS modified (polymorphic variant argument):
Example:
type t
@send external on: (t, @string [#click(int => unit) | #keydown(string => unit)]) => unit = "on"
@send external setLevel: (t, @int [#low | #high]) => unit = "setLevel"
let f = (t, next) => on(t, next())
let g = async (t, p) => setLevel(t, await p)
Before (js): `g` is a SyntaxError when the module loads
function f(t, next) {
t.on(next().NAME, next().VAL);
}
async function g(t, p) {
t.setLevel((() => {
switch (await p) {
…
}
})());
}
After (js):
function f(t, next) {
let tmp = next();
t.on(tmp.NAME, tmp.VAL);
}
async function g(t, p) {
let tmp = await p;
t.setLevel((() => {
switch (tmp) {
…
}
})());
}
JS modified (literal with a payload):
Example:
type t
@send external on: (t, @string [#click(int => unit) | #keydown(string => unit)]) => unit = "on"
@Val external wrap: (int => unit) => int => unit = "wrap"
let l = (t, handler) => on(t, #click(wrap(handler)))
Before (js):
function l(t, handler) {
t.on(({
NAME: "click",
VAL: wrap(handler)
}).NAME, ({
NAME: "click",
VAL: wrap(handler)
}).VAL);
}
After (js):
function l(t, handler) {
t.on("click", wrap(handler));
}
JS modified (while condition with statements):
Example:
type t
@send external has: (t, @string [#click(int => unit) | #keydown(string => unit)]) => bool = "has"
let w = async (t, p) => {
let n = ref(0)
while has(t, await p) {
n := n.contents + 1
}
n.contents
}
Before (js):
async function w(t, p) {
let n = 0;
while (t.has((await p).NAME, (await p).VAL)) {
n = n + 1 | 0;
};
return n;
}
After (js):
async function w(t, p) {
let n = 0;
while (true) {
let tmp = await p;
if (!t.has(tmp.NAME, tmp.VAL)) {
break;
}
n = n + 1 | 0;
};
return n;
}
Review: Which arguments keep their output? Literals, constants and
variables, and for an `@int` or `@string` dispatch, which reads the
argument once, any argument without side effects. An event-style
argument is read twice, so fields, array elements and conditionals of
variables stay in place, but a `@val` global or a `@get` property read
is bound, since it may run a getter. `gpr_2633_test.mjs` changes
because it pinned `foo.on(h(event).NAME, h(event).VAL)`.
Review: Why does `compile_while` change? The temporaries are statements,
and it put a condition with statements into a closure called on each
iteration, where an `await`, `break` or `continue` is a syntax error,
so `while has(t, await p)` would stop loading. Such a condition now
compiles to `while (true) { <statements>; if (!cond) break; <body> }`.
The change is the same as in #9177, with the same new output in
`complex_while_loop.mjs` and `test_while_side_effect.mjs`.
Signed-off-by: mununki <woonki.moon@gmail.com>
Changelog🐛 Bug fix
Semantic Diff1 commit · Full page Evaluate a polymorphic variant argument of an external once, in place8b9c6cb · mununki · Nov 04, 2026, 05:19 UTC
JS modified (polymorphic variant argument) type t
@send external on: (t, @string [#click(int => unit) | #keydown(string => unit)]) => unit = "on"
@send external setLevel: (t, @int [#low | #high]) => unit = "setLevel"
let f = (t, next) => on(t, next())
let g = async (t, p) => setLevel(t, await p)
function f(t, next) {
- t.on(next().NAME, next().VAL);
+ let tmp = next();
+ t.on(tmp.NAME, tmp.VAL);
}
async function g(t, p) {
+ let tmp = await p;
t.setLevel((() => {
- switch (await p) {
+ switch (tmp) {
…
}
})());
}JS modified (literal with a payload) type t
@send external on: (t, @string [#click(int => unit) | #keydown(string => unit)]) => unit = "on"
@val external wrap: (int => unit) => int => unit = "wrap"
let l = (t, handler) => on(t, #click(wrap(handler)))
function l(t, handler) {
- t.on(({
- NAME: "click",
- VAL: wrap(handler)
- }).NAME, ({
- NAME: "click",
- VAL: wrap(handler)
- }).VAL);
+ t.on("click", wrap(handler));
}JS modified (while condition with statements) type t
@send external has: (t, @string [#click(int => unit) | #keydown(string => unit)]) => bool = "has"
let w = async (t, p) => {
let n = ref(0)
while has(t, await p) {
n := n.contents + 1
}
n.contents
}
async function w(t, p) {
let n = 0;
- while (t.has((await p).NAME, (await p).VAL)) {
+ while (true) {
+ let tmp = await p;
+ if (!t.has(tmp.NAME, tmp.VAL)) {
+ break;
+ }
n = n + 1 | 0;
};
return n;
}Review
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9217 +/- ##
==========================================
+ Coverage 81.75% 81.78% +0.03%
==========================================
Files 457 457
Lines 65332 65380 +48
==========================================
+ Hits 53413 53474 +61
+ Misses 11919 11906 -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: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-9217 |
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. Breezy! 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". |
cristianoc
approved these changes
Nov 4, 2026
This branch has not been deployed
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: #9215
A polymorphic variant argument of an external, with
@intor with a@stringwhose constructors have a payload or an@as, was evaluated twice or inside a function the compiler generates, unless it was a literal or a variable. An event-style argument is passed as its tag and its payload, soon(t, next())compiled tot.on(next().NAME, next().VAL)and callednext()twice, andon(t, await p)awaitedptwice. An@intargument, or a@stringone with@as, went into an arrow function that switches on it:setLevel(t, await p)compiled tot.setLevel((() => { switch (await p) { ... } })()), which fails to load with a SyntaxError, and so did an awaited@intfield of an@objexternal. A literal whose payload has side effects,on(t, #click(wrap(handler))), copied the whole object literal into both arguments, and a@getgetter passed as an event-style argument ran twice.The translations in
Js_of_lam_variant(eval,eval_as_intandeval_as_event) return an expression, so they cannot bind the argument first; the FIXME about it dates from #2633.Lam_compile.compile_argumentsnow takes aholdstep that runs after the arguments are compiled and beforesequence_argumentsorders them. ForPjs_callandPjs_object_create,Lam_compile_external_call.hold_argbinds a polymorphic variant argument to a temporary after its statements, unless the translation can read it in place, so it runs once and in its place among the arguments. A literal event now passes its payload as it is. The temporaries are statements, andcompile_whileput a condition with statements into a closure called on each iteration, whereawait,breakandcontinueare syntax errors. Such a condition now compiles towhile (true) { <statements>; if (!cond) break; <body> }. That part is the same change as in #9177, with the same new output incomplex_while_loop.mjsandtest_while_side_effect.mjs, so whichever pull request lands second drops it.@intor@stringdispatch, which reads it once, and a field, array element or conditional of variables passed as an event-style argument. A@valglobal or a@getproperty read passed as an event-style argument is now bound, because it may run a getter.sequence_argumentsalready does for arguments with statements. Such a call in an operand of&&,||or?:compiles to statements under anif.@unwrap,@ignoreand other arguments of externals compile as before, and the runtime and Belt compile to the same JavaScript. The checked-in output changes only ingpr_2633_test.mjs, which pinnedfoo.on(h(event).NAME, h(event).VAL), and in the twowhiletests above.Tests:
polyvar_external_arg_test.reshas mocha tests that count the side effects of event-style,@as,@stringand@intarguments, check the order of the arguments around them, getters, refs, andawaitin arguments and in@objfields, and cover awhilecondition with a temporary, together withbreak,continueandawait. Compiled with master, the file fails to load, and four of its six synchronous tests fail.Checklist
changes/README.md), or no release notes are needed and a maintainer should applychangelog:skip.CONTRIBUTING.md).git commit -s).