fix(compile,runtime): 14 walls from Next.js standalone compile validation#5050
Merged
Conversation
proggeramlug
force-pushed
the
feat/nextjs-compile-walls
branch
from
33d6981 to
5b7b354
Compare
|
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 (2)
📝 WalkthroughWalkthroughCollects new HIR forms and lowering for super(...spread) and class-capture snapshots; adds codegen and runtime FFI/GC support for captures and spread-super, detects and wires lazy CommonJS requires (deferred init), applies entry process.env literals early, improves CJS detection, resolution, and native-module override handling. ChangesClass Dispatch and Constructor Semantics
Module Loading and Lazy Require Support
Native Module Property Overrides and Dynamic Dispatch
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry/src/commands/compile/cjs_wrap/wrap.rs (1)
454-490:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLazy aliases are still being hoisted when class hoisting is enabled.
lazy_specsblocks alias adoption above, but this block still emitsconst alias = _lazyreq_N;and strips the originalconst alias = require(...)inside the function. That eagerly reads the deferred binding at module scope and changes a function-local alias into an outer-scope binding.♻️ Minimal fix
let lines = aliases .iter() - .filter(|(alias, _, _)| !identifier_is_reassigned(source, alias)) + .filter(|(alias, spec, _)| { + !identifier_is_reassigned(source, alias) && !lazy_specs.contains(spec) + }) .filter_map(|(alias, spec, _range)| { let idx = require_specs.iter().position(|s| s == spec)?; if import_local_names[idx] == *alias { None } else { @@ let ranges = aliases .into_iter() - .filter(|(_, spec, _)| require_specs.iter().any(|s| s == spec)) + .filter(|(_, spec, _)| { + require_specs.iter().any(|s| s == spec) && !lazy_specs.contains(spec) + }) .filter(|(alias, _, _)| !identifier_is_reassigned(source, alias)) .map(|(_, _, range)| range) .collect::<Vec<_>>();🤖 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/src/commands/compile/cjs_wrap/wrap.rs` around lines 454 - 490, The emitted const aliases and strip ranges must exclude aliases that come from deferred lazy_specs; modify the branch that builds aliases (using extract_require_aliases_with_ranges and the subsequent filters that produce aliases and ranges) to ignore any alias whose spec is present in lazy_specs (i.e., add a filter like require_specs.iter().position(...) or a direct check against lazy_specs to skip when spec is in lazy_specs) so you do not emit "const alias = _lazyreq_N;" nor strip the original function-local "const alias = require(...)" — apply the same exclusion when collecting ranges (the .filter(...) chain that maps to range) so lazy-spec aliases are neither hoisted nor have their original declarations removed; keep existing checks like identifier_is_reassigned and import_local_names logic intact.
🧹 Nitpick comments (2)
crates/perry-transform/src/inline/mod.rs (1)
496-496: ⚡ Quick winAdd a regression assertion for
is_deferred_requirein the synthetic-import test.The test already checks adjacent import flags; extending it to this new field will prevent silent regressions.
Suggested patch
assert!(module.imports.iter().all(|i| { !i.is_native && i.module_kind == ModuleKind::NativeCompiled && i.resolved_path.as_deref() == Some(i.source.as_str()) && !i.type_only && !i.is_dynamic - && !i.is_dynamic_target + && !i.is_dynamic_target + && !i.is_deferred_require }));🤖 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/inline/mod.rs` at line 496, The synthetic-import test should assert the new is_deferred_require flag to prevent regressions: update the synthetic-import test to include an assertion that the imported synthetic's is_deferred_require field equals the expected value (e.g., assert_eq!(item.is_deferred_require, false)); locate the test that already checks adjacent import flags and add this single assertion referencing is_deferred_require so the test fails if the flag changes unexpectedly.crates/perry-runtime/src/object/native_module.rs (1)
2971-2982: ⚡ Quick winRemove the second
timersmatch arm.Line 2815 already returns
TIMERS_NAMESPACE_KEYSfor"timers", so this later branch never runs. Keeping a second hard-coded copy here is easy to drift the next time the timers surface changes.♻️ Suggested cleanup
- // Plain `timers` was missing — `require('node:timers').setImmediate` - // read undefined (Next.js's fast-set-immediate extension reads and - // patches it at module init). - "timers" => Some(&[ - b"setTimeout", - b"clearTimeout", - b"setInterval", - b"clearInterval", - b"setImmediate", - b"clearImmediate", - b"promises", - ]),🤖 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_module.rs` around lines 2971 - 2982, Remove the duplicate "timers" match arm that returns the hard-coded array (the block with b"setTimeout", b"clearTimeout", ...), and rely on the existing earlier branch that returns TIMERS_NAMESPACE_KEYS; delete this second arm to avoid drift and duplication and leave any surrounding match structure intact (ensure no trailing commas or fall-through gaps remain after removal).
🤖 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-codegen/src/expr/static_field_meta.rs`:
- Around line 131-181: The bug is that RegisterClassCaptures and
ClassCaptureValue use the compile-time ctx.class_ids(class_name) as the snapshot
key, causing different class instances to collide; fix by keying snapshots on
the concrete class object pointer/value at runtime instead of class_id: in
Expr::RegisterClassCaptures change the js_class_register_capture_values call to
take a PTR key (pass the class object register/pointer for class_name rather
than the I32 class_id) and adapt the argument types from I32 to PTR; in
Expr::ClassCaptureValue change the js_class_capture_value call to take the same
PTR key (pass the same class object register/pointer and use I32 only for the
index), and remove/stop relying on ctx.class_ids for lookup (instead obtain the
runtime class pointer/register for class_name—e.g., look up whatever place holds
the freshly created class value or load it at runtime—and use that register as
the first arg). Ensure the two helpers js_class_register_capture_values and
js_class_capture_value now use a PTR key consistently so snapshots are keyed by
the concrete class object.
In `@crates/perry-hir/src/ir/expr.rs`:
- Around line 374-394: The snapshot key must include a per-evaluation
identifier, not just class_name: change the IR variants RegisterClassCaptures
and ClassCaptureValue to carry an evaluation key (e.g., eval_key: String or u64)
and make CLASS_CAPTURE_VALUES use a composite key (class_name, eval_key) when
writing/reading; update all emitters/consumers that create RegisterClassCaptures
to generate and pass the eval_key at the class-declaration evaluation site and
update places that construct ClassCaptureValue to pass the matching eval_key so
reads target the correct per-invocation snapshot.
In `@crates/perry-hir/src/lower_decl/block.rs`:
- Around line 240-296: The end-of-body re-registration currently only inserts
the RegisterClassCaptures entries at the fallthrough end (insert_at) and misses
earlier return/early-exit paths and nested block exits; update the logic that
builds and inserts re_regs so that after computing re_regs (same way using
ctx.lookup_class_captures, captured -> captures, cap_args and
append_new_args_stmt), you also walk the body and for every Return statement
insert a clone of each re_reg immediately before it, and for any nested
Stmt::Block recurse into that block's .stmts and perform the same insertion
(insert before any Return in that block and at the block's end if it falls
through), rather than only inserting at the final insert_at; use
RegisterClassCaptures, re_regs, append_new_args_stmt, and body names to locate
and update the code.
In `@crates/perry-runtime/src/event_target.rs`:
- Around line 283-310: js_event_subclass_init currently always initializes
subclass instances as plain Event and drops CustomEvent.detail; add a new extern
"C" fn js_custom_event_subclass_init(this_value: f64, type_value: f64, options:
f64, argc: u32) -> f64 that mirrors js_event_subclass_init but extracts the
detail property from options (pass Some(detail_ptr) to init_event_fields) and
calls init_event_fields(event, type_value, options, b"CustomEvent",
Some(detail)), then return undefined_value(); update the lowering in
crates/perry-codegen/src/expr/this_super_call.rs so that subclasses of
CustomEvent are emitted to call js_custom_event_subclass_init while Event
subclasses keep calling js_event_subclass_init; keep the KEEP_* used anchors as
needed.
In `@crates/perry-runtime/src/object/class_registry.rs`:
- Around line 42-56: The helper is_bound_native_method_closure_value currently
treats any bound method whose trampoline equals closure::BOUND_METHOD_FUNC_PTR
as a native-module export, which incorrectly exempts ordinary bound methods;
change it to consult the native-module metadata by calling
bound_native_callable_module_and_method(...) (or the equivalent API that returns
module/method info) on the cleaned closure pointer and only return true when
that metadata indicates a native-module export; remove or stop relying solely on
comparing to closure::BOUND_METHOD_FUNC_PTR (keep the pointer check insofar as
needed to obtain a closure pointer), and ensure the function returns false for
regular bound methods that have no native-module metadata.
- Around line 4350-4355: The early return inside the
identify_global_builtin_constructor branch prevents the later fetch-parent
bookkeeping from running; remove the premature return so that after calling
global_builtin_constructor_class_id(...) and register_class(class_id,
parent_cid) the function continues and reaches the
fetch_parent_kind_in_chain(...) / Request/Response registration logic;
specifically, keep the register_class(...) call but eliminate the `return` in
the branch that handles identify_global_builtin_constructor(...) so the rest of
the function's parent-kind handling executes for dynamic subclasses that
resolved their parent via the builtin path.
In `@crates/perry-runtime/src/object/global_this.rs`:
- Around line 480-491: The current pointer-tag branch incorrectly treats any
non-zero class id as "usable"; change it to test real callability: for
pointer-tagged values set usable = true if crate::closure::is_closure_ptr(p) ||
crate::object::is_callable(p as *const crate::object::ObjectHeader) (or the
equivalent runtime helper that checks an object's callable flag), and if that
predicate is false call parent_closure_in_chain(bits) to decide usability; do
not rely on js_object_get_class_id(...) != 0 as the callable check so we don't
dispatch js_native_call_value on non-callables.
In `@crates/perry-runtime/src/object/native_module.rs`:
- Around line 4592-4608: Module.prototype.require is currently wired to
bound_native_callable_export_value("module","_load") which dispatches as a 3-arg
bound method and yields _load(request, undefined, undefined); replace this with
a 1-arg thunk that forwards the call as _load(request, this, false). Concretely:
allocate a JS function object with arity 1 (instead of using
bound_native_callable_export_value), implement the thunk to invoke the native
export "_load" with three arguments (request, the current JS this/receiver, and
the boolean false), and then set that thunk as the "require" field on the proto
(the same proto created by js_object_alloc and set via native_set_field and
closure_set_dynamic_prop). Keep the existing prototype creation
(js_object_alloc, native_set_field, closure_set_dynamic_prop) but replace the
bound_native_callable_export_value("module","_load") usage with the new 1-arg
wrapper thunk that forwards (request, this, false).
In `@crates/perry/src/commands/compile/cjs_wrap/wrap.rs`:
- Around line 224-292: The generated try-guard is applied per-specifier instead
of per-call-site, causing throws on all calls for a spec even if only one call
is inside a try; fix by switching from the boolean require_site_in_try(source,
spec) to per-call-site metadata: locate all occurrences of require(...) for each
spec (reuse the extraction that produced require_specs/import_local_names or
change require_site_in_try to return a Vec of offsets/flags), produce a list of
tuples like (local_name, is_in_try) for each occurrence, and then generate
require_cases that emits one if-branch per occurrence (using the specific local
and only inserting the throw sentinel when that occurrence's is_in_try is true).
Update require_site_in_try (or add a new function) to accept/return offsets so
the lexical try-detection is done per occurrence rather than once per spec, and
adjust the mapping between require_specs and import_local_names to preserve
ordering so each local is paired with its occurrence flag.
In `@crates/perry/src/commands/compile/collect_modules.rs`:
- Around line 1015-1027: The warning for unresolved Worker currently always
prints via eprintln in collect_modules.rs; wrap that eprintln so it only runs
when the configured output format is OutputFormat::Text (leaving the behavior of
pushing an empty Vec into worker_path_sets unchanged). Locate the block that
emits the eprintln referencing module_name and reason and guard the print with a
check against OutputFormat::Text (e.g. if output_format == OutputFormat::Text {
eprintln!(...) }), so JSON or other non-text output modes remain silent.
---
Outside diff comments:
In `@crates/perry/src/commands/compile/cjs_wrap/wrap.rs`:
- Around line 454-490: The emitted const aliases and strip ranges must exclude
aliases that come from deferred lazy_specs; modify the branch that builds
aliases (using extract_require_aliases_with_ranges and the subsequent filters
that produce aliases and ranges) to ignore any alias whose spec is present in
lazy_specs (i.e., add a filter like require_specs.iter().position(...) or a
direct check against lazy_specs to skip when spec is in lazy_specs) so you do
not emit "const alias = _lazyreq_N;" nor strip the original function-local
"const alias = require(...)" — apply the same exclusion when collecting ranges
(the .filter(...) chain that maps to range) so lazy-spec aliases are neither
hoisted nor have their original declarations removed; keep existing checks like
identifier_is_reassigned and import_local_names logic intact.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/native_module.rs`:
- Around line 2971-2982: Remove the duplicate "timers" match arm that returns
the hard-coded array (the block with b"setTimeout", b"clearTimeout", ...), and
rely on the existing earlier branch that returns TIMERS_NAMESPACE_KEYS; delete
this second arm to avoid drift and duplication and leave any surrounding match
structure intact (ensure no trailing commas or fall-through gaps remain after
removal).
In `@crates/perry-transform/src/inline/mod.rs`:
- Line 496: The synthetic-import test should assert the new is_deferred_require
flag to prevent regressions: update the synthetic-import test to include an
assertion that the imported synthetic's is_deferred_require field equals the
expected value (e.g., assert_eq!(item.is_deferred_require, false)); locate the
test that already checks adjacent import flags and add this single assertion
referencing is_deferred_require so the test fails if the flag changes
unexpectedly.
🪄 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: 60fefd9a-2a43-4600-9d09-789b2bc5524a
📒 Files selected for processing (55)
crates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/expr/dyn_extern_i18n.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/static_field_meta.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/lower_call/builtin.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/lower_call/property_get.rscrates/perry-codegen/src/lower_string_method.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-codegen/src/runtime_decls/strings_part2.rscrates/perry-hir/src/analysis/uses_this.rscrates/perry-hir/src/ir/decl.rscrates/perry-hir/src/ir/expr.rscrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/expr_call/mod.rscrates/perry-hir/src/lower/lower_expr.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower/module_decl.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-hir/src/lower_decl/class_captures.rscrates/perry-hir/src/lower_decl/class_decl.rscrates/perry-hir/src/lower_decl/class_members.rscrates/perry-hir/src/stable_hash/expr.rscrates/perry-hir/src/stable_hash/module.rscrates/perry-hir/src/stable_hash/tests.rscrates/perry-hir/src/walker/expr_mut.rscrates/perry-hir/src/walker/expr_ref.rscrates/perry-parser/src/lib.rscrates/perry-runtime/src/event_target.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/node_submodules/mod.rscrates/perry-runtime/src/object/class_constructors.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/reflect_support.rscrates/perry-runtime/src/proxy.rscrates/perry-transform/src/inline/mod.rscrates/perry/src/commands/compile.rscrates/perry/src/commands/compile/bootstrap.rscrates/perry/src/commands/compile/cjs_wrap/detect.rscrates/perry/src/commands/compile/cjs_wrap/extract_exports.rscrates/perry/src/commands/compile/cjs_wrap/extract_requires.rscrates/perry/src/commands/compile/cjs_wrap/hoist_classes.rscrates/perry/src/commands/compile/cjs_wrap/mod.rscrates/perry/src/commands/compile/cjs_wrap/wrap.rscrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/init_order.rscrates/perry/src/commands/compile/resolve.rscrates/perry/src/commands/compile/resolve/tests.rs
Comment on lines
+131
to
+181
| Expr::RegisterClassCaptures { | ||
| class_name, | ||
| captures, | ||
| } => { | ||
| let mut lowered: Vec<String> = Vec::with_capacity(captures.len()); | ||
| for c in captures { | ||
| lowered.push(lower_expr(ctx, c)?); | ||
| } | ||
| if let Some(&class_id) = ctx.class_ids.get(class_name) { | ||
| if class_id != 0 && !lowered.is_empty() { | ||
| let n = lowered.len(); | ||
| let buf = ctx.func.alloca_entry_array(DOUBLE, n); | ||
| for (i, v) in lowered.iter().enumerate() { | ||
| let slot = | ||
| ctx.block() | ||
| .gep(DOUBLE, &buf, &[(crate::types::I64, &i.to_string())]); | ||
| ctx.block().store(DOUBLE, v, &slot); | ||
| } | ||
| let ptr_reg = ctx.block().next_reg(); | ||
| ctx.block().emit_raw(format!( | ||
| "{} = getelementptr [{} x double], ptr {}, i64 0, i64 0", | ||
| ptr_reg, n, buf | ||
| )); | ||
| let cid_str = class_id.to_string(); | ||
| let len_str = n.to_string(); | ||
| ctx.block().call_void( | ||
| "js_class_register_capture_values", | ||
| &[ | ||
| (crate::types::I32, &cid_str), | ||
| (crate::types::PTR, &ptr_reg), | ||
| (crate::types::I64, &len_str), | ||
| ], | ||
| ); | ||
| } | ||
| } | ||
| Ok(double_literal(f64::from_bits(0x7FFC_0000_0000_0001))) | ||
| } | ||
| // Read slot `index` of the class's decl-site capture snapshot — | ||
| // STATIC method prologue rebinds (no instance to carry the | ||
| // `__perry_cap_*` fields). | ||
| Expr::ClassCaptureValue { class_name, index } => { | ||
| if let Some(&class_id) = ctx.class_ids.get(class_name) { | ||
| let cid_str = class_id.to_string(); | ||
| let idx_str = index.to_string(); | ||
| return Ok(ctx.block().call( | ||
| DOUBLE, | ||
| "js_class_capture_value", | ||
| &[(crate::types::I32, &cid_str), (crate::types::I32, &idx_str)], | ||
| )); | ||
| } | ||
| Ok(double_literal(f64::from_bits(0x7FFC_0000_0000_0001))) |
There was a problem hiding this comment.
Class-capture snapshots are keyed too coarsely.
RegisterClassCaptures writes the snapshot into CLASS_CAPTURE_VALUES[class_id], and ClassCaptureValue later reads it back with the same compile-time class_id. That means the second evaluation of a nested class overwrites the first one, so previously returned/exported class values start constructing or rebinding statics against the newest closed-over environment. A shape like const A = make(1); const B = make(2); A.staticGet() will read 2 here. This needs to be keyed off the concrete class value / fresh class object, not the shared template id.
🤖 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-codegen/src/expr/static_field_meta.rs` around lines 131 - 181,
The bug is that RegisterClassCaptures and ClassCaptureValue use the compile-time
ctx.class_ids(class_name) as the snapshot key, causing different class instances
to collide; fix by keying snapshots on the concrete class object pointer/value
at runtime instead of class_id: in Expr::RegisterClassCaptures change the
js_class_register_capture_values call to take a PTR key (pass the class object
register/pointer for class_name rather than the I32 class_id) and adapt the
argument types from I32 to PTR; in Expr::ClassCaptureValue change the
js_class_capture_value call to take the same PTR key (pass the same class object
register/pointer and use I32 only for the index), and remove/stop relying on
ctx.class_ids for lookup (instead obtain the runtime class pointer/register for
class_name—e.g., look up whatever place holds the freshly created class value or
load it at runtime—and use that register as the first arg). Ensure the two
helpers js_class_register_capture_values and js_class_capture_value now use a
PTR key consistently so snapshots are keyed by the concrete class object.
Comment on lines
+374
to
+394
| /// Snapshot the CURRENT values of a function-nested class's captured | ||
| /// outer-scope locals into the runtime `CLASS_CAPTURE_VALUES` table. | ||
| /// Emitted at the source-order position of the class declaration | ||
| /// (parallel to `RegisterClassParentDynamic`), so dynamic construction | ||
| /// of the class VALUE (`exports.C = C; … new mod.C()` — the webpack / | ||
| /// zod bundle pattern) can fill the synthesized `__perry_cap_<id>` | ||
| /// constructor params. Static `new C()` sites keep passing captures as | ||
| /// trailing args and don't consult the table. | ||
| RegisterClassCaptures { | ||
| class_name: String, | ||
| captures: Vec<Expr>, | ||
| }, | ||
|
|
||
| /// Read slot `index` of a class's decl-site capture snapshot | ||
| /// (`CLASS_CAPTURE_VALUES`, written by `RegisterClassCaptures`). Used by | ||
| /// STATIC method bodies of function-nested capturing classes — statics | ||
| /// have no instance to carry `__perry_cap_*` fields, so their prologue | ||
| /// rebinds read the snapshot instead (vendored zod's | ||
| /// `static create(...) { … typeName: k.ZodRecord … }` where `k` is an | ||
| /// enclosing-function local). | ||
| ClassCaptureValue { class_name: String, index: u32 }, |
There was a problem hiding this comment.
Capture snapshots need a per-evaluation key, not just class_name.
RegisterClassCaptures and ClassCaptureValue address the snapshot solely by class name, so two executions of the same function-local class declaration will overwrite each other’s captured environment. That breaks factory patterns where multiple returned class values or later static calls are supposed to retain the capture state from the invocation that created them.
🤖 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/ir/expr.rs` around lines 374 - 394, The snapshot key
must include a per-evaluation identifier, not just class_name: change the IR
variants RegisterClassCaptures and ClassCaptureValue to carry an evaluation key
(e.g., eval_key: String or u64) and make CLASS_CAPTURE_VALUES use a composite
key (class_name, eval_key) when writing/reading; update all emitters/consumers
that create RegisterClassCaptures to generate and pass the eval_key at the
class-declaration evaluation site and update places that construct
ClassCaptureValue to pass the matching eval_key so reads target the correct
per-invocation snapshot.
Comment on lines
+240
to
+296
| // Re-register capture snapshots for classes declared in this body at | ||
| // its END. The decl-site `RegisterClassCaptures` runs before later | ||
| // statements assign captured vars (tsc emits TS-enum namespaces AFTER | ||
| // the classes that reference them — vendored zod's | ||
| // ZodFirstPartyTypeKind), so static-method snapshot reads and post- | ||
| // return dynamic constructions need the FINAL values. Inserted before | ||
| // a trailing `return` when present; bodies with early returns keep the | ||
| // decl-site snapshot for those paths. | ||
| { | ||
| let mut re_regs: Vec<Stmt> = Vec::new(); | ||
| for stmt in &block.stmts { | ||
| if let ast::Stmt::Decl(ast::Decl::Class(class_decl)) = stmt { | ||
| let cname = class_decl.ident.sym.to_string(); | ||
| if let Some(captured) = ctx.lookup_class_captures(&cname) { | ||
| if !captured.is_empty() { | ||
| let captures: Vec<Expr> = | ||
| captured.iter().map(|id| Expr::LocalGet(*id)).collect(); | ||
| // Sibling code lowered BEFORE this class registered | ||
| // its captures (forward refs — zod's | ||
| // `function createZodEnum(...) { return new | ||
| // ZodEnum({...}) }` declared above the class) has | ||
| // `new <class>(…)` sites with NO cap args appended; | ||
| // the inline binder then misfills the ctor params. | ||
| // Append the raw outer ids now; sites lowered after | ||
| // registration already end with exactly these ids | ||
| // and are skipped (tail-match guard). Class members | ||
| // were handled by `append_self_sites` with remapped | ||
| // ids — their tails don't match the raw ids, but | ||
| // they ALREADY carry appends; restrict this pass to | ||
| // non-member code by walking the lowered body only | ||
| // (member bodies live in pending_classes, not here). | ||
| let cap_args: Vec<(perry_types::LocalId, perry_types::LocalId)> = | ||
| captured.iter().map(|id| (*id, *id)).collect(); | ||
| for s in body.iter_mut() { | ||
| super::class_captures::append_new_args_stmt( | ||
| s, &cname, &cap_args, true, | ||
| ); | ||
| } | ||
| re_regs.push(Stmt::Expr(Expr::RegisterClassCaptures { | ||
| class_name: cname, | ||
| captures, | ||
| })); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if !re_regs.is_empty() { | ||
| let insert_at = if matches!(body.last(), Some(Stmt::Return(_))) { | ||
| body.len() - 1 | ||
| } else { | ||
| body.len() | ||
| }; | ||
| for (i, s) in re_regs.into_iter().enumerate() { | ||
| body.insert(insert_at + i, s); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
End-of-body capture refresh misses earlier escape paths.
This refresh only runs on the outer function-body fallthrough path, so any earlier return that exposes the class value keeps the stale decl-site snapshot. The same problem applies to nested scoped blocks, because their capture state is never refreshed at block exit. Static capture reads and dynamic new mod.C() sites can still observe pre-mutation values on those paths.
🤖 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/block.rs` around lines 240 - 296, The
end-of-body re-registration currently only inserts the RegisterClassCaptures
entries at the fallthrough end (insert_at) and misses earlier return/early-exit
paths and nested block exits; update the logic that builds and inserts re_regs
so that after computing re_regs (same way using ctx.lookup_class_captures,
captured -> captures, cap_args and append_new_args_stmt), you also walk the body
and for every Return statement insert a clone of each re_reg immediately before
it, and for any nested Stmt::Block recurse into that block's .stmts and perform
the same insertion (insert before any Return in that block and at the block's
end if it falls through), rather than only inserting at the final insert_at; use
RegisterClassCaptures, re_regs, append_new_args_stmt, and body names to locate
and update the code.
Comment on lines
+4592
to
+4608
| // `Module.prototype` — Node's require-hook pattern (Next.js): | ||
| // `const mod = require('module'); const orig = mod.prototype.require; | ||
| // mod.prototype.require = function(request) {…}`. Expose a plain object | ||
| // carrying a `require` method so the read+patch round-trips; the patch | ||
| // is inert under AOT compilation (Perry resolves modules at compile | ||
| // time), but startup must not throw on the access. | ||
| let proto = js_object_alloc(0, 1); | ||
| native_set_field( | ||
| proto, | ||
| "require", | ||
| bound_native_callable_export_value("module", "_load"), | ||
| ); | ||
| crate::closure::closure_set_dynamic_prop( | ||
| closure_addr, | ||
| "prototype", | ||
| crate::value::js_nanbox_pointer(proto as i64), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/perry-runtime/src/object/native_module.rs"
echo "== Context around target lines =="
sed -n '4560,4635p' "$FILE" | cat -n
echo
echo "== Find where _load or 'module', '_load' is defined/used =="
rg -n "(_load|bound_native_callable_export_value\\(\"module\", \"_load\"|\"prototype\"\\s*,\\s*crate::value::js_nanbox_pointer\\(proto as i64\\))" "$FILE"
echo
echo "== Search for Module / require patching behavior in this file =="
rg -n "Module\\.prototype|prototype.*require|require-hook|orig\\.apply|require\\(\"|native_set_field\\(.*require" "$FILE"
echo
echo "== Check for any wrapper/thunk patterns near closure_set_dynamic_prop or native callable exports =="
rg -n "closure_set_dynamic_prop\\(|bound_native_callable_export_value\\(" "$FILE" | head -n 80Repository: PerryTS/perry
Length of output: 9935
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/perry-runtime/src/object/native_module.rs"
echo "== bound_native_callable_export_value implementation =="
sed -n '3320,3485p' "$FILE" | cat -n
echo
echo "== arity mapping for (module, _load) =="
sed -n '3840,3925p' "$FILE" | cat -n
echo
echo "== module property binding cases mentioning _load =="
sed -n '4960,5070p' "$FILE" | cat -n
echo
echo "== other _load-related cases =="
rg -n "\"_load\"|\\(\"module\", \"_load\"\\)" "$FILE" | head -n 50Repository: PerryTS/perry
Length of output: 18618
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/perry-runtime/src/object/native_module.rs"
echo "== locate BOUND_METHOD_FUNC_PTR + js_native_call_method =="
rg -n "BOUND_METHOD_FUNC_PTR|js_native_call_method\\b" "$FILE"Repository: PerryTS/perry
Length of output: 710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/perry-runtime/src/object/native_module.rs"
echo "== show around js_native_call_method usage at ~4167 =="
sed -n '4100,4225p' "$FILE" | cat -n
echo
echo "== show around dispatch_bound_method mention at ~5948 =="
sed -n '5880,6170p' "$FILE" | cat -nRepository: PerryTS/perry
Length of output: 21560
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find definition of js_native_call_method
rg -n "fn js_native_call_method\\b" crates/perry-runtime/src | head -n 20
# Inspect dispatch_bound_method implementation
rg -n "dispatch_bound_method" crates/perry-runtime/src/closure.rs
sed -n '5860,6160p' crates/perry-runtime/src/closure.rs | cat -n
# Inspect js_native_call_method implementation (exact file/lines)
# If multiple matches, show near each.
rg -n "js_native_call_method\\(" crates/perry-runtime/src | head -n 50Repository: PerryTS/perry
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "dispatch_bound_method" crates/perry-runtime/src | head -n 50
echo
echo "== show js_native_call_method implementation around native_call_method.rs:1422 =="
sed -n '1400,1525p' crates/perry-runtime/src/object/native_call_method.rs | cat -n
echo
echo "== find where BOUND_METHOD_FUNC_PTR closures are dispatched =="
rg -n "BOUND_METHOD_FUNC_PTR" crates/perry-runtime/src/object/native_call_method.rs crates/perry-runtime/src | head -n 50Repository: PerryTS/perry
Length of output: 13147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/perry-runtime/src/closure/dispatch.rs"
echo "== dispatch_bound_method implementation =="
rg -n "fn dispatch_bound_method\\b" "$FILE"
start=$(rg -n "fn dispatch_bound_method\\b" "$FILE" | head -n1 | cut -d: -f1)
sed -n "$((start-20)),$((start+120))p" "$FILE" | cat -n
echo
echo "== dispatch_bound_method further down (receiver/args handling) =="
sed -n "$((start+120)),$((start+260))p" "$FILE" | cat -nRepository: PerryTS/perry
Length of output: 2034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/perry-runtime/src/closure/dispatch.rs"
line_start=$(rg -n "fn dispatch_bound_method\\b" "$FILE" | head -n1 | cut -d: -f1)
from=$(( line_start > 20 ? line_start-20 : 1 ))
to=$(( line_start+220 ))
echo "== dispatch_bound_method excerpt: lines $from-$to =="
sed -n "${from},${to}p" "$FILE" | cat -nRepository: PerryTS/perry
Length of output: 12638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "Module\\.prototype\\.require|prototype\\.require\\b|\\.prototype\\s*\\.\\s*require\\b|\\brequire\\b\\s*:\\s*bound_native_callable_export_value\\(\"module\", \"_load\"\\)|\"require\"\\s*,\\s*bound_native_callable_export_value\\(\"module\", \"_load\"\\)" crates/perry-runtime/src | head -n 50
echo
rg -n "\\bModule\\b.*_load\\b|_load\\(request|require\\(request" crates/perry-runtime/src/object/native_module.rs | head -n 100
echo
# See if any code patches require elsewhere
rg -n "attach_module_cjs_constructor_statics\\(|_load\"\\)|\"require\"" crates/perry-runtime/src/object/native_module.rs | head -n 120Repository: PerryTS/perry
Length of output: 318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE1="crates/perry-runtime/src/object/native_module.rs"
FILE2="crates/perry-runtime/src/object/class_registry.rs"
echo "== find native_callable_export_arity definition =="
rg -n "fn native_callable_export_arity\\b" "$FILE1"
line=$(rg -n "fn native_callable_export_arity\\b" "$FILE1" | head -n1 | cut -d: -f1)
from=$(( line>40 ? line-40 : 1 ))
to=$(( line+120 ))
sed -n "${from},${to}p" "$FILE1" | cat -n
echo
echo "== find call_vtable_method definition =="
rg -n "fn call_vtable_method\\b" "$FILE2"
line2=$(rg -n "fn call_vtable_method\\b" "$FILE2" | head -n1 | cut -d: -f1)
from2=$(( line2>60 ? line2-60 : 1 ))
to2=$(( line2+160 ))
sed -n "${from2},${to2}p" "$FILE2" | cat -nRepository: PerryTS/perry
Length of output: 18759
Fix Module.prototype.require wrapper (currently wired to _load).
Module.prototype.require is set to bound_native_callable_export_value("module", "_load"), which has arity 3 (("module","_load") => Some(3)) and is dispatched via the bound-method path using the captured module namespace as the receiver. When require(request) is called, the callee receives _load(request, undefined, undefined) instead of Node’s _load(request, this, false), so hooks that rely on this (e.g. orig.apply(this, arguments)) can resolve against the wrong parent module. Expose a dedicated 1-arg require thunk that forwards _load(request, this, false) instead.
🤖 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_module.rs` around lines 4592 - 4608,
Module.prototype.require is currently wired to
bound_native_callable_export_value("module","_load") which dispatches as a 3-arg
bound method and yields _load(request, undefined, undefined); replace this with
a 1-arg thunk that forwards the call as _load(request, this, false). Concretely:
allocate a JS function object with arity 1 (instead of using
bound_native_callable_export_value), implement the thunk to invoke the native
export "_load" with three arguments (request, the current JS this/receiver, and
the boolean false), and then set that thunk as the "require" field on the proto
(the same proto created by js_object_alloc and set via native_set_field and
closure_set_dynamic_prop). Keep the existing prototype creation
(js_object_alloc, native_set_field, closure_set_dynamic_prop) but replace the
bound_native_callable_export_value("module","_load") usage with the new 1-arg
wrapper thunk that forwards (request, this, false).
Comment on lines
224
to
+292
| let require_cases = require_specs | ||
| .iter() | ||
| .zip(import_local_names.iter()) | ||
| .map(|(spec, local)| format!(" if (specifier === '{}') return {};", spec, local)) | ||
| .map(|(spec, local)| { | ||
| if require_site_in_try(source, spec) { | ||
| format!( | ||
| " if (specifier === '{spec}') {{ if (typeof {local} === 'boolean') \ | ||
| throw __perry_cjs_require_error('error', 'MODULE_NOT_FOUND', \ | ||
| \"Cannot find module '{spec}'\"); return {local}; }}" | ||
| ) | ||
| } else { | ||
| format!(" if (specifier === '{}') return {};", spec, local) | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join("\n"); | ||
| // Heuristic: is any `require('<spec>')` call site lexically inside a | ||
| // `try { … }` block? Reverse brace-depth scan from the call offset to | ||
| // the nearest unmatched `{`, checking whether `try` precedes it. | ||
| // String/comment contexts are not stripped — a false positive only | ||
| // turns the silent sentinel into a (more Node-faithful) throw. | ||
| fn require_site_in_try(source: &str, spec: &str) -> bool { | ||
| let needle_sq = format!("require('{}')", spec); | ||
| let needle_dq = format!("require(\"{}\")", spec); | ||
| let bytes = source.as_bytes(); | ||
| let mut search = 0usize; | ||
| loop { | ||
| let hit = source[search..] | ||
| .find(&needle_sq) | ||
| .or_else(|| source[search..].find(&needle_dq)); | ||
| let Some(rel) = hit else { return false }; | ||
| let at = search + rel; | ||
| // Walk backwards to the nearest unmatched `{`, repeatedly: each | ||
| // enclosing block is checked for a preceding `try`. | ||
| let mut depth = 0i32; | ||
| let mut i = at; | ||
| while i > 0 { | ||
| i -= 1; | ||
| match bytes[i] { | ||
| b'}' => depth += 1, | ||
| b'{' => { | ||
| if depth > 0 { | ||
| depth -= 1; | ||
| } else { | ||
| // Enclosing block opener — does `try` precede it? | ||
| let mut j = i; | ||
| while j > 0 | ||
| && (bytes[j - 1] == b' ' | ||
| || bytes[j - 1] == b'\t' | ||
| || bytes[j - 1] == b'\r' | ||
| || bytes[j - 1] == b'\n') | ||
| { | ||
| j -= 1; | ||
| } | ||
| if j >= 3 | ||
| && &bytes[j - 3..j] == b"try" | ||
| && (j == 3 || !bytes[j - 4].is_ascii_alphanumeric()) | ||
| { | ||
| return true; | ||
| } | ||
| // Keep walking outward (this block wasn't a try). | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| search = at + 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new try guard changes behavior for every call site of a specifier.
Once require_site_in_try(source, spec) returns true, the generated if (specifier === '{spec}') branch throws for all require('{spec}') calls, not just the one inside try. A module that has both const x = require('foo') and try { require("foo") } catch { ... } will now throw on the non-try path too, and the raw string search here can still miss spaced/mixed-quote call sites because it does not reuse the extracted require ranges. This needs per-call-site metadata rather than a single boolean per specifier.
🤖 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/src/commands/compile/cjs_wrap/wrap.rs` around lines 224 - 292,
The generated try-guard is applied per-specifier instead of per-call-site,
causing throws on all calls for a spec even if only one call is inside a try;
fix by switching from the boolean require_site_in_try(source, spec) to
per-call-site metadata: locate all occurrences of require(...) for each spec
(reuse the extraction that produced require_specs/import_local_names or change
require_site_in_try to return a Vec of offsets/flags), produce a list of tuples
like (local_name, is_in_try) for each occurrence, and then generate
require_cases that emits one if-branch per occurrence (using the specific local
and only inserting the throw sentinel when that occurrence's is_in_try is true).
Update require_site_in_try (or add a new function) to accept/return offsets so
the lexical try-detection is done per occurrence rather than once per spec, and
adjust the mapping between require_specs and import_local_names to preserve
ordering so each local is paired with its occurrence flag.
…tion
Compiling a real Next.js 16.2.9 app (output:'standalone', whole framework
in perry.compilePackages) surfaced 14 independent Perry bugs. Each was
minimized and fixed; the binary now compiles, links (123MB), and boots
through Next's environment-extension init chain.
Compile-time:
1. perry-parser looks_like_es_module: track regex literals — picomatch's
/(^[*!]|[/()[\]{}"])/ desynced the string state, masking the wrap's
trailing `export default _cjs` (parsed as Script → ImportExportInScript).
2. collect_modules/codegen: unresolvable worker_threads Worker filename is
a warning + runtime throw (js_throw_error_with_code), not a compile
error — Next's build-time worker pools never run in production.
3. cjs_wrap detect: empty/whitespace-only file is CJS (react's client-only
0-byte index.js needs the wrap's `export default {}`).
4. cjs_wrap detect: ALL token scans run on comment/string/regex/
nested-template-stripped source. Four real-world defeats: "import " in
a header comment; ESM `import` lines generated inside (nested) template
literals (next/dist/build/utils.js writes server.js via
`${m ? `import …` : `…`}`); regex-with-quote masking a trailing
module.exports (comment-json).
5. cjs_wrap hoist_classes: hoist column-0 classes only — an indented class
is nested inside a function; hoisting severed its closure over
enclosing locals (ws event classes: `this[f]` → ReferenceError).
6. resolve: exports-map resolution walks ALL condition branches and takes
the first target that exists on disk — Next's file tracing prunes
@swc/helpers' esm/ (the `import` condition target) while cjs/ exists.
Codegen:
7. lower_call/builtin #602 flip: ambiguous DB ctors (Client/Pool/Database/
DatabaseSync/Session/StatementSync/Redis/MongoClient/Decimal) fire only
on POSITIVE import-source evidence. None is a Node global; undici's
`var Client = class …` inside next/dist/compiled/@edge-runtime hit pg's
arm and emitted an undefined js_pg_client_new link reference.
8. property_get: `.normalize` forces the String path only at ≤1 arg —
Next's route normalizers define 2-arg normalize(pathname, matched).
lower_string_method: surplus normalize args evaluated then ignored
(spec) instead of a codegen error.
9. dyn_extern_i18n: a default-import alias of a Node builtin module used
as a VALUE materializes the native namespace instead of TAG_TRUE.
Runtime:
10. Event subclassing (ws CloseEvent/ErrorEvent/MessageEvent):
js_event_subclass_init applies Event fields to the subclass `this`;
js_register_class_parent_dynamic resolves globalThis builtin ctor
parents through the shared global_builtin_constructor_class_id table
(extracted from the dynamic instanceof path) and registers the chain
edge; is_event_instance walks the parent chain; SuperCall arm in
this_super_call.rs.
11. class_registry: `extends <bound native export>` (stream.Writable,
EventEmitter as BOUND_METHOD closures) keeps the parentless baseline
instead of throwing "not a constructor".
12. reflect_support obj_value_has_own_key: native-module namespaces expose
virtual keys — console.error's first defineProperty patch no longer
records {configurable:false} ("new property" defaults), so Next's
second/third console patches stop throwing.
13. node:timers is a real module value: submodule default object
(was TAG_TRUE sentinel), namespace key table for member reads, and a
GC-scanned native-namespace property override store so
`require('node:timers').setImmediate = patched` round-trips
(write via ordinary_set_with_receiver arm, read via vt_get_own_field).
14. field_get_set: `fn.toString` value read reifies like bind/call/apply
(Next preserves patched-fn toString via original.toString.bind);
native_module: Module.prototype{require} exists for require-hook's
`mod.prototype.require` read+patch.
Tests: unit tests added for parser regex/division, cjs detect (empty,
comment-import, nested-template, regex-mask), exports candidates.
Targeted suites green (perry-parser/hir/codegen/perry all green;
perry-runtime 1025/1027 — date full_year is the known pre-existing
macOS-local failure; url path_to_file_url + builtin_prototype_reject_new
are sporadic thread-interaction flakes, pass in isolation and reproduce
on clean main variations).
Behavior-changing notes for review: (7) requires import evidence where
the pre-#602 fallback fired on bare names; (4) is_commonjs now scans
stripped source — files whose only CJS/ESM markers lived in comments or
strings change classification (toward correctness).
Code-only PR: no version bump / changelog per maintainer-fold convention.
Validation harness (app, Node baseline, verify script):
/tmp/perry-nextjs-demo — Node baseline 21/21 green; Perry binary boots to
next/dist/compiled/zod init (errToObj, #838 family) — follow-up.
…red zod
Five related fixes for classes declared inside functions that capture
enclosing locals (the webpack/ncc bundle pattern; Next.js's vendored zod
was the reproducer — `z.string().min(2)` chains):
1. Decl-site capture snapshot for DYNAMIC construction: new
Expr::RegisterClassCaptures (emitted at the class's source-order decl
position, parallel to RegisterClassParentDynamic) snapshots the current
capture values into the runtime CLASS_CAPTURE_VALUES table
(GC-scanned); replay_registered_class_constructor fills the synthesized
__perry_cap_* ctor params from it, so `exports.C = C; new mod.C()`
constructs working instances. Static `new C()` sites keep passing
captures inline.
2. Tail-aligned capture binding: inline_constructor_param_values binds
__perry_cap_* params from the args TAIL and user params from the head.
The synthesized no-user-ctor capturing class has zero user params while
`new C({...})` passes user args — positional binding put the user arg
into the capture slot.
3. Super-inline capture forwarding: a capturing PARENT's ctor has trailing
cap params the user-written `super(...)` args don't include; the
super-inline site now appends the CHILD ctor's same-named cap param
values (the capture union guarantees they're in scope).
4. Spec default-ctor arg forwarding: the synthesized no-user-ctor for a
derived capturing class called `super()` with NO args, dropping the
construction-site user args (ZodType's `this._def = def` read
undefined). It now forwards explicit params matching the closest
pending-ancestor ctor's user arity.
5. Self-construction sites: `new <Self>(…)` inside the class's OWN method
bodies (zod's `_addCheck`) lowered before the class registered its
captures, so nothing was appended. After the method-prologue capture
rebind, those sites now get the rebind ids appended (nested closures
included, with capture-list patch-up).
Known cosmetic follow-up: __perry_cap_* fields are enumerable own
properties (visible in JSON.stringify of instances).
Repros validated: 20 standalone shapes + the real
next/dist/compiled/zod/index.cjs bundle (z.string().min(2).parse).
…d captures, super(...spread)
Continuing the vendored-zod (Next.js segment-config) chain:
1. Forward sibling-class references: a method body referencing a class
declared LATER in the same function body (`ZodType.optional()` →
`ZodOptional.create(...)`) lowered the Ident to the unknown-global
sentinel; `.create(a,b)` then dispatched into Object.create and threw
"Property description must be an object". New scoped
`forward_class_names` pre-pass in lower_fn_body_block_stmt; Ident
lowering resolves the name to ClassRef.
2. STATIC method captures: static bodies referencing enclosing-fn locals
(`static create(...) { typeName: k.ZodRecord }`) had no capture
machinery — prologue rebinds now read the decl-site snapshot via new
Expr::ClassCaptureValue → js_class_capture_value(class_id, index).
Snapshot is additionally RE-registered at the END of the enclosing
function body (tsc emits TS-enum namespaces after the classes that
reference them, so the decl-site values were still undefined).
3. `super(...spread)` (`constructor(){ super(...arguments) }` — tsc
pass-through ctor, zod ZodNumber/ZodBigInt): new Expr::SuperCallSpread
materializes the args array (js_array_push_spread_any also spreads the
arguments OBJECT, an array-like) and invokes the closest registered
ancestor ctor on the same `this` via js_super_construct_apply
(CLASS_CONSTRUCTORS walk + decl-site cap fill). Recognized by the
derived-ctor TDZ checks and the cap-store insert-after-super positions.
KNOWN GAP (documented in-code): super(...spread) constructions reached
through METHOD bodies (the standalone-ctor path) lose the parent's field
writes — z.number().int() chains still fail; z.string()/enum/record/
union/object/literal chains and the zod entry module all pass. An inline
variant was tried and reverted (segfault on rest-param forwarding).
Repros: /tmp/zodrepro pieces + t_sp*/t_ns* shapes (20+).
The recursion-guarded standalone-ctor call path
(call_local_constructor_symbol — `new Kid(...)` issued inside a method of
Kid) passed lowered args RAW/positionally, but a rest param or the
synthesized `arguments` param expects the caller to pack an array (the
inline path's inline_constructor_param_values rules). The user arg landed
raw in the rest slot, `super(...arguments)` spread an object with no
.length, and the parent ctor ran with zero args — second-generation zod
instances (`_addCheck` → `new ZodNumber({…})`) lost `_def`.
Reuse inline_constructor_param_values with the effective ctor's params
(own, else closest ancestor's — matching the adopted standalone-symbol
signature). Cross-module ctors (arity only) keep prior behavior.
Also reverts the SuperCallSpread inline-parent experiment to the runtime
registry apply (the experiment segfaulted on rest-param forwarding; with
this packing fix the registry path is correct in all contexts — verified
across 14 t_sp*/t_ns* shapes incl. the previously-failing in-method and
rest-param ones, plus zod number/string/enum/record/union chains).
…tration
Sibling code lowered BEFORE a function-nested capturing class registers
its captures — a hoisted function declared above the class (zod's
`function createZodEnum(values) { return new ZodEnum({...}) }`) — emitted
`new <class>(…)` with NO capture args; the inline binder then misfilled
the ctor params (first capture slot ate the user arg) and instances lost
both `_def` and their capture fields ("reading 'getParsedType'/'OK' of
undefined" at parse time).
The function-body end hook (where capture snapshots are re-registered)
now walks the lowered body and appends the raw outer capture ids to those
sites, descending nested closures and patching their capture lists.
Sites lowered AFTER registration already end with exactly the append
sequence and are skipped via a tail-match guard. The member-body appender
(`append_self_sites`) is refactored onto the same shared
`append_new_args_stmt/expr` helpers.
Validates: z.enum/optional/object.strict safeParse chains, the full
Next.js segment-config schema (seg.js), and the 15-repro t_sp*/t_ns*
regression suite.
…OT_FOUND
The wrap adopted EVERY static require by hoisting an import — including
unresolvable ones (`require('@opentelemetry/api')` where only Next's
vendored copy exists). The hoisted binding lowers to the boolean TRUE
sentinel at runtime, and the shim returned it, defeating the ubiquitous
try/require-fallback pattern: Node throws MODULE_NOT_FOUND and the catch
loads `next/dist/compiled/@opentelemetry/api`; the shim handed back
`true` and the catch never ran ("(boolean).createContextKey is not a
function" at Next.js boot).
Guard each adopted shim entry: a boolean binding means the module never
resolved — throw the spec ERR MODULE_NOT_FOUND instead, so the catch arm
loads the fallback. (Caveat documented: a real module default-exporting a
boolean would mis-trip; no such package shape observed.)
…op (p-queue)
next/dist/compiled/p-queue compile-validation wall. Two independent roots,
both in the webpack/ncc inner-module shape where a class is declared inside a
nested arrow `(e, t, n) => { class TimeoutError extends Error {…}; e.exports = …;
e.exports.TimeoutError = TimeoutError; }`.
1. extract_exports: the `exports.X = …` named-export regex matched
`e.exports.TimeoutError = …` (a property write on an INNER module's own
exports param). That false positive made the wrap emit
`export const TimeoutError = _cjs.TimeoutError;` at outer-module scope, which
shadowed the inner class binding so every inner reference read undefined.
Exclude `.` from the boundary class so only true `(module.)exports.X` matches.
2. js_fetch_or_value_super: `class PQ extends t {}` nested in a function lowers
its heritage Ident at class-DECL scope, but codegen re-emits the expression
inside the constructor where the captured slot is unrelated, so `parent_val`
arrives stale (undefined → "is not a constructor"). The decl-site
`js_register_class_parent_dynamic` DID see the live closure and recorded it in
CLASS_PARENT_CLOSURES; when `parent_val` isn't callable, walk the instance's
parent chain (new `parent_closure_in_chain`) and dispatch to the registered
parent closure with `this` bound so the parent body (`this._events = …`) runs.
3. replay_class_object_constructor: a class DECLARATION reached as a heap class
object (webpack `t["default"] = PQueue` read cross-module) carries no
per-evaluation `__perry_ctor_caps`; fall back to the decl-site
CLASS_CAPTURE_VALUES snapshot for the trailing `__perry_cap_*` ctor params,
mirroring the ClassRef replay path. Also a descriptive
"<value> is not a constructor" message.
p-queue repro chain (main/mini2/mini3/t_fnparent2) now matches Node; zod capture
suite + t_sp*/t_ns* regression set unchanged.
…ion bodies Wall #26 (Next.js standalone, next/dist/compiled/edge-runtime's `consumeUint8ArrayReadableStream`). A webpack/ncc inner module is a nested arrow `(e, A, t) => { A.gen = gen; ...; async function* gen(){} }` that exports a hoisted generator by forward-referencing it ABOVE the declaration. Perry's function-body hoisting pass (`lower_fn_body_block_stmt`) pre-defined and front-loaded regular/async FnDecls but EXCLUDED generators, which `lower_body_stmt` lowers to a top-level function plus a source-position `Let { init: FuncRef }`. So the forward reference saw an undefined local → `ReferenceError: gen is not defined` at eager module init. Spec function-declaration hoisting applies to generators too. Phase 1 now pre-defines generator FnDecl locals (so `lookup_local` resolves at the forward reference) and Phase 3 recognizes the `FuncRef`-init `Let` (not just `Closure`-init) as hoisted and moves it to the front. The FuncRef value is pure, so reordering it ahead of other statements is safe. Top-level generators were already fine; this only affects generators declared inside another function. Also fixes a stable-hash tag collision introduced with the wall-21 capture variants: `RegisterClassCaptures`/`ClassCaptureValue` had copy-pasted tags 12238/12239 (already used by `SuperPropertySet`/`ObjectSuperPropertySet`), failing `expr_variant_stable_hash_tags_are_unique`. Reassigned to 12241/12242. Repro (t_asyncgen_mod) now matches Node; yield-star / class-iterator / generator gap tests unchanged.
…cing it Wall #27 (Next.js standalone): `startServer` crashed with a stack-overflow SIGSEGV from an accessor getter recursing on itself (`exports.chainStreams` → `self.chainStreams` → …). Root cause is module resolution, not the getter. Next.js's app-render dir holds both `stream-ops.js` and `stream-ops.web.js`, and `stream-ops.js` does `require("./stream-ops.web")`. `resolve_with_extensions` tried `Path::with_extension("js")` first, which REPLACES the last dotted segment: `stream-ops.web` → `stream-ops.js` — the requiring module itself. Resolving the require to its own file made the module self-require, so its SWC `_export` re-export getter (`chainStreams: () => _streamopsweb.chainStreams`, where `_streamopsweb` now aliases the module's own exports) returned `exports.chainStreams`, i.e. the getter itself, and any read of it recursed until the stack overflowed. Node resolution APPENDS the extension to the full specifier (`./stream-ops.web` → `./stream-ops.web.js`) and never strips a non-module dotted segment. The loop now tries the append form first and only falls back to `with_extension` (replace) when the specifier already ends in a recognized module extension — that branch exists solely for Perry's TS-over-JS preference (`./foo.js` pruned but `./foo.ts` present) and must never swap a `.web`-style filename segment. Two regression tests cover both directions.
…dule init Wall #28 (Next.js standalone): the server booted past the earlier walls but crashed with `TypeError: Cannot read properties of undefined (reading 'd')` from `react-dom-server-legacy.node.production.js`, reading `ReactDOMSharedInternals.d`. `react-dom/index.js` selects its bundle with `if (process.env.NODE_ENV === 'production') module.exports = require('./cjs/react-dom.production.js') else module.exports = require('./cjs/react-dom.development.js')`. Next.js's standalone entry sets `process.env.NODE_ENV = 'production'` on its first line, before any `require`. In Node that assignment is observed by every later `require`d module. Perry hoists `require`s to eager imports that init BEFORE the entry body runs, so react-dom saw the unmodified env, took the `else` branch, and `require`d the development bundle — which Next.js's file tracing prunes from a standalone build. The require returned an empty module, so `ReactDOM.__DOM_INTERNALS_…` (hence `ReactDOMSharedInternals`) was undefined and `.d` threw. Fix: scan the entry module's unconditional top-level `process.env.<NAME> = "<literal>"` assignments (including one+ levels into the cjs-wrap IIFE where the wrapped entry's top-level statements live) and emit `js_setenv` for each in `main`, after the string pool init but before the module-init loop. This restores Node's observable ordering for the ubiquitous "set NODE_ENV then require" pattern without affecting runtime `process.env` writes (still lowered normally) or assignments nested in conditionals/functions (intentionally not hoisted, matching Node's lazy evaluation).
…Props) Wall #29 (Next.js standalone): boot crashed with `TypeError: Cannot assign to read only property 'name' of object '#<Object>'` from `next/dist/compiled/jsonwebtoken` (its bundled safe-buffer). safe-buffer does `function copyProps(e,r){for(var t in e){r[t]=e[t]}}` and calls `copyProps(Buffer, SafeBuffer)`. In Node `for (k in Buffer)` skips `name` and `length` (functions' intrinsic props are non-enumerable), so nothing is copied onto them. Perry stored a native constructor's `name` via `closure_set_dynamic_prop`, which defaults to ENUMERABLE — so `for (k in Buffer)` yielded "name", and `copyProps` assigned `SafeBuffer.name = "Buffer"`. SafeBuffer is a function whose own `name` is read-only, so the strict-mode write threw. (`getOwnPropertyDescriptor(Buffer,'name').enumerable` already reported false via the function-name special case, so enumeration disagreed with reflection.) Fix: `set_bound_native_closure_name` now pins the spec descriptor `{ writable:false, enumerable:false, configurable:true }` on `name`, so the for-in / enumerable-keys path skips it, matching Node and reflection.
…xt.js)
Perry eager-initialized every `require()`d module at program start, including
ones Node only loads when an enclosing function actually runs. Next.js's server
bundle `require()`s heavy dependencies inside request handlers (jsonwebtoken in
`api-resolver.js`/`try-get-preview-data.js`), so at boot Perry dragged their
whole module bodies — and Perry's gaps in them — into the startup path, while
Node never touches them until a request hits that code. This is what produced
the jsonwebtoken boot crashes (safe-buffer `for-in`/copyProps, native Stream as
`util.inherits` base).
Fix: a `require('S')` whose every call site is inside a function body is now
routed through the existing Deferred-module mechanism (the same compile-but-
lazy-init path dynamic `import()` uses). S still compiles and links; it just
isn't eager-initialized — its `__init` fires when the require shim's binding is
actually read, i.e. when the function calls `require('S')`, matching Node's
synchronous-require semantics (including circular-require partial exports via
the idempotent init guard).
Mechanism:
- `cjs_wrap`: `function_local_specs` (one masked-source scope-tracking pass)
classifies which require specifiers are purely function-local. The wrap skips
alias adoption for them (a function-scoped `const x = require()` is not a
module binding) and names their import binding `_lazyreq_N`.
- `collect_modules`: tags `Import.is_deferred_require` (new HIR field) for
`_lazyreq_`-named Perry-compiled imports.
- init classification: `classify_eager_modules`, `topo_sort_non_entry_modules`,
and per-module `module_init_deps` all skip `is_deferred_require` edges, so the
target stays `Deferred` unless some genuine top-level edge reaches it.
- codegen: reading a `_lazyreq_` binding emits an idempotent `<target>__init()`
before the cross-module value read.
Self-correcting by design: because the shim triggers init on the real
`require()` call, an over-eager classification never breaks correctness — it
only affects eager-init-loop membership. Verified end-to-end: a function-local
require of a side-effecting module no longer initializes at boot (matches Node)
yet still initializes, in order, when the function runs.
- class_registry: dynamic Request/Response subclasses resolved via the
global-builtin branch now record their fetch-parent kind (the early
return previously skipped the bookkeeping, so new X() lost the native
handle).
- class_registry: gate the extends-native-export exemption on the
native-module metadata (bound_native_callable_module_and_method) instead
of the raw BOUND_METHOD_FUNC_PTR sentinel, so reified
Function.prototype.{bind,call,apply} values no longer skip the
not-a-constructor TypeError.
- global_this: super-parent callability test now requires a closure or a
class object, not just a non-zero class id, so a stale non-callable slot
falls back to parent_closure_in_chain instead of dispatching on a
non-function.
- event_target: class X extends CustomEvent initializes as a CustomEvent
(constructor + detail), not always Event (new is_custom flag threaded
through js_event_subclass_init).
- collect_modules: gate the unresolved-Worker warning on OutputFormat::Text.
proggeramlug
force-pushed
the
feat/nextjs-compile-walls
branch
from
6ea192c to
3c5f8e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/perry-codegen/src/runtime_decls/strings.rs (1)
1131-1132: 💤 Low valueReturn type mismatch with runtime implementation.
The codegen declares
js_super_construct_applyasVOID, but the runtime implementation (crates/perry-runtime/src/object/class_constructors.rs) returnsf64. While practically harmless (the return is alwaysundefinedand discarded), aligning them would improve contract consistency.♻️ Suggested fix
- module.declare_function("js_super_construct_apply", VOID, &[I32, DOUBLE, DOUBLE]); + module.declare_function("js_super_construct_apply", DOUBLE, &[I32, DOUBLE, DOUBLE]);🤖 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-codegen/src/runtime_decls/strings.rs` around lines 1131 - 1132, The declared signature for js_super_construct_apply in strings.rs currently uses VOID but the runtime (class_constructors.rs) returns an f64; update the module.declare_function call for "js_super_construct_apply" (the line invoking module.declare_function("js_super_construct_apply", ...)) to use DOUBLE as the return type instead of VOID so the codegen signature matches the runtime implementation.
🤖 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/field_get_set.rs`:
- Around line 5151-5179: The wide-index lookup can be triggered for a
corrupt/oversized key array because the existing check "if key_count > 65536 {
return JSValue::undefined(); }" happens after the wide-index logic; move or
duplicate that guard to run before the block that compares key_count with
WIDE_KEY_INDEX_MIN_KEYS and calls wide_key_index_lookup so oversized key_count
values are rejected before any index build/lookup (i.e. add an early "if
key_count > 65536 { return JSValue::undefined(); }" immediately before the "if
key_count >= WIDE_KEY_INDEX_MIN_KEYS" check, keeping the rest of the
logic—wide_key_index_lookup, ACCESSORS_IN_USE handling,
js_object_get_field/overflow_get—unchanged).
In `@crates/perry-runtime/src/proxy.rs`:
- Around line 1162-1186: The code dereferences obj as *const
crate::object::ObjectHeader after extract_pointer without verifying it points to
a valid ObjectHeader; update the fast-path in the block that calls
extract_pointer so you first call crate::object::is_valid_obj_ptr(obj as *const
u8) and only then read unsafe { (*obj).class_id } and proceed to call
crate::object::get_module_name_from_namespace,
crate::object::native_namespace_prop_override_store, or
crate::object::set_buffer_pool_size; mirror the safety check pattern used
elsewhere (see the is_valid_obj_ptr usage near the other fast path) to avoid
undefined behavior when the pointer targets non-object heap values.
---
Nitpick comments:
In `@crates/perry-codegen/src/runtime_decls/strings.rs`:
- Around line 1131-1132: The declared signature for js_super_construct_apply in
strings.rs currently uses VOID but the runtime (class_constructors.rs) returns
an f64; update the module.declare_function call for "js_super_construct_apply"
(the line invoking module.declare_function("js_super_construct_apply", ...)) to
use DOUBLE as the return type instead of VOID so the codegen signature matches
the runtime implementation.
🪄 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: 8e81e87b-2c00-4e23-b087-f2d9f914faf8
📒 Files selected for processing (55)
crates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/expr/dyn_extern_i18n.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/static_field_meta.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/lower_call/builtin.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/lower_call/property_get.rscrates/perry-codegen/src/lower_string_method.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-codegen/src/runtime_decls/strings_part2.rscrates/perry-hir/src/analysis/uses_this.rscrates/perry-hir/src/ir/decl.rscrates/perry-hir/src/ir/expr.rscrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/expr_call/mod.rscrates/perry-hir/src/lower/lower_expr.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower/module_decl.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-hir/src/lower_decl/class_captures.rscrates/perry-hir/src/lower_decl/class_decl.rscrates/perry-hir/src/lower_decl/class_members.rscrates/perry-hir/src/stable_hash/expr.rscrates/perry-hir/src/stable_hash/module.rscrates/perry-hir/src/stable_hash/tests.rscrates/perry-hir/src/walker/expr_mut.rscrates/perry-hir/src/walker/expr_ref.rscrates/perry-parser/src/lib.rscrates/perry-runtime/src/event_target.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/node_submodules/mod.rscrates/perry-runtime/src/object/class_constructors.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/reflect_support.rscrates/perry-runtime/src/proxy.rscrates/perry-transform/src/inline/mod.rscrates/perry/src/commands/compile.rscrates/perry/src/commands/compile/bootstrap.rscrates/perry/src/commands/compile/cjs_wrap/detect.rscrates/perry/src/commands/compile/cjs_wrap/extract_exports.rscrates/perry/src/commands/compile/cjs_wrap/extract_requires.rscrates/perry/src/commands/compile/cjs_wrap/hoist_classes.rscrates/perry/src/commands/compile/cjs_wrap/mod.rscrates/perry/src/commands/compile/cjs_wrap/wrap.rscrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/init_order.rscrates/perry/src/commands/compile/resolve.rscrates/perry/src/commands/compile/resolve/tests.rs
✅ Files skipped from review due to trivial changes (4)
- crates/perry-hir/src/walker/expr_mut.rs
- crates/perry/src/commands/compile/bootstrap.rs
- crates/perry-hir/src/lower/context.rs
- crates/perry-hir/src/lower/module_decl.rs
🚧 Files skipped from review as they are similar to previous changes (43)
- crates/perry-hir/src/stable_hash/tests.rs
- crates/perry-hir/src/analysis/uses_this.rs
- crates/perry-runtime/src/node_submodules/mod.rs
- crates/perry-codegen/src/lower_string_method.rs
- crates/perry-hir/src/ir/decl.rs
- crates/perry-runtime/src/object/instanceof.rs
- crates/perry/src/commands/compile/cjs_wrap/extract_exports.rs
- crates/perry-hir/src/lower_decl/body_stmt.rs
- crates/perry-hir/src/stable_hash/module.rs
- crates/perry-transform/src/inline/mod.rs
- crates/perry-hir/src/lower/lowering_context.rs
- crates/perry/src/commands/compile/init_order.rs
- crates/perry/src/commands/compile/cjs_wrap/extract_requires.rs
- crates/perry-hir/src/ir/expr.rs
- crates/perry/src/commands/compile/resolve/tests.rs
- crates/perry-hir/src/lower/lower_expr.rs
- crates/perry-hir/src/lower_decl/class_members.rs
- crates/perry/src/commands/compile/cjs_wrap/hoist_classes.rs
- crates/perry-codegen/src/lower_call/property_get.rs
- crates/perry-hir/src/lower/expr_call/mod.rs
- crates/perry-codegen/src/expr/dyn_extern_i18n.rs
- crates/perry-codegen/src/lower_call/builtin.rs
- crates/perry-codegen/src/expr/mod.rs
- crates/perry-runtime/src/gc/mod.rs
- crates/perry/src/commands/compile.rs
- crates/perry-hir/src/stable_hash/expr.rs
- crates/perry-runtime/src/object/global_this.rs
- crates/perry/src/commands/compile/resolve.rs
- crates/perry-parser/src/lib.rs
- crates/perry-codegen/src/expr/static_field_meta.rs
- crates/perry-codegen/src/runtime_decls/strings_part2.rs
- crates/perry-codegen/src/lower_call/new.rs
- crates/perry-hir/src/lower_decl/class_decl.rs
- crates/perry/src/commands/compile/cjs_wrap/wrap.rs
- crates/perry-runtime/src/object/field_set_by_name.rs
- crates/perry-runtime/src/object/class_registry.rs
- crates/perry-codegen/src/codegen/entry.rs
- crates/perry/src/commands/compile/collect_modules.rs
- crates/perry-hir/src/walker/expr_ref.rs
- crates/perry-codegen/src/expr/this_super_call.rs
- crates/perry/src/commands/compile/cjs_wrap/detect.rs
- crates/perry-runtime/src/object/native_module.rs
- crates/perry-hir/src/lower_decl/class_captures.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/perry-codegen/src/runtime_decls/strings.rs (1)
1131-1132: 💤 Low valueReturn type mismatch with runtime implementation.
The codegen declares
js_super_construct_applyasVOID, but the runtime implementation (crates/perry-runtime/src/object/class_constructors.rs) returnsf64. While practically harmless (the return is alwaysundefinedand discarded), aligning them would improve contract consistency.♻️ Suggested fix
- module.declare_function("js_super_construct_apply", VOID, &[I32, DOUBLE, DOUBLE]); + module.declare_function("js_super_construct_apply", DOUBLE, &[I32, DOUBLE, DOUBLE]);🤖 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-codegen/src/runtime_decls/strings.rs` around lines 1131 - 1132, The declared signature for js_super_construct_apply in strings.rs currently uses VOID but the runtime (class_constructors.rs) returns an f64; update the module.declare_function call for "js_super_construct_apply" (the line invoking module.declare_function("js_super_construct_apply", ...)) to use DOUBLE as the return type instead of VOID so the codegen signature matches the runtime implementation.
🤖 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/field_get_set.rs`:
- Around line 5151-5179: The wide-index lookup can be triggered for a
corrupt/oversized key array because the existing check "if key_count > 65536 {
return JSValue::undefined(); }" happens after the wide-index logic; move or
duplicate that guard to run before the block that compares key_count with
WIDE_KEY_INDEX_MIN_KEYS and calls wide_key_index_lookup so oversized key_count
values are rejected before any index build/lookup (i.e. add an early "if
key_count > 65536 { return JSValue::undefined(); }" immediately before the "if
key_count >= WIDE_KEY_INDEX_MIN_KEYS" check, keeping the rest of the
logic—wide_key_index_lookup, ACCESSORS_IN_USE handling,
js_object_get_field/overflow_get—unchanged).
In `@crates/perry-runtime/src/proxy.rs`:
- Around line 1162-1186: The code dereferences obj as *const
crate::object::ObjectHeader after extract_pointer without verifying it points to
a valid ObjectHeader; update the fast-path in the block that calls
extract_pointer so you first call crate::object::is_valid_obj_ptr(obj as *const
u8) and only then read unsafe { (*obj).class_id } and proceed to call
crate::object::get_module_name_from_namespace,
crate::object::native_namespace_prop_override_store, or
crate::object::set_buffer_pool_size; mirror the safety check pattern used
elsewhere (see the is_valid_obj_ptr usage near the other fast path) to avoid
undefined behavior when the pointer targets non-object heap values.
---
Nitpick comments:
In `@crates/perry-codegen/src/runtime_decls/strings.rs`:
- Around line 1131-1132: The declared signature for js_super_construct_apply in
strings.rs currently uses VOID but the runtime (class_constructors.rs) returns
an f64; update the module.declare_function call for "js_super_construct_apply"
(the line invoking module.declare_function("js_super_construct_apply", ...)) to
use DOUBLE as the return type instead of VOID so the codegen signature matches
the runtime implementation.
🪄 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: 8e81e87b-2c00-4e23-b087-f2d9f914faf8
📒 Files selected for processing (55)
crates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/expr/dyn_extern_i18n.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/static_field_meta.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/lower_call/builtin.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/lower_call/property_get.rscrates/perry-codegen/src/lower_string_method.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-codegen/src/runtime_decls/strings_part2.rscrates/perry-hir/src/analysis/uses_this.rscrates/perry-hir/src/ir/decl.rscrates/perry-hir/src/ir/expr.rscrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/expr_call/mod.rscrates/perry-hir/src/lower/lower_expr.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower/module_decl.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-hir/src/lower_decl/class_captures.rscrates/perry-hir/src/lower_decl/class_decl.rscrates/perry-hir/src/lower_decl/class_members.rscrates/perry-hir/src/stable_hash/expr.rscrates/perry-hir/src/stable_hash/module.rscrates/perry-hir/src/stable_hash/tests.rscrates/perry-hir/src/walker/expr_mut.rscrates/perry-hir/src/walker/expr_ref.rscrates/perry-parser/src/lib.rscrates/perry-runtime/src/event_target.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/node_submodules/mod.rscrates/perry-runtime/src/object/class_constructors.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/reflect_support.rscrates/perry-runtime/src/proxy.rscrates/perry-transform/src/inline/mod.rscrates/perry/src/commands/compile.rscrates/perry/src/commands/compile/bootstrap.rscrates/perry/src/commands/compile/cjs_wrap/detect.rscrates/perry/src/commands/compile/cjs_wrap/extract_exports.rscrates/perry/src/commands/compile/cjs_wrap/extract_requires.rscrates/perry/src/commands/compile/cjs_wrap/hoist_classes.rscrates/perry/src/commands/compile/cjs_wrap/mod.rscrates/perry/src/commands/compile/cjs_wrap/wrap.rscrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/init_order.rscrates/perry/src/commands/compile/resolve.rscrates/perry/src/commands/compile/resolve/tests.rs
✅ Files skipped from review due to trivial changes (4)
- crates/perry-hir/src/walker/expr_mut.rs
- crates/perry/src/commands/compile/bootstrap.rs
- crates/perry-hir/src/lower/context.rs
- crates/perry-hir/src/lower/module_decl.rs
🚧 Files skipped from review as they are similar to previous changes (43)
- crates/perry-hir/src/stable_hash/tests.rs
- crates/perry-hir/src/analysis/uses_this.rs
- crates/perry-runtime/src/node_submodules/mod.rs
- crates/perry-codegen/src/lower_string_method.rs
- crates/perry-hir/src/ir/decl.rs
- crates/perry-runtime/src/object/instanceof.rs
- crates/perry/src/commands/compile/cjs_wrap/extract_exports.rs
- crates/perry-hir/src/lower_decl/body_stmt.rs
- crates/perry-hir/src/stable_hash/module.rs
- crates/perry-transform/src/inline/mod.rs
- crates/perry-hir/src/lower/lowering_context.rs
- crates/perry/src/commands/compile/init_order.rs
- crates/perry/src/commands/compile/cjs_wrap/extract_requires.rs
- crates/perry-hir/src/ir/expr.rs
- crates/perry/src/commands/compile/resolve/tests.rs
- crates/perry-hir/src/lower/lower_expr.rs
- crates/perry-hir/src/lower_decl/class_members.rs
- crates/perry/src/commands/compile/cjs_wrap/hoist_classes.rs
- crates/perry-codegen/src/lower_call/property_get.rs
- crates/perry-hir/src/lower/expr_call/mod.rs
- crates/perry-codegen/src/expr/dyn_extern_i18n.rs
- crates/perry-codegen/src/lower_call/builtin.rs
- crates/perry-codegen/src/expr/mod.rs
- crates/perry-runtime/src/gc/mod.rs
- crates/perry/src/commands/compile.rs
- crates/perry-hir/src/stable_hash/expr.rs
- crates/perry-runtime/src/object/global_this.rs
- crates/perry/src/commands/compile/resolve.rs
- crates/perry-parser/src/lib.rs
- crates/perry-codegen/src/expr/static_field_meta.rs
- crates/perry-codegen/src/runtime_decls/strings_part2.rs
- crates/perry-codegen/src/lower_call/new.rs
- crates/perry-hir/src/lower_decl/class_decl.rs
- crates/perry/src/commands/compile/cjs_wrap/wrap.rs
- crates/perry-runtime/src/object/field_set_by_name.rs
- crates/perry-runtime/src/object/class_registry.rs
- crates/perry-codegen/src/codegen/entry.rs
- crates/perry/src/commands/compile/collect_modules.rs
- crates/perry-hir/src/walker/expr_ref.rs
- crates/perry-codegen/src/expr/this_super_call.rs
- crates/perry/src/commands/compile/cjs_wrap/detect.rs
- crates/perry-runtime/src/object/native_module.rs
- crates/perry-hir/src/lower_decl/class_captures.rs
🛑 Comments failed to post (2)
crates/perry-runtime/src/object/field_get_set.rs (1)
5151-5179:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard oversized
key_countbefore wide-index lookup/build.At Line 5154,
wide_key_index_lookup(...)can execute before the existingkey_count > 65536safety return at Line 5178. That lets oversized/corrupt key arrays trigger large map builds and long scans on the hot read path.Proposed fix
- // `#5054`: wide objects get a validated key→index map so per-key reads - // stay O(1) instead of O(key_count). A `None` falls through to the - // linear scan below (the index is an accelerator, not authoritative). - if key_count >= WIDE_KEY_INDEX_MIN_KEYS { + if key_count > 65536 { + return JSValue::undefined(); + } + + // `#5054`: wide objects get a validated key→index map so per-key reads + // stay O(1) instead of O(key_count). A `None` falls through to the + // linear scan below (the index is an accelerator, not authoritative). + if key_count >= WIDE_KEY_INDEX_MIN_KEYS { if let Some(i) = wide_key_index_lookup(keys_id, key_bytes, key, keys, key_count) { if ACCESSORS_IN_USE.with(|c| c.get()) { if let Ok(name) = std::str::from_utf8(key_bytes) { if let Some(acc) = get_accessor_descriptor(obj as usize, name) { @@ - if key_count > 65536 { - return JSValue::undefined(); - }📝 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.if key_count > 65536 { return JSValue::undefined(); } // `#5054`: wide objects get a validated key→index map so per-key reads // stay O(1) instead of O(key_count). A `None` falls through to the // linear scan below (the index is an accelerator, not authoritative). if key_count >= WIDE_KEY_INDEX_MIN_KEYS { if let Some(i) = wide_key_index_lookup(keys_id, key_bytes, key, keys, key_count) { if ACCESSORS_IN_USE.with(|c| c.get()) { if let Ok(name) = std::str::from_utf8(key_bytes) { if let Some(acc) = get_accessor_descriptor(obj as usize, name) { if acc.get != 0 { let receiver = crate::value::js_nanbox_pointer(obj as i64); return invoke_accessor_getter(acc.get, receiver); } return JSValue::undefined(); } } } return if (i as usize) < alloc_limit { js_object_get_field(obj, i) } else { match overflow_get(obj as usize, i as usize) { Some(bits) => JSValue::from_bits(bits), None => JSValue::undefined(), } }; } }🤖 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/field_get_set.rs` around lines 5151 - 5179, The wide-index lookup can be triggered for a corrupt/oversized key array because the existing check "if key_count > 65536 { return JSValue::undefined(); }" happens after the wide-index logic; move or duplicate that guard to run before the block that compares key_count with WIDE_KEY_INDEX_MIN_KEYS and calls wide_key_index_lookup so oversized key_count values are rejected before any index build/lookup (i.e. add an early "if key_count > 65536 { return JSValue::undefined(); }" immediately before the "if key_count >= WIDE_KEY_INDEX_MIN_KEYS" check, keeping the rest of the logic—wide_key_index_lookup, ACCESSORS_IN_USE handling, js_object_get_field/overflow_get—unchanged).crates/perry-runtime/src/proxy.rs (1)
1162-1186:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd
is_valid_obj_ptrcheck before dereferencing as ObjectHeader.Line 1165 dereferences
objto read.class_idafter only checkingis_pointer()and non-null. However,is_pointer()only validates the NaN-box tag (POINTER_TAG), not whether the pointer targets a validObjectHeader. The pointer could be to a string, typed-array, or other non-object heap value, making the dereference undefined behavior even if the comparison fails and the block is skipped.Compare to line 1039 in this same file, which uses
crate::object::is_valid_obj_ptr(obj as *const u8)before dereferencing. The fast path above (lines 1138-1150) validates viatry_read_gc_headerbefore reading the ObjectHeader.🛡️ Recommended fix
{ let jv = crate::value::JSValue::from_bits(target.to_bits()); if jv.is_pointer() { let obj = extract_pointer(target.to_bits()) as *const crate::object::ObjectHeader; - if !obj.is_null() && unsafe { (*obj).class_id } == crate::object::NATIVE_MODULE_CLASS_ID + if !obj.is_null() + && crate::object::is_valid_obj_ptr(obj as *const u8) + && unsafe { (*obj).class_id } == crate::object::NATIVE_MODULE_CLASS_ID {📝 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.let jv = crate::value::JSValue::from_bits(target.to_bits()); if jv.is_pointer() { let obj = extract_pointer(target.to_bits()) as *const crate::object::ObjectHeader; if !obj.is_null() && crate::object::is_valid_obj_ptr(obj as *const u8) && unsafe { (*obj).class_id } == crate::object::NATIVE_MODULE_CLASS_ID { let module_name = unsafe { crate::object::get_module_name_from_namespace(target) }; if let (false, Some(prop)) = (module_name.is_empty(), property_key_to_rust_string(key)) { if prop != "__module__" { if module_name == "buffer.Buffer" && prop == "poolSize" { crate::object::set_buffer_pool_size(value); } else { crate::object::native_namespace_prop_override_store( module_name, &prop, value, ); } return true; } } } }🤖 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/proxy.rs` around lines 1162 - 1186, The code dereferences obj as *const crate::object::ObjectHeader after extract_pointer without verifying it points to a valid ObjectHeader; update the fast-path in the block that calls extract_pointer so you first call crate::object::is_valid_obj_ptr(obj as *const u8) and only then read unsafe { (*obj).class_id } and proceed to call crate::object::get_module_name_from_namespace, crate::object::native_namespace_prop_override_store, or crate::object::set_buffer_pool_size; mirror the safety check pattern used elsewhere (see the is_valid_obj_ptr usage near the other fast path) to avoid undefined behavior when the pointer targets non-object heap values.
…native_addon.rs Keeps collect_modules.rs under the 2000-line file-size cap (the wall fixes pushed it from 1988 to 2031). Moves the self-contained compile-package Node-native-addon probe group (package-root resolution + binding.gyp / prebuilds / gypfile / loader-dep / *.node markers + the refusal) into a sibling module; behavior unchanged.
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.
Summary
Compiling a real Next.js 16.2.9 app (
output: 'standalone', the whole framework viaperry.compilePackages) end-to-end surfaced 14 independent Perry bugs. Each was minimized to a small repro and fixed. With this PR the standaloneserver.jscompiles, links (123MB native binary), and boots through Next's entire environment-extension init chain; the next blocker is unrelated to these fixes (vendored zoderrToObj, #838 cross-module-bundle family).Fixes
Compile-time
looks_like_es_moduleregex-literal desync (picomatch/(^[*!]|[/()[\]{}"])/masked the wrap's trailingexport default _cjs→ImportExportInScript)worker_threadsWorker path → warning + runtime throw instead of compile error (Next build-time worker pools)client-only0-byte index.js)is_commonjsscans comment/string/regex/nested-template-stripped source (4 real-world defeats incl.build/utils.jsgenerating ESM inside nested template literals)@swc/helpersesm/)Codegen
7. #602 flip (behavior-changing): ambiguous DB ctors (
Client/Pool/…) need positive import-source evidence — undici's bundledvar Client = classhit pg's arm → undefinedjs_pg_client_newat link8.
.normalizeString-path gated to ≤1 arg (Next normalizers have 2-argnormalize); String path itself now spec-ignores surplus args9. Builtin-module default-import alias as a VALUE materializes the native namespace (was
TAG_TRUE)Runtime
10.
class X extends Eventworks end-to-end (definition/super(type)/instanceof/dispatchEvent) — ws WebSocket events11.
extends stream.Writable-style bound-native parents no longer throw12.
console.errorcan bedefineProperty-patched repeatedly (virtual native-module keys visible toobj_value_has_own_key)13.
require('node:timers')is a real, patchable namespace (default object + member keys + GC-scanned override store) — Next fast-setImmediate14.
fn.toStringvalue-read reified (likebind/call/apply);Module.prototype.requireexists for require-hook patchingTests
date full_yearis the known pre-existing macOS-local failure;url path_to_file_urlandbuiltin_prototype_methods_reject_dynamic_neware sporadic thread-interaction flakes (pass in isolation; appear on clean main runs too).Review attention
new Client()without a pg import no longer dispatches to pg).Validation harness
/tmp/perry-nextjs-demo(local): App Router blog app w/ SSR + dynamic routes + API CRUD; Node baseline 21/21 green viaverify.sh; same script ready to verify the Perry binary.Summary by CodeRabbit
New Features
Bug Fixes