Split Lambda into the IR and its traversals - #8618
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57bf99c06f
ℹ️ 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".
|
|
||
| - Normalize Lambda terms where they are built: a match guard stays structured data until its fallthrough is known, and `apply` and `mk_builtin` go through the folding constructors. https://github.com/rescript-lang/rescript/pull/8615 | ||
| - Replace non-escaping local mutable blocks with scalar bindings when all uses are direct field accesses, generalizing reference unboxing to multi-field records and references captured by JavaScript closures. https://github.com/rescript-lang/rescript/pull/8617 | ||
| - Split `lambda.ml` into the IR and its traversals, static exits and path translation, so the module defining `Lambda.t` no longer reaches into `Env`, `Path` or `Parsetree`. https://github.com/rescript-lang/rescript/pull/8618 |
There was a problem hiding this comment.
The advertised dependency split is incomplete: compiler/ml/lambda.mli:55 still exposes find_name : Parsetree.attribute -> ..., and lambda.ml:66-67 still consumes Parsetree.attribute through Ast_payload. Consequently, Lambda continues to reach into Parsetree, contrary to this new changelog entry and the intended layering boundary; move the attribute-decoding/field-metadata helpers out of Lambda, or revise the claim if retaining that dependency is intentional.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right, and I've taken the second of the two options you offer: the changelog now claims only what this PR delivers — Lambda no longer reaches into Env or Path.
The Parsetree dependency is genuinely still here at this commit, exactly as you describe. It disappears one PR up, in #8619, where @as stops being an attribute and find_name goes away with it. Narrowing the claim rather than moving the helper keeps that removal in the PR that actually earns it.
Fixed in b415a7e.
57bf99c to
e1e5ae0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## lambda/scalar-replacement #8618 +/- ##
==========================================================
Coverage 77.35% 77.35%
==========================================================
Files 468 471 +3
Lines 63464 63476 +12
==========================================================
+ Hits 49090 49102 +12
Misses 14374 14374
🚀 New features to boost your workflow:
|
e1e5ae0 to
b415a7e
Compare
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
b415a7e to
2e7adbf
Compare
compile_orhandlers asked whether the term it had built was a jump to the exit it was about to wrap a catch around, and asked by calling make_key - which builds a canonical form for comparison - then matching the result and binding the raise's arguments from it. So a fragment of a key reached generated code, and keys are the one place in Lambda built without the constructors: make_key rebuilds raw, so a term it produced was not normalized. The substitution was load-bearing rather than incidental. The term may be [let x = e in exit j x], and it is discarded once recognized, so taking the arguments as written would leave x unbound. make_key happened to substitute alias bindings while canonicalizing, which is what made the arguments safe. as_exit_call does that deliberately: peel alias bindings, substitute them into the arguments, and report the exit. raw_action then has no callers - it was the only consumer of make_key whose result was emitted rather than compared. Generated JavaScript is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
make_key was the last thing in lambda.ml constructing terms raw. It could, owning the type, and the argument for letting it was that folding would change the key: it substitutes alias bindings as it canonicalizes, substitution can put a constant where a variable was, and the constructors would then fold what the raw rebuild left alone. That is true of the mechanism and false of the outcome. Routing every construction through the constructors leaves generated JavaScript unchanged across the runtime, Belt and the 626 test modules, so the keys still make the same sharing decisions. The argument was worth checking rather than asserting. It is safe to fold here now for a second reason: since the preceding commit, no key reaches generated code. They are compared - in same_actions, up_ok_action and the action store - and nothing else. What remains of L-constructors in make_key is pattern matching. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
lambda.ml defined the representation and then a good deal that merely used it. The distinction that matters is whether a function needs to construct terms without going through the constructors: the constructors and the folds they run do, and nothing else does. Everything in the second category can live outside, and saying so in the module structure makes the rule checkable rather than a convention. Lambda_traverse takes the generic walks - shallow_exists, shallow_map_sharing, iter, free_variables, subst_lambda - and make_key, which the preceding commit made expressible through the constructors. Lambda_exits takes the static exit mechanism: the counters, make_exit, as_simple_exit, make_catch_delayed. An exit number and the catch a handler is wrapped in are a protocol over the representation, not part of it. Transl_path takes transl_normal_path and its three callers. This is translation, and it was the only reason lambda.ml depended on Env and Path. Moving the last two out made the type private to them, which found two places building raw because they could rather than because they had to: make_exit's Lstaticraise and transl_normal_path's Lconst, Lglobal_module, Lvar and Lprim. Both now go through the constructors, which for these is the same term. Five helpers that only the folds use - eq_primitive_approx, eq_comparison, const_eq_approx, cmp_int32, cmp_float - stop being exported. lambda.ml goes from 1495 lines to 1200. Generated JavaScript is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
2e7adbf to
ffad101
Compare
|
@codex review |
Codex Review SummaryThis 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffad101c51
ℹ️ 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".
| if env == Ident.empty then args | ||
| else Ext_list.map args (Lambda_traverse.subst_lambda env) ) | ||
| | Llet (Alias, x, ex, body) -> | ||
| go (Ident.add x (Lambda_traverse.subst_lambda env ex) env) body |
There was a problem hiding this comment.
This new path drops alias bindings and substitutes their expressions into static-exit arguments, but the only test-file changes in this commit rename existing traversal calls; there is no focused coverage for chained aliases, multiple exit arguments, or the resulting generated JavaScript. A regression here can leave exit arguments unbound or change their evaluation, so add Lambda-level and end-to-end fixtures for these cases as required by the repository’s testing guidance.
AGENTS.md reference: AGENTS.md:L41-L43
Useful? React with 👍 / 👎.
2 participants
lambda.mlheld the IR and everything that walked it, so the module definingLambda.treached intoEnv,PathandParsetree. Traversals, static exithandling and path translation move to
lambda_traverse.ml,lambda_exits.mland
transl_path.ml, leaving the IR module with types and constructors.make_keywas the remaining way to build a term without going through aconstructor. The escape route it existed for, recognising a jump to an exit,
is now handled directly, and
make_keyitself builds through the constructorsbefore moving out with the other traversals.
Part of #8573. Stacked on #8617.