Repository navigation
Conversation
Why:
An optional argument of an external passed as `~x=?e`, where `e` is a
call, a dict or array lookup, a conditional or anything else that
cannot be read twice to test it first, is compiled to
`Primitive_option.valFromOption(e)`. `valFromOption` reads a property
of its argument to find a nested `None`, so it threw a `TypeError`
when `e` was `None`, though its doc comment says that it keeps
`undefined`. #8380 replaced `Primitive_option.toUndefined`, which
tested for `undefined` before calling `valFromOption`, on that
premise. The same call is emitted for a JSX key, also with
`-bs-jsx-preserve`, for the argument of an inlined function that
forwards `~x?` to an external, and for an optional `@int` argument, or
`@string` argument with `@as`, which switches on it. `valFromOption`
now returns `undefined` for `undefined`, as it does for `null`, which
fixes all of them.
JS modified (Primitive_option.valFromOption):
Before (js):
function valFromOption(x) {
if (x === null || x.BS_PRIVATE_NESTED_SOME_NONE === undefined) {
return x;
}
…
}
After (js):
function valFromOption(x) {
if ((x == null) || x.BS_PRIVATE_NESTED_SOME_NONE === undefined) {
return x;
}
…
}
Output modified (optional argument that is None):
Example:
%%raw(`function greet(name) { return name === undefined ? "Hello" : "Hello " + name }`)
@Val external greet: (~name: string=?, unit) => string = "greet"
let greetUser = (names: dict<string>, id) => greet(~name=?names->Dict.get(id), ())
Console.log(greetUser(dict{"1": "Ann"}, "2"))
Before (js):
function greetUser(names, id) {
return greet(Primitive_option.valFromOption(names[id]));
}
After (js):
function greetUser(names, id) {
return greet(Primitive_option.valFromOption(names[id]));
}
Before (text):
TypeError: Cannot read properties of undefined (reading 'BS_PRIVATE_NESTED_SOME_NONE')
After (text):
Hello
Review: Why change the runtime and not the compiler? Every path that
emits the unchecked call goes through `valFromOption`: `Arg_optional`
in `ocaml_to_js_eff`, the JSX key in `Lam_compile_primitive`, inlined
functions, and `@int` and `@as` arguments, which switch on its result.
`get_default_undefined_from_optional` returns an expression, so a test
in the compiler needs the argument bound to a temporary: a statement
for each such argument, through `hold_arg` for calls and separately for
JSX keys, plus temporaries for the arguments to its left (see
`sequence_arguments`). Bringing back `toUndefined` would undo #8380 for
one comparison. This way the generated code stays as it is, and code
that 13.0.0-alpha.5 or later already compiled works once the runtime
is updated.
Review: Does the extra test cost the other callers anything? They pass a
value known to be `Some`: a match on a nested option
(`Pval_from_option`), and the branch after `x !== undefined` of an
`@obj` assignment or of an optional argument that can be read twice.
For them `x == null` replaces `x === null`, still one comparison; in a
micro-benchmark of 2×10^8 calls the difference is within noise for
objects or numbers alone and under 0.3 ns a call for mixed inputs. The
runtime grows by one character, and no other compiled module of the
runtime, the stdlib or the tests changes.
Review: Could an argument that can be read twice now be compiled to
`valFromOption(x)` alone, without `x !== undefined ? … : undefined`?
It could, but that would change much output and call the runtime for
`None`. The next commit does it only for a JS property read, which may
run a getter.
Signed-off-by: mununki <[email protected]>
Changelog🐛 Bug fix
Semantic Diff2 commits · Full page Pass undefined for an optional external argument or JSX key that is Nonec907500 · mununki · Nov 09, 2026, 15:21 UTC
JS modified (Primitive_option.valFromOption)
function valFromOption(x) {
- if (x === null || x.BS_PRIVATE_NESTED_SOME_NONE === undefined) {
+ if ((x == null) || x.BS_PRIVATE_NESTED_SOME_NONE === undefined) {
return x;
}
…
}Output modified (optional argument that is None) %%raw(`function greet(name) { return name === undefined ? "Hello" : "Hello " + name }`)
@val external greet: (~name: string=?, unit) => string = "greet"
let greetUser = (names: dict<string>, id) => greet(~name=?names->Dict.get(id), ())
Console.log(greetUser(dict{"1": "Ann"}, "2"))
function greetUser(names, id) {
return greet(Primitive_option.valFromOption(names[id]));
}
-TypeError: Cannot read properties of undefined (reading 'BS_PRIVATE_NESTED_SOME_NONE')
+HelloReview
✏️ Evaluate optional
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9283 +/- ##
==========================================
+ Coverage 81.92% 81.93% +0.01%
==========================================
Files 456 456
Lines 65469 65501 +32
==========================================
+ Hits 53635 53669 +34
+ Misses 11834 11832 -2
🚀 New features to boost your workflow:
|
✅ Compiler benchmarksAllocated wordsIn total: 683,549,844 → 683,550,251 (±0.00%).
(inputs changed): this pull request changes runtime or Belt sources, so these benchmarks compile different code than on the base, and their changes don't count as regressions. InstructionsIn total: 9,664,672,601 → 9,665,568,705 (+0.01%).
(inputs changed): this pull request changes runtime or Belt sources, so these benchmarks compile different code than on the base, and their changes don't count as regressions. Output sizeIn total: 25,569,598 → 25,570,804 (±0.00%). None of the 13 benchmarks changed by 0.1% or more. Pull request 308b456 compared with base b82d944 in this run. |
…once
Why:
An `@obj` external builds its object as a literal and assigns an
optional argument whose presence is not known after it, under
`if (tmp !== undefined)`. So that argument was evaluated after the
arguments that follow it: `make(~size=?size(), ~mode=mode(), ())`
called `mode()` first, and a later argument with side effects could
change what the optional argument read, such as a `@get` getter, a
`@val` global, `Dict.get`, an array element, a mutable field or a ref.
`hold_obj_args` now binds such an argument to a temporary after its
statements, unless evaluating it after the literal gives the same
values and effects.
As with the temporaries of #9217, `sequence_arguments` then evaluates
the arguments to the left of the temporary before it, with
`evaluate_now`. That bound an optional argument known to be present as
a whole, so that it lost its presence and was assigned after the
literal too, and it bound variables and fields of records although
defining a temporary as such a value cannot change them. `evaluate_now`
now evaluates the value in an `Optional_block` and keeps the block, and
`sequence_arguments` leaves an argument that runs no code, a variable,
a constant or a field of a record, in place when the statements after
it only define variables as such values. A JS property read may run a
getter, so it does not count.
An optional argument that is a JS property read, as from a `@get`
external or a `@val` name with a dot, was read twice, to test it for
`None` and to unwrap it, by a call, a JSX key and an `@obj` external
alike. A getter ran twice, and one that returned a value and then
`undefined` made the call throw a `TypeError`. Such an argument is now
read once: a call and a JSX key give it to
`Primitive_option.valFromOption`, which returns `undefined` for `None`
since the previous commit, and an `@obj` external binds it.
`Js_analyzer.is_okay_to_duplicate ~property_reads:false` tells these
arguments apart.
JS modified (optional argument with side effects):
Example:
@obj external make: (~size: int=?, ~mode: string, unit) => {..} = ""
let o = (size, mode) => make(~size=?size(), ~mode=mode(), ())
Before (js):
function o(size, mode) {
let tmp = {
mode: mode()
};
let tmp$1 = size();
if (tmp$1 !== undefined) {
tmp.size = tmp$1;
}
return tmp;
}
After (js):
function o(size, mode) {
let tmp = size();
let tmp$1 = {
mode: mode()
};
if (tmp !== undefined) {
tmp$1.size = tmp;
}
return tmp$1;
}
JS modified (optional argument that a later argument changes):
Example:
type source
@get external name: source => option<string> = "name"
@obj external makeName: (~name: string=?, ~mode: string, unit) => {..} = ""
let n = (s, rename) => makeName(~name=?name(s), ~mode=rename(s), ())
Before (js):
function n(s, rename) {
let tmp = {
mode: rename(s)
};
if (s.name !== undefined) {
tmp.name = s.name;
}
return tmp;
}
After (js):
function n(s, rename) {
let tmp = s.name;
let tmp$1 = {
mode: rename(s)
};
if (tmp !== undefined) {
tmp$1.name = tmp;
}
return tmp$1;
}
JS modified (arguments before a bound optional argument):
Example:
type props = {color: string, width?: string}
@obj
external style: (~color: string=?, ~icon: string=?, ~width: string=?, ~height: string, unit) => {..} = ""
let a = (props, height) => style(~color=props.color, ~width=?props.width, ~height=height(), ())
let b = (icon, width, height) => style(~icon=icon(), ~width=?width(), ~height=height(), ())
Before (js):
function a(props, height) {
let tmp = {
color: props.color,
height: height()
};
if (props.width !== undefined) {
tmp.width = props.width;
}
return tmp;
}
function b(icon, width, height) {
let tmp = {
icon: icon(),
height: height()
};
let tmp$1 = width();
if (tmp$1 !== undefined) {
tmp.width = tmp$1;
}
return tmp;
}
After (js):
function a(props, height) {
let tmp = props.width;
let tmp$1 = {
color: props.color,
height: height()
};
if (tmp !== undefined) {
tmp$1.width = tmp;
}
return tmp$1;
}
function b(icon, width, height) {
let tmp = icon();
let tmp$1 = width();
let tmp$2 = {
icon: tmp,
height: height()
};
if (tmp$1 !== undefined) {
tmp$2.width = tmp$1;
}
return tmp$2;
}
JS modified (optional argument known to be present before a temporary):
Example:
type item = {id: int}
@obj external mk: (~item: item=?, ~level: @int [#low | @as(10) #high], unit) => {..} = ""
let k = async (f, p) => mk(~item=f(), ~level=await p, ())
Before (js):
async function k(f, p) {
let tmp = f();
let tmp$1 = await p;
let tmp$2 = {
level: (() => {
switch (tmp$1) {
…
}
})()
};
if (tmp !== undefined) {
tmp$2.item = Primitive_option.valFromOption(tmp);
}
return tmp$2;
}
After (js):
async function k(f, p) {
let tmp = f();
let tmp$1 = await p;
return {
item: tmp,
level: (() => {
switch (tmp$1) {
…
}
})()
};
}
JS modified (optional property read):
Example:
type source
@get external name: source => option<string> = "name"
@Val external greet: (~name: string=?, unit) => string = "greet"
@Val external userName: option<string> = "window.userName"
@obj external makeName: (~name: string=?, ~mode: string, unit) => {..} = ""
let g = s => greet(~name=?name(s), ())
let u = () => greet(~name=?userName, ())
let m = (o: {"name": option<string>}) => makeName(~name=?o["name"], ~mode="dark", ())
Before (js):
function g(s) {
return greet(s.name !== undefined ? Primitive_option.valFromOption(s.name) : undefined);
}
function u() {
return greet(window.userName !== undefined ? Primitive_option.valFromOption(window.userName) : undefined);
}
function m(o) {
let tmp = {
mode: "dark"
};
if (o.name !== undefined) {
tmp.name = o.name;
}
return tmp;
}
After (js):
function g(s) {
return greet(Primitive_option.valFromOption(s.name));
}
function u() {
return greet(Primitive_option.valFromOption(window.userName));
}
function m(o) {
let tmp = {
mode: "dark"
};
let tmp$1 = o.name;
if (tmp$1 !== undefined) {
tmp.name = tmp$1;
}
return tmp;
}
JS modified (temporaries for optional arguments of an @obj external):
Before (representation): Lam_compile_external_obj.hold_obj_args
~x=?f(), ~m=g() ↦ after the literal
~x=?p.x, ~m=g() ↦ after the literal
~x=?f(), ~m=p.m ↦ after the literal
~x=?f(), g() ↦ after the literal
~x=?p.x, ~m=p.m ↦ after the literal
~x=?f(), ~m="dark" ↦ after the literal
~x=?v, ~m=g() ↦ after the literal
~x=?f(), ~y=?g() ↦ after the literal
note: p.x stands for a field, a global or a lookup, and v for an immutable variable
After (representation):
~x=?f(), ~m=g() ↦ temporary
~x=?p.x, ~m=g() ↦ temporary
~x=?f(), ~m=p.m ↦ temporary
~x=?f(), g() ↦ temporary
~x=?p.x, ~m=p.m ↦ after the literal
~x=?f(), ~m="dark" ↦ after the literal
~x=?v, ~m=g() ↦ after the literal
~x=?f(), ~y=?g() ↦ after the literal
note: p.x stands for a field, a global or a lookup, and v for an immutable variable
JS modified (arguments before a statement):
Before (representation): Lam_compile.sequence_arguments
r.c, then let tmp = r.x ↦ temporary
r.c, then let tmp = s.x ↦ temporary
s.c, then let tmp = r.x ↦ temporary
f(), then let tmp = r.x ↦ temporary
r.c, then let tmp = g() ↦ temporary
~c=f(), then a statement ↦ temporary for the option
note: r.c is a variable or a field of a record, s.x a JS property read, and ~c=f() an optional argument known to be present
After (representation):
r.c, then let tmp = r.x ↦ in place
r.c, then let tmp = s.x ↦ temporary
s.c, then let tmp = r.x ↦ temporary
f(), then let tmp = r.x ↦ temporary
r.c, then let tmp = g() ↦ temporary
~c=f(), then a statement ↦ temporary for f(), option in place
note: r.c is a variable or a field of a record, s.x a JS property read, and ~c=f() an optional argument known to be present
JS modified (optional arguments read twice):
Before (representation): Js_analyzer.is_okay_to_duplicate
x, M.x, globalName ↦ test, then unwrap
r.field ↦ test, then unwrap
s.prop, o["prop"], window.x ↦ test, then unwrap
f(), a[i], c ? x : y ↦ unwrap once
After (representation): Js_analyzer.is_okay_to_duplicate ~property_reads:false
x, M.x, globalName ↦ test, then unwrap
r.field ↦ test, then unwrap
s.prop, o["prop"], window.x ↦ unwrap once
f(), a[i], c ? x : y ↦ unwrap once
note: r.field is a field of a record, s.prop a JS property read, such as a `@get` getter, and window.x a `@val` name with a dot
Review: Which calls keep their output? An `@obj` call in which nothing
that the literal evaluates after the optional argument can change it or
be changed by it: constants, immutable variables and functions, reads
without side effects when the optional argument has none either, as in
`make(~x=?p.x, ~m=p.m, ())`, and other optional arguments whose
presence is not known, which are assigned after it, in order. Variables
and fields of records are still tested and unwrapped in place. No
checked-in output changes besides the new tests.
Review: Which arguments get a temporary they do not need? A later
argument is taken to change an optional one unless it is `delay_safe`
or neither has side effects, and the JS IR does not say whether a field
is mutable or a call pure. So `make(~a=?props.a, ~b=Int.toString(n), ())`
binds `props.a`, as `a` above binds `props.width`. A field of a JS
object type, `o["x"]`, cannot be told from a getter, so an `@obj`
external binds it even when no later argument can change it, as `m`
above, while a call no longer tests it inline.
Review: Why in `hold_obj_args` and not in `hold_arg`? Whether the
argument has to be bound depends on the arguments after it, which
`hold_arg` does not see, and only an object-creating external evaluates
an argument after the ones that follow it. The binding does not depend
on the type of the argument, so an optional polymorphic variant
argument keeps its place whether or not `hold_arg` binds a later
argument.
Review: Why is `delay_safe` passed to `hold_obj_args`? It is
`Lam_compile.delay_safe`, which needs the variables the function
assigns, and `Lam_compile` depends on this module.
Review: Why change `evaluate_now` and `sequence_arguments` here? Before
this commit only the temporaries of #9217 for polymorphic variant
arguments made `sequence_arguments` bind the arguments to their left,
as in `k` above, where `item` lost its place in `Object.keys`. Binding
optional arguments of any type would make that common: without the
two changes, `b` above would assign `icon` after the literal as well,
and `a` would bind `props.color`. Both functions are in
`lam_compile.ml`, which #9231 does not change.
Review: Why a flag on `is_okay_to_duplicate` and not another predicate?
The two differ only in property reads, and its other callers, `min`
and `max` of booleans and the object of a `Pjs_object_get`, keep the
default. `Js_ast_util.named_expression` takes the same flag, which
`assemble_obj_args` passes.
Review: Is a `@val` global still read twice? Yes, a plain one is, like a
value of a JS module: only a property read is taken to run a getter, as
a `@get` external is there to do. A `@val` name with a dot and a global
with `@scope` compile to a property read and are read once.
Review: What still runs out of order? The side effects of a `unit` or
`@ignore` argument are put before the literal or the call, so
`mk(~a=f(), g())` calls `g()` first, as before. A getter is taken to
have no side effects, so `make(~name=?name(s), ~count=count(s), ())`
with two `@get` externals still reads `count` first.
Signed-off-by: mununki <[email protected]>
mununki
force-pushed
the
fix-optional-external-arguments
branch
from
November 9, 2026 16:41
cab875b to
308b456
Compare
mununki
added a commit
that referenced
this pull request
Nov 9, 2026
…place
Why:
The review of #9217 asked for two changes. A `@val` global passed as
an event-style argument was bound to a temporary because it may run a
getter, while a `@module` value, which may be a getter as well, was
read in place. Reading both in place is simpler and keeps the output
globals had before #9217. And an `@int` argument, or a `@string`
argument with `@as`, was bound whenever it had side effects, along
with the arguments to its left, though the arrow function that
switches on it is called where the argument is. Only an `await`, a
syntax error in that function, has to be evaluated before it.
JS modified (@Val global as an event-style argument):
Example:
type t
@Val external ev: [#click(int => unit) | #keydown(int => unit)] = "ev"
@send external on: (t, @string [#click(int => unit) | #keydown(int => unit)]) => unit = "on"
let listen = t => on(t, ev)
Before (js):
function listen(t) {
let tmp = ev;
t.on(tmp.NAME, tmp.VAL);
}
After (js):
function listen(t) {
t.on(ev.NAME, ev.VAL);
}
JS modified (@int argument with side effects between others):
Example:
type t
@send external set: (t, string, @int [#low | #high], string) => unit = "set"
let update = (t, g, f, h) => set(t, g(), f(), h())
Before (js):
function update(t, g, f, h) {
let tmp = g();
let tmp$1 = f();
t.set(tmp, (() => {
switch (tmp$1) {
case "low" :
return 0;
case "high" :
return 1;
}
})(), h());
}
After (js):
function update(t, g, f, h) {
t.set(g(), (() => {
switch (f()) {
case "low" :
return 0;
case "high" :
return 1;
}
})(), h());
}
JS modified (temporaries for @int and @as arguments):
Before (representation): Lam_compile_external_call.hold_arg
f() ↦ temporary
pick(async () => await p) ↦ temporary
%raw(`"low"`) ↦ temporary
await p ↦ temporary
%raw(`await p`) ↦ temporary
~l=?f(), presence not known ↦ temporary
x, x.contents, b ? x : y ↦ in place
After (representation): Lam_compile_external_call.hold_arg
f() ↦ in place
pick(async () => await p) ↦ in place
%raw(`"low"`) ↦ in place
await p ↦ temporary
%raw(`await p`) ↦ temporary
~l=?f(), presence not known ↦ in place
x, x.contents, b ? x : y ↦ in place
Tests: `polyvar_external_arg_test.res` now expects a `@val` global passed
as an event-style argument to be read for the tag and again for the
payload. It adds tests for the order of the arguments around synchronous
and awaited `@string` and `@int` arguments; an optional `@int` argument
and an optional `@obj` field whose presence is not known, including
`None`; and an async function and raw code in an `@int` or `@as` argument.
Compiled with the previous commit, only the getter test fails.
Review: What is still bound? An `@int` or `@as` argument with an
`await` outside the functions it contains, as `setLevel(t, await p)`,
and the arguments to its left, so that they still run first. Raw JS
code counts as awaiting unless it is a function or a literal, since
the compiler cannot see what it does; that is the output #9217 gave
it. An event-style argument with side effects is still bound, since
it is read twice, and so is a JS property read: a `@get` getter, and a
`@val` or `@module` value with `@scope`, which compiles to the same
property read and cannot be told from it.
Review: Why is an optional argument whose presence is not known read in
place too? #9283, which this branch builds on, made the temporary
unnecessary. `lvlOpt(t, ~l=?f(), ())` compiles to
`Primitive_option.valFromOption(f())`, which returns `undefined` for
`None` since then instead of throwing a TypeError, and
`hold_obj_args` binds such a field of an `@obj` external itself when
evaluating it after the literal could change a value or the order of
effects.
Review: Why is there no changelog fragment? #9217 was released in
13.0.0-alpha.7 with the entry "Fix an `@string` or `@int` argument of
an external being evaluated twice, as in `on(t, mk())`, or breaking
`await`", which still holds: an argument with side effects is still
evaluated once, and an awaited one is still bound. This commit brings
back the output that compilers before #9217 gave a `@val` global, and
otherwise only drops temporaries.
Signed-off-by: mununki <[email protected]>
mununki
added a commit
that referenced
this pull request
Nov 9, 2026
…s on them
Why:
The arrow function that switches on an `@int` argument, or a `@string`
argument with `@as`, took no parameter and read the argument inside, so
it captured the variables the argument reads. V8 keeps a captured
variable alive for as long as any closure created in the same call is,
so in the review of #9231 twenty callbacks kept 160 MB alive. 11.1 and
12.3 do this too, with pure arguments as well. Reading the argument
inside the function also made an `await` in it a syntax error, which is
why #9217 bound awaited arguments to temporaries. Calling the function
with the argument evaluates it in the caller, where an `await` is
legal, and captures nothing.
JS modified (@int argument):
Example:
type t
@send external setLevel: (t, @int [#low | @as(10) #high]) => unit = "setLevel"
let setup = (t, classify, big) => {
setLevel(t, classify(big))
() => t
}
let later = async (t, p) => setLevel(t, await p)
Before (js):
function setup(t, classify, big) {
t.setLevel((() => {
switch (classify(big)) {
case "low" :
return 0;
case "high" :
return 10;
}
})());
return () => t;
}
async function later(t, p) {
let tmp = await p;
t.setLevel((() => {
switch (tmp) {
…
}
})());
}
After (js):
function setup(t, classify, big) {
t.setLevel((x => {
switch (x) {
case "low" :
return 0;
case "high" :
return 10;
}
})(classify(big)));
return () => t;
}
async function later(t, p) {
t.setLevel((x => {
switch (x) {
…
}
})(await p));
}
JS modified (where an @int or @as argument is evaluated):
Before (representation): Lam_compile_external_call.hold_arg
setLevel(t, f()) ↦ read inside the function
setLevel(t, await p) ↦ temporary
setLevel(t, %raw(`await p`)) ↦ temporary
setLevelOpt(t, ~level=?f(), ()) ↦ read inside the function
After (representation): Lam_compile_external_call.hold_arg
setLevel(t, f()) ↦ passed to the function
setLevel(t, await p) ↦ passed to the function
setLevel(t, %raw(`await p`)) ↦ passed to the function
setLevelOpt(t, ~level=?f(), ()) ↦ passed to the function
Tests: `polyvar_external_arg_test.res` checks with a `WeakRef` and `gc()`
that a closure returned next to an `@int` argument does not keep the
record the argument reads alive. Compiled with #9217 or the previous
commit, that test fails. Its output, the outputs of the tests of #9283
and `ImportJsValue.res.js` in the gentype tests show the function called
with the argument.
Review: How does the test call `gc()`? `v8.setFlagsFromString` with
`--expose-gc` gives new contexts a `gc` function, which
`vm.runInNewContext("gc")` returns, as Jest's leak detection does. A
`WeakRef` keeps its target until the end of the current task, so the
test waits for a timer before it collects. It failed 20 runs out of 20
with the previous commit and passed 20 out of 20 with this one.
Review: Why does raw code that ends in a line comment, such as
`"light" // default mode`, still break the module as an `@int` or `@as`
argument? The line comment hides the parenthesis that closes the call,
as it does for any argument: a `string` argument given that raw code
compiles to `t.setName("light" // default mode)` with every compiler.
#9217 put such an argument in a temporary on a line of its own, which
hid the bug. The previous commit, like compilers before #9217, left it
inside the `switch`, where it hid the rest of the line, as the review of
#9231 found. Ending raw code that ends in a line comment with a line
break belongs in the printer, as a fix of its own.
Review: What goes? `Js_of_lam_variant.awaits` and `eval_in_place`, the
temporaries for awaited arguments, and `Js_exp_make.of_block`, which had
no other use. `hold_arg` now binds only an event-style argument, which
cannot be optional, so it loses its `arg_label` parameter.
Signed-off-by: mununki <[email protected]>
mununki
added a commit
that referenced
this pull request
Nov 9, 2026
…eir place
Why:
An external leaves a `unit` or `@ignore` argument out of the JS call,
and `translate_ffi` and `assemble_obj_args` evaluated the effects of one
before the whole call or object. So `setX(t, f(), g())` called `g()`
before `f()`, with 11.1 and 12.3 and with every kind of external. The
temporaries of #9217 hid it only after a polymorphic variant argument
with side effects, and the previous commits brought it back there
(regressions 2 and 5 in the review of #9231). `hold_arg` now makes the
effects a statement of the argument, so `sequence_arguments` evaluates
the arguments to its left first, binding only those that the statement
could change, as it does for any argument with statements. Nothing is
hoisted any more, so the effect lists of `ocaml_to_js_eff`,
`assemble_args_*` and `assemble_obj_args` go, with `add_eff` and
`Js_exp_make.fuse_to_seq`, which had no other use, and
`ocaml_to_js_eff` becomes `ocaml_to_js`.
JS modified (unit argument):
Example:
type t
@send external setX: (t, int, unit) => unit = "setX"
let update = (t, f, g) => setX(t, f(), g())
Before (js):
function update(t, f, g) {
g();
t.setX(f());
}
After (js):
function update(t, f, g) {
let tmp = f();
g();
t.setX(tmp);
}
JS modified (unit argument in a while condition):
Example:
type t
@send external ready: (t, unit) => bool = "ready"
let wait = (t, g) =>
while ready(t, g()) {
()
}
Before (js):
function wait(t, g) {
while (g(), t.ready()) {
};
}
After (js):
function wait(t, g) {
while (true) {
g();
if (!t.ready()) {
break;
}
};
}
JS modified (optional @ignore argument):
Example:
type t
@send external push: (t, int, ~extra: @ignore int=?, unit) => unit = "push"
let add = (t, g) => push(t, 1, ~extra=?g(), ())
Before (js):
function add(t, g) {
Primitive_option.valFromOption(g());
t.push(1);
}
After (js):
function add(t, g) {
g();
t.push(1);
}
JS modified (external whose arguments are all @ignore):
Example:
@Val external count: (@ignore int, @ignore int) => int = "count"
@module external path: @ignore int => {..} = "node:path"
let counted = (f, g) => count(f(), g())
let loaded = g => path(g())
Before (js):
function counted(f, g) {
return count(f(), g());
}
function loaded(g) {
return Nodepath;
}
After (js):
function counted(f, g) {
f();
g();
return count();
}
function loaded(g) {
g();
return Nodepath;
}
JS modified (where the effects of a unit or @ignore argument run):
Before (representation): Lam_compile_external_call.ocaml_to_js_eff
setX(t, f(), g()), the unit argument g() ↦ before the whole call
makeThen(~a=f(), g()), the unit argument g() ↦ before the object
push(t, 1, ~extra=?g(), ()) ↦ valFromOption(g()), before the call
count(f(), g()) ↦ passed to count
path(g()) ↦ dropped
After (representation): Lam_compile_external_call.hold_arg
setX(t, f(), g()), the unit argument g() ↦ statement in its place
makeThen(~a=f(), g()), the unit argument g() ↦ statement in its place
push(t, 1, ~extra=?g(), ()) ↦ g(), statement in its place
count(f(), g()) ↦ statements in their place, not passed
path(g()) ↦ statement in its place
Tests: `bs_ignore_effect_test.res` checks the order of `unit` and `@ignore`
arguments after others, with `@send` and `@obj` externals, a `unit`
argument that changes an earlier one, an absent optional `@ignore`
argument, and externals whose arguments are all `@ignore`.
`polyvar_external_arg_test.res` checks the order after `@string` and `@int`
arguments, labelled, optional and in an `@obj` literal, whose optional
field known to be present stays in the literal, and a `unit` argument that
changes an event-style `@val` global before it is read. Compiled with
#9217 or the previous commit, these tests fail. `bs_ignore_effect_test.res`
also checks that a `@send` external whose object is marked `@ignore` still
calls the method on that object.
Review: Why does some output get bigger? A `unit` argument with side
effects now has a statement. A while condition with statements becomes
a loop that breaks, and a call with one, as an element of an array or
an argument of another call, evaluates the elements or arguments to
its left first, binding those with side effects:
`[ready(t, g()), ready(t, h())]` compiles to
`g(); let tmp = t.ready(); h(); return [tmp, t.ready()];` instead of
`[(g(), t.ready()), (h(), t.ready())]`. Hoisting the effects as before
is right only when every argument to their left can be read after
them, which would keep the effect lists that this commit removes; the
old order was wrong whenever that did not hold. Outside the new tests,
three such sequences in checked-in outputs become statements, two in
`bs_ignore_effect_test.mjs` and one in
`optional_external_arg_once_test.mjs` of #9283.
Review: Why does `hold_args` take the declaration of the external?
`translate_ffi` calls the method of a `@send` external on its first
argument whatever that argument's spec says, and the frontend accepts
`@ignore` there, as in a `@send` external `push` of type
`(@ignore array<int>, int) => int`. So `hold_args` leaves that argument
as it is, where it would replace an `@ignore` one with `undefined`, and
`push(arr, 1)` still compiles to `arr.push(1)`.
Review: Why are the arguments of an external whose arguments are all
`@ignore` no longer passed? Every other external leaves `@ignore`
arguments out. `translate_ffi` took such an external for a JS value
and called it with the arguments it got, which `hold_arg` now replaces
with `undefined`. A JS value whose type is a function type it does not
spell out, such as `@val external v: u = "v"` with
`type u = (int, int) => int`, has no argument specs and still gets its
arguments, as `ExternalArity.res` checks. A `@module` external of the
module itself still is the module, but it no longer drops the effects
of its arguments.
Review: What about an optional `@ignore` argument? Only its payload is
evaluated, for its effects, so it no longer goes through
`Primitive_option.valFromOption`, whose result was dropped. One without
side effects, such as a variable, a field or a `@get` property, is not
read at all, where it was tested for `None` and unwrapped, or given to
`valFromOption`.
Signed-off-by: mununki <[email protected]>
mununki
added a commit
that referenced
this pull request
Nov 9, 2026
Why:
The first commit reads a `@val` global in place for the tag and for
the payload of an event-style argument, like a `@module` value. A
`@val` with a dotted name such as `"window.ev"` compiles to a variable
printed as `window.ev`, so it was read in place too, though it is the
same property read as `@val @scope("window") external ev: … = "ev"`,
which keeps its temporary (a lower-severity regression in the review
of #9231). `Js_analyzer.is_okay_to_duplicate ~property_reads:false`,
which #9283 added for optional arguments, already tells such a name
from a variable, so `duplicable` now defers to it, as the review
suggested, and adds array elements and conditionals, and fields of
them.
JS modified (@Val with a dotted name as an event-style argument):
Example:
type t
@Val external ev: [#click(int => unit) | #keydown(int => unit)] = "window.ev"
@send external on: (t, @string [#click(int => unit) | #keydown(int => unit)]) => unit = "on"
let listen = t => on(t, ev)
Before (js):
function listen(t) {
t.on(window.ev.NAME, window.ev.VAL);
}
After (js):
function listen(t) {
let tmp = window.ev;
t.on(tmp.NAME, tmp.VAL);
}
JS modified (event-style arguments read twice):
Before (representation): Js_of_lam_variant.eval_as_event_in_place
x, r.ev, ev, M.ev ↦ read twice
window.ev, as @Val "window.ev" ↦ read twice
window.ev, as @Val @scope("window") "ev" ↦ temporary
s.ev, as @get ↦ temporary
arr[i], c ? x : y, rs[i].ev ↦ read twice
f() ↦ temporary
note: ev is a @Val global and M.ev a @module value
After (representation): Js_of_lam_variant.eval_as_event_in_place
x, r.ev, ev, M.ev ↦ read twice
window.ev, as @Val "window.ev" ↦ temporary
window.ev, as @Val @scope("window") "ev" ↦ temporary
s.ev, as @get ↦ temporary
arr[i], c ? x : y, rs[i].ev ↦ read twice
f() ↦ temporary
note: ev is a @Val global and M.ev a @module value
Tests: `polyvar_external_arg_test.res` checks that a `@val` with a dotted
name is read once, like the same name with `@scope`. Compiled with the
previous commit, it fails.
Review: Why does `duplicable` add array elements and conditionals, and
fields of them, to `is_okay_to_duplicate`? That predicate leaves them
out, and the other callers that pass `~property_reads:false` read such
an argument once instead: an optional argument of a call goes to
`Primitive_option.valFromOption`, and one of an `@obj` external to a
temporary after the literal. The alternative for an event-style
argument is a temporary before the call, a statement that makes
`sequence_arguments` bind the arguments to its left that have side
effects, so these reads without side effects stay in place, as #9217
left them. A field of a variable is already in `is_okay_to_duplicate`;
`duplicable` takes fields itself only to reach the array element or
conditional under them, as in `rs[i].ev`. No other output changes.
Review: Why is a `@val` global without a dot still read twice, though a
getter may then give the tag of one event and the payload of another?
The review of #9217 asked to treat a `@val` global like a `@module`
value, which Babel or TypeScript may compile to a getter as well, and
to read both in place, as compilers before #9217 did. A later argument
that changes the global is no longer a problem since the previous
commit: its effects are statements, before which the global is bound.
Review: Why no changelog fragment? A dotted `@val` was bound in
13.0.0-alpha.7 as well, so this only takes back what the first commit
changed for it.
Signed-off-by: mununki <[email protected]>
4 tasks done
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-9283 |
Member
Author
|
@codex review |
Codex Review Summary: ✅ CompletedThis 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. Another round soon, please! 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". |
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: #9226
Fixes: #9225
An optional argument of an external passed as
~x=?ethrew aTypeErrorwhenewasNoneand was not a variable or a field of one, as in~name=?names->Dict.get(id),~x=?f(),~x=?a[i]or a conditional. It compiles toPrimitive_option.valFromOption(e), which read a property ofundefined. This regressed in 13.0.0-alpha.5 (#8380), and it also broke optional JSX keys (with-bs-jsx-preservetoo), inlined functions that forward~x?, and optional@intand@asarguments. In an@objexternal, an optional argument whose presence is not known was evaluated after every later argument, because it is assigned after the object literal:make(~size=?size(), ~mode=mode(), ())calledmode()first, and a later argument could change what it read. An optional argument that is a JS property read, such as a@getgetter, was read twice, so a getter ran twice and could make the call throw. These arguments now passundefinedforNone, run in source order and are read once.Primitive_option.valFromOptionnow returnsundefinedforundefined(x == nullinstead ofx === null), as its doc comment says and #8380 assumed. Every path that emits the unchecked call goes through it, so the generated code stays the same, and code that 13.0.0-alpha.5 or later already compiled works with the updated runtime.Lam_compile_external_obj.hold_obj_argsbinds a presence-unknown optional@objargument of any type to a temporary when an argument that the literal evaluates after it could change it or be changed by it, usingLam_compile.delay_safe(#9177). As for the temporaries of #9217,sequence_argumentsthen evaluates the arguments to its left first.evaluate_nownow evaluates only the value of anOptional_block, so an optional argument known to be present stays in the literal.sequence_argumentsalso leaves an argument that runs no code (a variable, a constant or a field of a record) in place when the statements after it only define such values; a JS property read may run a getter, so it does not count.Js_analyzer.is_okay_to_duplicate ~property_reads:falserejects JS property reads and@valnames with a dot. With it, a call and a JSX key pass such an argument tovalFromOptiononce, andassemble_obj_argsbinds it.@valglobals passed as optional arguments compile as before (x !== undefined ? valFromOption(x) : undefined).@objcall keeps its output unless a later argument can change the optional one or be changed by it. Constants, immutable variables, functions, other presence-unknown optional arguments, and reads without side effects after an optional read without side effects all compile as before. Because the JS IR has no mutability information,make(~a=?props.a, ~b=h(), ())now bindsprops.a. A field of a JS object type,o["a"], cannot be told apart from a getter, so an@objexternal binds it as well.unitand@ignorearguments still run before the literal or the call, and getters are still treated as free of side effects.hold_argandjs_of_lam_variant.mlare unchanged. This covers items 1 and 4 and the An external call throws aTypeErrorwhen~x=?is given a call or lookup that returnsNone#9226 section of the Evaluate @int, @as, unit and @ignore arguments of externals in their place #9231 review on master, so Evaluate @int, @as, unit and @ignore arguments of externals in their place #9231 can rebase onto it.Primitive_option.mjs/.cjs(x === nullbecomes(x == null)). The runtime, the stdlib, Belt and the other tests compile to the same JavaScript.Tests:
optional_external_arg_none_test.respassesNonefrom a call, a lookup and a conditional to@val,@sendand@newexternals, an inlined wrapper,@intand@stringarguments, and a JSX key. master compiles it to the same JavaScript, and its five tests fail with master's runtime.optional_external_arg_once_test.rescounts getter reads (a call, an@intargument, a JSX key, an@objfield, a@valname with a dot). It also checks the order and values of@objfields next to fields andunitarguments with side effects, and next to getters, globals,Dict.get, array elements, refs, mutable record fields and polymorphic variant fields that a later field changes, as well as the key order of a field known to be present. 13 of its 14 tests fail when compiled with master; the other checks that a field before a getter that changes it keeps its value.Checklist
changes/README.md), or no release notes are needed and a maintainer should applychangelog:skip.CONTRIBUTING.md).git commit -s).