fix(vm): complete Node 26 parity - #7382
Open
TheHypnoo wants to merge 4 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughNode VM parity now covers context options, dynamic scripts, direct ChangesNode VM parity
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant VM_API
participant NodeVM
participant DynEval
participant RealmIntrinsics
participant ModuleNamespace
VM_API->>NodeVM: create context or compile source
NodeVM->>DynEval: execute script, eval, or module source
DynEval->>RealmIntrinsics: resolve globals and attach prototypes
DynEval-->>NodeVM: return value or evaluation error
NodeVM->>ModuleNamespace: populate exports and return evaluation promise
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 10
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/dyn_eval/env.rs (1)
279-317: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGC-managed values are cached in plain Rust locals and reused after allocating calls. All three sites copy a NaN-boxed
f64into an ordinary local, then call a function that can allocate and relocate the referenced object, then reuse the stale copy. Rust locals are neither GC roots nor pins in this runtime, so the copy is not rewritten when the object moves. The shared remediation is to reload the value from its rooted slot orRuntimeHandleScopehandle at every use after a possible collection point.
crates/perry-runtime/src/dyn_eval/env.rs#L279-L317: drop thecurrentlocal and readroot_get(cur_idx)at lines 280, 299, and 311, matching the other uses in the same loop.crates/perry-runtime/src/dyn_eval/env.rs#L398-L411: re-derive the bindings object withenv_object_bindings(root_get(cur_idx))before eachobject_write_bindingcall, becauseobject_has_bindingallocates an interned key string.crates/perry-runtime/src/object/class_registry/construct.rs#L153-L162: drop theconstructor_valuelocal and passconstructor.get_nanbox_f64()tosynthetic_class_id_for_functionandinstall_script_prototypes.🤖 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/dyn_eval/env.rs` around lines 279 - 317, Reload GC-managed values from rooted storage after any allocating call instead of reusing stale Rust locals: in crates/perry-runtime/src/dyn_eval/env.rs:279-317, remove current and use root_get(cur_idx) at the indicated accesses; in crates/perry-runtime/src/dyn_eval/env.rs:398-411, re-derive env_object_bindings(root_get(cur_idx)) before each object_write_binding call, while object_has_binding may allocate; in crates/perry-runtime/src/object/class_registry/construct.rs:153-162, remove constructor_value and pass constructor.get_nanbox_f64() directly to synthetic_class_id_for_function and install_script_prototypes.Sources: Coding guidelines, Learnings
crates/perry-runtime/src/object/descriptors.rs (1)
1627-1632: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
vmnamespace properties configurable except when the module is specificallyvm.constants.The current condition sets
configurable: falsefor everyvmkey, includingrunInNewContext, soObject.defineProperty(vm, 'runInNewContext', ...)anddelete vm.runInNewContextfail unlike Node. This does not affectfs.constantskeys, which are explicitly handled above and not covered by the tail case.🤖 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 1627 - 1632, Update the configurable argument in the build_data_descriptor call for the vm namespace tail case so properties are configurable for vm and non-vm modules, but remain non-configurable when module_name is exactly "vm.constants". Preserve the separate fs.constants handling above.crates/perry-runtime/src/node_vm.rs (1)
1496-1524: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUnrooted heap values cross allocating writes in both VM metadata-cache registration paths. Both paths allocate an object or closure, register it in a process-global metadata map, and then perform further allocating field writes while holding only a bare raw pointer or NaN-boxed
f64local. Rust stack locals are not scanned and raw pointer locals are neither roots nor pins, so a moving collection during those writes makes the local stale.scan_vm_roots_mutrepairs the map keys throughvisit_metadata_usize_slot, but it cannot repair the locals or the returned value.
crates/perry-runtime/src/node_vm.rs#L1496-L1524: root thejs_object_allocresult throughRuntimeHandleScope, derive the map key and everyset_fieldreceiver from the reloaded handle, and build the returnedvalueafter the final write.crates/perry-runtime/src/node_vm.rs#L1739-L1757: root thewith_source_locationresult, reload it beforeset_builtin_closure_length, before thecompiled_function_sourcesinsert, and before eachset_value_fieldcall.As per coding guidelines, GC-managed values must remain rooted across every possible collection point, and root stores must dominate subsequent allocating sites.
🤖 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/node_vm.rs` around lines 1496 - 1524, Root the object returned by js_object_alloc in the node_vm.rs metadata-cache path at lines 1496-1524 with RuntimeHandleScope, reload the handle for the metadata-map key and every set_field receiver, and construct the returned value only after the final allocating write. In the sibling node_vm.rs path at lines 1739-1757, root the with_source_location result and reload it before set_builtin_closure_length, the compiled_function_sources insert, and each set_value_field call; preserve these roots across every possible collection point.Source: Coding guidelines
🧹 Nitpick comments (7)
crates/perry-runtime/src/dyn_eval/tests.rs (2)
993-997: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the named Promise class-id constant.
Line 995 hardcodes
0xFFFF_0027.crates/perry-runtime/src/object/instanceof.rsdefinesCLASS_ID_PROMISEfor this value. If the reserved id changes, the literal silently tests the wrong class. Reference the constant, as the neighbouring assertion already does withcrate::error::CLASS_ID_TYPE_ERROR.🤖 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/dyn_eval/tests.rs` around lines 993 - 997, The js_instanceof call in the assertion hardcodes the Promise class id as 0xFFFF_0027, but the constant CLASS_ID_PROMISE is already defined in crate::object and should be referenced instead. Replace the hardcoded literal 0xFFFF_0027 with crate::object::CLASS_ID_PROMISE to ensure the test automatically stays in sync if the class id constant is updated.
1000-1012: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for strict-mode object-environment writes.
This PR threads a new
strictflag throughenv::assign,object_write_binding, andbridge::set_index. The added tests exercise only sloppy writes.Add a test that evaluates a
"use strict"script whose assignment targets a non-writable or frozen sandbox property, and assert that aTypeErroris thrown. Add a matching sloppy-mode test that asserts the write is silently ignored. This pins the behavior of the new parameter at both ends.Do you want me to draft these tests?
🤖 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/dyn_eval/tests.rs` around lines 1000 - 1012, Add two new tests in the tests.rs file to cover strict-mode object-environment writes with the new strict flag threaded through env::assign, object_write_binding, and bridge::set_index. First, create a test that evaluates a "use strict" script attempting to assign to a non-writable or frozen sandbox property and asserts that a TypeError is thrown. Second, add a corresponding sloppy-mode test using the same assignment target that asserts the write is silently ignored instead, verifying the behavior difference between the two modes.crates/perry-runtime/src/dyn_eval/expr.rs (1)
129-137: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRemove the duplicated global lookup.
global_has_owncallsbridge::global_has_property, then callsbridge::global_lookup, which callsglobal_has_propertyagain internally. Each call unwraps the proxy and performs a property lookup.
eval_identalready computedglobal_lookupbefore reaching line 123, andeval_unarycallsglobal_has_ownfor every unresolvedtypeofoperand. Pass the already-computed value instead of recomputing it.♻️ Proposed refactor
-fn global_has_own(ctx: &Ctx, name: &str) -> bool { - let global = root_get(ctx.global_idx); - bridge::global_has_property(global, name) - || !bridge::is_undefined(bridge::global_lookup( - global, - root_get(ctx.intrinsics_idx), - name, - )) +fn global_has_own(ctx: &Ctx, name: &str) -> bool { + let global = root_get(ctx.global_idx); + if bridge::global_has_property(global, name) { + return true; + } + !bridge::is_undefined(bridge::global_lookup( + global, + root_get(ctx.intrinsics_idx), + name, + )) }
eval_identcan then skip the second probe entirely, because a non-undefinedglobal_lookupresult already returned at line 121.🤖 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/dyn_eval/expr.rs` around lines 129 - 137, The global_has_own function performs redundant property lookups by calling bridge::global_lookup internally, which already calls bridge::global_has_property. Since eval_ident and eval_unary have already computed the global_lookup result before calling global_has_own, refactor the function to accept the pre-computed lookup result as a parameter instead of recomputing it. Update global_has_own to use the passed-in lookup value to determine if the property exists globally, and update all callers (eval_ident and eval_unary) to pass the already-computed global_lookup result. This eliminates the duplicate bridge::global_has_property and bridge::global_lookup calls within global_has_own.crates/perry-runtime/src/node_vm.rs (1)
1098-1112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
options.timeoutis validated but not enforced.
validate_run_optionsvalidatestimeout,displayErrors, andbreakOnSigint, andexecute_in_stateignores all three. A caller that passestimeoutto bound runaway script code receives no time bound. Add a short comment that records the gap, so a later reader does not assume enforcement exists.🤖 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/node_vm.rs` around lines 1098 - 1112, Add a short comment near validate_run_options or the relevant execute_in_state call documenting that options.timeout, displayErrors, and breakOnSigint are validated but not enforced during execution, so timeout does not bound runaway scripts.crates/perry-codegen/src/lower_call/native_table/node_misc.rs (1)
179-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale comment above the
vm.createContextrow.The comment states that the surface covers "APIs that require a vm context object but do not execute code inside it yet". This PR makes contexts execute code, so the comment now contradicts the behavior. The signature change itself matches
js_vm_create_context(sandbox: f64, options: f64)and the&[DOUBLE, DOUBLE]declaration incrates/perry-codegen/src/runtime_decls/stdlib_ffi/net_http.rs.🤖 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/lower_call/native_table/node_misc.rs` around lines 179 - 190, Update the comment above the `NativeModSig` entry for `vm.createContext` to reflect that contexts now execute code, removing the stale statement that they do not execute code inside them. Leave the signature and runtime mapping unchanged.crates/perry-runtime/src/value/to_string.rs (1)
1022-1025: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared "VM compiled source, else
func_ptrsource" fallback. The same three-step chain is now copied into three files: look upnode_vm::compiled_function_source_for_closure(addr), otherwise readfunc_ptrfrom theClosureHeaderand callbuiltins::function_source_for_func_ptr. Each copy is correct, but a future change to the precedence must be applied three times, and the copies can drift.Add one helper, for example
crate::builtins::function_source_for_closure_addr(addr: usize) -> String, and call it from all three sites.
crates/perry-runtime/src/value/to_string.rs#L1022-L1025: replace the lookup and the followingfunc_ptrfallback with the shared helper.crates/perry-runtime/src/object/global_this/array_error.rs#L567-L571: replace theunwrap_or_elsechain with the shared helper.crates/perry-runtime/src/object/native_call_method/primitive_methods.rs#L125-L131: replace theunwrap_or_elsechain with the shared helper.🤖 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/value/to_string.rs` around lines 1022 - 1025, Extract the duplicated closure-source precedence into builtins::function_source_for_closure_addr(addr: usize) -> String, performing the compiled-function lookup followed by the ClosureHeader func_ptr fallback. Replace the existing chains at crates/perry-runtime/src/value/to_string.rs:1022-1025, crates/perry-runtime/src/object/global_this/array_error.rs:567-571, and crates/perry-runtime/src/object/native_call_method/primitive_methods.rs:125-131 with calls to this helper.crates/perry-runtime/src/object/native_module_dispatch/dispatch_v_z.rs (1)
160-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the
createScriptbranding decision.Branding now lives in two places. The codegen native table routes
createScripttojs_vm_create_script_branded, which brands internally, and this dispatcher appliesbrand_vm_script_instanceagain on its own path. A future VM method that needs branding must be changed in both files, and the two lists can drift.Move the branding into
crate::node_vm::dispatch_vm_methodfor the"createScript"arm, and let this dispatcher returndispatch_vm_methodunchanged.js_vm_create_script_brandedcan then call the same branded helper.♻️ Proposed simplification
- ("vm", m) => { - let value = crate::node_vm::dispatch_vm_method(m, arg(0), arg(1), arg(2)); - if m == "createScript" { - crate::object::class_registry::brand_vm_script_instance(value) - } else { - value - } - } + ("vm", m) => crate::node_vm::dispatch_vm_method(m, arg(0), arg(1), arg(2)),Then brand inside
dispatch_vm_method's"createScript"arm.🤖 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_dispatch/dispatch_v_z.rs` around lines 160 - 167, Move the createScript branding logic into the "createScript" arm of crate::node_vm::dispatch_vm_method, ensuring it returns the branded VM script instance. Update the "vm" branch here to return dispatch_vm_method’s result unchanged, removing the local brand_vm_script_instance call while preserving other method behavior.
🤖 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/lower_call/native_table/node_core/inspector_vm.rs`:
- Line 313: Add an LLVM declaration entry alongside the existing native runtime
declarations in inspector_vm.rs for the js_vm_create_script_branded runtime
symbol, matching its external C signature with a DOUBLE return type and two
DOUBLE parameters so the emitted call has a declared FFI import.
In `@crates/perry-runtime/src/dyn_eval/expr.rs`:
- Around line 722-736: Update the direct-eval handling in eval_call to recreate
the caller’s call context when invoking eval_script_in, using the appropriate
fresh variable environment so strict and sloppy eval declarations follow the
correct lexical and var-binding rules. Also enforce the
context.codeGeneration.strings permission before evaluating the source, ensuring
generated-string paths such as compileFunction cannot bypass the
disabled-strings check.
In `@crates/perry-runtime/src/dyn_eval/mod.rs`:
- Around line 292-318: Correct the documentation above script_environment so it
states that the last object_envs entry has the highest lookup precedence,
matching object_environment_chain and the existing function_from_strings_in
description; leave the implementation unchanged.
In `@crates/perry-runtime/src/node_vm.rs`:
- Around line 1341-1348: Update execute_in_state and the dyn_eval evaluation
path to pass strings_allowed into dyn_eval, enforcing the restriction at the
eval/Function intrinsic entry points instead of scanning source text; remove the
substring-based rejection so strings in comments or literals remain valid. Also
propagate state.wasm_allowed through the execution path and enforce the wasm
restriction where wasm evaluation is initiated, rather than assigning it to the
unused _wasm_allowed variable.
- Around line 1896-1908: Update the documentation above
prune_dead_vm_owner_entries to reflect that VM_CONTEXTS is thread-local and only
the calling thread’s contexts are pruned. State that foreign-thread VM_CONTEXTS
entries are unreachable from this call, while the process-global residual
applies only to VM_SCRIPTS and VM_COMPILED_FUNCTION_SOURCES.
- Around line 940-970: Update install_module_accessor after storing the module
in capture slot 0 to register the closure’s one-slot capture layout and trigger
the post-store GC barrier, using the existing capture-layout helper or
rebuild_closure_layout_and_barriers with closure and 1. Ensure the captured
module pointer is traceable and rewriteable before the accessor closure is
returned.
- Around line 809-820: Update the statement processing in the filter_map chain
to strip only the `export ` prefix (preserving declaration keywords like
`const`, `let`, `var`, `function`, `class`) instead of stripping the full
declaration keyword prefixes. After evaluating the resulting executable
statements, read the exported declarations from the evaluated environment
context and populate them into the namespace so that all export forms (export
const, export function, export class, export { ... }) are properly available.
- Around line 59-65: The thread-local MAIN_CONTEXT is shared across different
execution contexts while scan_vm_roots_mut is registered only per-thread,
creating a mismatch that can leave stale raw pointers after GC relocations.
Ensure MAIN_CONTEXT is properly handled in the GC root scanning path and
includes appropriate relocation or forwarding logic when accessed from different
threads. Add integration tests that verify VM contexts survive minor copy/move
GC operations and add tests that re-root contexts across different threads to
confirm no stale pointers remain after relocation.
In `@crates/perry-runtime/src/object/class_registry/construct.rs`:
- Around line 163-173: Update the pointer-validation guard surrounding the
unsafe class_id assignment to also verify the allocation’s GC type is
GC_TYPE_OBJECT, matching the established check in instanceof. Only write
(*object).class_id after confirming the pointer is non-null, valid, above the
handle band, and specifically an object allocation; preserve all existing checks
and behavior otherwise.
In `@crates/perry-runtime/src/object/instanceof.rs`:
- Around line 74-81: Update recorded_prototype_instanceof_builtin to root value
with crate::gc::RuntimeHandleScope before calling
js_get_global_this_builtin_value, then reload the rooted value for
ordinary_has_instance_prototype_walk after lookup. Validate that the resolved
constructor is an object; return None when it is unresolved or non-object
instead of committing Some(false), while preserving the existing prototype-walk
result for valid constructors.
---
Outside diff comments:
In `@crates/perry-runtime/src/dyn_eval/env.rs`:
- Around line 279-317: Reload GC-managed values from rooted storage after any
allocating call instead of reusing stale Rust locals: in
crates/perry-runtime/src/dyn_eval/env.rs:279-317, remove current and use
root_get(cur_idx) at the indicated accesses; in
crates/perry-runtime/src/dyn_eval/env.rs:398-411, re-derive
env_object_bindings(root_get(cur_idx)) before each object_write_binding call,
while object_has_binding may allocate; in
crates/perry-runtime/src/object/class_registry/construct.rs:153-162, remove
constructor_value and pass constructor.get_nanbox_f64() directly to
synthetic_class_id_for_function and install_script_prototypes.
In `@crates/perry-runtime/src/node_vm.rs`:
- Around line 1496-1524: Root the object returned by js_object_alloc in the
node_vm.rs metadata-cache path at lines 1496-1524 with RuntimeHandleScope,
reload the handle for the metadata-map key and every set_field receiver, and
construct the returned value only after the final allocating write. In the
sibling node_vm.rs path at lines 1739-1757, root the with_source_location result
and reload it before set_builtin_closure_length, the compiled_function_sources
insert, and each set_value_field call; preserve these roots across every
possible collection point.
In `@crates/perry-runtime/src/object/descriptors.rs`:
- Around line 1627-1632: Update the configurable argument in the
build_data_descriptor call for the vm namespace tail case so properties are
configurable for vm and non-vm modules, but remain non-configurable when
module_name is exactly "vm.constants". Preserve the separate fs.constants
handling above.
---
Nitpick comments:
In `@crates/perry-codegen/src/lower_call/native_table/node_misc.rs`:
- Around line 179-190: Update the comment above the `NativeModSig` entry for
`vm.createContext` to reflect that contexts now execute code, removing the stale
statement that they do not execute code inside them. Leave the signature and
runtime mapping unchanged.
In `@crates/perry-runtime/src/dyn_eval/expr.rs`:
- Around line 129-137: The global_has_own function performs redundant property
lookups by calling bridge::global_lookup internally, which already calls
bridge::global_has_property. Since eval_ident and eval_unary have already
computed the global_lookup result before calling global_has_own, refactor the
function to accept the pre-computed lookup result as a parameter instead of
recomputing it. Update global_has_own to use the passed-in lookup value to
determine if the property exists globally, and update all callers (eval_ident
and eval_unary) to pass the already-computed global_lookup result. This
eliminates the duplicate bridge::global_has_property and bridge::global_lookup
calls within global_has_own.
In `@crates/perry-runtime/src/dyn_eval/tests.rs`:
- Around line 993-997: The js_instanceof call in the assertion hardcodes the
Promise class id as 0xFFFF_0027, but the constant CLASS_ID_PROMISE is already
defined in crate::object and should be referenced instead. Replace the hardcoded
literal 0xFFFF_0027 with crate::object::CLASS_ID_PROMISE to ensure the test
automatically stays in sync if the class id constant is updated.
- Around line 1000-1012: Add two new tests in the tests.rs file to cover
strict-mode object-environment writes with the new strict flag threaded through
env::assign, object_write_binding, and bridge::set_index. First, create a test
that evaluates a "use strict" script attempting to assign to a non-writable or
frozen sandbox property and asserts that a TypeError is thrown. Second, add a
corresponding sloppy-mode test using the same assignment target that asserts the
write is silently ignored instead, verifying the behavior difference between the
two modes.
In `@crates/perry-runtime/src/node_vm.rs`:
- Around line 1098-1112: Add a short comment near validate_run_options or the
relevant execute_in_state call documenting that options.timeout, displayErrors,
and breakOnSigint are validated but not enforced during execution, so timeout
does not bound runaway scripts.
In `@crates/perry-runtime/src/object/native_module_dispatch/dispatch_v_z.rs`:
- Around line 160-167: Move the createScript branding logic into the
"createScript" arm of crate::node_vm::dispatch_vm_method, ensuring it returns
the branded VM script instance. Update the "vm" branch here to return
dispatch_vm_method’s result unchanged, removing the local
brand_vm_script_instance call while preserving other method behavior.
In `@crates/perry-runtime/src/value/to_string.rs`:
- Around line 1022-1025: Extract the duplicated closure-source precedence into
builtins::function_source_for_closure_addr(addr: usize) -> String, performing
the compiled-function lookup followed by the ClosureHeader func_ptr fallback.
Replace the existing chains at
crates/perry-runtime/src/value/to_string.rs:1022-1025,
crates/perry-runtime/src/object/global_this/array_error.rs:567-571, and
crates/perry-runtime/src/object/native_call_method/primitive_methods.rs:125-131
with calls to this helper.
🪄 Autofix
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: cf942bb2-3c5c-42cc-983a-c0dd1c5d05c8
📒 Files selected for processing (24)
changelog.d/7382-node-vm-node26-parity.mdcrates/perry-codegen/src/lower_call/native_table/node_core/inspector_vm.rscrates/perry-codegen/src/lower_call/native_table/node_misc.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi/net_http.rscrates/perry-hir/src/lower/lower_expr/arm_bin.rscrates/perry-runtime/src/dyn_eval/bridge.rscrates/perry-runtime/src/dyn_eval/env.rscrates/perry-runtime/src/dyn_eval/expr.rscrates/perry-runtime/src/dyn_eval/interp.rscrates/perry-runtime/src/dyn_eval/mod.rscrates/perry-runtime/src/dyn_eval/tests.rscrates/perry-runtime/src/error.rscrates/perry-runtime/src/node_submodules/mod.rscrates/perry-runtime/src/node_vm.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/descriptors.rscrates/perry-runtime/src/object/global_this/array_error.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/native_call_method/primitive_methods.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/native_module/namespace_builders.rscrates/perry-runtime/src/object/native_module_dispatch/dispatch_v_z.rscrates/perry-runtime/src/value/to_string.rs
| method: "createScript", | ||
| class_filter: None, | ||
| runtime: "js_vm_create_script", | ||
| runtime: "js_vm_create_script_branded", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. LLVM declaration for the new symbol.
rg -n 'js_vm_create_script' crates/perry-codegen/src
# 2. Runtime definition and export.
rg -n -C3 'fn js_vm_create_script' crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 1302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== declarations matching create_script =="
rg -n 'js_vm_create_script|js_vm_create_script_branded|declare_function' crates -g '*.rs'
echo
echo "== inspector_vm native_table excerpt =="
sed -n '280,330p' crates/perry-codegen/src/lower_call/native_table/node_core/inspector_vm.rs
echo
echo "== linked runtime exports around native_module =="
rg -n -C6 '#\[no_mangle\]|pub extern "C" fn js_vm_create_script_branded|fn js_vm_create_script|declare_function' crates/perry-runtime/src/object/native_module.rs crates/perry-runtime/src/node_vm.rsRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact declaration/query occurrences =="
git -C crates/perry-codegen ls-files src | awk '/runtime_decls/ || /inspector_vm.rs/' | while read -r f; do
echo "--- $f"
rg -n 'js_vm_create_script(branded)?' "$f" || true
done
echo
echo "== focused runtime occurrences =="
rg -n -C3 'pub extern "C" fn js_vm_create_script|js_vm_create_script' crates/perry-runtime/src/object/native_module.rs crates/perry-runtime/src/node_vm.rs crates/perry-runtime/ || true
echo
echo "== tracked files containing create_script =="
git ls-files | xargs rg -n 'js_vm_create_script' || trueRepository: PerryTS/perry
Length of output: 8657
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== declaration file locations =="
fd -a '^(runtime_decls|vm)' crates/perry-codegen crates/perry-stdlib crates/perry-runtime 2>/dev/null | head -100
echo
echo "== exact js_vm_create_script occurrences in repository =="
git ls-files | xargs rg -n 'js_vm_create_script(branded)?' || true
echo
echo "== object/native_module.rs around branded wrapper =="
sed -n '90,130p' crates/perry-runtime/src/object/native_module.rs
echo
echo "== low-level native export declarations in inspector_vm.rs =="
sed -n '290,325p' crates/perry-codegen/src/lower_call/native_table/node_core/inspector_vm.rsRepository: PerryTS/perry
Length of output: 4069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runtime_declaration / fn_decl type definitions =="
rg -n 'type RuntimeDecl|struct RuntimeDecl|declare_function|fn_decl' crates/perry-codegen/src -g '*.rs' -C 4 | head -220 || true
echo
echo "== runtime_decls top-level declaration definitions =="
sed -n '1,120p' crates/perry-codegen/src/runtime_decls/mod.rs
sed -n '1,120p' crates/perry-codegen/src/runtime_decls/objects.rsRepository: PerryTS/perry
Length of output: 33469
Add an LLVM declaration for js_vm_create_script_branded.
crates/perry-codegen/src/lower_call/native_table/node_core/inspector_vm.rs now emits a native call to js_vm_create_script_branded, the runtime provides #[no_mangle] pub extern "C" fn js_vm_create_script_branded, but no LLVM declaration for it exists yet. Add the matching declaration, e.g. DOUBLE return with [DOUBLE, DOUBLE] params, so lowering has a declared FFI import.
🤖 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/lower_call/native_table/node_core/inspector_vm.rs`
at line 313, Add an LLVM declaration entry alongside the existing native runtime
declarations in inspector_vm.rs for the js_vm_create_script_branded runtime
symbol, matching its external C signature with a DOUBLE return type and two
DOUBLE parameters so the emitted call has a declared FFI import.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-runtime/src/dyn_eval/interp.rs (1)
357-370: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon’t bind
argumentsinside arrow call frames.The interpreter creates an
argumentsobject and defines it for everyinvoke_interp_fnframe, including arrow functions.make_function_valuealready tracksis_arrow, so add an arrow flag toInterpFnor pass the arrow context toinvoke_interp_fn, then omit theargumentsdefinition and let arrows inherit it from the enclosing scope.🤖 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/dyn_eval/interp.rs` around lines 357 - 370, Update the invoke_interp_fn frame setup to know whether the target function is an arrow, using the existing make_function_value is_arrow state or an equivalent InterpFn field. Only create and env::define the arguments object for non-arrow functions; arrow frames must omit their own binding so scope lookup inherits the enclosing arguments.crates/perry-runtime/src/dyn_eval/env.rs (1)
445-457: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThrow
ReferenceErrorfor strict unresolvable assignments before creating a binding.In strict mode,
assign()must not continue toobject_write_binding()orenv_write()after reaching the root with nopresentbinding.js_put_value_set()only rejects existing non-writable/locked properties and can create a miss;env_write()creates the own binding unconditionally. Throw viasuper::bridge::throw_reference_error(...)on the unresolvable root path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/dyn_eval/env.rs` around lines 445 - 457, Update the unresolvable root branch in assign() so strict assignments throw ReferenceError via super::bridge::throw_reference_error(...) before object_write_binding() or env_write() can create a binding. Preserve the existing binding writes for non-strict assignments and truncate roots on the handled return path.
🧹 Nitpick comments (1)
crates/perry-runtime/src/dyn_eval/mod.rs (1)
410-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompute the variable environment index once.
Lines 410-414 and Lines 419-423 contain the same conditional. The two expressions must stay in sync; a future edit to one changes the eval scoping semantics without changing the other.
Bind the index once and use it in both places.
♻️ Proposed refactor
let ret_idx = root_push(bridge::undefined()); + // Strict direct eval owns a fresh variable environment; sloppy direct + // eval publishes `var` bindings in the caller's variable environment. + let variable_env_idx = if strict { + lexical_env_idx + } else { + caller_variable_env_idx + }; let ctx = interp::Ctx { this_idx: global_idx, ret_idx, global_idx, intrinsics_idx, - variable_env_idx: if strict { - lexical_env_idx - } else { - caller_variable_env_idx - }, + variable_env_idx, strict, strings_allowed, wasm_allowed, }; - let variable_env_idx = if strict { - lexical_env_idx - } else { - caller_variable_env_idx - }; let _ = interp::exec_direct_eval_stmts(&ctx, &statements, lexical_env_idx, variable_env_idx);🤖 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/dyn_eval/mod.rs` around lines 410 - 423, In the surrounding dynamic evaluation setup, compute the strict-versus-nonstrict environment index once and bind it before constructing the environment configuration. Reuse that binding for both the `variable_env_idx` field and the later local variable, removing the duplicated conditional while preserving the existing strict scoping behavior.
🤖 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/dyn_eval/expr.rs`:
- Around line 854-861: Update the blocked WebAssembly rejection paths in the
expression evaluation logic, including both occurrences near the existing
checks, to call roots_truncate(obj_idx) before returning
bridge::wasm_codegen_rejection. Preserve the current rejection behavior and the
instantiate exception while ensuring root slots are truncated on every early
return.
- Around line 800-806: Update both member-call paths in
crates/perry-runtime/src/dyn_eval/expr.rs at lines 800-806 and 836-840: resolve
each callee once into a rooted slot using the existing member/index lookup,
classify that resolved value, and dispatch it directly instead of re-reading it
through call_method at line 828 or call_method_value at line 863. Preserve
single getter evaluation and existing codegen-blocking behavior in both paths.
- Around line 749-779: Update the direct-eval success path in the `Ident`/`eval`
handling to store the result of `super::eval_direct_in` before returning,
truncate the root stack using `resolved_idx`, then return the stored result.
Preserve the existing argument evaluation and error behavior while ensuring
every resolved eval slot is released before either return path.
In `@crates/perry-runtime/src/dyn_eval/interp.rs`:
- Around line 553-556: Update the hoist_fn_decls call in the eval setup to pass
ctx.strict as its declaration-mode argument instead of false, ensuring strict
direct eval function declarations are created in the fresh lexical environment
rather than leaking to the root environment.
In `@crates/perry-runtime/src/dyn_eval/mod.rs`:
- Around line 328-339: In crates/perry-runtime/src/dyn_eval/mod.rs lines
328-339, root_push global_this and intrinsics before calling
prepare_function_args, then use root_get values when constructing the closure
via alloc_interp_closure. In crates/perry-runtime/src/dyn_eval/mod.rs lines
396-402, move the root_push calls for global_this, intrinsics, caller_env, and
caller_variable_env above parse_script_statements, and use the rooted values
afterward so all GC-managed parameters remain valid across allocation points.
---
Outside diff comments:
In `@crates/perry-runtime/src/dyn_eval/env.rs`:
- Around line 445-457: Update the unresolvable root branch in assign() so strict
assignments throw ReferenceError via super::bridge::throw_reference_error(...)
before object_write_binding() or env_write() can create a binding. Preserve the
existing binding writes for non-strict assignments and truncate roots on the
handled return path.
In `@crates/perry-runtime/src/dyn_eval/interp.rs`:
- Around line 357-370: Update the invoke_interp_fn frame setup to know whether
the target function is an arrow, using the existing make_function_value is_arrow
state or an equivalent InterpFn field. Only create and env::define the arguments
object for non-arrow functions; arrow frames must omit their own binding so
scope lookup inherits the enclosing arguments.
---
Nitpick comments:
In `@crates/perry-runtime/src/dyn_eval/mod.rs`:
- Around line 410-423: In the surrounding dynamic evaluation setup, compute the
strict-versus-nonstrict environment index once and bind it before constructing
the environment configuration. Reuse that binding for both the
`variable_env_idx` field and the later local variable, removing the duplicated
conditional while preserving the existing strict scoping behavior.
🪄 Autofix
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: cf04f90d-1491-4fa0-bd22-7f379dbc15f0
📒 Files selected for processing (16)
crates/perry-codegen/src/lower_call/native_table/node_misc.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi/net_http.rscrates/perry-runtime/src/dyn_eval/bridge.rscrates/perry-runtime/src/dyn_eval/env.rscrates/perry-runtime/src/dyn_eval/expr.rscrates/perry-runtime/src/dyn_eval/interp.rscrates/perry-runtime/src/dyn_eval/mod.rscrates/perry-runtime/src/node_vm.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this/array_error.rscrates/perry-runtime/src/object/global_this_webassembly.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/native_call_method/primitive_methods.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/value/to_string.rs
💤 Files with no reviewable changes (1)
- crates/perry-codegen/src/lower_call/native_table/node_misc.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/perry-runtime/src/value/to_string.rs
- crates/perry-codegen/src/runtime_decls/stdlib_ffi/net_http.rs
- crates/perry-runtime/src/object/native_call_method/primitive_methods.rs
- crates/perry-runtime/src/object/instanceof.rs
- crates/perry-runtime/src/object/class_registry/construct.rs
- crates/perry-runtime/src/object/native_module.rs
- crates/perry-runtime/src/dyn_eval/bridge.rs
- crates/perry-runtime/src/node_vm.rs
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
Complete Perry's Node.js 26.5.0 compatibility for
node:vmacross contexts, scripts, compiled functions, cross-realm values, metadata, and experimental modules.Changes
Script,runIn*Context, andcompileFunctionevaluation with persistent lexical state, strict writes, exact call arguments, and context extensions.Module,SourceTextModule, andSyntheticModulelifecycle, namespace, evaluation, and error behavior.instanceof.Related issue
Fixes #6768
Test plan
Final
node:vmreport (parity_report_20260804_152948.json):./scripts/pre-tag-check.sh --quickpasses the affected formatting, file-size, GC store-site, and address-classification gates. Its public benchmark freshness check remains red because the committed public artifact already differs from the current benchmark inputs; this PR does not modify benchmark inputs, artifacts, or runners.Checklist
CLAUDE.md, orCHANGELOG.mdchangesnode:vmgate passes with zero non-PASS countersCONTRIBUTING.mdand agreed to the Code of ConductSummary by CodeRabbit
node:vmcompatibility with Node.js 26.5.0, including context options, scripts, compiled functions, cached-data metadata, and experimental VM modules.eval, persistent bindings, object-backed environments, and context-specific globals and prototypes.instanceofbehavior, function source display, and VM error location reporting.vm.constants.