fix(codegen,hir): cross-module member-new fills captures from the class's decl-site snapshot - #5640
Merged
Merged
Conversation
…l-site snapshot in the ctor
A function-nested class that captures an enclosing-scope local (e.g. a
hoisted sibling `function r_`) and is constructed via a member-callee
`new ns.C(...)` from a DIFFERENT compiled module routes to the runtime
construct path (`construct_registered_class_ref`) — which supplies NO
capture args, since the captured enclosing local is out of scope at the
construction site and the class can't be resolved statically there.
The synthesized `__perry_cap_*` ctor params then bound to garbage and the
captured local read as a non-callable: Next.js's app-route-turbo `rJ`
constructor (`this.methods = r_(e)`, constructed via
`new w.AppRouteRouteModule({...})` from the app-route template chunk)
threw `TypeError: value is not a function`, so the route module's init
aborted before self-registering its exports and every dynamic/API route
500'd with 'Cannot find module route.js'.
The W6 same-module member-new fix only filled captures from the decl-site
snapshot when the `new` site was statically routed to
`lower_new_member_captured` — the cross-module runtime construct never
reached it. Move the snapshot recovery INTO the synthesized constructor
body: at ctor entry each `__perry_cap_*` param is rebound to
`js_class_capture_value_or(class_id, slot, param)`. The ctor body is
compiled in the class's home module, where `class_name` resolves to its
real `class_id`, so EVERY construction path — inline, same-module member,
and cross-module runtime — recovers the captured value from the class's
own decl-site snapshot. Same-module bare-`new` is unchanged: the live cap
arg equals the snapshot value, so `_or` returns the identical value.
Adds a `fallback: Option<Box<Expr>>` to `Expr::ClassCaptureValue` (codegen
emits `js_class_capture_value_or` when present, plain
`js_class_capture_value` otherwise — the static-method prologue path is
unchanged); the walkers and stable-hash descend it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtends ChangesCross-module capture recovery via ClassCaptureValue fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-hir/src/lower_decl/class_captures.rs (1)
527-536: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRebind captures before pre-
super()user code.Because
insert_atis aftersuper(), legal derived-ctor code that reads a captured outer beforesuper()is remapped tofresh_param_idbut runs before snapshot recovery. Insert only thethis.__perry_cap_*assignments aftersuper(); the local-only rebinds can run at function entry.Proposed ordering fix
- let insert_at = super_pos.map(|p| p + 1).unwrap_or(0); - // Order at `insert_at`: rebinds (param = snapshot-or-param) FIRST, then - // the `this.__perry_cap_* = param` field stashes, then the user body. - let prologue: Vec<Stmt> = rebind_stmts.into_iter().chain(assignment_stmts).collect(); - for (i, stmt) in prologue.into_iter().enumerate() { - ctor.body.insert(insert_at + i, stmt); - } + let rebind_count = rebind_stmts.len(); + for (i, stmt) in rebind_stmts.into_iter().enumerate() { + ctor.body.insert(i, stmt); + } + + let assignment_insert_at = super_pos + .map(|p| p + 1 + rebind_count) + .unwrap_or(rebind_count); + for (i, stmt) in assignment_stmts.into_iter().enumerate() { + ctor.body.insert(assignment_insert_at + i, stmt); + }🤖 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-hir/src/lower_decl/class_captures.rs` around lines 527 - 536, Reorder the derived-constructor prologue in class_captures::lower_decl so only the `this.__perry_cap_*` assignment statements are inserted after `super()`, while the local capture rebinds run at function entry. Update the `super_pos`/`insert_at` insertion logic and the `prologue` construction so `rebind_stmts` are emitted before any pre-`super()` user code, preserving snapshot recovery before captured-outer reads.
🤖 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-hir/src/lower_decl/class_captures.rs`:
- Around line 470-489: The ctor capture rebinding in class_captures should
prefer the live parameter value and only fall back to the decl-site snapshot
when the param is missing/undefined. Update the logic around the ctor capture
recovery path in the class capture lowering code so the binding for each capture
checks the current constructor param first, and uses ClassCaptureValue or the
class snapshot only as a fallback for the cross-module member-new case. Keep
this change localized to the capture rebinding flow used by the ctor body and
the __perry_cap_* stashing path.
---
Outside diff comments:
In `@crates/perry-hir/src/lower_decl/class_captures.rs`:
- Around line 527-536: Reorder the derived-constructor prologue in
class_captures::lower_decl so only the `this.__perry_cap_*` assignment
statements are inserted after `super()`, while the local capture rebinds run at
function entry. Update the `super_pos`/`insert_at` insertion logic and the
`prologue` construction so `rebind_stmts` are emitted before any pre-`super()`
user code, preserving snapshot recovery before captured-outer reads.
🪄 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: 9a3e5296-a51e-47e6-8827-ec06d0ed0656
📒 Files selected for processing (7)
crates/perry-codegen/src/expr/static_field_meta.rscrates/perry-hir/src/ir/expr.rscrates/perry-hir/src/lower_decl/class_captures.rscrates/perry-hir/src/stable_hash/expr.rscrates/perry-hir/src/walker/expr_mut.rscrates/perry-hir/src/walker/expr_ref.rscrates/perry/tests/issue_5437_cross_module_member_new_capture.rs
proggeramlug
marked this pull request as draft
June 24, 2026 14:54
Contributor
Author
|
Converting to draft — CodeRabbit's two correctness findings are valid:
|
Reworks the synthesized-constructor capture rebind for two correctness
findings on the cross-module member-new fix:
- Param-first recovery: the live new-site cap arg now wins whenever
present; the decl-site snapshot is consulted only when the param is
undefined (the cross-module construct signal). Previously the
snapshot-first js_class_capture_value_or could override a same-module
new C() whose captured outer was mutated after the class declaration
(let x="a"; class C{ctor(){this.x=x}}; x="b"; new C() gave "a",
node gives "b"). New runtime FFI js_param_or_class_capture_value plus
a prefer_fallback flag on Expr::ClassCaptureValue (default false keeps
the snapshot-first behavior for static-method prologue rebinds).
- Rebind placement: the param rebinds now go at function entry, before
any pre-super() user code (a derived ctor may read a captured outer
before super()); only the this.__perry_cap_* field stashes wait until
after super().
Adds regression tests for both findings (match node --experimental-strip-types).
proggeramlug
marked this pull request as ready for review
June 24, 2026 16:11
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-runtime/src/object/class_constructors.rs`:
- Around line 223-235: The helper js_param_or_class_capture_value is using live
undefined as the missing-argument signal, which causes same-module captures to
fall back to stale decl-site values instead of preserving an actual undefined.
Fix this by adding a separate presence flag or an unambiguous sentinel in the
synthesized constructor path, and update js_param_or_class_capture_value to only
use CLASS_CAPTURE_VALUES for truly absent capture args. Also extend the
ctor-related tests to cover same-module captures that become undefined at
runtime.
🪄 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: 05d6014d-a708-4861-a215-e0380d75779f
📒 Files selected for processing (7)
crates/perry-codegen/src/expr/static_field_meta.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-hir/src/ir/expr.rscrates/perry-hir/src/lower_decl/class_captures.rscrates/perry-hir/src/stable_hash/expr.rscrates/perry-runtime/src/object/class_constructors.rscrates/perry/tests/issue_5437_capture_rebind_param_first.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/perry-codegen/src/expr/static_field_meta.rs
- crates/perry-hir/src/lower_decl/class_captures.rs
Comment on lines
+223
to
+235
| pub extern "C" fn js_param_or_class_capture_value(param: f64, class_id: u32, index: u32) -> f64 { | ||
| if param.to_bits() != crate::value::TAG_UNDEFINED { | ||
| return param; | ||
| } | ||
| // param is `undefined` (cross-module construct dropped the cap arg): | ||
| // recover from the decl-site snapshot when one is registered for this | ||
| // class; otherwise stay `undefined`. | ||
| CLASS_CAPTURE_VALUES.with(|m| { | ||
| m.borrow() | ||
| .get(&class_id) | ||
| .and_then(|v| v.get(index as usize).copied()) | ||
| .map(f64::from_bits) | ||
| .unwrap_or(param) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
undefined cannot safely mean “missing capture arg”.
Line 224 treats any live undefined capture as the cross-module fallback signal. That still misbinds same-module cases like let x = "a"; class C { ... } x = undefined; new C(): the helper will resurrect the stale decl-site snapshot instead of preserving the real live undefined. This also leaves a regression gap in the new tests. You'll need a separate presence bit or another sentinel that cannot collide with a valid JS value, then thread that through the synthesized ctor path.
🤖 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-runtime/src/object/class_constructors.rs` around lines 223 -
235, The helper js_param_or_class_capture_value is using live undefined as the
missing-argument signal, which causes same-module captures to fall back to stale
decl-site values instead of preserving an actual undefined. Fix this by adding a
separate presence flag or an unambiguous sentinel in the synthesized constructor
path, and update js_param_or_class_capture_value to only use
CLASS_CAPTURE_VALUES for truly absent capture args. Also extend the ctor-related
tests to cover same-module captures that become undefined at runtime.
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
1 participant
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.
What
A
new ns.Class(...)construct whereClassis imported from another module supplied no capture args — the captures live in the class's home module and aren't visible at the cross-modulenewsite — so a captured value (e.g. a hoisted function) resolved to garbage/undefined and method calls on it threwvalue is not a function.Found via Next.js dynamic/API routes (#5437): the app-route-turbo
rJroute-module-class constructor doesthis.methods = r_(e)wherer_is a captured module-scope fn; cross-module member-new dropped it → route__initaborted →require(route.js)undefined → 500 on every dynamic/API route.Fix
At ctor entry, rebind cross-module-captured params from the class's decl-site capture snapshot via
js_class_capture_value_or(the same mechanism as the W6/TDZ capture fixes), driven by a new additive HIRExprnode emitted at the cross-module member-new site. (perry-hirir/walkers/hash +static_field_meta.rscodegen.)Validation
issue_5437_cross_module_member_new_capture(1 passed); broad capture/inheritance testissue_4972_derived_class_capture_super(3 passed) — no regression. 5 minimal repros of the cross-module member-new-capture shape match node.rJ/r_value is not a functionis eliminated; static routes stay byte-identical (no fix(codegen): #5437 — inline method-dispatch tower must build args per-case arity, not one global rest decision #5622 regression). (Dynamic routes still 500 on a separate, deeper scale-only blocker — capturedundefinedlosing its NaN-box tag — tracked separately.)Refs #5437.
Summary by CodeRabbit
super().#5437, including param-first behavior and cross-module “member-new” capture recovery.