Hand back the term when an optimization pass changes nothing by cristianoc · Pull Request #8620 · rescript-lang/rescript

cristianoc · 2026-09-04T12:34:50Z

Every rewriting pass carried its own copy of a full Lambda traversal, around
twenty arms apiece of the form

| Lwhile (l1, l2) -> Lambda.while_ (simplif l1) (simplif l2)

that rebuilt a node identically. They now delegate to
Lambda_traverse.shallow_map_sharing, which rebuilds through the same smart
constructors but returns the node untouched when no child changed. What is
left in each pass is the arms that do something. The arms that were kept also
rebuilt unconditionally when their own analysis found nothing, so those hand
back the term too, and simplify_exits decides up front whether the term has
any static exit at all rather than walking it twice to find out.

A note on what this is worth. Measured over the runtime and Belt corpora, the
Lambda optimization pipeline is about 1% of user CPU, so its isolated 8.4%
gain is roughly 0.08% of compile time and the end-to-end build difference is
indistinguishable from zero. The case for this change is that five passes lose
around 230 lines of boilerplate, and that "did this pass change anything"
becomes observable, which #8573 needs for reasoning about pass timing. It is
not a speedup and should not be read as one.

Sharing is invisible to generated output, so no fixture can see it regressing:
ounit_lambda_traverse_tests.ml checks it directly, and the two per-pass
suites cover the hand-written arms. The -debug-ir labels are also corrected
here, having drifted from the passes whose output they name.

Part of #8573. Stacked on #8619, top of the stack.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ac231a6212

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

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".

chatgpt-codex-connector · 2026-09-04T12:41:46Z

+      let rec extract rev_wrap = function
+        | [] -> (rev_wrap, [])
+        | (_, binding) :: _ as bindings
+          when Lam_hit.hit_variables collections binding ->
+          (rev_wrap, bindings)


Preserve tail-first extraction of recursive bindings

When a recursive group starts with a still-recursive binding but a later binding becomes independent after aux (for example, [a = ...b...; b = <now-independent block>]), the previous implementation examined the reversed group and hoisted that independent suffix. This forward walk stops immediately on a, so b remains in the recursive group and can be forced through recursive lowering, including dummy/update paths for blocks. Preserve tail-first extraction while retaining sharing, and add focused OUnit or generated-output coverage for this case.

AGENTS.md reference: AGENTS.md:L181-L186

Useful? React with 👍 / 👎.

I don't think this one holds — both the old and the new implementation extract a prefix, not a suffix.

Three things make the old code a forward walk:

The old iter ended in | [] -> (List.rev groups, set), so groups was handed on in source order, not reversed.

Ext_list.fold_left (compiler/ext/ext_list.ml:756) is | a :: l -> fold_left l (f accu a) f — a forward fold, head first.

The stop flag latched: once any binding hit collections, every later binding took the stop || … branch and stayed in the recursive group.

So the old fold extracted the maximal non-hitting prefix and left everything from the first hit onward recursive, which is precisely what the new extract does.

On your example [a = …b…; b = <now-independent block>]: a refers to b, so a hits collections, extraction stops at a, and b stays in the recursive group — under both implementations alike.

I also checked the two new arms that bypass lambda_of_groups: lambda_of_groups ~rev_bindings:[] result folds over an empty list and is the identity, and Lambda.letrec (compiler/ml/lambda.ml:662) is a plain constructor with no empty-list normalization. So Lambda.letrec groups body' matches the old path exactly, including the degenerate empty-bindings case.

Happy to reconsider if you're reading fold_left differently.

codecov · 2026-09-04T13:00:30Z

Codecov Report

❌ Patch coverage is 96.03960% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.38%. Comparing base (326f17c) to head (05a5a6e).

Files with missing lines	Patch %	Lines
compiler/core/lam_pass_deep_flatten.ml	82.14%	5 Missing ⚠️
compiler/core/lam_pass_remove_alias.ml	92.30%	2 Missing ⚠️
compiler/core/lam_pass_exits.ml	91.66%	1 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##           lambda/as-in-the-ast    #8620      +/-   ##
========================================================
- Coverage                 77.41%   77.38%   -0.03%     
========================================================
  Files                       471      474       +3     
  Lines                     63549    63488      -61     
========================================================
- Hits                      49194    49132      -62     
- Misses                    14355    14356       +1

Files with missing lines	Coverage Δ
compiler/core/lam_compile_main.ml	`89.36% <100.00%> (+0.09%)`	⬆️
compiler/core/lam_exit_count.ml	`100.00% <100.00%> (ø)`
compiler/core/lam_pass_collapse_var_aliases.ml	`100.00% <100.00%> (+2.27%)`	⬆️
compiler/core/lam_pass_count.ml	`93.97% <100.00%> (ø)`
compiler/core/lam_pass_lets_dce.ml	`85.07% <100.00%> (-3.93%)`	⬇️
compiler/core/lam_util.ml	`90.62% <100.00%> (+0.62%)`	⬆️
tests/ounit_tests/ounit_deep_flatten_tests.ml	`100.00% <100.00%> (ø)`
tests/ounit_tests/ounit_exits_tests.ml	`100.00% <100.00%> (ø)`
tests/ounit_tests/ounit_lambda_traverse_tests.ml	`100.00% <100.00%> (ø)`
tests/ounit_tests/ounit_tests_main.ml	`100.00% <ø> (ø)`
... and 3 more

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pkg-pr-new · 2026-09-04T14:07:39Z

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@8620

