Repository navigation
Conversation
Why:
`Lam_util.refine_let` rewrote `let x = e; f(x)` to `f(e)` whenever `f`
did not mention `x`. JavaScript evaluates the callee of a call before
the arguments, so `e` then ran after `f`: a `let` that read state saw
what the callee had changed, and an effect of the `let` came after the
effects of the callee, or after its `await`. The same happened to the
binding `Lam_beta_reduce` makes for an argument of an inlined function,
and to the binding `Lam_pass_sroa` makes for a field of a local record.
The rewrite now applies only when the two commute: one of them has no
side effects and reads nothing that may change, or neither has side
effects. The rewrite of `let x = e; p(x)` for a primitive `p` did the
same to the call of an external function or class, which JavaScript
reads from its global or module before the argument; it now applies to
such a call only when `e` has no side effects. `refine_let` takes the
assigned variables for this, and `motion` and `conflict` move from
`Lam_beta_reduce_util` to `Lam_analysis`.
JS modified (let read before a callee that changes it):
Example:
type counter = {mutable n: int}
let c = {n: 0}
let next = () => {
c.n = c.n + 1
(x: int) => x
}
let readBeforeCall = () => {
let before = c.n
next()(before)
}
Before (js):
function readBeforeCall() {
return next()(c.n);
}
After (js):
function readBeforeCall() {
let before = c.n;
return next()(before);
}
JS modified (let with an effect before an awaited callee):
Example:
let effectBeforeAwait = async () => {
let x = log("argument")
(await getHandler())(x)
}
Before (js):
async function effectBeforeAwait() {
return (await getHandler())(log("argument"));
}
After (js):
async function effectBeforeAwait() {
let x = log("argument");
return (await getHandler())(x);
}
JS modified (let that replaces the field the callee is read from):
Example:
type handler = {mutable call: int => int}
let h = {call: x => x + 1}
let swap = () => {
h.call = x => x + 100
1
}
let effectBeforeFieldRead = () => {
let x = swap()
h.call(x)
}
Before (js):
function effectBeforeFieldRead() {
return h.call(swap());
}
After (js):
function effectBeforeFieldRead() {
let x = swap();
return h.call(x);
}
JS modified (field of a local record):
Example:
type snapshot = {mutable unused: int, saved: int}
let recordField = () => {
let s = {unused: 0, saved: c.n}
next()(s.saved)
}
Before (js):
function recordField() {
return next()(c.n);
}
After (js):
function recordField() {
let s_saved = c.n;
return next()(s_saved);
}
JS modified (let that replaces an external function):
Example:
@scope("globalThis") @Val external hook: int => int = "letBeforeCalleeHook"
let effectBeforeExternal = () => {
let x = install()
hook(x)
}
Before (js):
function effectBeforeExternal() {
return globalThis.letBeforeCalleeHook(install());
}
After (js):
function effectBeforeExternal() {
let x = install();
return globalThis.letBeforeCalleeHook(x);
}
Review: How were the changes tested? `let_before_callee_test` has nine
tests. Compiled with master, eight fail. One of them is for an `@new`
external. `inline_arg_order_cross_module_test` gains a test that fails
with master as well.
Review: Which calls still take the value in place? Those where the order
cannot matter: `let x = swap(); cb(x)` with a parameter `cb` compiles to
`cb(swap())`, and `let x = c.n; h.call(x)` to `h.call(c.n)`, since
neither the field read nor `c.n` has side effects.
Review: Which checked-in output changes? One, in `Stdlib_PromiseTest`:
`let s = switch e {...}; resolve(s)` keeps `s` instead of assigning the
result of the `switch` to a `tmp`, because a `switch` counts as having
side effects and `resolve` is the external `Promise.resolve`.
Review: Why does a call through a field of an immutable record keep the
binding, as in `let x = f(); r.call(x)`? The field may still change: a
record with a mutable field can be coerced to a type where the field
is immutable, so `f()` may assign it through the other type.
Review: Where do the assigned variables matter? `Lam_pass_sroa` makes the
`Lassign`s and is the last Lambda pass, so the set is empty in
`Lam_pass_deep_flatten`, `Lam_pass_lets_dce` and the inliner today;
they compute it anyway, as `Lam_pass_remove_alias` does. It is not
empty where `Lam_compile` inlines a function of another module under
`-bs-cross-module-opt`, which happens after `Lam_pass_sroa`. There an
argument that reads a local ref, by then an assigned variable, stays
bound before a callee that assigns it; the new test in
`inline_arg_order_cross_module_test` covers that.
Signed-off-by: mununki <[email protected]>
Changelog🐛 Bug fix
Semantic Diff2 commits · Full page Evaluate a let before the callee of the call it feeds195aca5 · mununki · Nov 13, 2026, 13:35 UTC
JS modified (let read before a callee that changes it) type counter = {mutable n: int}
let c = {n: 0}
let next = () => {
c.n = c.n + 1
(x: int) => x
}
let readBeforeCall = () => {
let before = c.n
next()(before)
}
function readBeforeCall() {
- return next()(c.n);
+ let before = c.n;
+ return next()(before);
}JS modified (let with an effect before an awaited callee) let effectBeforeAwait = async () => {
let x = log("argument")
(await getHandler())(x)
}
async function effectBeforeAwait() {
- return (await getHandler())(log("argument"));
+ let x = log("argument");
+ return (await getHandler())(x);
}JS modified (let that replaces the field the callee is read from) type handler = {mutable call: int => int}
let h = {call: x => x + 1}
let swap = () => {
h.call = x => x + 100
1
}
let effectBeforeFieldRead = () => {
let x = swap()
h.call(x)
}
function effectBeforeFieldRead() {
- return h.call(swap());
+ let x = swap();
+ return h.call(x);
}JS modified (field of a local record) type snapshot = {mutable unused: int, saved: int}
let recordField = () => {
let s = {unused: 0, saved: c.n}
next()(s.saved)
}
function recordField() {
- return next()(c.n);
+ let s_saved = c.n;
+ return next()(s_saved);
}JS modified (let that replaces an external function) @scope("globalThis") @val external hook: int => int = "letBeforeCalleeHook"
let effectBeforeExternal = () => {
let x = install()
hook(x)
}
function effectBeforeExternal() {
- return globalThis.letBeforeCalleeHook(install());
+ let x = install();
+ return globalThis.letBeforeCalleeHook(x);
}Review
✨ Collect the assigned variables without allocating for each nodeb54982a · mununki · Nov 13, 2026, 14:14 UTC · new
|
🎉 Compiler benchmarksAllocated wordsIn total: 640,953,418 → 636,265,418 (-0.73%). Generated projects: 920,899,498 → 920,079,781 (-0.09%). 🎉 Improvement: 23 benchmarks allocate more than 1% less.
94 more changes
InstructionsIn total: 9,209,714,034 → 9,216,006,858 (+0.07%).
Output sizeIn total: 25,843,406 → 25,843,830 (±0.00%). None of the 13 benchmarks changed by 0.1% or more. Pull request b54982a compared with base ed08a8c in this run. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9296 +/- ##
=======================================
Coverage 82.07% 82.07%
=======================================
Files 455 455
Lines 65361 65414 +53
=======================================
+ Hits 53642 53690 +48
- Misses 11719 11724 +5
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
`Lambda_traverse.assigned_variables` walked a term with `iter`, which allocates a closure for every node. The previous commit calls it in each `Lam_pass_deep_flatten` pass, which made the compiler allocate 0.6% more on the benchmarks. The walk now passes one callback to `shallow_exists`. With it, compiling `Stdlib_List.res` allocates 2,549,873 words where master allocates 2,588,255, since `Lam_pass_remove_alias` and `Lam_pass_lets_dce` already called it. Signed-off-by: mununki <[email protected]>
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-9296 |
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. 🎉 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 #9228.
let x = e; f(x)compiled tof(e)wheneverfdid not mentionx. JavaScript evaluates the callee of a call before the arguments, soeran afterf:The same rewrite,
Lam_util.refine_let, is applied to the binding the inliner makes for an argument and to the bindingLam_pass_sroamakes for a field of a local record, so those are fixed too, as is aletwith an effect before an awaited callee or before a callee read from a field that theletassigns.The change
let x = e; f(x)only wheneandfcommute: one of them has no side effects and reads nothing that may change, or neither has side effects.let x = swap(); cb(x)with a parametercbis stillcb(swap()), andlet x = c.n; h.call(x)is stillh.call(c.n).@valor@newexternal, which JavaScript reads from its global or module before the argument. It now folds such a call only whenehas no side effects.refine_lettakes the assigned variables, which its callers compute, andmotionandconflictmove fromLam_beta_reduce_utiltoLam_analysisso that both use them.Output
One checked-in output changes: in
Stdlib_PromiseTest,let s = switch e {...}; resolve(s)keepssinstead of atmp.A call through a record field keeps the binding when the value has side effects, as in
let x = f(); r.call(x), even when the field is immutable. #9223 proposed to treat immutable fields as fixed, but a record with a mutable field can be coerced to a type where the field is immutable, so the field can change.Not changed
An inlined function whose body is the call of an external, as in
let callHook = x => hook(x)andcallHook(install()), still compiles tohook(install()), which readshookbeforeinstall()runs. That is the fast path of beta reduction, notrefine_let, and binding there affects every inlined wrapper around an external, so it is left for a separate change.Tests
let_before_callee_testhas nine tests. Compiled with master, eight fail.inline_arg_order_cross_module_testgains one for a function of another module inlined under-bs-cross-module-opt, which fails with master too.