From c907500383d7f69acff17d53663d916a72ee8705 Mon Sep 17 00:00:00 2001 From: mununki Date: Tue, 10 Nov 2026 00:21:48 +0900 Subject: [PATCH 1/2] Pass undefined for an optional external argument or JSX key that is None MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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, 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 --- .../optional-external-argument-none.fix.md | 1 + compiler/core/js_of_lam_option.ml | 4 +- .../@rescript/runtime/Primitive_option.res | 2 +- .../runtime/lib/es6/Primitive_option.mjs | 2 +- .../runtime/lib/js/Primitive_option.cjs | 2 +- .../src/optional_external_arg_none_test.mjs | 148 ++++++++++++++++++ .../src/optional_external_arg_none_test.res | 105 +++++++++++++ 7 files changed, 260 insertions(+), 4 deletions(-) create mode 100644 changes/optional-external-argument-none.fix.md create mode 100644 tests/tests/src/optional_external_arg_none_test.mjs create mode 100644 tests/tests/src/optional_external_arg_none_test.res diff --git a/changes/optional-external-argument-none.fix.md b/changes/optional-external-argument-none.fix.md new file mode 100644 index 00000000000..f4682822ead --- /dev/null +++ b/changes/optional-external-argument-none.fix.md @@ -0,0 +1 @@ +- Fix a `TypeError` when an optional external argument such as `~x=?dict->Dict.get(k)`, or an optional JSX key, is `None`. diff --git a/compiler/core/js_of_lam_option.ml b/compiler/core/js_of_lam_option.ml index ef9afc1fa3e..b0179790ed6 100644 --- a/compiler/core/js_of_lam_option.ml +++ b/compiler/core/js_of_lam_option.ml @@ -77,7 +77,9 @@ let get_default_undefined_from_optional (arg : J.expression) : J.expression = if Js_analyzer.is_okay_to_duplicate arg then (* FIXME: no need do such inlining*) E.econd (is_not_none arg) (val_from_option arg) E.undefined - else E.runtime_call Primitive_modules.option "valFromOption" [arg] + else + (* [valFromOption] returns [undefined] for [None] as well *) + E.runtime_call Primitive_modules.option "valFromOption" [arg] let option_unwrap (arg : J.expression) : J.expression = let desc = arg.expression_desc in diff --git a/packages/@rescript/runtime/Primitive_option.res b/packages/@rescript/runtime/Primitive_option.res index 05b4e65f71b..d348909a6d7 100644 --- a/packages/@rescript/runtime/Primitive_option.res +++ b/packages/@rescript/runtime/Primitive_option.res @@ -63,7 +63,7 @@ let fromNull = (type t, x: Primitive_js_extern.null): option => /** Preserves `None`/`undefined` and unwraps one outer option layer. Nested `Some` values stay encoded one level deeper. */ let valFromOption = (x: Obj.t): Obj.t => - if x !== Obj.repr(Primitive_js_extern.null) && isNested(x) { + if !Primitive_js_extern.testAny(x) && isNested(x) { let {depth}: nested = Obj.magic(x) if depth == 0 { Obj.magic(None) diff --git a/packages/@rescript/runtime/lib/es6/Primitive_option.mjs b/packages/@rescript/runtime/lib/es6/Primitive_option.mjs index 9fe7522ae00..13be2c44f1a 100644 --- a/packages/@rescript/runtime/lib/es6/Primitive_option.mjs +++ b/packages/@rescript/runtime/lib/es6/Primitive_option.mjs @@ -46,7 +46,7 @@ function fromNull(x) { } 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; } let depth = x.BS_PRIVATE_NESTED_SOME_NONE; diff --git a/packages/@rescript/runtime/lib/js/Primitive_option.cjs b/packages/@rescript/runtime/lib/js/Primitive_option.cjs index 5b50ebc6680..e297a59a405 100644 --- a/packages/@rescript/runtime/lib/js/Primitive_option.cjs +++ b/packages/@rescript/runtime/lib/js/Primitive_option.cjs @@ -46,7 +46,7 @@ function fromNull(x) { } 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; } let depth = x.BS_PRIVATE_NESTED_SOME_NONE; diff --git a/tests/tests/src/optional_external_arg_none_test.mjs b/tests/tests/src/optional_external_arg_none_test.mjs new file mode 100644 index 00000000000..5b5b448b867 --- /dev/null +++ b/tests/tests/src/optional_external_arg_none_test.mjs @@ -0,0 +1,148 @@ +// Generated by ReScript, PLEASE EDIT WITH CARE + +import * as Mocha from "mocha"; +import * as Test_utils from "./test_utils.mjs"; +import * as Primitive_option from "@rescript/runtime/lib/es6/Primitive_option.mjs"; +import * as JsxRuntime from "react/jsx-runtime"; + +function greet(name) { + return name === undefined ? "Hello" : "Hello " + name +} + +function Greeting(name) { + this.text = greet(name) +} +; + +let makeTarget = (() => { + const calls = [] + const record = (...args) => { + calls.push(args.map(String).join(" ")) + } + return { calls, greet, setLevel: record, setMode: record } +}); + +let opaque = (x => x); + +function greetUser(names, id) { + return greet(Primitive_option.valFromOption(names[id])); +} + +function hello(name, param) { + return greet(name !== undefined ? Primitive_option.valFromOption(name) : undefined); +} + +function helloUser(names, id) { + return greet(Primitive_option.valFromOption(names[id])); +} + +function item(keys, id) { + return JsxRuntime.jsx("li", { + children: id + }, Primitive_option.valFromOption(keys[id])); +} + +Mocha.describe("Optional_external_arg_none_test", () => { + Mocha.test("None from a call, a lookup or a conditional", () => { + let names = { + ann: "Ann" + }; + Test_utils.eq("File \"optional_external_arg_none_test.res\", line 68, characters 7-14", "Hello Ann", greetUser(names, "ann")); + Test_utils.eq("File \"optional_external_arg_none_test.res\", line 69, characters 7-14", "Hello", greetUser(names, "bob")); + let list = ["Ann"]; + let at = i => greet(Primitive_option.valFromOption(list[i])); + Test_utils.eq("File \"optional_external_arg_none_test.res\", line 72, characters 7-14", [ + "Hello Ann", + "Hello" + ], [ + at(0), + at(1) + ]); + Test_utils.eq("File \"optional_external_arg_none_test.res\", line 73, characters 7-14", "Hello", greet(Primitive_option.valFromOption(opaque(undefined)))); + let pick = (b, name) => greet(Primitive_option.valFromOption(b ? name : undefined)); + Test_utils.eq("File \"optional_external_arg_none_test.res\", line 75, characters 7-14", [ + "Hello Ann", + "Hello" + ], [ + pick(true, "Ann"), + pick(false, "Ann") + ]); + }); + Mocha.test("None passed to @send and @new externals", () => { + let t = makeTarget(); + Test_utils.eq("File \"optional_external_arg_none_test.res\", line 80, characters 7-14", "Hello", t.greet(Primitive_option.valFromOption(opaque(undefined)))); + Test_utils.eq("File \"optional_external_arg_none_test.res\", line 81, characters 7-14", "Hello", new Greeting(Primitive_option.valFromOption(opaque(undefined))).text); + }); + Mocha.test("None forwarded by an inlined function", () => { + let names = { + ann: "Ann" + }; + Test_utils.eq("File \"optional_external_arg_none_test.res\", line 86, characters 7-14", [ + "Hello Ann", + "Hello" + ], [ + helloUser(names, "ann"), + helloUser(names, "bob") + ]); + }); + Mocha.test("None for @int and @string arguments", () => { + let t = makeTarget(); + let levels = ["high"]; + let modes = ["dark"]; + t.setLevel((() => { + switch (Primitive_option.valFromOption(levels[0])) { + case "low" : + return 0; + case "high" : + return 10; + } + })()); + t.setLevel((() => { + switch (Primitive_option.valFromOption(levels[1])) { + case "low" : + return 0; + case "high" : + return 10; + } + })()); + t.setMode((() => { + switch (Primitive_option.valFromOption(modes[0])) { + case "light" : + return "light-mode"; + case "dark" : + return "dark-mode"; + } + })()); + t.setMode((() => { + switch (Primitive_option.valFromOption(modes[1])) { + case "light" : + return "light-mode"; + case "dark" : + return "dark-mode"; + } + })()); + Test_utils.eq("File \"optional_external_arg_none_test.res\", line 97, characters 7-14", [ + "10", + "undefined", + "dark-mode", + "undefined" + ], t.calls); + }); + Mocha.test("a JSX key that is None", () => { + let keys = { + a: "key-a" + }; + Test_utils.eq("File \"optional_external_arg_none_test.res\", line 102, characters 7-14", "key-a", Primitive_option.fromNullable(item(keys, "a").key)); + Test_utils.eq("File \"optional_external_arg_none_test.res\", line 103, characters 7-14", undefined, Primitive_option.fromNullable(item(keys, "b").key)); + }); +}); + +export { + makeTarget, + opaque, + greetUser, + hello, + helloUser, + item, +} +/* Not a pure module */ diff --git a/tests/tests/src/optional_external_arg_none_test.res b/tests/tests/src/optional_external_arg_none_test.res new file mode 100644 index 00000000000..9ffedc59676 --- /dev/null +++ b/tests/tests/src/optional_external_arg_none_test.res @@ -0,0 +1,105 @@ +@@config({flags: ["-bs-jsx", "4"]}) + +open Mocha +open Test_utils + +// An optional argument of an external passed as `~x=?e` is left out, that +// is passed as undefined, when `e` is None, and so is a JSX key. A call, a +// lookup or a conditional cannot be read twice to test it first, so it is +// unwrapped by Primitive_option.valFromOption, which keeps undefined. + +%%raw(` +function greet(name) { + return name === undefined ? "Hello" : "Hello " + name +} + +function Greeting(name) { + this.text = greet(name) +} +`) + +@val external greet: (~name: string=?, unit) => string = "greet" + +type greeting +@new external makeGreeting: (~name: string=?, unit) => greeting = "Greeting" +@get external text: greeting => string = "text" + +type target + +let makeTarget: unit => target = %raw(`() => { + const calls = [] + const record = (...args) => { + calls.push(args.map(String).join(" ")) + } + return { calls, greet, setLevel: record, setMode: record } +}`) + +@get external calls: target => array = "calls" + +@send external greetWith: (target, ~name: string=?, unit) => string = "greet" + +@send +external setLevel: (target, ~level: @int [#low | @as(10) #high]=?, unit) => unit = "setLevel" + +@send +external setMode: ( + target, + ~mode: @string [@as("light-mode") #light | @as("dark-mode") #dark]=?, + unit, +) => unit = "setMode" + +// Opaque, so that the optimizer cannot tell the argument is None. +let opaque: 'a => 'a = %raw(`x => x`) + +let greetUser = (names: dict, id) => greet(~name=?names->Dict.get(id), ()) + +// Inlined into its callers, where the argument replaces [name]. +let hello = (~name=?, ()) => greet(~name?, ()) + +let helloUser = (names: dict, id) => hello(~name=?names->Dict.get(id), ()) + +let item = (keys: dict, id) =>
  • Dict.get(id)}>{React.string(id)}
  • + +@get external elementKey: React.element => Nullable.t = "key" + +describe(__MODULE__, () => { + test("None from a call, a lookup or a conditional", () => { + let names = dict{"ann": "Ann"} + eq(__LOC__, "Hello Ann", greetUser(names, "ann")) + eq(__LOC__, "Hello", greetUser(names, "bob")) + let list = ["Ann"] + let at = i => greet(~name=?list->Array.get(i), ()) + eq(__LOC__, ("Hello Ann", "Hello"), (at(0), at(1))) + eq(__LOC__, "Hello", greet(~name=?opaque(None), ())) + let pick = (b, name) => greet(~name=?b ? Some(name) : None, ()) + eq(__LOC__, ("Hello Ann", "Hello"), (pick(true, "Ann"), pick(false, "Ann"))) + }) + + test("None passed to @send and @new externals", () => { + let t = makeTarget() + eq(__LOC__, "Hello", t->greetWith(~name=?opaque(None), ())) + eq(__LOC__, "Hello", makeGreeting(~name=?opaque(None), ())->text) + }) + + test("None forwarded by an inlined function", () => { + let names = dict{"ann": "Ann"} + eq(__LOC__, ("Hello Ann", "Hello"), (helloUser(names, "ann"), helloUser(names, "bob"))) + }) + + test("None for @int and @string arguments", () => { + let t = makeTarget() + let levels = [#high] + let modes = [#dark] + t->setLevel(~level=?levels->Array.get(0), ()) + t->setLevel(~level=?levels->Array.get(1), ()) + t->setMode(~mode=?modes->Array.get(0), ()) + t->setMode(~mode=?modes->Array.get(1), ()) + eq(__LOC__, ["10", "undefined", "dark-mode", "undefined"], calls(t)) + }) + + test("a JSX key that is None", () => { + let keys = dict{"a": "key-a"} + eq(__LOC__, Some("key-a"), item(keys, "a")->elementKey->Nullable.toOption) + eq(__LOC__, None, item(keys, "b")->elementKey->Nullable.toOption) + }) +}) From 308b45686d3097e54acab46e83021041e82db15f Mon Sep 17 00:00:00 2001 From: mununki Date: Tue, 10 Nov 2026 00:21:48 +0900 Subject: [PATCH 2/2] Evaluate optional @obj arguments in source order and read properties once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 = "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 = "name" @val external greet: (~name: string=?, unit) => string = "greet" @val external userName: option = "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}) => 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 --- .../optional-external-argument-once.fix.md | 2 + compiler/core/js_analyzer.ml | 9 +- compiler/core/js_analyzer.mli | 11 +- compiler/core/js_ast_util.ml | 5 +- compiler/core/js_ast_util.mli | 5 +- compiler/core/js_of_lam_option.ml | 2 +- compiler/core/lam_compile.ml | 33 +- compiler/core/lam_compile_external_obj.ml | 72 ++- compiler/core/lam_compile_external_obj.mli | 7 +- .../src/optional_external_arg_once_test.mjs | 440 ++++++++++++++++++ .../src/optional_external_arg_once_test.res | 356 ++++++++++++++ 11 files changed, 914 insertions(+), 28 deletions(-) create mode 100644 changes/optional-external-argument-once.fix.md create mode 100644 tests/tests/src/optional_external_arg_once_test.mjs create mode 100644 tests/tests/src/optional_external_arg_once_test.res diff --git a/changes/optional-external-argument-once.fix.md b/changes/optional-external-argument-once.fix.md new file mode 100644 index 00000000000..aec248a9582 --- /dev/null +++ b/changes/optional-external-argument-once.fix.md @@ -0,0 +1,2 @@ +- Evaluate an optional `@obj` argument such as `~x=?f()` in source order. +- Read an optional external argument that is a JS property, such as a `@get` getter, only once. diff --git a/compiler/core/js_analyzer.ml b/compiler/core/js_analyzer.ml index 85f46a68a8a..9eab7e1e39d 100644 --- a/compiler/core/js_analyzer.ml +++ b/compiler/core/js_analyzer.ml @@ -365,8 +365,13 @@ let rev_toplevel_flatten block = is_constant a && is_constant b | _ -> false *) -let rec is_okay_to_duplicate (e : J.expression) = +let rec is_okay_to_duplicate ?(property_reads = true) (e : J.expression) = match e.expression_desc with + | Var (Id id) + when (not property_reads) && Ext_ident.is_js id + && String.contains id.name '.' -> + false | Var _ | Bool _ | Str _ | Template_literal _ | Number _ -> true - | Static_index (e, _s, _off) -> is_okay_to_duplicate e + | Static_index (e, _s, off) -> + (property_reads || off <> None) && is_okay_to_duplicate ~property_reads e | _ -> false diff --git a/compiler/core/js_analyzer.mli b/compiler/core/js_analyzer.mli index 281ec6c4127..e325033937f 100644 --- a/compiler/core/js_analyzer.mli +++ b/compiler/core/js_analyzer.mli @@ -70,8 +70,9 @@ val rev_toplevel_flatten : J.block -> J.block (* val is_constant : J.expression -> bool *) -(** Simple expression, - no computation involved so that it is okay to be duplicated -*) - -val is_okay_to_duplicate : J.expression -> bool +val is_okay_to_duplicate : ?property_reads:bool -> J.expression -> bool +(** Simple expression, no computation involved so that it is okay to be + duplicated. With [~property_reads:false], reading it twice also gives the + same value twice: it reads no JS property, which may run a getter each + time, as one from a [@get] external or a [@val] name with a dot does. + Fields of records count. *) diff --git a/compiler/core/js_ast_util.ml b/compiler/core/js_ast_util.ml index c761a060067..ad164470fe1 100644 --- a/compiler/core/js_ast_util.ml +++ b/compiler/core/js_ast_util.ml @@ -26,8 +26,9 @@ module S = Js_stmt_make -let named_expression (e : J.expression) : (J.statement * Ident.t) option = - if Js_analyzer.is_okay_to_duplicate e then None +let named_expression ?property_reads (e : J.expression) : + (J.statement * Ident.t) option = + if Js_analyzer.is_okay_to_duplicate ?property_reads e then None else let obj = Ext_ident.create_tmp () in let obj_code = S.define_variable ~kind:Strict obj e in diff --git a/compiler/core/js_ast_util.mli b/compiler/core/js_ast_util.mli index 3977f0e4831..24cde0e1816 100644 --- a/compiler/core/js_ast_util.mli +++ b/compiler/core/js_ast_util.mli @@ -22,4 +22,7 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. *) -val named_expression : J.expression -> (J.statement * Ident.t) option +val named_expression : + ?property_reads:bool -> J.expression -> (J.statement * Ident.t) option +(** A temporary bound to the expression, unless + {!Js_analyzer.is_okay_to_duplicate} holds for it *) diff --git a/compiler/core/js_of_lam_option.ml b/compiler/core/js_of_lam_option.ml index b0179790ed6..5ad8ee143a7 100644 --- a/compiler/core/js_of_lam_option.ml +++ b/compiler/core/js_of_lam_option.ml @@ -74,7 +74,7 @@ let get_default_undefined_from_optional (arg : J.expression) : J.expression = match desc with | Optional_block (x, _) -> x (* invariant: option encoding *) | _ -> - if Js_analyzer.is_okay_to_duplicate arg then + if Js_analyzer.is_okay_to_duplicate ~property_reads:false arg then (* FIXME: no need do such inlining*) E.econd (is_not_none arg) (val_from_option arg) E.undefined else diff --git a/compiler/core/lam_compile.ml b/compiler/core/lam_compile.ml index f4841957cba..c0859e2184e 100644 --- a/compiler/core/lam_compile.ml +++ b/compiler/core/lam_compile.ml @@ -282,7 +282,8 @@ let rec delay_safe ~assignable (e : J.expression) = Building an array, block or object literal is pure, so a literal stays in place and its elements are evaluated now instead. That also keeps the literal visible to translations that look into their arguments, such as - the rows of a dict literal or a [@variadic] array. *) + the rows of a dict literal or a [@variadic] array. So does an + [Optional_block], which keeps an optional argument known to be present. *) let rec evaluate_now ~assignable (e : J.expression) : J.block * J.expression = match e.expression_desc with | _ when delay_safe ~assignable e -> ([], e) @@ -291,6 +292,9 @@ let rec evaluate_now ~assignable (e : J.expression) : J.block * J.expression = if Js_analyzer.no_side_effect_expression a then (rest, b) else ({J.statement_desc = Exp a; comment = None; source_loc = None} :: rest, b) + | Optional_block (a, identity) -> + let held, a = evaluate_now ~assignable a in + (held, {e with expression_desc = Optional_block (a, identity)}) | Array xs -> let held, xs = evaluate_all_now ~assignable xs in (held, {e with expression_desc = Array xs}) @@ -327,12 +331,29 @@ and evaluate_all_now ~assignable (xs : J.expression list) : let blocks, xs = Ext_list.split_map xs (evaluate_now ~assignable) in (List.concat blocks, xs) +(* Whether evaluating [e] runs no code: it has no side effects and reads no JS + property, which may run a getter. A record field counts. *) +let runs_no_code (e : J.expression) = + Js_analyzer.is_okay_to_duplicate ~property_reads:false e + +(* Whether [block] only defines variables as values that run no code, as a + temporary bound to a record field does *) +let defines_pure_values (block : J.block) = + Ext_list.for_all block (fun (s : J.statement) -> + match s.statement_desc with + | Variable {value = None} -> true + | Variable {value = Some e} -> runs_no_code e + | _ -> false) + (* Assembles compiled arguments for a call or a literal that evaluates them left to right. Each argument comes with the statements that have to run before its value is read. Putting all statements first and then the values would move the statements of an argument in front of the values of the arguments to its left, so when an argument has statements, every - argument to its left is evaluated before them with [evaluate_now]. + argument to its left is evaluated before them with [evaluate_now]. An + argument that runs no code stays in place when the statements after it + only define values that run none either, since then neither can change + the other. With [~method_callee:true], the first argument is a JS method read [obj.m] being called: the object is held and the property read stays in @@ -351,7 +372,8 @@ let sequence_arguments ?(method_callee = false) ~assignable Ext_list.fold_right args (last, [], []) (fun (block, value) (i, code, values) -> let held, value = - if code = [] then ([], value) + if code = [] || (runs_no_code value && defines_pure_values code) + then ([], value) else match value.expression_desc with | Static_index (obj, name, pos) when method_callee && i = 0 -> @@ -1908,7 +1930,10 @@ let compile output_prefix = | {primitive = Pjs_object_create labels; args} -> let args_code, args_expr = compile_arguments - ~hold:(Lam_compile_external_obj.hold_obj_args labels) + ~hold: + (Lam_compile_external_obj.hold_obj_args + ~delay_safe:(delay_safe ~assignable:lambda_cxt.meta.assignable) + labels) lambda_cxt args in let block, exp = diff --git a/compiler/core/lam_compile_external_obj.ml b/compiler/core/lam_compile_external_obj.ml index ebb97c50c85..3187fd15f7e 100644 --- a/compiler/core/lam_compile_external_obj.ml +++ b/compiler/core/lam_compile_external_obj.ml @@ -36,24 +36,72 @@ module S = Js_stmt_make But the default to be undefined seems reasonable *) +(* Whether [assemble_obj_args] assigns the optional argument [arg] after the + object literal, since its presence is not known *) +let presence_unknown (arg : J.expression) = + Js_of_lam_option.destruct_optional arg ~for_sure_none:false + ~for_sure_some:(fun _ -> false) + ~not_sure:(fun () -> true) + +(* The arguments that [assemble_obj_args] evaluates in the object literal or + before it, which are all but the optional ones whose presence is not + known *) +let rec literal_args (labels : External_arg_spec.obj_params) + (args : (J.block * J.expression) list) = + match (labels, args) with + | {obj_arg_label = Obj_label _; obj_arg_type = Arg_cst _} :: labels, args -> + literal_args labels args + | {obj_arg_label = Obj_optional _} :: labels, (_, arg) :: args + when presence_unknown arg -> + literal_args labels args + | ( {obj_arg_label = Obj_empty | Obj_label _ | Obj_optional _} :: labels, + (_, arg) :: args ) -> + arg :: literal_args labels args + | [], [] -> [] + | [], _ :: _ | _ :: _, [] -> assert false + (* [Lam_compile_external_call.hold_arg] for the compiled arguments of an object-creating external, which pair with the labels as in - [assemble_obj_args] *) -let rec hold_obj_args (labels : External_arg_spec.obj_params) + [assemble_obj_args]. That function assigns an optional argument whose + presence is not known after the object literal, so it evaluates the + argument after the arguments that follow it. Unless that gives the same + values and effects, the argument is bound to a temporary after its + statements, which keeps it in its place. It does when the argument is + [delay_safe], and when each argument that the literal evaluates after it + is [delay_safe] or neither of the two has side effects. *) +let rec hold_obj_args ~(delay_safe : J.expression -> bool) + (labels : External_arg_spec.obj_params) (args : (J.block * J.expression) list) = match (labels, args) with | {obj_arg_label = Obj_label _; obj_arg_type = Arg_cst _} :: labels, args -> - hold_obj_args labels args + hold_obj_args ~delay_safe labels args | {obj_arg_label = Obj_empty} :: labels, arg :: args -> - arg :: hold_obj_args labels args + arg :: hold_obj_args ~delay_safe labels args | {obj_arg_label = Obj_label _; obj_arg_type} :: labels, arg :: args -> Lam_compile_external_call.hold_arg ~arg_label:Arg_label ~arg_type:obj_arg_type arg - :: hold_obj_args labels args + :: hold_obj_args ~delay_safe labels args | {obj_arg_label = Obj_optional _; obj_arg_type} :: labels, arg :: args -> - Lam_compile_external_call.hold_arg ~arg_label:Arg_optional - ~arg_type:obj_arg_type arg - :: hold_obj_args labels args + let ((block, value) as held) = + Lam_compile_external_call.hold_arg ~arg_label:Arg_optional + ~arg_type:obj_arg_type arg + in + let order_matters (later : J.expression) = + not + (delay_safe later + || Js_analyzer.no_side_effect_expression value + && Js_analyzer.no_side_effect_expression later) + in + (if + presence_unknown value + && (not (delay_safe value)) + && List.exists order_matters (literal_args labels args) + then + let tmp = Ext_ident.create_tmp () in + ( Ext_list.append_one block (S.define_variable ~kind:Strict tmp value), + E.var tmp ) + else held) + :: hold_obj_args ~delay_safe labels args | [], [] -> [] | [], _ :: _ | _ :: _, [] -> assert false @@ -130,10 +178,10 @@ let assemble_obj_args (labels : External_arg_spec.obj_params) obj_arg_label = Obj_optional {name = label; for_sure_no_nested_option}; } -> ( - (* Need make sure whether assignment is effectful or not - to avoid code duplication - *) - match Js_ast_util.named_expression arg with + (* [arg] is read for its presence and for its value, so it is + bound to a temporary unless reading it twice gives the + same value twice *) + match Js_ast_util.named_expression ~property_reads:false arg with | None -> ( let acc, new_eff = Lam_compile_external_call.ocaml_to_js_eff diff --git a/compiler/core/lam_compile_external_obj.mli b/compiler/core/lam_compile_external_obj.mli index 22ef0262938..86f9b40f901 100644 --- a/compiler/core/lam_compile_external_obj.mli +++ b/compiler/core/lam_compile_external_obj.mli @@ -26,11 +26,16 @@ object literal. *) val hold_obj_args : + delay_safe:(J.expression -> bool) -> External_arg_spec.obj_params -> (J.block * J.expression) list -> (J.block * J.expression) list (** {!Lam_compile_external_call.hold_arg} for the compiled arguments of an - object-creating external *) + object-creating external. An optional argument whose presence is not + known is also bound to a temporary when evaluating it after the object + literal, as {!assemble_obj_args} does, could change a value or the order + of effects. [delay_safe] tells whether evaluating an argument after other + code gives the same value and effects. *) val assemble_obj_args : External_arg_spec.obj_params -> J.expression list -> J.block * J.expression diff --git a/tests/tests/src/optional_external_arg_once_test.mjs b/tests/tests/src/optional_external_arg_once_test.mjs new file mode 100644 index 00000000000..a7f7dec24ae --- /dev/null +++ b/tests/tests/src/optional_external_arg_once_test.mjs @@ -0,0 +1,440 @@ +// Generated by ReScript, PLEASE EDIT WITH CARE + +import * as Mocha from "mocha"; +import * as Test_utils from "./test_utils.mjs"; +import * as Stdlib_Dict from "@rescript/runtime/lib/es6/Stdlib_Dict.mjs"; +import * as Primitive_option from "@rescript/runtime/lib/es6/Primitive_option.mjs"; +import * as JsxRuntime from "react/jsx-runtime"; + +let makeSource = (() => ({ + reads: 0, + value: "first", + get name() { + this.reads++ + return this.value + }, + get level() { + this.reads++ + return this.reads === 1 ? "high" : undefined + } +})); + +let makeBumper = (c => ({ + get bump() { + c.count = c.count + 1 + return "bumped" + } +})); + +let setGlobalSource = (s => { + globalThis.optionalExternalArgOnceSource = s +}); + +let setGlobalName = (v => { + globalThis.optionalExternalArgOnceName = v +}); + +function greet(name) { + return name === undefined ? "Hello" : "Hello " + name +} +; + +let makeTarget = (() => { + const calls = [] + return { calls, setLevel: (...args) => { calls.push(args.map(String).join(" ")) } } +}); + +let call = (f => f()); + +function keys(o) { + return Object.keys(o); +} + +Mocha.describe("Optional_external_arg_once_test", () => { + Mocha.test("a getter passed as an optional argument", () => { + let s = makeSource(); + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 150, characters 7-14", [ + "Hello first", + 1 + ], [ + greet(Primitive_option.valFromOption(s.name)), + s.reads + ]); + let s$1 = makeSource(); + let t = makeTarget(); + t.setLevel((() => { + switch (Primitive_option.valFromOption(s$1.level)) { + case "low" : + return 0; + case "high" : + return 10; + } + })()); + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 155, characters 7-14", [ + ["10"], + 1 + ], [ + t.calls, + s$1.reads + ]); + let s$2 = makeSource(); + let key = JsxRuntime.jsx("li", { + children: "item" + }, Primitive_option.valFromOption(s$2.name)).key; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 159, characters 7-14", [ + "first", + 1 + ], [ + (key == null) ? undefined : Primitive_option.some(key), + s$2.reads + ]); + let s$3 = makeSource(); + let tmp = { + mode: "m" + }; + let tmp$1 = s$3.name; + if (tmp$1 !== undefined) { + tmp.name = tmp$1; + } + let o = tmp; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 163, characters 7-14", [ + "first", + 1 + ], [ + o.name, + s$3.reads + ]); + }); + Mocha.test("a global with a dot passed as an optional argument", () => { + let s = makeSource(); + setGlobalSource(s); + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 169, characters 7-14", [ + "Hello first", + 1 + ], [ + greet(Primitive_option.valFromOption(optionalExternalArgOnceSource.name)), + s.reads + ]); + let s$1 = makeSource(); + setGlobalSource(s$1); + let tmp = { + mode: "m" + }; + let tmp$1 = optionalExternalArgOnceSource.name; + if (tmp$1 !== undefined) { + tmp.name = tmp$1; + } + let o = tmp; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 174, characters 7-14", [ + "first", + 1 + ], [ + o.name, + s$1.reads + ]); + }); + Mocha.test("an optional field before a field with side effects", () => { + let log = []; + let mark = (s, v) => { + log.push(s); + return v; + }; + let tmp = mark("size", 1); + let tmp$1 = { + mode: mark("mode", "dark") + }; + if (tmp !== undefined) { + tmp$1.size = tmp; + } + let o = tmp$1; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 184, characters 7-14", [ + "size", + "mode" + ], log); + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 185, characters 7-14", [ + 1, + "dark" + ], [ + o.size, + o.mode + ]); + }); + Mocha.test("an optional field between fields with side effects", () => { + let log = []; + let mark = (s, v) => { + log.push(s); + return v; + }; + let tmp = mark("first", "a"); + let tmp$1 = mark("size", undefined); + let tmp$2 = { + first: tmp, + mode: mark("mode", "b") + }; + if (tmp$1 !== undefined) { + tmp$2.size = tmp$1; + } + let o = tmp$2; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 200, characters 7-14", [ + "first", + "size", + "mode" + ], log); + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 201, characters 7-14", [ + "a", + undefined, + "b" + ], [ + o.first, + o.size, + o.mode + ]); + }); + Mocha.test("an optional field before a unit argument with side effects", () => { + let log = []; + let mark = (s, v) => { + log.push(s); + return v; + }; + let tmp = mark("size", 2); + let tmp$1 = (mark("unit", undefined), {}); + if (tmp !== undefined) { + tmp$1.size = tmp; + } + let o = tmp$1; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 211, characters 7-14", [ + "size", + "unit" + ], log); + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 212, characters 7-14", 2, o.size); + }); + Mocha.test("an optional field known to be present stays in the literal", () => { + let log = []; + let mark = (s, v) => { + log.push(s); + return v; + }; + let tmp = mark("first", 1); + let tmp$1 = mark("size", 2); + let tmp$2 = { + first: tmp, + mode: mark("mode", "m") + }; + if (tmp$1 !== undefined) { + tmp$2.size = tmp$1; + } + let o = tmp$2; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 227, characters 7-14", [ + "first", + "size", + "mode" + ], log); + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 228, characters 7-14", [ + "first", + "mode", + "size" + ], Object.keys(o)); + }); + Mocha.test("a getter that a later field changes", () => { + let s = makeSource(); + let tmp = s.name; + let tmp$1 = { + mode: call(() => { + s.value = "second"; + return "getter"; + }) + }; + if (tmp !== undefined) { + tmp$1.name = tmp; + } + let o = tmp$1; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 243, characters 7-14", "first", o.name); + }); + Mocha.test("a field before a getter that changes it", () => { + let c = { + count: 1 + }; + let s = makeBumper(c); + let tmp = c.count; + let tmp$1 = s.bump; + let tmp$2 = { + count: tmp, + mode: call(() => "m") + }; + if (tmp$1 !== undefined) { + tmp$2.name = tmp$1; + } + let o = tmp$2; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 250, characters 7-14", [ + 1, + "bumped", + 2 + ], [ + o.count, + o.name, + c.count + ]); + }); + Mocha.test("a global that a later field changes", () => { + setGlobalName("first"); + let tmp = optionalExternalArgOnceName; + let tmp$1 = { + mode: call(() => { + setGlobalName("second"); + return "global"; + }) + }; + if (tmp !== undefined) { + tmp$1.name = tmp; + } + let o = tmp$1; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 265, characters 7-14", "first", o.name); + }); + Mocha.test("a dict entry that a later field changes", () => { + let d = { + k: "first" + }; + let tmp = d["k"]; + let tmp$1 = { + mode: call(() => { + Stdlib_Dict.$$delete(d, "k"); + return "dict"; + }) + }; + if (tmp !== undefined) { + tmp$1.name = tmp; + } + let o = tmp$1; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 280, characters 7-14", "first", o.name); + }); + Mocha.test("an array element that a later field changes", () => { + let a = ["first"]; + let tmp = a[0]; + let tmp$1 = { + mode: call(() => { + a[0] = "second"; + return "array"; + }) + }; + if (tmp !== undefined) { + tmp$1.name = tmp; + } + let o = tmp$1; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 295, characters 7-14", "first", o.name); + }); + Mocha.test("a ref that a later field changes", () => { + let r = "first"; + let tmp = r; + let tmp$1 = { + mode: call(() => { + r = undefined; + return "ref"; + }) + }; + if (tmp !== undefined) { + tmp$1.name = tmp; + } + let o = tmp$1; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 310, characters 7-14", "first", o.name); + }); + Mocha.test("a record field that a later field changes", () => { + let item = call(() => ({ + label: "item", + name: "first" + })); + let tmp = item.name; + let tmp$1 = { + label: item.label, + mode: call(() => { + item.name = undefined; + return "record"; + }) + }; + if (tmp !== undefined) { + tmp$1.name = tmp; + } + let o = tmp$1; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 326, characters 7-14", [ + "item", + "first", + "record" + ], [ + o.label, + o.name, + o.mode + ]); + }); + Mocha.test("optional polymorphic variant fields", () => { + let level = "high"; + let tmp = level; + let tmp$1 = { + mode: call(() => { + level = "low"; + return "level"; + }) + }; + if (tmp !== undefined) { + tmp$1.level = (() => { + switch (tmp) { + case "low" : + return 0; + case "high" : + return 10; + } + })(); + } + let o = tmp$1; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 341, characters 7-14", [ + 10, + "level" + ], [ + o.level, + o.mode + ]); + let mode = "light"; + let tmp$2 = mode; + let tmp$3 = call(() => { + mode = "dark"; + return "high"; + }); + let tmp$4 = { + level: (() => { + switch (tmp$3) { + case "low" : + return 0; + case "high" : + return 10; + } + })() + }; + if (tmp$2 !== undefined) { + tmp$4.mode = (() => { + switch (tmp$2) { + case "light" : + return "light-mode"; + case "dark" : + return "dark-mode"; + } + })(); + } + let o$1 = tmp$4; + Test_utils.eq("File \"optional_external_arg_once_test.res\", line 354, characters 7-14", [ + "light-mode", + 10 + ], [ + o$1.mode, + o$1.level + ]); + }); +}); + +export { + makeSource, + makeBumper, + setGlobalSource, + setGlobalName, + makeTarget, + call, + keys, +} +/* Not a pure module */ diff --git a/tests/tests/src/optional_external_arg_once_test.res b/tests/tests/src/optional_external_arg_once_test.res new file mode 100644 index 00000000000..3cb13c21133 --- /dev/null +++ b/tests/tests/src/optional_external_arg_once_test.res @@ -0,0 +1,356 @@ +@@config({flags: ["-bs-jsx", "4"]}) + +open Mocha +open Test_utils + +// An optional argument of an external whose presence is not known is +// evaluated once, in source order. A JS property read, which may run a getter, +// is read once, to test it for None and to unwrap it. An object-creating +// external assigns such an argument after the object literal is built, but +// evaluates it before the arguments that follow it when they have side +// effects, or read what its side effects change. The arguments before it, and +// optional arguments known to be present, stay in the literal. + +@obj +external make: (~size: int=?, ~mode: string, unit) => {"size": option, "mode": string} = "" + +@obj +external makeBetween: ( + ~first: string, + ~size: int=?, + ~mode: string, + unit, +) => {"first": string, "size": option, "mode": string} = "" + +@obj +external makeKnown: ( + ~first: int=?, + ~size: int=?, + ~mode: string, + unit, +) => {"first": option, "size": option, "mode": string} = "" + +@obj external makeSize: (~size: int=?, unit) => {"size": option} = "" + +@obj +external makeName: ( + ~name: string=?, + ~mode: string, + unit, +) => {"name": option, "mode": string} = "" + +@obj +external makeCounted: ( + ~count: int, + ~name: string=?, + ~mode: string, + unit, +) => {"count": int, "name": option, "mode": string} = "" + +@obj +external makeLabelled: ( + ~label: string, + ~name: string=?, + ~mode: string, + unit, +) => {"label": string, "name": option, "mode": string} = "" + +@obj +external makeLevel: ( + ~level: @int [#low | @as(10) #high]=?, + ~mode: string, + unit, +) => {"level": option, "mode": string} = "" + +@obj +external makeModeFirst: ( + ~mode: @string [@as("light-mode") #light | @as("dark-mode") #dark]=?, + ~level: @int [#low | @as(10) #high], + unit, +) => {"mode": option, "level": int} = "" + +type source + +// [level] is present on the first read only. +let makeSource: unit => source = %raw(`() => ({ + reads: 0, + value: "first", + get name() { + this.reads++ + return this.value + }, + get level() { + this.reads++ + return this.reads === 1 ? "high" : undefined + } +})`) + +@get external name: source => option = "name" + +type counter = {mutable count: int} + +// Reading [bump] increments the count of the counter it was made with. +let makeBumper: counter => source = %raw(`c => ({ + get bump() { + c.count = c.count + 1 + return "bumped" + } +})`) + +@get external bump: source => option = "bump" +@get external level: source => option<[#low | #high]> = "level" +@get external reads: source => int = "reads" +@set external setValue: (source, string) => unit = "value" + +let setGlobalSource: source => unit = %raw(`s => { + globalThis.optionalExternalArgOnceSource = s +}`) + +// A property read, though it compiles to a JS name. +@val external globalSourceName: option = "optionalExternalArgOnceSource.name" + +@val external globalName: option = "optionalExternalArgOnceName" + +let setGlobalName: string => unit = %raw(`v => { + globalThis.optionalExternalArgOnceName = v +}`) + +%%raw(` +function greet(name) { + return name === undefined ? "Hello" : "Hello " + name +} +`) + +@val external greet: (~name: string=?, unit) => string = "greet" + +type target + +let makeTarget: unit => target = %raw(`() => { + const calls = [] + return { calls, setLevel: (...args) => { calls.push(args.map(String).join(" ")) } } +}`) + +@get external calls: target => array = "calls" + +@send +external setLevel: (target, ~level: @int [#low | @as(10) #high]=?, unit) => unit = "setLevel" + +@get external elementKey: React.element => Nullable.t = "key" + +type item = {label: string, mutable name: option} + +// Calls the function, which the optimizer cannot inline then. +let call: (unit => 'a) => 'a = %raw(`f => f()`) + +let keys = (o: {..}) => Object.keysToArray(Obj.magic(o)) + +describe(__MODULE__, () => { + test("a getter passed as an optional argument", () => { + let s = makeSource() + eq(__LOC__, ("Hello first", 1), (greet(~name=?name(s), ()), reads(s))) + + let s = makeSource() + let t = makeTarget() + t->setLevel(~level=?level(s), ()) + eq(__LOC__, (["10"], 1), (calls(t), reads(s))) + + let s = makeSource() + let key =
  • {React.string("item")}
  • ->elementKey + eq(__LOC__, (Some("first"), 1), (key->Nullable.toOption, reads(s))) + + let s = makeSource() + let o = makeName(~name=?name(s), ~mode="m", ()) + eq(__LOC__, (Some("first"), 1), (o["name"], reads(s))) + }) + + test("a global with a dot passed as an optional argument", () => { + let s = makeSource() + setGlobalSource(s) + eq(__LOC__, ("Hello first", 1), (greet(~name=?globalSourceName, ()), reads(s))) + + let s = makeSource() + setGlobalSource(s) + let o = makeName(~name=?globalSourceName, ~mode="m", ()) + eq(__LOC__, (Some("first"), 1), (o["name"], reads(s))) + }) + + test("an optional field before a field with side effects", () => { + let log = [] + let mark = (s, v) => { + log->Array.push(s) + v + } + let o = make(~size=?mark("size", Some(1)), ~mode=mark("mode", "dark"), ()) + eq(__LOC__, ["size", "mode"], log) + eq(__LOC__, (Some(1), "dark"), (o["size"], o["mode"])) + }) + + test("an optional field between fields with side effects", () => { + let log = [] + let mark = (s, v) => { + log->Array.push(s) + v + } + let o = makeBetween( + ~first=mark("first", "a"), + ~size=?mark("size", None), + ~mode=mark("mode", "b"), + (), + ) + eq(__LOC__, ["first", "size", "mode"], log) + eq(__LOC__, ("a", None, "b"), (o["first"], o["size"], o["mode"])) + }) + + test("an optional field before a unit argument with side effects", () => { + let log = [] + let mark = (s, v) => { + log->Array.push(s) + v + } + let o = makeSize(~size=?mark("size", Some(2)), mark("unit", ())) + eq(__LOC__, ["size", "unit"], log) + eq(__LOC__, Some(2), o["size"]) + }) + + test("an optional field known to be present stays in the literal", () => { + let log = [] + let mark = (s, v) => { + log->Array.push(s) + v + } + let o = makeKnown( + ~first=mark("first", 1), + ~size=?mark("size", Some(2)), + ~mode=mark("mode", "m"), + (), + ) + eq(__LOC__, ["first", "size", "mode"], log) + eq(__LOC__, ["first", "mode", "size"], keys(o)) + }) + + test("a getter that a later field changes", () => { + let s = makeSource() + let o = makeName( + ~name=?name(s), + ~mode=call( + () => { + setValue(s, "second") + "getter" + }, + ), + (), + ) + eq(__LOC__, Some("first"), o["name"]) + }) + + test("a field before a getter that changes it", () => { + let c = {count: 1} + let s = makeBumper(c) + let o = makeCounted(~count=c.count, ~name=?bump(s), ~mode=call(() => "m"), ()) + eq(__LOC__, (1, Some("bumped"), 2), (o["count"], o["name"], c.count)) + }) + + test("a global that a later field changes", () => { + setGlobalName("first") + let o = makeName( + ~name=?globalName, + ~mode=call( + () => { + setGlobalName("second") + "global" + }, + ), + (), + ) + eq(__LOC__, Some("first"), o["name"]) + }) + + test("a dict entry that a later field changes", () => { + let d = dict{"k": "first"} + let o = makeName( + ~name=?d->Dict.get("k"), + ~mode=call( + () => { + d->Dict.delete("k") + "dict" + }, + ), + (), + ) + eq(__LOC__, Some("first"), o["name"]) + }) + + test("an array element that a later field changes", () => { + let a = ["first"] + let o = makeName( + ~name=?a->Array.get(0), + ~mode=call( + () => { + a[0] = "second" + "array" + }, + ), + (), + ) + eq(__LOC__, Some("first"), o["name"]) + }) + + test("a ref that a later field changes", () => { + let r = ref(Some("first")) + let o = makeName( + ~name=?r.contents, + ~mode=call( + () => { + r := None + "ref" + }, + ), + (), + ) + eq(__LOC__, Some("first"), o["name"]) + }) + + test("a record field that a later field changes", () => { + let item = call(() => {label: "item", name: Some("first")}) + let o = makeLabelled( + ~label=item.label, + ~name=?item.name, + ~mode=call( + () => { + item.name = None + "record" + }, + ), + (), + ) + eq(__LOC__, ("item", Some("first"), "record"), (o["label"], o["name"], o["mode"])) + }) + + test("optional polymorphic variant fields", () => { + let level = ref(Some(#high)) + let o = makeLevel( + ~level=?level.contents, + ~mode=call( + () => { + level := Some(#low) + "level" + }, + ), + (), + ) + eq(__LOC__, (Some(10), "level"), (o["level"], o["mode"])) + + let mode = ref(Some(#light)) + let o = makeModeFirst( + ~mode=?mode.contents, + ~level=call( + () => { + mode := Some(#dark) + #high + }, + ), + (), + ) + eq(__LOC__, (Some("light-mode"), 10), (o["mode"], o["level"])) + }) +})