@rescript/belt

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/belt@8620

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@8620

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@8620

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@8620

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@8620

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@8620

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@8620

commit: 05a5a6e

Every rewriting pass carried its own copy of a full Lambda traversal, around twenty arms per pass of the form | Lwhile (l1, l2) -> Lambda.while_ (simplif l1) (simplif l2) that rebuilt a node identically. They now delegate to Lambda_traverse.shallow_map_sharing, which rebuilds through the same smart constructors but returns the node untouched when no child changed. What is left in each pass is the arms that do something. The arms that were kept also rebuilt unconditionally when their own analysis found nothing, so simplify_alias now falls through to the sharing traversal in those cases rather than reassembling an identical node, and its string-switch arm guards on the scrutinee being a known constant instead of rewriting either way. Measured over a 149 module stdlib build, 1937 pass invocations: total allocation falls from 2570634 words to 1093262, and the number of runs that hand back their input unchanged rises from 256 to 1130. Generated JavaScript is unchanged throughout. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H

simplify_exits counted exits, then walked the term a second time to rewrite them, whether or not there were any. Over a stdlib build 397 of its 447 runs have nothing to rewrite, so the counter now creates its table on the first static exit and reports None when it never does, and the pass returns its input without the second walk. An empty table is not the same as no work: a Lstaticcatch that nothing raises to is dropped by the pass, so a catch marks the term as having exits even though it adds no count. The occurrence and substitution tables in simplify_lets were sized 83 and 32 while holding far less. A no-op run of simplify_exits now costs 13 words rather than 91, and of simplify_lets 114 rather than 258. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H

deep_flatten took every let apart into groups and reassembled it, so a chain that needed no regrouping came back as a fresh copy of itself. flatten only restructures a binding when it hoists something out of the right hand side, splits a null conversion, or eliminates a tuple; every other binding is emitted as the same binding, so aux now rebuilds those in place. The reassembly runs through Lam_util.refine_let, which is not a constructor: it promotes Strict to Alias when the right hand side is safe to duplicate, downgrades to StrictOpt, and inlines a binding whose body immediately consumes it. Skipping it would drop those rewrites, so the fast path still calls it, and refine_let takes the binding it is rebuilding and returns it untouched when nothing is refined. A beta residue is itself a let chain that flatten deliberately leaves alone, so it has to be recognized before classifying by shape. Over a stdlib build the runs that hand back their input rise from 108 to 253 of 447, rebuilds that produce an identical tree fall from 217 to 72, and the words spent on them from 181858 to 54902. The number of runs that actually change the tree is 122 either way, so no rewrite is lost. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H

Three arms rebuilt their node whether or not anything under it changed: the jsx-preserve primitive mapped its remaining arguments eagerly, the cross-module application reassembled itself when the callee turned out not to be inlinable, and normal () in the applied-variable arm built a fresh list of arguments before deciding it had nothing to inline. The cross-module arm rebuilt with ?ap_transformed_jsx:None, dropping the flag the original application carried. Sharing keeps it, which changes no generated output anywhere in the suite. simplify_alias now never rebuilds a tree it did not change: over a stdlib build, 398 of its 447 runs hand back their input, none of the remaining 49 produce an identical tree, and 49 is what it changed before this too. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H

The three arms that fall through to rebuilding a binding now share: the two that go through refine_let pass it the binding they are rebuilding, and the alias arm compares its parts before building a new node. That last one rebuilt with Lambda.let_ Alias v (simplif l1) (simplif l2) and simplif records substitutions as it walks, so the right to left evaluation of arguments means the body was simplified before the bound expression. Naming the results in reading order would have reversed that, so the order is now written out. Over a stdlib build, runs that hand back their input rise from 36 to 75 of 149 and rebuilds producing an identical tree fall from 41 to 2, with 72 runs changing the tree either way. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H

The Lletrec arm of deep_flatten rebuilt unconditionally: it mapped every binding into a fresh list, split that with a fold carrying a stop flag, and reassembled through lambda_of_groups, whether or not a binding could be lifted out of the group. It now maps with sharing, replaces the fold with a walk that stops at the first binding referring back into the group, and returns the original when nothing was extracted and nothing underneath changed. That arm was where nearly all the remaining waste was. Over a stdlib build, runs of deep_flatten that hand back their input rise from 253 to 324 of 447 and rebuilds producing an identical tree fall from 72 to 1, with 122 runs changing the tree either way. The unit test covers the sharing rather than the extraction. Removing the sharing changes no generated output, so no snapshot can catch it, while breaking the extraction moves output the existing suite already compares. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H

