fix(module): complete Node 26 parity - #7312
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds Node.js 26.5.0 ChangesNode module parity
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-hir/src/lower/expr_call/native_module/imported_module_dispatch.rs (1)
138-195: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove the
imported_callableguard after the existing.callspecial cases.
EventEmitter,Stream, andServerare registered asApiKind::Class, soServer.call(this, handler)and{ EventEmitter }.call(this, ...) / { Stream }.call(this, ...)hit this guard first and returnOk(Err(args)). Place the guard after theevents,stream, andhttp/httpsServerbranches so the constructor-forward paths keep priority.🤖 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/expr_call/native_module/imported_module_dispatch.rs` around lines 138 - 195, The imported_callable guard in the method_name == "call" branch currently preempts constructor special cases. Move it below the stream, events, and http/https Server branches so those constructor-forward paths retain priority, while preserving the guard’s existing behavior for other callable imports.crates/perry-runtime/src/object/descriptors.rs (1)
614-625: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoot the class-method descriptor value.
The class-method branch also stores
valueinto a raw descriptor field afterjs_object_alloc_with_shape; use a root handle and store the reloaded bits throughget_nanbox_f64()before rebuilding layout.🤖 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/descriptors.rs` around lines 614 - 625, Update the class-method descriptor path around js_object_alloc_with_shape to root value before allocation, then store the handle’s reloaded NaN-box bits via get_nanbox_f64() into the raw descriptor field before rebuilding the layout. Preserve the existing descriptor shape and publication flow.Source: Learnings
🟠 Major comments (20)
crates/perry-runtime/src/process/node_module/source_map.rs-105-112 (1)
105-112: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThree sites re-implement the heap-address check instead of calling
is_plausible_heap_addr. Each site takes a raw pointer from a NaN-boxed value, guards it withis_pointer()plus a null test, and then dereferencesptr.sub(crate::gc::GC_HEADER_SIZE)as aGcHeader. A pointer payload that is not a plausible heap address passes both guards and produces an out-of-bounds read. The shared root cause is the missing canonical predicate.
crates/perry-runtime/src/process/node_module/source_map.rs#L105-L112: insource_map_line_lengths_getter, replace the null test withcrate::value::addr_class::is_plausible_heap_addrbefore theGcHeaderread.crates/perry-runtime/src/process/node_module/source_map.rs#L380-L383: insource_map_array_element, apply the same guard to the pointer taken on Line 376.crates/perry-runtime/src/process/node_module/source_map.rs#L487-L488: insource_map_array_value, apply the same guard to the pointer taken on Line 483.The PR summary states that this layer "adopts shared address-classification helpers". These three sites did not adopt them.
Based on learnings: "use the canonical predicate
crate::value::addr_class::is_plausible_heap_addrfor the handle-band/heap-floor check. Do not duplicate lower-level address checks elsewhere; review should fail code paths that re-implement or bypassis_plausible_heap_addr".🤖 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/process/node_module/source_map.rs` around lines 105 - 112, Replace the duplicated null-only heap checks with the canonical crate::value::addr_class::is_plausible_heap_addr predicate before each GcHeader dereference: update source_map_line_lengths_getter at crates/perry-runtime/src/process/node_module/source_map.rs:105-112, source_map_array_element at :380-383 using its pointer from line 376, and source_map_array_value at :487-488 using its pointer from line 483; preserve module_undefined() for invalid pointers and do not reimplement lower-level address validation.Source: Learnings
crates/perry-runtime/src/process/node_module/source_map.rs-681-688 (1)
681-688: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe marker match is too strict for common inline source maps.
Line 681 requires the exact prefix
sourceMappingURL=data:application/json;base64,. Node matches the data URL with a pattern that also accepts a media-type parameter, sosourceMappingURL=data:application/json;charset=utf-8;base64,is valid. TypeScript and esbuild both emit that form.findSourceMapreturnsundefinedfor those files.Match
sourceMappingURL=data:application/jsonfirst, then accept an optionalcharsetparameter before;base64,.Lines 684-688 have a related gap.
.lines().next().trim()keeps a trailing*/when the comment is a block comment, and the base64 decode then fails. Stop the encoded text at the first character that is outside the base64 alphabet.🤖 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/process/node_module/source_map.rs` around lines 681 - 688, Update the source-map marker parsing in findSourceMap to accept data URLs beginning with sourceMappingURL=data:application/json, including an optional charset parameter before ;base64,. When extracting encoded, stop at the first character outside the base64 alphabet so block-comment terminators such as */ are excluded before decoding.crates/perry-runtime/src/process/node_module/source_map.rs-392-412 (1)
392-412: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThe collected argument vector is not rooted across
to_number().
source_map_collect_argscopies raw NaN-boxed values into aVec<f64>.source_map_arg_numberthen callsJSValue::to_number(), which runsvalueOfortoStringfor an object argument. That is user code and a collection point.
source_map_find_origin_thunkcoerces argument 0 on Line 602 and argument 1 on Line 603. If the first coercion triggers a moving collection, the pointer stored in the second element of the vector is stale when Line 603 reads it.source_map_find_entry_thunkhas the same sequence on Lines 549-550.Root the argument values before any coercion, for example by rooting each element in a
RuntimeHandleScopeinsidesource_map_collect_argsand reading them back through handles.Based on learnings that a NaN-boxed
f64held in Rust storage must be rooted and reloaded from its handle after an operation that can invoke user code or allocate. As per coding guidelines: "GC-managed values must remain rooted across every possible collection point".🤖 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/process/node_module/source_map.rs` around lines 392 - 412, Root every collected argument in source_map_collect_args using a RuntimeHandleScope, and reload each value from its handle before returning or coercing it. Ensure source_map_arg_number and the sequential coercions in source_map_find_origin_thunk and source_map_find_entry_thunk never read stale NaN-boxed values after to_number() may trigger user code or garbage collection.Sources: Coding guidelines, Learnings
crates/perry-runtime/src/process/node_module/source_map.rs-612-620 (1)
612-620: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
line as i64 - 1can overflow for a large negative argument.
source_map_arg_numberaccepts any finite double, including a large negative one. A float-to-integer cast saturates, sofindOrigin(-1e300)producesi64::MINand the following- 1overflows. Withdebug-assertionsoroverflow-checkson, which theperry-devprofile normally enables, the process panics. The same applies tocol.map(|n| n as i64 - 1).Use
saturating_sub.🐛 Proposed fix
let entry = if let Some(line) = line { source_map_lookup( payload, - line as i64 - 1, - col.map(|n| n as i64 - 1).unwrap_or(i64::MAX), + (line as i64).saturating_sub(1), + col.map(|n| (n as i64).saturating_sub(1)) + .unwrap_or(i64::MAX), ) } else {🤖 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/process/node_module/source_map.rs` around lines 612 - 620, Update the one-based coordinate conversions in the source-map lookup block to use saturating subtraction after casting to i64. Apply this to both the line expression and the column mapping so large negative finite inputs clamp at i64::MIN without overflowing, while preserving the existing i64::MAX fallback behavior.crates/perry-runtime/src/process/node_module/source_map.rs-309-329 (1)
309-329: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t attach the previous entry’s name to 4-field segments.
name_idxis cumulative across mappings, but a 4-field segment has nonameassociation. Only 5-field segments should set the entry’sname; carry-over makes later named entries resolve the prior name instead of this segment’s name.🤖 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/process/node_module/source_map.rs` around lines 309 - 329, The mapping decode logic in the 4-field branch incorrectly carries forward name_idx. Update the name selection within the original segment handling so only 5-field segments set and return a name, while 4-field segments return no name regardless of has_name or prior mappings; preserve cumulative source and coordinate state.crates/perry-runtime/src/object/native_module.rs-973-999 (1)
973-999: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not cache an
undefinedresolution inNATIVE_ESM_EXPORT_VALUES.The function caches whatever
native_module_property_by_name_implreturns, includingundefined. Several resolution paths in this file depend on lazily installed state: the per-module dispatch rows installed byjs_nm_install_<module>(), thefs.promisessubmodule singleton, andperf_hooks.performance. If a named ESM import reads a property before that state exists, theundefinedresult is pinned for the rest of the process, and only an explicitsyncBuiltinESMExports()call clears it.Skip the cache insert when the resolved value is
undefined, so a later read can resolve the real export.🐛 Proposed fix
let value = unsafe { native_module_property_by_name_impl( module.as_ptr(), module.len(), property.as_ptr(), property.len(), false, ) }; + if value.to_bits() == crate::value::TAG_UNDEFINED { + return value; + } NATIVE_ESM_EXPORT_VALUES.with(|values| { values.borrow_mut().insert(key, value.to_bits()); }); crate::gc::runtime_write_barrier_root_nanbox(value.to_bits()); value🤖 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 973 - 999, Update js_native_module_esm_export_value so the result from native_module_property_by_name_impl is inserted into NATIVE_ESM_EXPORT_VALUES only when it is not undefined. Preserve the existing cache lookup and write-barrier behavior for defined values, allowing later reads to retry resolution when the initial result is undefined.crates/perry-runtime/src/object/native_module/callable_exports/module_cjs.rs-253-271 (1)
253-271: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrototype method names and attributes do not match Node.
Two parity gaps:
set_bound_native_closure_name(closure, "")discards thenameargument, soModule.prototype.load.nameis""instead of"load".- The methods are installed with
PropertyAttrs::new(true, true, true), which makes them enumerable. Node installs prototype methods as writable, non-enumerable, configurable, sofor (const k in mod)must not yield_compile,load, orrequire.🛠️ Proposed fix
- set_bound_native_closure_name(closure.get_raw_mut_ptr(), ""); + set_bound_native_closure_name(closure.get_raw_mut_ptr(), name);crate::object::set_property_attrs( proto_ptr as usize, name.to_string(), - crate::object::PropertyAttrs::new(true, true, true), + crate::object::PropertyAttrs::new(true, false, true), );Also applies to: 366-380
🤖 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/callable_exports/module_cjs.rs` around lines 253 - 271, Update module_prototype_method to pass the name argument to set_bound_native_closure_name so each method reports its correct name, and change the Module prototype method property descriptors from PropertyAttrs::new(true, true, true) to writable, non-enumerable, configurable attributes for _compile, load, and require.crates/perry-runtime/src/object/native_module/callable_exports/module_cjs.rs-235-271 (1)
235-271: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Module.prototype.requireandModule.prototype._compilereturnundefined.
module_prototype_method_thunkignores its arguments and returnsundefined.loadroutes tojs_module_instance_load, butrequireand_compiledo not load anything. A program that callsmod.require("./x")receivesundefinedinstead of the module exports.
crates/perry-runtime/src/process/node_module.rsalready providesmodule_require_thunk, andcrate::module_require::js_require_path_moduleperforms the load. Do you want me to wirerequireto the existing require path and open an issue for_compile?🤖 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/callable_exports/module_cjs.rs` around lines 235 - 271, Update module_prototype_method_thunk and module_prototype_method so the "require" method uses the existing module_require_thunk and require path, allowing mod.require("./x") to return the loaded module exports instead of undefined. Preserve the current load routing through js_module_instance_load, and leave _compile behavior unchanged unless existing symbols require a distinct mapping.crates/perry-codegen/src/expr/dyn_extern_i18n.rs-417-469 (1)
417-469: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve namespace identity instead of materializing a fresh object for each read.
Expr::ExternFuncRefforctx.namespace_importscurrently callsmaterialize_compiled_namespace, which allocates, populates, finalizes, and releases a new namespace object on every lowering. Whole-value namespace reads should load the already-populated member globals from the module’s singleton namespace object 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-codegen/src/expr/dyn_extern_i18n.rs` around lines 417 - 469, Update materialize_compiled_namespace and the Expr::ExternFuncRef handling for ctx.namespace_imports to return the module’s existing singleton namespace object rather than allocating and populating a new object per read. Load the already-populated namespace/member globals through the established namespace representation, preserving namespace identity and avoiding the js_object_alloc, member lowering, finalization, and release path for whole-value reads.crates/perry-runtime/src/module_require.rs-211-239 (1)
211-239: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBoth resolvers use
Path::with_extension, which replaces an existing extension instead of appending one. Node'sLOAD_AS_FILEappends.js,.json, and.nodeto the request.with_extensionreplaces everything after the last dot, sorequire('./index.min')probesindex.jsrather thanindex.min.js, andrequire('./v1.2')probesv1.js. Dotted filenames without an extension are common in published packages, and the compile-side and runtime-side resolvers must agree.
crates/perry-runtime/src/module_require.rs#L211-L239: inresolve_file, build each candidate by appending to the file name, for examplepath.with_file_name(format!("{}.{ext}", file_name)), instead ofpath.with_extension(ext).crates/perry/src/commands/compile/collect_modules/static_require_transform.rs#L244-L253: apply the same append-based candidate construction inresolve_require_path, keeping its extension list unchanged so compile-time and runtime resolution stay identical.🤖 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/module_require.rs` around lines 211 - 239, Update resolve_file in crates/perry-runtime/src/module_require.rs (lines 211-239) and resolve_require_path in crates/perry/src/commands/compile/collect_modules/static_require_transform.rs (lines 244-253) to append each existing extension to the full filename rather than replacing its current extension. Preserve the existing extension lists and keep both resolvers’ candidate ordering identical.crates/perry/src/commands/compile/cjs_wrap/wrap.rs-1082-1097 (1)
1082-1097: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftMemoize the require-graph reachability walk.
cyclic_require_specsrunsrequire_graph_reachesonce per require specifier, and each call performs a fresh DFS that canonicalizes and reads every transitive dependency file. Thevisitedset is per-call, so nothing is shared between specifiers or between modules. For a wrapped CJS module with N specifiers over a graph of E edges, the wrap performs O(N·E)canonicalizesyscalls plus file reads, andwrap_commonjs_with_body_offsetruns for every CJS file in the build. On a largenode_modulesgraph this dominates collection time.Two changes keep the behavior and bound the cost:
- Build the dependency set of
source_pathonce, then run a single DFS that records which specifiers reachsource_key.- Cache the parsed specifier list per canonical path (and the reachability result per
(path, target)) in a process-level map, so repeated wraps reuse it.
require_graph_reachesalso recurses without a depth bound. Deep chains can exhaust the stack; an explicit worklist removes that risk while you restructure the walk.Also applies to: 1177-1198
🤖 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 1082 - 1097, Refactor cyclic_require_specs and require_graph_reaches to memoize parsed require specifiers by canonical path and reachability by (path, target) in process-level caches. Build source_path’s dependency set once, then use one iterative worklist traversal to record which resolved specifiers reach source_key instead of running a fresh DFS per specifier. Preserve existing resolution behavior while avoiding recursive stack growth and reusing cached file reads and canonicalization across wrapped modules.crates/perry-runtime/src/module_require.rs-802-805 (1)
802-805: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoot
MODULE_PATH_REGISTRYvalues through the GC scanner
MODULE_PATH_REGISTRYstores rawf64bits and reads them back as exported values. Add it as a mutable root scanner ingc/mod.rsso copied-minor GC can mark these exports and rewrite the cached bits after the target object moves.🤖 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/module_require.rs` around lines 802 - 805, Update the GC root scanning implementation in gc/mod.rs to include MODULE_PATH_REGISTRY as a mutable root. Scan each cached raw f64 value as a NaN-boxed export, mark it during copied-minor GC, and rewrite the registry entry with the relocated bits after objects move; preserve the existing registry access and locking behavior.Source: Coding guidelines
crates/perry/src/commands/compile/collect_modules/static_require_transform.rs-133-151 (1)
133-151: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA resolvable
.nodeaddon now gains an AOT import edge and can fail the build.
resolve_require_pathprobes thenodeextension, sorequire('./build/Release/foo.node')now resolves. This branch keeps the runtimerequirebut still emitsimport _lazyreq_static_N from "<abs>.node";. That import enters the module graph, andcollect_module_onecallsrefuse_node_addon_binaryon the resolved path, which hard-errors. Before this change the same call site produced no import edge, so the build succeeded and only the runtimerequirefailed if reached.Exclude
nodetargets from the emitted import so a native addon keeps its runtime-only behavior.🐛 Proposed fix
if let Some(target) = require_target.as_ref() { + // A `.node` addon has no AOT representation; an import edge would + // reach `refuse_node_addon_binary` and fail the whole build. + let is_native_addon = + target.extension().and_then(|e| e.to_str()) == Some("node"); if !matches!( target.extension().and_then(|e| e.to_str()), Some("ts" | "tsx" | "mts" | "cts" | "js" | "mjs") ) { - if discovered_side_effects.insert(target.clone()) { + if !is_native_addon && discovered_side_effects.insert(target.clone()) { let binding = unique_lazy_require_name(source, &mut next_id); imports.push(format!( "import {binding} from {:?};", target.to_string_lossy() )); } continue; } }🤖 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/collect_modules/static_require_transform.rs` around lines 133 - 151, Update the extension filter in the require-target handling branch so resolved .node targets are excluded from the emitted static import. Preserve the existing runtime require path and side-effect imports for the other non-JS extensions, while keeping native addons runtime-only as before.crates/perry/src/commands/compile/collect_modules.rs-1286-1300 (1)
1286-1300: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce the two-key opt-in for AOT-promoted
node_modulesimports.A resolved
Interpretednode_modulesfile is now promoted toNativeCompiledand added toctx.aot_discovered_modules;collect_module_one()then treats that path asis_in_compiled_pkg, so it avoidsshould_use_js_runtimeand skips the final V8-free gate. That single imported file can enter the binary even when the owning package is not listed inctx.compile_packages/ctx.allow_compile_packages. The same path also makesrefuse_compile_package_native_addon(ctx, &canonical)reachable. Either gate single-filenode_modulespromotion withallow_compile_packagesas part ofcompilePackagestrust, or avoid promoting these discovered imports so the runtime JS failure stays explicit.🤖 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/collect_modules.rs` around lines 1286 - 1300, Update the AOT promotion logic for resolved Interpreted imports in the module collection flow so node_modules files are promoted only when both compile_packages and allow_compile_packages authorize the owning package. Apply this gate before inserting into ctx.aot_discovered_modules or changing the kind to NativeCompiled, preserving the existing exclusions for Perry-native and declaration files; otherwise retain the original resolved.kind so the V8-free failure remains explicit.crates/perry/src/commands/compile/run_pipeline.rs-2874-2890 (1)
2874-2890: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScope
defaultto the namespace member when claiming var exports.
imported_varsis flat inCrossModuleCtx, while namespace member metadata is keyed by(namespace, member). The loop inserts both"default"and"module.exports"unconditionally, so a later bareimport { default } from ...orimport defaultin the same module can be treated as var-shaped and routed through the getter. Keep the"module.exports"insert unguarded, but gate"default"on the module exporting a var-shaped default.🤖 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/run_pipeline.rs` around lines 2874 - 2890, Update the loop around namespace member handling so imported_vars always includes "module.exports" but includes "default" only when the module’s default export is var-shaped. Keep the namespace metadata and import prefix/origin registrations unchanged, using the existing default-export shape information available in this pipeline.crates/perry-runtime/src/module_require.rs-263-269 (1)
263-269: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse Node exports condition order instead of scan order. Node exports resolution uses match order in the JSON object;
"default"should only act as the final fallback whennodeorrequiredoes not precede it. Since serde_json has nopreserve_orderfeature set, re-iterate without a fixed lookup order and return the first matching condition 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/module_require.rs` around lines 263 - 269, The exports-condition resolution loops in crates/perry-runtime/src/module_require.rs lines 263-269 and crates/perry/src/commands/compile/collect_modules/static_require_transform.rs lines 284-290 must honor the JSON object's iteration order rather than imposing a fixed condition priority. In both corresponding resolution paths, iterate once through each map, consider only node, require, and default, and return the first condition whose resolve_exports result matches; this preserves default as a fallback only when it appears after applicable conditions.crates/perry-runtime/src/process/node_module.rs-1290-1309 (1)
1290-1309: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA multi-line type alias is only partially stripped.
The scan at lines 1299-1303 stops at the first
\nor\r. For a common multi-line alias the remainder stays in the output:type A = { x: number; };The pass blanks only
type A = {. The linesx: number;and};remain. The result is invalid JavaScript, so the stripped source fails to parse.The alias body can be balanced by tracking
{,[,(depth on masked code bytes, and by stopping at a newline only when every depth is zero.🐛 Proposed fix to balance the alias body
- while cursor < bytes.len() - && !(mask[cursor] && matches!(bytes[cursor], b';' | b'\n' | b'\r')) - { - cursor += 1; - } + let mut depth = 0usize; + while cursor < bytes.len() { + if mask[cursor] { + match bytes[cursor] { + b'{' | b'[' | b'(' => depth += 1, + b'}' | b']' | b')' => depth = depth.saturating_sub(1), + b';' | b'\n' | b'\r' if depth == 0 => break, + _ => {} + } + } + cursor += 1; + }🤖 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/process/node_module.rs` around lines 1290 - 1309, Update the alias-scanning logic in the visible module-space stripping routine so multi-line type aliases are consumed through their complete balanced body. Track masked `{}`, `[]`, and `()` nesting while scanning after `=`, and only treat `\n` or `\r` as terminators when all nesting depths are zero; preserve semicolon termination and call `module_space_span` over the entire alias.crates/perry-runtime/src/process/node_module.rs-1156-1201 (1)
1156-1201: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe mask misclassifies nested template literals and regex literals.
Two lexical forms break the state machine:
- Nested template literals. The
TEMPLATEstate does not track${ ... }interpolation depth. For`a${`b`}c`, the inner opening backtick ends the outer template. The following backtick then re-opens a template. Every byte after that point is classified with inverted polarity, so real code is treated as string content and string content is treated as code.- Regex literals. A
/that starts a regex is neither//nor/*, so the state staysCODE. A{,interface,type, orasinside a regex body is then treated as code.module_strip_interfacesandmodule_strip_type_clausecan space out real source in that case.Both cases can corrupt the stripped output rather than fail loudly. Track interpolation depth for
${and}in theTEMPLATEstate, and add a regex-literal state that uses the previous significant code byte to distinguish division from a regex start.🤖 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/process/node_module.rs` around lines 1156 - 1201, Update the masking state machine to track nested template interpolation depth, switching between template text and embedded code at `${` and matching `}` so nested backticks remain correctly classified. Add a regex-literal state and use the previous significant code byte to distinguish regex starts from division, while handling escapes and regex character classes. Preserve existing comment, string, and template masking behavior for non-nested literals.crates/perry-runtime/src/process/node_module.rs-127-137 (1)
127-137: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the parent as an object, not as any non-string pointer.
has_parentaccepts every pointer value that is not a string. A closure, an array, or a typed handle passes this check.nm_dispatch_modulemaps("module", "Module")tojs_module_module_new(arg(0), arg(1)), so user code can pass any value asparent. Lines 147-151 then treat that pointer as anObjectHeaderand read field index 5 as an array. For a closure receiver this reads past the closure payload.
module_object_ptralready validates the GC object type. Use it for the check.🛡️ Proposed fix to classify the parent value
- let parent_value = JSValue::from_bits(parent.get_nanbox_f64().to_bits()); - let has_parent = parent_value.is_pointer() && !parent_value.is_string(); + let has_parent = module_object_ptr(parent.get_nanbox_f64()).is_some();Based on learnings: "when classifying raw pointers for … receiver/address classification logic, use the canonical predicate … Do not duplicate lower-level address checks elsewhere".
🤖 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/process/node_module.rs` around lines 127 - 137, Update the has_parent classification in the parent initialization flow to use the canonical module_object_ptr validation instead of accepting every non-string pointer. Preserve module_null() for invalid or non-object parents, ensuring only validated object pointers reach the later ObjectHeader field access.Source: Learnings
crates/perry-runtime/src/process/node_module.rs-1278-1289 (1)
1278-1289: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBlank the whole
import type/export typestatement during stripping.
module_strip_type_aliasesonly blankstypealiases, and the later strips only handlesatisfies/asclauses. Statements likeimport type { A } from "./types";,export type { A };, andimport type A from "./a";remain in the emitted JavaScript, and can import runtime-less type modules.🤖 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/process/node_module.rs` around lines 1278 - 1289, Update module_strip_type_aliases to detect import type and export type statements before processing type aliases, then blank each complete statement through its terminator while preserving surrounding code. Handle named, default, and from-based forms such as import type { A }, export type { A }, and import type A, without stripping ordinary imports or exports.
🟡 Minor comments (14)
crates/perry-runtime/src/process/node_module/source_map.rs-649-656 (1)
649-656: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the existing path-to-file-URL path for inline source sources.
format!("file://{}", path.to_string_lossy())does not percent-encode spaces or special characters, and it leaves Windows and\\?\paths in an invalidfile:URL shape. Usejs_url_path_to_file_url/the existingurlhelper with the current platform soSourceMap#payload.sourcesmatches Node.🤖 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/process/node_module/source_map.rs` around lines 649 - 656, Update the source URL construction in the source-map sources population flow to use the existing js_url_path_to_file_url or url helper with the current platform instead of formatting file:// directly from the path. Preserve the canonicalize fallback and ensure inline source entries are percent-encoded and correctly shaped across POSIX, Windows, and \\?\ paths.crates/perry-runtime/src/process/node_module/source_map.rs-231-235 (1)
231-235: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet
SourceMap.prototypeproperty attributes explicitly.
closure_set_dynamic_proponly stores the prototype value; it does not record descriptor attributes, soprototyperemains writable/enumerable/configurable unless the object property exists under a different name. Add a non-writable, non-enumerable, non-configurable descriptor forprototype, matching other class-constructor prototype install patterns.🤖 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/process/node_module/source_map.rs` around lines 231 - 235, Update the constructor prototype installation near closure_set_dynamic_prop so SourceMap.prototype is defined with non-writable, non-enumerable, and non-configurable attributes. Reuse the existing class-constructor prototype descriptor mechanism rather than only storing the value through closure_set_dynamic_prop.crates/perry-runtime/src/process/node_module/source_map.rs-386-388 (1)
386-388: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
index as u32truncates and can pass the bound check.
indexis ani64that comes from decoded VLQ deltas, so a craftedmappingsstring controls it. For a value such as0x1_0000_0000, the integer-to-integer cast truncates to0and the comparison againstlensucceeds. The function then returnssources[0]instead ofundefined.Compare in
i64before the cast.🐛 Proposed fix
- if index as u32 >= len { + if index >= i64::from(len) { return undefined_value(); }🤖 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/process/node_module/source_map.rs` around lines 386 - 388, Update the bounds check around the source lookup to compare the i64 index directly against len before casting, then cast only after validation. Preserve the existing undefined_value() return for out-of-range indices and prevent large decoded VLQ values from truncating into valid indices.crates/perry-runtime/src/process/node_module/source_map.rs-51-77 (1)
51-77: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the SourceMap internal slots non-enumerable.
The
_payloadand_lineLengthsfields are inserted intokeys_array, but they are not marked non-enumerable;js_object_set_field/js_object_alloc_with_shapedo not set descriptor attributes. Keep these as real internal slots, but mark the shaped keys non-enumerable through descriptor attributes or hide them from the observable keys array if they should not appear on the instance shape.🤖 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/process/node_module/source_map.rs` around lines 51 - 77, Update the SourceMap object construction around SOURCE_MAP_CLASS_ID so `_payload` and `_lineLengths` remain usable internal slots but are marked non-enumerable in their property descriptors. Reuse the existing shape or descriptor-attribute mechanism instead of changing field storage, and ensure the keys are absent from observable enumeration while remaining accessible internally.crates/perry-runtime/src/process/node_module/source_map.rs-689-691 (1)
689-691: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse an indifferent-padding base64 engine for inline source maps.
base64::engine::general_purpose::STANDARDin the workspace-pinnedbase64 0.22.1requires canonical padding and rejects unpaddeddata:payloads. Decode thissourceMappingURLwith aGeneralPurposeengine configured withdecode_padding_mode(DecodePaddingMode::Indifferent).🤖 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/process/node_module/source_map.rs` around lines 689 - 691, Update the inline source-map decoding in the surrounding sourceMappingURL handling to use a GeneralPurpose base64 engine configured with DecodePaddingMode::Indifferent instead of STANDARD, while preserving the existing module_undefined fallback for decode failures.crates/perry-runtime/src/object/native_module.rs-1037-1040 (1)
1037-1040: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
js_module_run_mainis a silent no-op.In Node,
module.runMain()executes the main entry point. This stub returnsundefinedand performs no work, so a program that relies onrunMain()continues silently with no effect and no diagnostic. Add a comment that records the intentional limitation, or emit a deferred "not implemented" error so the gap is observable.I can open an issue to track the real implementation. Tell me if you want that.
🤖 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 1037 - 1040, Update js_module_run_main to make its unsupported behavior observable: either add a comment documenting that module.runMain execution is intentionally unavailable, or emit a deferred “not implemented” error while preserving the exported function’s ABI and return contract.crates/perry-runtime/src/object/native_module/callable_exports/module_cjs.rs-78-110 (1)
78-110: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
globalPathsdepends onHOMEandPREFIXonly.On Windows
HOMEis usually unset, so the two per-user entries are dropped and only/usr/local/lib/noderemains. Node derives the list fromUSERPROFILEon Windows and from the execution prefix elsewhere. Consider falling back toUSERPROFILEand to a platform-appropriate prefix.🤖 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/callable_exports/module_cjs.rs` around lines 78 - 110, Update module_cjs_global_paths_value to use USERPROFILE as the Windows fallback when HOME is unavailable, while preserving HOME-based entries elsewhere. Replace the hard-coded "/usr/local" default for PREFIX with a platform-appropriate execution-prefix fallback, keeping the existing PREFIX environment override and generated path structure intact.crates/perry-runtime/src/object/native_module/callable_exports/module_cjs.rs-390-405 (1)
390-405: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAccessor descriptors are installed as non-configurable.
install_accessorappliesPropertyAttrs::new(false, false, false)toconstructor,isPreloading, andparent. In Node these are configurable, andModule.prototype.constructoris also writable. Withconfigurable: false,Object.definePropertyanddeleteon those keys fail, andgetOwnPropertyDescriptorreports the wrong flags.🤖 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/callable_exports/module_cjs.rs` around lines 390 - 405, Update the accessor installation for Module prototype properties in the surrounding initialization code so constructor, isPreloading, and parent use configurable descriptors, while preserving parent’s setter and making constructor writable as required. Ensure getOwnPropertyDescriptor, Object.defineProperty, and delete reflect Node’s expected flags by adjusting the relevant install_accessor/property attribute handling rather than changing getter behavior.crates/perry-runtime/src/object/native_module/callable_export_arity_table.rs-238-239 (1)
238-239: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign
SourceMaparity with Node 26.
findPackageJSON(specifier[, base])has arity1becausebasehas a default value.module.findSourceMap(path)also has arity1. However,new SourceMap(payload[, { lineLengths }])omits thelineLengthsobject, so its Node 26lengthis2; also update the exhaustive reference entry.🤖 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/callable_export_arity_table.rs` around lines 238 - 239, Update the callable arity table entry for module SourceMap to return arity 2, reflecting the constructor’s optional second argument, while keeping findPackageJSON and findSourceMap at arity 1. Update the corresponding exhaustive reference entry for SourceMap as well.crates/perry-runtime/src/module_require.rs-410-433 (1)
410-433: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
link_parentsetsparentbut never appends the child toparent.children.The compile-side shim in
crates/perry/src/commands/compile/cjs_wrap/wrap.rspushes the child record intomodule.children(thelink_childfragment). The runtime path does not. A module reached throughrequire_paththerefore gets aparentlink while the parent'schildrenarray stays empty, so the two directions of the relationship disagree.The synthetic parent created on Line 424 also has only an
idfield and nochildrenarray, so appending requires allocating one. Add the array to the synthetic parent, then push the child record when it is not already present.🤖 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/module_require.rs` around lines 410 - 433, Update link_parent to initialize a children array on synthetic parents, then append the record to the resolved parent’s children array when it is not already present. Preserve the existing parent-link and cache behavior, and mirror the child insertion semantics used by the compile-side link_child fragment.crates/perry/src/commands/compile/cjs_wrap/wrap.rs-385-408 (1)
385-408: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThe parent handoff into a child module load is not exception-safe in either layer. Both layers publish the requiring module, load the child, then clear the published value on the next statement. Neither clears it when the child load exits abruptly. A
MODULE_NOT_FOUNDthrow or any user-level throw during child evaluation leaves the handoff set, and the next module that consumes it receives an unrelatedmodule.parent.try/catcharoundrequireis common in CommonJS, so the abrupt path is reachable in normal programs.
crates/perry/src/commands/compile/cjs_wrap/wrap.rs#L385-L408: wrap the emitted__perry_require_path_module(...)call intry { ... } finally { globalThis.__perry_cjs_pending_parent = undefined; }so the generated shim clears the global on both paths.crates/perry-runtime/src/module_require.rs#L443-L450: replace the set-call-take sequence with a guard type whoseDropclearsPENDING_REQUIRE_PARENT, so an unwind out ofjs_require_path_modulecannot leave a stale parent filename in the thread-local.🤖 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 385 - 408, Make both parent-handoff layers exception-safe: in crates/perry/src/commands/compile/cjs_wrap/wrap.rs lines 385-408, update the emitted code around __perry_require_path_module to clear globalThis.__perry_cjs_pending_parent in a finally block; in crates/perry-runtime/src/module_require.rs lines 443-450, replace the set-call-take sequence in js_require_path_module with a guard type whose Drop clears PENDING_REQUIRE_PARENT during unwinding. Ensure normal and abrupt child loads leave no stale parent handoff.crates/perry/src/commands/compile/collect_modules/static_require_transform.rs-439-441 (1)
439-441: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the orphaned doc comment.
This doc comment describes the relative-specifier existence helper that the change deleted. It now documents whatever item follows it, and its text ("Non-relative specifiers return
trueso they keep today's hoisting") no longer matches any code in the file. Delete it, or move it ontoresolve_static_requirewith corrected wording.🤖 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/collect_modules/static_require_transform.rs` around lines 439 - 441, Remove the orphaned doc comment preceding the deleted relative-specifier helper, since it no longer documents the following code. Do not move it unless attaching it to resolve_static_require with wording that accurately describes that function’s current behavior.crates/perry-runtime/src/process/node_module.rs-1083-1103 (1)
1083-1103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve
loadhook results for dynamic import parity.
js_module_dynamic_import_apply_hookscalls theloadchain at lines 1083-1100, but discards its value and returns the resolved URL unchanged. Add one of the following:
- If this API is intentionally observe-only, document that
loadhooks receive only call-order/deregistration semantics and cannot rewrite the imported module.- If the hook result should affect execution, pass the returned
{ url, source, format }from the activeloadchain into the dynamic-import emitter instead of ignoring it.🤖 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/process/node_module.rs` around lines 1083 - 1103, Update js_module_dynamic_import_apply_hooks so the result of the active load_chain is not discarded: capture the returned { url, source, format } value and pass it into the dynamic-import emitter so load hooks can rewrite module execution. If this API is intentionally observe-only, instead document that contract explicitly and preserve the unchanged URL behavior.crates/perry-runtime/src/process/node_module.rs-505-512 (1)
505-512: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
ERR_INVALID_ARG_TYPEfor non-string specifiers.Node rejects a non-
string/non-URLspecifierinmodule.findPackageJSON(specifier)withTypeError [ERR_INVALID_ARG_TYPE]. KeepERR_MODULE_NOT_FOUNDonly for the empty-string resolution failure so type errors are not mixed with search failures.🤖 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/process/node_module.rs` around lines 505 - 512, Update the specifier validation around js_string_key_bytes so non-string/non-URL values throw a TypeError with code ERR_INVALID_ARG_TYPE, while preserving ERR_MODULE_NOT_FOUND exclusively for empty-string package resolution failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b6b5ae57-657b-4c16-93b1-37688bfbe354
📒 Files selected for processing (35)
changelog.d/7312-node-module-node26-parity.mdcrates/perry-codegen/src/collectors/cjs_scaffolding.rscrates/perry-codegen/src/expr/dyn_extern_i18n.rscrates/perry-codegen/src/expr/property_get.rscrates/perry-codegen/src/lower_call/native_module_dispatch.rscrates/perry-codegen/src/lower_call/native_table/node_core/module_sea_tls_test.rscrates/perry-codegen/src/runtime_decls/objects.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-codegen/src/runtime_decls/strings_part2.rscrates/perry-hir/src/dynamic_import.rscrates/perry-hir/src/dynamic_import/top_level_await.rscrates/perry-hir/src/lower/expr_call/native_module/imported_module_dispatch.rscrates/perry-hir/src/lower/expr_new.rscrates/perry-runtime/src/module_require.rscrates/perry-runtime/src/object/descriptors.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rscrates/perry-runtime/src/object/namespace_create.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/native_module/callable_export_arity_table.rscrates/perry-runtime/src/object/native_module/callable_exports.rscrates/perry-runtime/src/object/native_module/callable_exports/module_cjs.rscrates/perry-runtime/src/object/native_module/module_keys.rscrates/perry-runtime/src/object/native_module_dispatch/dispatch_m_p.rscrates/perry-runtime/src/object/to_string_tag.rscrates/perry-runtime/src/process.rscrates/perry-runtime/src/process/env_misc.rscrates/perry-runtime/src/process/node_module.rscrates/perry-runtime/src/process/node_module/source_map.rscrates/perry-runtime/src/symbol/get.rscrates/perry/src/commands/compile/cjs_wrap/wrap.rscrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/collect_modules/discovery.rscrates/perry/src/commands/compile/collect_modules/static_require_transform.rscrates/perry/src/commands/compile/run_pipeline.rscrates/perry/src/commands/compile/types.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/perry-runtime/src/module_require.rs (3)
619-648: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReload
handler_ptrafter the allocatingstring_valuecall.
handler_ptr(Line 632-633) is a rawusizeextracted fromhandler_handlebeforestring_value(&filename)runs (Line 639).string_valuecan allocate and trigger GC. If the closure is moved during that allocation,handler_ptris stale, andjs_closure_call2at Line 640-644 dereferences a dangling pointer. Reload the pointer fromhandler_handleright before the call, matching the pattern already used forfactory_handleearlier in this file.🔒️ Proposed fix
let handler_handle = scope.root_nanbox_f64(f64::from_bits(handler.bits())); let handler_ptr = crate::value::js_nanbox_get_pointer(handler_handle.get_nanbox_f64()) as usize; if !handler.is_pointer() || !crate::closure::is_closure_ptr(handler_ptr) { crate::process::module_throw_plain_type_error( "Module._extensions[extension] is not a function", ); } let filename_value = string_value(&filename); + let handler_ptr = + crate::value::js_nanbox_get_pointer(handler_handle.get_nanbox_f64()) as usize; crate::closure::js_closure_call2( handler_ptr as *mut ClosureHeader, record_handle.get_nanbox_f64(), filename_value, );Based on learnings, "if you hold an object/value represented as a NaN-boxed
f64and you then perform an allocating or user-code-invoking operation... root the value usingcrate::gc::RuntimeHandleScopeand reload it from the rewritten handle (e.g., viaget_nanbox_f64()) before any subsequent reuse."🤖 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/module_require.rs` around lines 619 - 648, In run_custom_extension, do not reuse the handler_ptr extracted before string_value(&filename), since that call may move the closure during GC. Reload the closure pointer from the rooted handler_handle immediately before js_closure_call2, preserving the existing validation and invocation flow.Source: Learnings
518-572: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove the require-cache guard before
__perry_cjs_factoryexecution.AOT CJS wrappers register the module early while
loadedisfalse, then setloaded = trueonly after the wrapper runs. For a cyclicrequire, the nested call can hit this AOT factory branch and call the same factory while the parent wrapper is still executing. Return the early-registered pending module record fromrequire_path(or markloadedpending) before invokingjs_closure_call0(factory), and let cached-exports handling apply after the factory returns.🤖 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/module_require.rs` around lines 518 - 572, Move the cached_record guard in require_path ahead of js_closure_call0 so cyclic requires return the already-registered pending module record before invoking __perry_cjs_factory. Preserve link_parent and run_custom_extension for this early-return path, and retain the existing cached-exports handling after the factory completes.
412-495: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCache the synthesized placeholder parent.
When
cached_parentis undefined,link_parentallocates a placeholder and returns it, but it never writes it back intocache_handleundercache_key. Repeated requires from the same uncached parent, such as ambientrequire('a')followed byrequire('b'), therefore get distinctmodule.parentobjects with separatechildrenarrays. Store the placeholder incache_handleafter allocation so siblings share one parent object.🤖 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/module_require.rs` around lines 412 - 495, Update link_parent’s cached_parent.is_undefined() branch to store the newly allocated placeholder parent in cache_handle under cache_key before returning it, while preserving the existing parent initialization and reuse behavior for cached entries.
🤖 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.
Outside diff comments:
In `@crates/perry-runtime/src/module_require.rs`:
- Around line 619-648: In run_custom_extension, do not reuse the handler_ptr
extracted before string_value(&filename), since that call may move the closure
during GC. Reload the closure pointer from the rooted handler_handle immediately
before js_closure_call2, preserving the existing validation and invocation flow.
- Around line 518-572: Move the cached_record guard in require_path ahead of
js_closure_call0 so cyclic requires return the already-registered pending module
record before invoking __perry_cjs_factory. Preserve link_parent and
run_custom_extension for this early-return path, and retain the existing
cached-exports handling after the factory completes.
- Around line 412-495: Update link_parent’s cached_parent.is_undefined() branch
to store the newly allocated placeholder parent in cache_handle under cache_key
before returning it, while preserving the existing parent initialization and
reuse behavior for cached entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a1482bc-be29-4b1b-a7ad-af42ce0c08a0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlcrates/perry-hir/src/lower/expr_call/native_module/imported_module_dispatch.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/module_require.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/native_module/callable_exports/module_cjs.rscrates/perry-runtime/src/process.rscrates/perry-runtime/src/process/node_module.rscrates/perry-runtime/src/process/node_module/source_map.rscrates/perry-runtime/src/symbol/get.rscrates/perry-runtime/src/url/node_compat.rscrates/perry/src/commands/compile/cjs_wrap/wrap.rscrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/collect_modules/static_require_transform.rscrates/perry/src/commands/compile/run_pipeline.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/perry-runtime/src/symbol/get.rs
- crates/perry/src/commands/compile/run_pipeline.rs
- crates/perry-hir/src/lower/expr_call/native_module/imported_module_dispatch.rs
- crates/perry/src/commands/compile/collect_modules.rs
- crates/perry/src/commands/compile/collect_modules/static_require_transform.rs
- crates/perry-runtime/src/object/native_module/callable_exports/module_cjs.rs
- crates/perry-runtime/src/process.rs
- crates/perry/src/commands/compile/cjs_wrap/wrap.rs
- crates/perry-runtime/src/object/native_module.rs
- crates/perry-runtime/src/process/node_module.rs
- crates/perry-runtime/src/process/node_module/source_map.rs
…6-parity # Conflicts: # crates/perry-codegen/src/gc_map.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/perry/src/commands/compile/collect_modules/static_require_transform.rs (1)
363-380: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftComplete the exports selection before resolving targets.
resolve_require_exports()returns a target for every key whenexportsis a string or array, and it falls back to root conditions when no exact./features/*entry exists. As a result,require("pkg/features/foo")may resolve only"."when"."is the only root export, andpackage_subpath_is_blocked()returns the same incorrect allow/block decision.Match
".", the exact subpath, or a matching export pattern first, then iterate declared child maps in order with the standard CJS condition set includingnode-addons,module-sync,require,node, anddefault.🤖 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/collect_modules/static_require_transform.rs` around lines 363 - 380, The resolve_require_exports function must select the appropriate export entry before resolving its target: only treat "." or the exact key as direct matches, and support matching export patterns for subpaths instead of returning arbitrary string/array or root-condition targets. For nested child maps, iterate declarations in order using the CJS conditions node-addons, module-sync, require, node, and default, then resolve the selected target so package_subpath_is_blocked receives the correct result.crates/perry-codegen/src/expr/index_set.rs (1)
1610-1734: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftRoot
obj_boxandidx_boxbefore loweringvaluein the dynamic index-set fallback.
obj_boxandidx_boxare lowered beforelower_value_for_dynamic_index_set, then live through allocation points inside that call. That leaves the receiver/key registers naming from-space after a GC move. Add a receiver guard forobject, a non-literal key guard forindex, re-read them after the value lowering, and release the key guard before the receiver guard.🤖 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/index_set.rs` around lines 1610 - 1734, In the dynamic index-set fallback, update the lowering flow around lower_value_for_dynamic_index_set to guard object and non-literal index roots before lowering value, then re-lower or re-read obj_box and idx_box afterward so they reference current-space values. Release the index guard before releasing the receiver guard, while preserving the existing symbol/string/numeric dispatch logic.Source: Learnings
crates/perry-runtime/src/gc/roots/stack_maps.rs (2)
491-512: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUse the encoded entry width for record counts.
When header bit 0 is clear,
entryis 12 bytes. Line 528 still uses a 16-byte stride and offset. It reads an instruction offset instead of the function record count.The decoder then computes the wrong stream start and rejects or misdecodes every ILP32 map. The collector receives no valid native roots.
Use
entryfor the stride andentry - 4for the record-count offset. Add a 32-bit-entry parser test.🤖 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/gc/roots/stack_maps.rs` around lines 491 - 512, Update the stack-map decoder’s function-table parsing to use the computed entry width: apply entry as the record stride and entry - 4 as the record-count field offset, including the logic around stream-start calculation and record decoding. Preserve the existing 64-bit behavior while correctly parsing 32-bit entries, and add a parser test covering a 32-bit-entry map.Source: Coding guidelines
280-319: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winDecode shifted AArch64 stack adjustments.
The masks require the shift bit to be zero. AArch64
sub sp, sp,#imm, lsl#12`` does not match this branch.For a trailing shifted allocation,
fp_to_sp_offsetstops early and returns a stack-pointer offset that is too small. The fast walker then visits incorrect slots and can miss live roots.Ignore the shift bit in the opcode masks. Apply the shift when decoding the immediate. Handle the same encoding for
add x29, sp,#imm`` if it occurs.🤖 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/gc/roots/stack_maps.rs` around lines 280 - 319, Update the AArch64 instruction masks and immediate decoding in the stack-map prologue scan around ADD_FP_SP_MASK, ADD_FP_SP_PATTERN, SUB_SP_SP_MASK, and SUB_SP_SP_PATTERN to ignore the shift bit and support both immediate shift modes. Decode the 12-bit immediate and apply the encoded lsl `#12` shift for add x29, sp and trailing sub sp, sp instructions so fp_offset accumulates the true byte offset.Source: Coding guidelines
🤖 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/gc_map.rs`:
- Around line 898-900: Update the arch_supported logic in the target
classification flow to use an explicit platform-and-pointer-width allowlist
matching the runtime loader and walkers. Exclude arm64_32 targets before any
arm64 prefix match, and reject otherwise unsupported operating-system targets
rather than enabling native GC maps based solely on architecture prefixes.
In `@crates/perry-hir/src/lower/expr_member.rs`:
- Around line 419-427: Remove the process.namespace exception from the guard in
the member-lowering path around process_metadata_native_property, so namespace
imports no longer use process_native_property for sourceMapsEnabled. Ensure
p.sourceMapsEnabled falls through to the existing live NativeMethodCall branch,
matching bare process and globalThis.process behavior while preserving other
metadata property handling.
---
Outside diff comments:
In `@crates/perry-codegen/src/expr/index_set.rs`:
- Around line 1610-1734: In the dynamic index-set fallback, update the lowering
flow around lower_value_for_dynamic_index_set to guard object and non-literal
index roots before lowering value, then re-lower or re-read obj_box and idx_box
afterward so they reference current-space values. Release the index guard before
releasing the receiver guard, while preserving the existing
symbol/string/numeric dispatch logic.
In `@crates/perry-runtime/src/gc/roots/stack_maps.rs`:
- Around line 491-512: Update the stack-map decoder’s function-table parsing to
use the computed entry width: apply entry as the record stride and entry - 4 as
the record-count field offset, including the logic around stream-start
calculation and record decoding. Preserve the existing 64-bit behavior while
correctly parsing 32-bit entries, and add a parser test covering a 32-bit-entry
map.
- Around line 280-319: Update the AArch64 instruction masks and immediate
decoding in the stack-map prologue scan around ADD_FP_SP_MASK,
ADD_FP_SP_PATTERN, SUB_SP_SP_MASK, and SUB_SP_SP_PATTERN to ignore the shift bit
and support both immediate shift modes. Decode the 12-bit immediate and apply
the encoded lsl `#12` shift for add x29, sp and trailing sub sp, sp instructions
so fp_offset accumulates the true byte offset.
In
`@crates/perry/src/commands/compile/collect_modules/static_require_transform.rs`:
- Around line 363-380: The resolve_require_exports function must select the
appropriate export entry before resolving its target: only treat "." or the
exact key as direct matches, and support matching export patterns for subpaths
instead of returning arbitrary string/array or root-condition targets. For
nested child maps, iterate declarations in order using the CJS conditions
node-addons, module-sync, require, node, and default, then resolve the selected
target so package_subpath_is_blocked receives the correct result.
🪄 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: 6dd0ac79-1e66-46ff-9fd2-769c43b849ba
📒 Files selected for processing (22)
crates/perry-codegen/src/dialect/mod.rscrates/perry-codegen/src/expr/index_set.rscrates/perry-codegen/src/expr/index_set_helpers.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/gc_map.rscrates/perry-codegen/src/linker.rscrates/perry-codegen/src/statepoint_report.rscrates/perry-codegen/src/stmt/mod.rscrates/perry-hir/src/lower/expr_member.rscrates/perry-runtime/src/eh.rscrates/perry-runtime/src/eh_walker.rscrates/perry-runtime/src/gc/roots.rscrates/perry-runtime/src/gc/roots/stack_maps.rscrates/perry-runtime/src/module_require.rscrates/perry-runtime/src/object/global_this/fetch_globals.rscrates/perry-runtime/src/process/node_module.rscrates/perry-runtime/src/process/node_module/source_map.rscrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/collect_modules/static_require_transform.rscrates/perry/src/commands/compile/run_pipeline.rscrates/perry/src/commands/compile/types.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/perry/src/commands/compile/types.rs
- crates/perry/src/commands/compile/run_pipeline.rs
- crates/perry/src/commands/compile/collect_modules.rs
- crates/perry-runtime/src/process/node_module/source_map.rs
- crates/perry-runtime/src/module_require.rs
…6-parity # Conflicts: # crates/perry-codegen/src/expr/index_set.rs # crates/perry-codegen/src/expr/mod.rs
|
Heads-up (not something to fix on your side): the failing The fix is up in #7384. Once it merges, re-running this arm on your branch (or rebasing onto the updated |
2 participants
Summary
Complete Perry's Node.js 26.5.0 compatibility for
node:moduleacross CommonJS, loader hooks, SourceMap, resolution, and builtin-export identity.Changes
Module/CommonJS surface, cache lifecycle, cycles, JSON/CJS loading, parent/children links, and package resolution.syncBuiltinESMExports()behavior.main(305698688) and fix the resulting parity, warnings, GC inventory, compiler-output metadata, and file-size regressions.Related issue
Closes #6769
Validation
Final node:module report (
parity_report_20260804_115429.json):Focused regressions also pass:
test_gap_node_module_3118plustest_gap_process_misc_3045plustest_gap_diagchannel_3082_3084_3085_3086test_gap_6301_event_target_subclassThe public benchmark freshness check remains red on current
main: both the source and harness fingerprints already differ from the committed artifact. This PR does not modify benchmark inputs, expected results, or benchmark runners.Checklist
node:modulegate passes with zero non-PASS counters