Repository navigation
Pass undefined for an optional external argument or JSX key that is None - #9288
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 <woonki.moon@gmail.com>
Changelog🐛 Bug fix
Semantic Diff1 commit · 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9288 +/- ##
=======================================
Coverage 81.92% 81.92%
=======================================
Files 456 456
Lines 65469 65464 -5
=======================================
- Hits 53635 53631 -4
+ Misses 11834 11833 -1
🚀 New features to boost your workflow:
|
✅ Compiler benchmarksAllocated wordsIn total: 683,553,916 → 683,552,892 (±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,671,992 → 9,664,671,992 (±0.00%). None of the 33 benchmarks changed by 0.1% or more. Output sizeIn total: 25,569,598 → 25,569,647 (±0.00%). None of the 13 benchmarks changed by 0.1% or more. Pull request c907500 compared with base b82d944 in this run. |
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-9288 |
|
@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. More of your lovely PRs 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". |
cristianoc
left a comment
cristianoc
left a comment
There was a problem hiding this comment.
Reviewed c907500. The runtime fix preserves undefined and null and still unwraps nested options correctly. Focused Node checks passed, and the main CI, Coverage and Changelog workflows passed. I did not rerun the full compiler suite locally.
The remaining failure is the separate Semantic Diff pull_request_target run: GitHub reports startup_failure and it has no jobs, so there is no failing test from that run to diagnose. The pull_request Semantic Diff run succeeded. The startup failure still needs attention, but I found no code regression blocking approval.
|
|
||
| function valFromOption(x) { | ||
| if (x === null || x.BS_PRIVATE_NESTED_SOME_NONE === undefined) { | ||
| if ((x == null) || x.BS_PRIVATE_NESTED_SOME_NONE === undefined) { |
There was a problem hiding this comment.
Interesting that this differs in terms of parentheses, maybe something for a followup.
There was a problem hiding this comment.
testAny compiles to Is_null_or_undefined, which Js_dump put in parentheses whenever it was not at the top level, instead of by precedence as for x === null. Its operand was also printed at a fixed low level, so it could lose parentheses it needs: Nullable.isNullable(c ? a : b) compiled to c ? a : b == null, and so did an if or switch expression there. That goes back to at least 11.1. Filed as #9290 and fixed in #9291 by printing it with the precedence of ===.
Fixes: #9226
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.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.valFromOptionpass a value known to beSome; for themx == nullreplacesx === null, still one comparison.Primitive_option.mjs/.cjs. The rest of 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.Checklist
changes/README.md), or no release notes are needed and a maintainer should applychangelog:skip.CONTRIBUTING.md).git commit -s).