fix(deforest): rewrite producer call sites in class member bodies - #5772
Conversation
Deforestation promotes an array-producer function (`function f(){ const
out = []; ...out.push(x)...; return out; }`) to take the accumulator as a
synthetic trailing `__deforest_out` parameter, then rewrites every call
site to allocate the array and pass it in.
`detect_producers` scans free functions, module-init, AND class
member bodies (methods + constructors) when deciding whether a producer
is safe to rewrite — a plain `let v = f()` inside a method is a
"supported" call site, so the producer is admitted. But phase 3 only
rewrote call sites in module-init and free functions. A producer whose
call site lived in a class method therefore had its signature rewritten
(gaining the out-param) while the method's call kept its original arity.
Codegen then passes `undefined` for the missing argument, so the body
runs `out.push(...)` / `return out` on a non-array — a SIGSEGV, or a
downstream `TypeError: is not iterable` when the returned value is later
spread / for-of'd.
This is the same arity-mismatch class as the in-closure bail-out
(PerryTS#5136); there the fix was to drop the producer, but method bodies are
ordinary statement lists we can rewrite rather than bail on.
Fix: make detection, fresh-id seeding, and the phase-3 rewrite all cover
the identical complete set of code bodies — module-init, free functions,
constructors, methods, getters, setters, and static methods:
- `deforest/mod.rs` (phase 3): rewrite call sites in every class member
body, not just module-init + free functions.
- `deforest/detect.rs`: extend the three safety scans (funcref misuse,
unsafe call sites, in-closure usage) to getters/setters/static methods
so admission and rewrite stay symmetric.
- `deforest/walk.rs` (`max_local_id`): include getter/setter/static-
method locals so the synthetic-id seed can't collide with a local
already living in those bodies.
Adds a regression test: a producer called via `let v = helper()` inside
a class method is deforested AND its call site is rewritten to pass the
accumulator (arity matches the rewritten producer).
📝 WalkthroughWalkthroughProducer deforestation is extended to cover class ChangesDeforest class member coverage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/perry-transform/src/deforest/mod.rs (1)
177-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCentralize class-member traversal before it drifts again.
This member list now lives in
detect.rs,walk.rs, andrun(), which is the exact contract that got out of sync here. A shared helper over class-memberFunctionbodies would make future member-kind additions much harder to miss.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-transform/src/deforest/mod.rs` around lines 177 - 230, The class-member traversal logic is duplicated across multiple places and is already drifting out of sync; centralize it into a shared helper that walks all class-member Function bodies. Refactor the member-body rewriting in the deforest pipeline (the logic around module.classes, class.constructor, class.methods, class.getters, class.setters, and class.static_methods) to call that helper so future member-kind additions only need one update.crates/perry-transform/src/deforest/tests.rs (1)
282-398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an accessor/static-member regression case.
This only exercises
class.methods, so the new getter/setter/static-method loops indetect_producers,max_local_id, and phase 3 can still regress without any test failing. Parameterizing the fixture by member kind would cover the new surface with little extra code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-transform/src/deforest/tests.rs` around lines 282 - 398, Add a regression test that covers class accessor/static member bodies, not just class.methods, because the new getter/setter/static-method handling in detect_producers, max_local_id, and phase 3 can still miss call-site rewrites. Reuse the existing deforestation fixture pattern in deforest/tests.rs by parameterizing the member kind and asserting the producer still gains the synthetic accumulator param and every FuncRef(1) call inside the member body is rewritten to arity 1. Keep the same style as deforests_producer_called_from_class_method so the new test exercises the same run() pipeline across getter, setter, and static method cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry-transform/src/deforest/mod.rs`:
- Around line 177-230: The class-member traversal logic is duplicated across
multiple places and is already drifting out of sync; centralize it into a shared
helper that walks all class-member Function bodies. Refactor the member-body
rewriting in the deforest pipeline (the logic around module.classes,
class.constructor, class.methods, class.getters, class.setters, and
class.static_methods) to call that helper so future member-kind additions only
need one update.
In `@crates/perry-transform/src/deforest/tests.rs`:
- Around line 282-398: Add a regression test that covers class accessor/static
member bodies, not just class.methods, because the new
getter/setter/static-method handling in detect_producers, max_local_id, and
phase 3 can still miss call-site rewrites. Reuse the existing deforestation
fixture pattern in deforest/tests.rs by parameterizing the member kind and
asserting the producer still gains the synthetic accumulator param and every
FuncRef(1) call inside the member body is rewritten to arity 1. Keep the same
style as deforests_producer_called_from_class_method so the new test exercises
the same run() pipeline across getter, setter, and static method cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9bb37e94-34dd-4068-9992-8e044f4b2b36
📒 Files selected for processing (4)
crates/perry-transform/src/deforest/detect.rscrates/perry-transform/src/deforest/mod.rscrates/perry-transform/src/deforest/tests.rscrates/perry-transform/src/deforest/walk.rs
… rewrite (#5780 cluster A) (#5788) Class methods / constructors / accessors that reference `super` now act as bail-out bodies in the deforest pass. Commit 5d47364 (#5772) extended the DEFOREST phase-3 call-site rewriter to class member bodies to fix the arity-mismatch SIGSEGV (#5136-class). A member body that also uses `super.x`, `super[e]`, or `super(…)` had its [[HomeObject]] setup corrupted by the synthetic-local introduction, causing a `TypeError: Cannot convert undefined or null to object` at runtime (~24 test262 cases, cluster A of #5780). Fix (detect.rs): `body_has_super` walks a member-body statement list for any `Expr::SuperPropertyGet / SuperCall / SuperMethodCall / ObjectSuper*` variant. In the third (unsafe-call-site) detection pass any producer called from a super-using body is added to the `unsupported_call` exclusion set, so the producer is dropped from the candidate map before the rewrite phase begins. Belt-and-suspenders (mod.rs): the phase-3 rewriter skips any class member body for which `body_has_super` returns true, ensuring the [[HomeObject]] frame is never disturbed even if the detect exclusion were somehow bypassed. Super-free class methods continue to be deforested as before (#5772 fix preserved); only bodies that actually use `super` take the bail path — a missed optimisation, not a correctness loss. Tests: two new unit tests in deforest/tests.rs — `rejects_deforest_when_class_method_uses_super` (the regression) and `still_deforests_when_method_has_no_super` (control / #5772 guard). Co-authored-by: Claude <noreply@anthropic.com>
Problem
Deforestation promotes an array-producer function
to take the accumulator as a synthetic trailing
__deforest_outparameter, then rewrites every call site to allocate the array and pass it in.detect_producersscans free functions, module-init, and class member bodies (methods + constructors) when deciding whether a producer is safe to rewrite — a plainlet v = f()inside a method counts as a supported call site, so the producer is admitted. But phase 3 only rewrote call sites in module-init and free functions.So a producer whose call site lives in a class method had its signature rewritten (gaining the out-param) while the method's call kept its original arity. Codegen then passes
undefinedfor the missing argument, and the body runsout.push(...)/return outon a non-array:When the (now
undefined) result is later spread /for…of'd, it surfaces instead asTypeError: is not iterable.This is the same arity-mismatch class as the in-closure bail-out (#5136). There the fix was to drop the producer; method/ctor/accessor bodies are ordinary statement lists, so we can rewrite them rather than bail.
Fix
Make detection, fresh-id seeding, and the phase-3 rewrite cover the identical complete set of code bodies — module-init, free functions, constructors, methods, getters, setters, and static methods:
deforest/mod.rs(phase 3): rewrite producer call sites in every class member body, not just module-init + free functions.deforest/detect.rs: extend the three safety scans (funcref misuse, unsafe call sites, in-closure usage) to getters/setters/static methods so admission and rewrite stay symmetric (previously they covered only methods + constructors).deforest/walk.rs(max_local_id): include getter/setter/static-method locals so the synthetic-id seed can't collide with a local already living in those bodies.Test
Adds
deforests_producer_called_from_class_method: a producer called vialet v = helper()inside a class method is deforested and its call site is rewritten to pass the accumulator (the surviving call's arity matches the rewritten producer). Fullperry-transformsuite green (40 passed).Summary by CodeRabbit
Bug Fixes
Tests