fix(transform): do not beta-reduce a local arrow whose param is captured by a nested closure - #10653
proggeramlug wants to merge 3 commits into
Conversation
…red by a nested closure closure_local_inline's beta-reduction clones the arrow's return expression fresh per call site and substitutes each parameter with that call's argument via substitute_locals. For a parameter read inside a NESTED closure (e.g. (f, isOpt) => arr.forEach(([k,v]) => check(k, v, isOpt))), substitute_locals bakes a non-LocalGet argument straight into that nested closure's body and drops it from the closure's captures list, but never mints a fresh func_id for the rewritten closure literal. Codegen compiles exactly one body per func_id (whichever Expr::Closure occurrence its module-wide scan sees first), so every call site's clone of the nested closure keeps sharing the SAME func_id -- with more than one call site, only the first-seen clone's baked-in argument is ever compiled, and every other call silently runs it too. Bail out of the beta-reduction when any parameter is captured by a nested closure, leaving such an arrow as a real, per-call closure -- each invocation then creates its own closure instance whose nested callback correctly captures that call's argument by reference.
…ultiple call sites Covers the #10567 repro (nested destructuring forEach callback), an arrow with several params where the captured one is neither first nor last, nested arrows (outer -> middle -> inner, two closure boundaries away), an arrow declared inside a class method, a by-write capture control (a multi-statement arrow body, never a closure_local_inline candidate), and the plain-function controls from the original issue.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughChangesArrow closure capture
Priority: ⬆️ High Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The fix preserves per-invocation closure captures for the reported repeated-call cases, with no remaining concrete merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Landed in merge train 222 (#10732), released as v0.5.1601 — main is now Closing rather than merging is how trains work here: the eight PRs were cherry-picked onto one tree, validated together, and landed under the train's own commit, so GitHub cannot mark this one merged even though your change is on main. Close-keywords in a source PR body never fire under this scheme, so the issues this train resolved were closed from the train's body instead. The tree passed: all nine cheap gates, |
Summary
A closure created inside a local arrow function, capturing that arrow's own
parameter, kept seeing the FIRST call's argument on every later call to the
same arrow — even though the arrow itself was re-invoked with a different
value each time. Only the nested-closure-into-array-callback shape was
affected; a directly-invoked inner closure and a plain function declaration
already worked correctly.
Root cause (file:line)
crates/perry-transform/src/closure_local_inline.rs— therun/process_stmts/arrow_candidate/rewrite_callspipeline — beta-reducesa
let f = (a, b) => <single return expr>local whose every use is a directcall: it clones the return expression fresh per call site and hands the
clone to
substitute_locals(crates/perry-transform/src/inline/substitute.rs)with a param→argument map for that call.
substitute_locals'sExpr::Closurearm (lines 72–105) substitutes intothe body and remaps
captures/mutable_captures: when a captured id maps tosomething other than a
LocalGet— i.e. the argument is a literal — thecapture is dropped from the list ("the closure body no longer references
this id"), and the literal is baked straight into the closure's body instead.
That is correct for that one clone — but the closure keeps its original
func_id; nothing in this path mints a fresh one for the rewritten literal.Codegen compiles exactly one body per
func_id— whicheverExpr::Closureoccurrence its module-wide scan encounters first (see the analogous note in
lower_decl/class_decl.rs:238: "the generator transform... only visitsmodule.functions... codegen compiles ONE function body perfunc_id"). Withmore than one call site of the same local arrow, every clone of the nested
closure shares that one
func_id, so only the first-seen clone's baked-inliteral is ever compiled — every other call silently runs it too.
Confirmed with
--trace hir: forthe post-transform HIR shows the enclosing function's body containing
two
ArrayForEachstatements, bothcallback: Closure { func_id: 2, ... }— one with
Bool(false)baked into itsReturn, the other withBool(true)— sharing the identical
func_id. At runtime, both calls toiterprintedfalse.try_inline_simple_call/try_inline_call(the FuncRef-keyed inliner incrates/perry-transform/src/inline/call_inliner.rs) already has a guard forthis exact class of bug (issue #858:
collect_closure_captured_local_ids+ forcing such a parameter to bematerialized as a fresh
Letinstead of substituted as a literal) — butclosure_local_inline.rsis a separate pass (it beta-reduces a plainlocal closure variable, which is never a
func_candidates/FuncReftarget)and never applied that guard.
PR #10615's
DeclCensusrewrite incrates/perry-hir/src/lower/shared_mutable_capture.rs(cited in my brief as a possible sibling fix) is unrelated: that machinery
only fires on
Expr::RegisterClassCaptures— i.e. a class LIFTED out of anenclosing function — and this repro has no class at all. I confirmed by
inspection and by reproducing the bug that
#10615's branch does not touchclosure_local_inline.rsand would not fix this issue; I am not closing#10567 as a duplicate.
Fix
arrow_candidatenow rejects a candidate when any of its own parameters isread inside a closure nested in its body (reusing the existing
collect_closure_captured_local_idshelper fromcrates/perry-transform/src/inline/closure_analysis.rs,the same helper #858's fix uses). Such an arrow is left as a real, per-call
closure: each invocation allocates its own closure instance whose nested
callback correctly captures that call's argument by reference — the same
path that already worked for the issue's
outer/innercontrol.Tests added
test-files/test_gap_10567_arrow_param_closure_capture.ts: the issue's ownrepro, plus the requested variants —
nor last;
a
closure_local_inlinecandidate at all — this documents that theexisting, unaffected path keeps working);
outer/inner,twice).Validated byte-for-byte against
node --experimental-strip-types(Node26.5.1).
Proof it fails on the baseline: checked out
closure_local_inline.rsfrom this branch's parent commit (
68a5454396,mainbefore this fix) withthe new test file added, rebuilt, and ran it — the repro, several-params,
nested-arrows, and arrow-in-method sections all print the FIRST call's
argument on every call:
On this branch: every line matches Node byte-for-byte (
diffagainst Node'soutput is empty).
Validation
cargo test --release -p perry-transform --tests: 152 passed, 0failed.
Gap suite (filtered): the new test plus every existing closure/inline/forEach
gap test that could plausibly be touched by this change, including the
original Closure-captured numeric params read as 0 inside object-literal
: Datemethod (@perryts/mysql MyDateTime.toDate shape) #858 regression test:test_gap_10567_arrow_param_closure_capture,test_issue_858_closure_numeric_capture,test_parity_inline_closure_capturing_local,test_gap_9090_closure_literal_identity,test_gap_finally_inline_nested_closure,test_gap_closures,test_closure_complex,test_edge_closures,test_returning_closures,test_obj_closure_call,test_gap_collection_foreach_member_receiver_thisarg,test_gap_foreach_live_index_read_no_skip,test_gap_set_map_foreach_fused_receiver,test_issue_5432_fetch_headers_foreach— allPASS, no regressions. (test_issue_610_foreachfails to compile on thishost for an unrelated, pre-existing reason: it uses
perry/ui, and thisLinux box has no
libperry_ui_gtk4.abuilt — confirmed independent of thischange.)
python3 scripts/check_test_registration.py: OK (338 files checked).cargo fmt --all -- --check: clean.Lint (
SKIP_COMPILE_GATES=1 ./scripts/run_lint_gates.sh): 76/77 gatespassed; pre-existing red:
Public benchmark evidence freshness(
benchmarks/ci_public_baseline_check.py), red on every PR in this repo,untouched by this change.
Performance (
perf stat -e instructions,task-clock, 3 runs each, thishost):
LocalGetargument (safe shape — never hits the bug;validate(isOpt)callingiter(isOpt)once per invocation, 3M invocations)iter(false); iter(true);)0)9000000)The single-call-site/
LocalGet-argument shape is unaffected (within noise)—
substitute_localsalready preserves aLocalGet-mapped capture insteadof stripping it, so my guard's rejection is conservative there but costs
nothing measurable on this benchmark. The exact bug shape regresses ~7.3%
instructions: this is the correctness cost of no longer sharing a single,
wrongly-baked closure body across call sites with different arguments —
the baseline number is not a valid "before" to preserve, since it is
measuring a program that computes the wrong answer (
0instead of9000000). Node wall time on the same workload: 163ms (JIT-optimizedsmall-loop specialization AOT native codegen does not attempt); consistent
with prior PRs' notes on this class of micro-loop.
What I did not verify
shared, no working
PERRY_NO_AUTO_OPTIMIZE-free auto-optimize path inreasonable time) is documented to stall under auto-optimize mode; relying
on CI's sharded gap suite per the standard process, per the owner's
standing "ignore CI, PR open is the stop condition" instruction.
@noble/curves2.2.0 end-to-end repro named in the issue — verifiedthe issue's own minimal repro and the requested variants directly, not
the full package build.
@noble/curves1.2.0ReferenceError: Cannot access 'wnaf' before initializationnoted in theissue is explicitly called out there as possibly a different defect; not
investigated here.
New bug noticed (not fixed here)
While tracing
substitute_locals'sExpr::Closurehandling(
crates/perry-transform/src/inline/substitute.rs), I noticed it is theonly caller-side mechanism that strips a capture on literal substitution
without mangling
func_id— any other future caller ofsubstitute_localsthat clones a body containing a nested closure and doesn't independently
apply the #858-style guard would reproduce this exact class of bug. Worth a
follow-up: either thread a fresh-
func_idallocator throughsubstitute_localsitself, or add adebug_assert!/lint that every callerof
collect_closure_captured_local_ids-adjacent inlining paths applies thesame guard. Filing as future work, not attempting the broader refactor here
per the ~800-line-diff guidance.
Fixes #10567
Summary by CodeRabbit
Bug Fixes
Tests