fix: Next.js standalone bring-up — walls 36–44 (dynamic-parent classes, forward-capture, super.method, cjs export-hint, self-new in capturing closure)#5125
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (19)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (16)
📝 WalkthroughWalkthroughVersion 0.5.1168 "Next.js standalone bring-up" bundles six grouped bug fixes: CJS ChangesCJS/HIR/Codegen Multi-Fix: Forward Capture, Dynamic super, Constructor Collision
Sequence Diagram(s)sequenceDiagram
rect rgba(135, 206, 235, 0.5)
Note over ModuleInit,CLASS_DYNAMIC_PARENT_VALUE: Module initialization: stash dynamic parent value
ModuleInit->>js_register_class_parent_dynamic: class_id, parent_value (evaluated extends expr)
js_register_class_parent_dynamic->>CLASS_DYNAMIC_PARENT_VALUE: stash raw NaN-boxed bits
end
rect rgba(144, 238, 144, 0.5)
Note over SuperCallCodegen,js_fetch_or_value_super: super() at constructor call site: retrieve stashed value
SuperCallCodegen->>js_get_dynamic_parent_value: class_id
js_get_dynamic_parent_value->>CLASS_DYNAMIC_PARENT_VALUE: read stashed bits
CLASS_DYNAMIC_PARENT_VALUE-->>js_get_dynamic_parent_value: parent_value f64
SuperCallCodegen->>js_fetch_or_value_super: parent_val, this_box, args
js_fetch_or_value_super->>run_class_constructor_on_this_flat: parent_cid, this_raw, args (INT32_TAG path)
run_class_constructor_on_this_flat-->>js_fetch_or_value_super: success → undefined
end
rect rgba(255, 200, 100, 0.5)
Note over Codegen,call_vtable_method: super.method() dynamic dispatch: deadlock-safe resolution and invoke
Codegen->>js_super_method_call_dynamic: child_class_id, method_name, this, args
js_super_method_call_dynamic->>CLASS_VTABLE_REGISTRY: resolve parent chain under read lock
CLASS_VTABLE_REGISTRY-->>js_super_method_call_dynamic: vtable entry found
js_super_method_call_dynamic->>call_vtable_method: invoke (lock dropped)
call_vtable_method-->>Codegen: result f64
end
rect rgba(255, 150, 150, 0.5)
Note over lower_new,call_local_constructor_symbol: new ClassName collision detection
lower_new->>lower_new: check ctor_alias_collision: closure_captures non-empty && ctor_symbol_exists
alt collision detected
lower_new->>call_local_constructor_symbol: redirect (avoid LocalId aliasing)
else no collision
lower_new->>lower_new: inline constructor body
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/object/native_call_method.rs (1)
3648-3763:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror the unlock-before-invoke fix in the raw-pointer class path.
This resolves the deadlock only for the
jsval.is_pointer()branch. The raw-pointer fallback later in this same function still returnscall_vtable_method(...)while holdingCLASS_VTABLE_REGISTRY.read(), so untagged class instances can still hang on the same lazy-require()→ class-registration path this change is trying to eliminate. Please apply the same resolve-then-drop-guard pattern there too.🤖 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/native_call_method.rs` around lines 3648 - 3763, The deadlock safety fix shown in the diff applies only to the jsval.is_pointer() branch by resolving the method under the CLASS_VTABLE_REGISTRY read lock and releasing it before calling the method body. The raw-pointer fallback path later in the same function still returns call_vtable_method(...) while holding the read lock, which can deadlock on the same lazy-require() and class-registration path. Apply the same resolve-then-drop-guard pattern to the raw-pointer fallback: wrap the registry lookup logic in a separate scope that captures the necessary method information (similar to the ResolvedMethod enum) and releases the lock before invoking call_vtable_method or any other method-execution function.
🤖 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_decl.rs`:
- Around line 293-313: In the `class_decl.rs` file at lines 293-313 (anchor) and
1295-1304 (sibling), when handling the `parent_name == "default"` case, the code
is incorrectly setting `extends_name` to `Some(parent_name)` in the tuple
returned from the match expression. This causes inherited-field and accessor
lookups to fail later because they key off `extends_name`, and using the literal
"default" string doesn't resolve to the actual parent class. Change the second
element of both the Ok and Err tuple returns from `Some(parent_name)` to `None`,
so that `extends_name` remains unset and the dynamic relationship is carried
only through `extends_expr`, allowing the real parent edge to be wired at
registration time without breaking metadata resolution.
In `@crates/perry-runtime/src/object/class_constructors.rs`:
- Around line 261-270: The super.method() lookup in this function only uses
lookup_class_method_in_chain, which fails to find methods registered at runtime
via js_register_function_prototype_method, CLASS_PROTOTYPE_METHODS, or synthetic
prototype-object parents created by js_set_function_prototype. Instead of
directly calling lookup_class_method_in_chain, route the parent method lookup
through the full parent lookup tower that checks both class methods and
prototype-registered methods, similar to how regular this.method() dispatch
works. This ensures that super.m() can find methods defined on parent prototypes
(like Base.prototype.m in a subclass of function Base(){}).
In `@crates/perry-runtime/src/object/class_registry.rs`:
- Around line 272-284: The static map CLASS_DYNAMIC_PARENT_VALUE stores raw
NaN-boxed parent bits but is not integrated into the garbage collection
side-table machinery, leaving it unscanned and unrewritten during moving GC
operations which can result in stale pointers. Locate the existing GC scan,
snapshot, and reset functions that handle other class side tables in this file,
and add corresponding logic to visit and rewrite the raw u64 bits stored in
CLASS_DYNAMIC_PARENT_VALUE during these GC operations, ensuring that any
pointer-tagged parent values are properly updated to their relocated addresses.
In `@crates/perry-runtime/src/object/global_this.rs`:
- Around line 480-502: When run_class_constructor_on_this_flat() returns false
in the INT32_TAG block, the code currently falls through with callee still being
the INT32-tagged ClassRef. This ClassRef then gets passed to later code (the
usable logic and js_native_call_value), which would incorrectly treat it as
callable and result in undefined being returned without running the base
constructor. Fix this by ensuring that when run_class_constructor_on_this_flat()
returns false, execution does not fall through to js_native_call_value with the
same ClassRef value—either by returning early in the error case, or by
preventing the ClassRef from being treated as callable in the subsequent logic.
In `@crates/perry/src/commands/compile/cjs_wrap/extract_exports.rs`:
- Around line 405-414: The `line_start_ok` check in the extract_exports.rs file
is too permissive because it only validates that the current line starts with
whitespace, ignoring what precedes the previous newline. To tighten this
boundary check, after confirming the current line is indentation-only, you must
also search backward to find the previous newline and verify that the last
non-whitespace character before that previous newline is a valid statement
boundary (such as semicolon, closing brace, or opening brace). This additional
validation prevents multiline expressions like `0 && (\n module.exports = { X:
null }\n)` from incorrectly passing the check, since the opening parenthesis on
the previous line indicates an ongoing expression context.
---
Outside diff comments:
In `@crates/perry-runtime/src/object/native_call_method.rs`:
- Around line 3648-3763: The deadlock safety fix shown in the diff applies only
to the jsval.is_pointer() branch by resolving the method under the
CLASS_VTABLE_REGISTRY read lock and releasing it before calling the method body.
The raw-pointer fallback path later in the same function still returns
call_vtable_method(...) while holding the read lock, which can deadlock on the
same lazy-require() and class-registration path. Apply the same
resolve-then-drop-guard pattern to the raw-pointer fallback: wrap the registry
lookup logic in a separate scope that captures the necessary method information
(similar to the ResolvedMethod enum) and releases the lock before invoking
call_vtable_method or any other method-execution function.
🪄 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: 3a7ec8eb-77cb-4096-b3e1-4b1cac485321
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
CHANGELOG.mdCLAUDE.mdCargo.tomlcrates/perry-codegen/src/expr/super_method.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-hir/src/destructuring/pattern_binding.rscrates/perry-hir/src/lower/expr_function.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_decl/class_decl.rscrates/perry-hir/src/lower_decl/mod.rscrates/perry-runtime/src/object/class_constructors.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry/src/commands/compile/cjs_wrap/extract_exports.rscrates/perry/src/commands/compile/cjs_wrap/hoist_classes.rscrates/perry/src/commands/compile/cjs_wrap/wrap.rs
| } else if parent_name == "default" { | ||
| // `class X extends _mod.default` — the interop ESM | ||
| // default-export-class pattern (Next.js `NextNodeServer | ||
| // extends base-server`'s default `Server`). The trailing | ||
| // property `default` never resolves through `lookup_class`, | ||
| // and a `.default` export is always a real user/registered | ||
| // class — never a native-module member like `http.Agent` | ||
| // (which inherits via a *named* property and is handled by | ||
| // the colliding-name / parentless branches). Route through | ||
| // the dynamic `extends_expr` path so `super(opts)` | ||
| // re-evaluates the alias at construction time and runs the | ||
| // base constructor, and the decl-time | ||
| // `RegisterClassParentDynamic` wires the real parent edge | ||
| // (inherited methods / `instanceof`). The companion hoist | ||
| // guard in `extract_top_level_class_decls` keeps this class | ||
| // inside the IIFE so the require alias is assigned before the | ||
| // registration runs. | ||
| match lower_expr(ctx, super_class) { | ||
| Ok(expr) => (None, Some(parent_name), None, Some(Box::new(expr))), | ||
| Err(_) => (None, Some(parent_name), None, None), | ||
| } |
There was a problem hiding this comment.
Don't preserve "default" as extends_name here.
Later in this file, inherited-field and accessor lookups key off extends_name. Setting it to the literal export slot "default" means class X extends _mod.default still misses the real parent's metadata, so ctor field scanning can allocate duplicate own fields and inherited accessors still won't be recognized even though runtime parent registration succeeds. Leave extends_name unset unless you can resolve the underlying class name, and let extends_expr carry the dynamic relationship.
Also applies to: 1295-1304
🤖 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_decl.rs` around lines 293 - 313, In the
`class_decl.rs` file at lines 293-313 (anchor) and 1295-1304 (sibling), when
handling the `parent_name == "default"` case, the code is incorrectly setting
`extends_name` to `Some(parent_name)` in the tuple returned from the match
expression. This causes inherited-field and accessor lookups to fail later
because they key off `extends_name`, and using the literal "default" string
doesn't resolve to the actual parent class. Change the second element of both
the Ok and Err tuple returns from `Some(parent_name)` to `None`, so that
`extends_name` remains unset and the dynamic relationship is carried only
through `extends_expr`, allowing the real parent edge to be wired at
registration time without breaking metadata resolution.
proggeramlug
changed the title
|
Update: wall 44 added. | 44 | With wall 44, the server runs request handling all the way through the OTEL tracer into the render pipeline. Next blocker (wall 45, not in this PR): a render error's |
proggeramlug
force-pushed
the
feat/nextjs-walls-38plus
branch
from
4c231e4 to
46e6bdf
Compare
|
Rebased onto latest Wall 45 status (not in this PR): fully diagnosed as a 3-layer bug — (1) |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/perry-runtime/src/object/class_registry.rs (1)
272-284:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire
CLASS_DYNAMIC_PARENT_VALUEinto the existing root lifecycle.This table stores raw NaN-boxed parent values, but the GC root paths later in this file never visit or rewrite it, and Line 4373 also skips the usual
runtime_write_barrier_root_nanbox(...). After a moving or incremental GC,js_get_dynamic_parent_value()can handsuper()stale parent bits or miss a freshly-registered parent entirely. Please treat it like the other class side tables: add entries inClassSideTableRootSlot/class_side_table_root_snapshot, visit it from both scan paths, clear it intest_clear_class_side_table_roots, and route inserts through a helper that emits the root write barrier.Also applies to: 4362-4378, 4500-4519
🤖 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_registry.rs` around lines 272 - 284, The CLASS_DYNAMIC_PARENT_VALUE static table stores raw NaN-boxed parent values but is not integrated into the GC root lifecycle, causing stale or missed parent values after GC cycles. Integrate it like other class side tables by: adding an entry to ClassSideTableRootSlot enum, adding snapshot and visit logic in class_side_table_root_snapshot for both GC scan paths, adding a clear statement in test_clear_class_side_table_roots, and creating a helper function that routes inserts through runtime_write_barrier_root_nanbox to emit the proper root write barrier when registering new parent values.
🤖 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.
Duplicate comments:
In `@crates/perry-runtime/src/object/class_registry.rs`:
- Around line 272-284: The CLASS_DYNAMIC_PARENT_VALUE static table stores raw
NaN-boxed parent values but is not integrated into the GC root lifecycle,
causing stale or missed parent values after GC cycles. Integrate it like other
class side tables by: adding an entry to ClassSideTableRootSlot enum, adding
snapshot and visit logic in class_side_table_root_snapshot for both GC scan
paths, adding a clear statement in test_clear_class_side_table_roots, and
creating a helper function that routes inserts through
runtime_write_barrier_root_nanbox to emit the proper root write barrier when
registering new parent values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2df103c3-038c-46ad-a152-8740b9df5889
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
CHANGELOG.mdCLAUDE.mdCargo.tomlcrates/perry-codegen/src/expr/super_method.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-hir/src/destructuring/pattern_binding.rscrates/perry-hir/src/lower/expr_function.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_decl/class_decl.rscrates/perry-hir/src/lower_decl/mod.rscrates/perry-runtime/src/object/class_constructors.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry/src/commands/compile/cjs_wrap/extract_exports.rscrates/perry/src/commands/compile/cjs_wrap/hoist_classes.rscrates/perry/src/commands/compile/cjs_wrap/wrap.rs
✅ Files skipped from review due to trivial changes (3)
- Cargo.toml
- CHANGELOG.md
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (14)
- crates/perry-hir/src/lower_decl/class_decl.rs
- crates/perry-codegen/src/expr/this_super_call.rs
- crates/perry-runtime/src/object/global_this.rs
- crates/perry/src/commands/compile/cjs_wrap/hoist_classes.rs
- crates/perry/src/commands/compile/cjs_wrap/wrap.rs
- crates/perry-hir/src/destructuring/pattern_binding.rs
- crates/perry-hir/src/lower_decl/mod.rs
- crates/perry-codegen/src/expr/super_method.rs
- crates/perry/src/commands/compile/cjs_wrap/extract_exports.rs
- crates/perry-runtime/src/object/native_call_method.rs
- crates/perry-codegen/src/lower_call/new.rs
- crates/perry-runtime/src/object/class_constructors.rs
- crates/perry-hir/src/lower/expr_function.rs
- crates/perry-hir/src/lower_decl/block.rs
proggeramlug
force-pushed
the
feat/nextjs-walls-38plus
branch
from
46e6bdf to
9ccba92
Compare
|
Addressed the rebase + lint failure + CodeRabbit feedback in the latest force-push ( Rebase & lint
CodeRabbit fixes (4/5 applied)
Skipped with reason Verified wall-38/42 repro still green after the changes: |
1 participant
Next.js 16.2.9 standalone bring-up — walls 36–43
Continues the
compile-as-package/ Next.js work. With these fixes the Next.js standalone server compiles (auto-optimize, ~124 MB), boots, listens, routes, and runs request handling through to the OpenTelemetry tracer (previously it died inside theNextNodeServerconstructor). Each fix has an isolated repro.require()took the write lock → deadlock).class X extends _mod.default(interop ESM default-export base) ran no base ctor. cjs_wrap hoist guard now scans theextendshead for IIFE-local refs;.defaultmember-extends routes throughextends_expr; super/newuse the decl-time-stashed parent (CLASS_DYNAMIC_PARENT_VALUE) and invoke a ClassRef parent viarun_class_constructor_on_this_flat.0 && (module.exports = { X: null })Babel export-hint was extracted as a real named export, overriding the real_export(exports, { X: () => X })getter withundefined. Shape-2 scan now requires a statement boundary (skips expression context like0 && (…)).require(unresolvable)now throws aMODULE_NOT_FOUND-coded error (Node parity) sotry { require(p) } catch (e) { if (e.code !== 'MODULE_NOT_FOUND') throw }degrades gracefully (Next swallows the absent instrumentation hook).let/constresolved to aglobalThisread →ReferenceError(router-serverinitialize()request handler readsrelativeProjectDirdeclared ~400 lines below → HTTP 500). New sharedpre_register_forward_captured_letsboxes only bindings referenced by an earlier closure; preallocs the box at entry (kept out ofhoisted_id_setso non-hoistableconst = arrowdecls aren't reordered).super.method()on a dynamic-parent class returned a bogus0.0(staticextends_namewalk missed).NextNodeServer.makeRequestHandler'ssuper.getRequestHandler()→ number → "value is not a function". Newjs_super_method_call_dynamicresolves from the registered parent (never the child → no override recursion), drops the registry lock before the call (wall-37 safe)._export(exports, { SpanKind: () => SpanKind })getter forward-captures the laterconst { …, SpanKind } = api→ "SpanKind is not defined". The forward-capture helper now handles array/object destructuring leaves +{ key }shorthand and is shared bylower_fn_expr(theconst _cjs = (function(){…})()wrapper).Validation
perry-hir,perry-codegen,perry-runtime,perrytest suites pass.typed_array_indexof_includes"failure" under a full run passes in isolation (known test-pollution flake).issue_4903_listen_callback_deferredtests fail on a pre-existing auto-optimize linker mismatch (perry_ffi::error::system_error_value, code untouched by this PR) — environmental to the worktree, unrelated to these changes.Known next blocker (not in this PR)
new SelfClass(thisAlias.field)inside a closure that capturedconst t = thismis-binds the new instance's ctorthis→ tracerBaseContext.setValuethrows. Tracked as wall 44.Summary by CodeRabbit
module.exports = { ... }export detection when it appears in expression contexts.super()andsuper.method(...)for classes with dynamically resolved parents, including inheritance interop likemodule.ClassName.default.let/const(including destructuring) referenced by earlier closures in the same block, plus forward class-name resolution.require()shim fallback to throw aMODULE_NOT_FOUNDerror with a clearer message.newinlining in a specific constructor/local capture collision scenario.