Skip to content

fix(deforest): exclude super-using class member bodies from call-site rewrite (#5780 cluster A) - #5788

Merged
proggeramlug merged 1 commit into
mainfrom
fix/deforest-super-prop-5780
Jun 29, 2026
Merged

fix(deforest): exclude super-using class member bodies from call-site rewrite (#5780 cluster A)#5788
proggeramlug merged 1 commit into
mainfrom
fix/deforest-super-prop-5780

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Root cause

Commit 5d47364 (#5772) extended the DEFOREST pass to cover class member bodies (constructors, methods, getters, setters, static methods) as valid call-site rewrite targets — fixing the arity-mismatch SIGSEGV where a producer called from a class method had its signature rewritten but the call site was never updated (#5136-class).

A member body that also uses super (super.x, super[e], super(…)) had its [[HomeObject]] setup corrupted by the synthetic-local introduction done by the call-site rewriter, causing:

TypeError: Cannot convert undefined or null to object

at runtime for ~24 test262 cases (cluster A of #5780).

Fix

detect.rsbody_has_super(stmts) walks a member body for any super expression variant (SuperPropertyGet, SuperCall, SuperMethodCall, SuperMethodCallSpread, SuperCallSpread, ObjectSuper*). In the third detection pass (unsafe-call-site scan) any class member body for which body_has_super returns true has all its producer calls flagged as unsafe, removing those producers from the candidate set before the rewrite phase. A producer whose only callers are super-using bodies is therefore excluded entirely; one with callers in both super-using and super-free bodies is also excluded (conservative but correct).

mod.rs — belt-and-suspenders guard: phase-3 skips rewrite_call_sites_in_stmts for any class member body where body_has_super is true, ensuring [[HomeObject]] is never disturbed even if the detect exclusion were somehow bypassed.

Before / after

Scenario Before After
class C extends B { m() { super.x; } } near a deforestable helper() super.x throws TypeError works correctly
class C { m() { const v = helper(); return v.length; } } (no super) deforested, correct deforested, correct (no regression)

Tests

Two new unit tests in crates/perry-transform/src/deforest/tests.rs:

  • rejects_deforest_when_class_method_uses_super — producer called from a super-using method is excluded from deforestation; signature unchanged after run().
  • still_deforests_when_method_has_no_super — control: same producer called from a super-free method is still deforested (fix(deforest): rewrite producer call sites in class member bodies #5772 guard).

All 11 deforest unit tests pass; cargo fmt --all -- --check and bash scripts/check_file_size.sh clean.

Closes #5780 (cluster A — super.prop / super[expr] / super() in class methods adjacent to deforestable helpers).


Generated by Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved deforestation safety for class members that use super, preventing producer calls in those bodies from being rewritten.
    • Producer candidates are now excluded when they appear in methods, getters, setters, constructors, or static methods that reference super.
    • Class member bodies without super continue to be optimized as before.

… rewrite (#5780 cluster A)

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).
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds body_has_super and flag_producer_calls_in_super_body helpers to detect super references in class member bodies. Wires these into detect_producers (third pass) and the run rewriting loop so that producer candidates and call-site rewrites are suppressed when the enclosing class member body uses super. Two regression tests cover both the rejected and accepted cases.

Deforestation super-body guard

Layer / File(s) Summary
super detection and producer flagging helpers
crates/perry-transform/src/deforest/detect.rs
body_has_super recursively walks stmts/exprs to detect super variants; flag_producer_calls_in_super_body traverses the same body and marks any producer appearing as a callee as unsafe.
detect_producers third-pass guard
crates/perry-transform/src/deforest/detect.rs
Third pass now calls body_has_super for each class member (constructor, method, getter, setter, static method); routes to flag_producer_calls_in_super_body instead of scan_unsafe_call_sites when super is present.
Call-site rewriting guards in run
crates/perry-transform/src/deforest/mod.rs
Constructor and method/getter/setter/static-method rewriting in run is now wrapped in !body_has_super(...) guards; detect re-exports are reformatted (no functional change).
Regression tests
crates/perry-transform/src/deforest/tests.rs
rejects_deforest_when_class_method_uses_super asserts empty detection and no param mutation; still_deforests_when_method_has_no_super asserts detection and synthetic accumulator param added.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • PerryTS/perry#5772: Directly introduced producer call-site rewriting inside class member bodies (the feature this PR guards against when super is present), overlapping in detect.rs and mod.rs.

Poem

🐰 Hop hop, I sniff the super trail,
Where class bodies dare to call their base.
I plant a guard without fail —
No deforest in this sacred space!
The accumulator waits outside,
While super.foo flows with pride. 🌿

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: excluding super-using class member bodies from deforest call-site rewriting.
Description check ✅ Passed The description covers the root cause, fix, before/after behavior, related issue, and tests, even though it uses custom headings.
Linked Issues check ✅ Passed The PR implements the cluster A super-related regression fix in #5780 and preserves deforestation for super-free class members.
Out of Scope Changes check ✅ Passed The changes stay focused on deforest detection, rewrite guards, and regression tests with no evident unrelated scope creep.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/deforest-super-prop-5780

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
crates/perry-transform/src/deforest/tests.rs (2)

494-579: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the rewritten call-site arity here too.

This control only checks that helper gained the synthetic param. It would still pass if run() rewrote the producer signature but left the class-method call as helper(), which is the original arity-mismatch regression. Mirror the args.len() == 1 assertion from deforests_producer_called_from_class_method or reuse that helper.

🤖 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 494 - 579, The new
control test in still_deforests_when_method_has_no_super only verifies that
helper gains the synthetic accumulator parameter, but it does not confirm the
call site was rewritten. Update this test to also assert the rewritten
Expr::Call inside the class method now has one argument, mirroring
deforests_producer_called_from_class_method, so run() cannot regress by changing
the producer signature while leaving helper() at the old arity.

400-492: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Broaden this regression beyond super.foo on an instance method.

The implementation changes also guard constructors, accessors, static methods, and other super forms, but this test only covers Expr::SuperPropertyGet in one method body. A bug in those newly touched branches would still merge untested.

🤖 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 400 - 492, Broaden
the regression in rejects_deforest_when_class_method_uses_super to cover all
super-using class contexts, not just Expr::SuperPropertyGet in a single instance
method. Add cases for constructors, getters/setters, and static methods (and any
other super forms handled by the new logic), then assert detect_producers still
excludes the producer and run() leaves the helper signature unchanged for each
case. Use the existing make_simple_producer, detect_producers, and run setup to
keep the test focused while exercising the newly touched branches.
🤖 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/tests.rs`:
- Around line 494-579: The new control test in
still_deforests_when_method_has_no_super only verifies that helper gains the
synthetic accumulator parameter, but it does not confirm the call site was
rewritten. Update this test to also assert the rewritten Expr::Call inside the
class method now has one argument, mirroring
deforests_producer_called_from_class_method, so run() cannot regress by changing
the producer signature while leaving helper() at the old arity.
- Around line 400-492: Broaden the regression in
rejects_deforest_when_class_method_uses_super to cover all super-using class
contexts, not just Expr::SuperPropertyGet in a single instance method. Add cases
for constructors, getters/setters, and static methods (and any other super forms
handled by the new logic), then assert detect_producers still excludes the
producer and run() leaves the helper signature unchanged for each case. Use the
existing make_simple_producer, detect_producers, and run setup to keep the test
focused while exercising the newly touched branches.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 70218386-cdc5-4a2b-b59d-7d281b940ac2

📥 Commits

Reviewing files that changed from the base of the PR and between 4603b8b and 0733197.

📒 Files selected for processing (3)
  • crates/perry-transform/src/deforest/detect.rs
  • crates/perry-transform/src/deforest/mod.rs
  • crates/perry-transform/src/deforest/tests.rs

@proggeramlug
proggeramlug merged commit f2782a3 into main Jun 29, 2026
15 checks passed
@proggeramlug
proggeramlug deleted the fix/deforest-super-prop-5780 branch June 29, 2026 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

REGRESSION: super.prop throws 'Cannot convert undefined or null to object' (~24) + new.target-in-direct-eval over-rejected (6)

2 participants