fix(deforest): bail on producers called from super-containing member bodies (#5780) - #5791
Closed
proggeramlug wants to merge 1 commit into
Closed
proggeramlug wants to merge 1 commit into
proggeramlug wants to merge 1 commit into
Conversation
…bodies (#5780) Commit 5d47364 extended Phase 3 call-site rewriting to class member bodies (constructors, methods, accessors). When a member body contains a super reference the [[HomeObject]] context can be disturbed by injecting the deforest temporaries, causing super.prop reads to resolve to undefined and throw at runtime. Fix: treat a class member body that contains any super expression as an unsafe context for deforestation — the same conservative bail used for closure bodies (#5136). * detect.rs (4th pass): for member bodies where `body_has_super_ref` is true, use `scan_candidate_funcrefs_in_stmts` to exclude ALL producers called there instead of only those inside nested closures. This prevents a deforested producer from gaining the +1 out-param while its call site in the super-containing body keeps the original arity (the arity-mismatch SIGSEGV class). * mod.rs (Phase 3): add `if !body_has_super_ref(&body)` guards around every class member rewrite block as defence-in-depth. * New helpers: `body_has_super_ref` / `stmt_has_super_ref` / `expr_has_super_ref` cover all 9 Super HIR variants; exported from the deforest crate for potential reuse. * tests.rs: add `rejects_producer_called_from_super_containing_method` to lock in the detection and signature-preservation invariants.
📝 WalkthroughWalkthroughAdds a Deforest super-ref bail-out
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Contributor
Author
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/perry-transform/src/deforest/detect.rs`:
- Around line 488-495: The super detection in expr_has_super_ref only inspects
direct expression children via walk_expr_children, so nested Expr::Closure
bodies are skipped and super references inside them are missed. Update
expr_has_super_ref to recurse into closure bodies as well, using the existing
traversal entry point in detect.rs and the expr_has_super_ref helper so nested
closures are treated as super-bearing and detect_producers cannot admit unsafe
helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bae21260-ae2f-4dde-b634-20e05ecdb0f5
📒 Files selected for processing (3)
crates/perry-transform/src/deforest/detect.rscrates/perry-transform/src/deforest/mod.rscrates/perry-transform/src/deforest/tests.rs
Comment on lines
+488
to
+495
| let mut found = false; | ||
| walk_expr_children(e, &mut |child| { | ||
| if !found { | ||
| found = expr_has_super_ref(child); | ||
| } | ||
| }); | ||
| found | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Recurse into nested closure bodies when detecting super.
Line 489 only walks expression children, but walk_expr_children does not enter Expr::Closure bodies. A method like const g = () => super.x; const v = helper(); is therefore treated as super-free, so detect_producers can still admit helper and Phase 3 can still rewrite the member body—the same regression this PR is trying to fence off.
Suggested fix
fn expr_has_super_ref(e: &Expr) -> bool {
if matches!(
e,
Expr::SuperCall(_)
| Expr::SuperCallSpread(_)
| Expr::SuperMethodCall { .. }
| Expr::SuperMethodCallSpread { .. }
| Expr::SuperPropertyGet { .. }
| Expr::SuperPropertySet { .. }
| Expr::ObjectSuperPropertyGet { .. }
| Expr::ObjectSuperPropertySet { .. }
| Expr::ObjectSuperMethodCall { .. }
) {
return true;
}
+ if let Expr::Closure { body, .. } = e {
+ if body_has_super_ref(body) {
+ return true;
+ }
+ }
let mut found = false;
walk_expr_children(e, &mut |child| {
if !found {
found = expr_has_super_ref(child);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mut found = false; | |
| walk_expr_children(e, &mut |child| { | |
| if !found { | |
| found = expr_has_super_ref(child); | |
| } | |
| }); | |
| found | |
| } | |
| if let Expr::Closure { body, .. } = e { | |
| if body_has_super_ref(body) { | |
| return true; | |
| } | |
| } | |
| let mut found = false; | |
| walk_expr_children(e, &mut |child| { | |
| if !found { | |
| found = expr_has_super_ref(child); | |
| } | |
| }); | |
| found | |
| } |
🤖 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/detect.rs` around lines 488 - 495, The
super detection in expr_has_super_ref only inspects direct expression children
via walk_expr_children, so nested Expr::Closure bodies are skipped and super
references inside them are missed. Update expr_has_super_ref to recurse into
closure bodies as well, using the existing traversal entry point in detect.rs
and the expr_has_super_ref helper so nested closures are treated as
super-bearing and detect_producers cannot admit unsafe helpers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5780.
What broke
Commit 5d47364 extended the deforestation Phase 3 call-site rewriter to
class member bodies (constructors, methods, getters, setters,
static_methods). When a member body also contains a
superpropertyread or method call, injecting the deforest temporaries (
let v = []before the call) can disturb the
[[HomeObject]]context thatsuperreads rely on at runtime. The result is
super.propresolving toundefined/nulland throwingTypeError: Cannot convert undefined or null to object(24 failing cases in the gap suite, cluster A).Fix
Treat a class member body that contains any super expression as an
unsafe context for deforestation — the same conservative bail
already used for closure bodies (#5136).
detect.rs— 4th detection passWhen
body_has_super_ref(&body)is true for a class member body, callscan_candidate_funcrefs_in_stmts(new helper) instead ofscan_producers_used_in_closures. The new helper marks allcandidate
FuncRefs in that body — not only those inside closures —as excluded. This prevents a producer from being deforested when its
only call site lives in a super-containing member body: without this
gate the producer gains the
+1out-param while its call site keepsthe original arity, an arity-mismatch SIGSEGV of the same class as #5136.
mod.rs— Phase 3Added
if !body_has_super_ref(&body)guards around every class memberrewrite block as defence-in-depth. Detection already excludes the
affected producers, so these guards never fire in practice — but they
make the invariant explicit and protect against future detection gaps.
New helpers
body_has_super_ref/stmt_has_super_ref/expr_has_super_refcover all 9
Super*HIR variants:SuperCall,SuperCallSpread,SuperMethodCall,SuperMethodCallSpread,SuperPropertyGet,SuperPropertySet,ObjectSuperPropertyGet,ObjectSuperPropertySet,ObjectSuperMethodCall.tests.rsAdded
rejects_producer_called_from_super_containing_methodto lock inboth the detection exclusion (producer not in the candidates map) and
the signature-preservation invariant (no synthetic out-param added).
What is NOT changed
rewritten as before (the fix is surgical — super-free methods are
unaffected).
analyze_producer,body_has_closure, all detection passes 1–3,and the existing
deforests_producer_called_from_class_methodregression test are unchanged.
Generated by Claude Code
Summary by CodeRabbit
superreferences are no longer rewritten in ways that could change behavior.