count_helper created its table on the first static exit and reported None when it never did, which meant threading a lazy accessor through the whole counting walk. A four line predicate answers the same question before counting starts, so the counter goes back to the shape it had and the pass returns its input untouched when there is nothing to rewrite. That predicate names the two nodes Lam_pass_exits rewrites, so a case added there that rewrites anything else has to be added here too or the pass silently stops firing. It says so. subst_helper also hands its term back when a retained catch or an unresolved raise comes through unchanged. The three tests cover what nothing else can. Removing either sharing site, or the removal of a catch nothing raises to, leaves the generated JavaScript byte for byte identical, and each mutation fails exactly one of them: a dead catch is dropped by code generation anyway, so its removal here is invisible to every output fixture we have. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H

Reviewing the sharing series on clarity rather than allocation found three places where chasing the property made the code worse, all of them mine. simplify_alias's string switch had become a when-guard containing a match, which ran the same lookup twice and left an arm the guard makes unreachable. It now finds the constant once and branches on that. regroups_binding mirrors flatten's cases one for one, including why a null conversion of a variable is left alone while any other one is split. Nothing said the two have to stay in step, or that drifting costs the flattening silently, because the binding then takes the fast path and never reaches flatten at all. Two passes bound `as original` for a value already in scope as `lam`, giving one idiom two spellings across seven passes. The traversal every pass delegates to had no test. Breaking the sharing in its Lapply and Lswitch arms leaves every fixture in the repository byte for byte identical and no test failing, while all seven passes quietly lose the property. The new test checks each constructor twice: that an identity map hands the node back, and that a replacing map does not, since a node whose children were never visited would pass the first by doing nothing. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H

The -debug-ir labels had drifted from the sequence they describe. "initial" was dumped after collapse_var_aliases rather than before it, "flatten1" and "before-simplify-exits" each dumped a term already dumped under another name, "simplify_alias_before" named the pass that came next rather than the one that had run, and the output of guard_raises was labelled simplify_lets. Every dump is now named after the pass whose output it holds, and the three rounds of deep_flatten, simplify_alias and simplify_exits are numbered so a dump can be placed in the sequence. The initial dump now happens before collapse_var_aliases, so it is the term the pipeline was handed. Removed the commented-out scc pass with its dump label, and the commented-out collect_info and simplify_alias that followed sroa. The area guide linked to lam_convert.ml, which no longer exists, and said six constructors normalize as they build. There are seven: apply, prim, switch, stringswitch, if_, seq and not_. It now also carries the pass sequence as a table, with which statistics each pass consumes: only simplify_alias reads them, and a fresh collect_info runs immediately before each of its three rounds. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H

The stacked-PR guidance did not say whether you hand `gh stack link` branches or PR numbers, so it read as though the PRs had to exist first. Either works, and branches alone are enough: the command pushes them and opens the PRs it does not find. Say what linking does to the bases too. It moves each one onto the branch below, which looks like a misconfigured PR if you are not expecting it, and CI keeps running throughout. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H

cknitt · 2026-09-05T05:04:25Z

@codex review

chatgpt-codex-connector · 2026-09-05T05:04:41Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-05T05:08:27.750015Z	`05a5a6e`	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector · 2026-09-05T05:08:26Z

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: 05a5a6e656

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

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".

chatgpt-codex-connector Bot reviewed Sep 4, 2026

View reviewed changes

cristianoc force-pushed the lambda/passes-share-terms branch from ac231a6 to c80c1a9 Compare September 4, 2026 12:47

cristianoc changed the base branch from lambda/as-in-the-ast to master September 4, 2026 12:48

cristianoc force-pushed the lambda/passes-share-terms branch from c80c1a9 to 0240fbf Compare September 4, 2026 13:21

cristianoc changed the base branch from master to lambda/as-in-the-ast September 4, 2026 13:28

cristianoc force-pushed the lambda/passes-share-terms branch from 8869a30 to 8a3cd41 Compare September 4, 2026 13:45

cristianoc requested a review from cknitt September 4, 2026 13:50

cristianoc mentioned this pull request Sep 4, 2026

Stamp parsetree v1: what remains before the AST can be frozen for PPXs #8624

Open

cristianoc force-pushed the lambda/passes-share-terms branch from 8a3cd41 to 25bca19 Compare September 5, 2026 00:18

cristianoc and others added 10 commits September 5, 2026 02:48

cristianoc force-pushed the lambda/passes-share-terms branch from 25bca19 to 05a5a6e Compare September 5, 2026 00:50

Navigation Menu

Uh oh!

Hand back the term when an optimization pass changes nothing - #8620

Hand back the term when an optimization pass changes nothing#8620
cristianoc wants to merge 10 commits into
lambda/as-in-the-astfrom
lambda/passes-share-terms

cristianoc commented Sep 4, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Sep 4, 2026

Uh oh!

cristianoc Sep 4, 2026

Uh oh!

codecov Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

pkg-pr-new Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

cknitt commented Sep 5, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 5, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot commented Sep 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

cristianoc commented Sep 4, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

cristianoc Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pkg-pr-new Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cknitt commented Sep 5, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot commented Sep 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Sep 4, 2026 •

edited

Loading

pkg-pr-new Bot commented Sep 4, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Sep 5, 2026 •

edited

Loading