fix(runtime): enumerate native module namespace values - #8269
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Limit details: You’ve used all 8 included reviews currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
|
Merging. Runtime suite 2567 passed / 0 failed, all six script gates and The rooting reads correctly, which matters because resolving a callable export can allocate: receiver, key list, output array and per-key values are each rooted, with a per-iteration scope and re-reads through One detail I checked rather than assumed: One suggestion, not a blocker: bump the node-suite baseline. |
… baseline 990->983 enumeration.rs was at 14 bare get_raw_*_ptr reads against its ceiling of 5 after #8269 merged, turning `lint` (and so pr-gate) red on main for every rebased PR. The reads were handle-rooted and correct; the conversion to with_{mut,const}_ptr is behaviorally identical and audit-visible. Baseline locked down to 983 per the ratchet's own "debt fell" prompt.
… baseline 990->983 (#8291) enumeration.rs was at 14 bare get_raw_*_ptr reads against its ceiling of 5 after #8269 merged, turning `lint` (and so pr-gate) red on main for every rebased PR. The reads were handle-rooted and correct; the conversion to with_{mut,const}_ptr is behaviorally identical and audit-visible. Baseline locked down to 983 per the ratchet's own "debt fell" prompt. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
…n identity — pi boot threw Cyclic __proto__
perry-compiled pi (13MB esbuild bundle) died at startup with
`TypeError: Cyclic __proto__ value` out of `js_object_set_prototype_of`,
with obj_bits == proto_bits exactly — the two ARGUMENTS were the same
pointer — while the chain behind the proto was healthy. None of the
bundle's 17 textual `Object.setPrototypeOf(` sites fired a JS logging
shim, because the self-set was manufactured upstream of the call: the
closure-literal singleton caches handed back ONE ClosureHeader for two
evaluations of the same function literal, so `setPrototypeOf(wrapped,
original)` (a graceful-fs-style wrap pattern) received one object twice
and correctly refused the "cycle".
Mechanism: `expr/closure.rs` routed closure literals through
`js_closure_alloc_singleton` (captureless arrows) and
`js_closure_alloc_with_captures_singleton` (arrows with captures, and
non-arrow literals whose captures are all boxes) keyed by
(func_ptr, capture bits). Two evaluations of the same literal with
bit-identical captures — e.g. an arrow capturing the same constant, or
any captureless arrow — came back `===`-equal. ECMA-262
OrdinaryFunctionCreate requires a fresh object per evaluation, and the
distinction is observable through `===`, expando properties, WeakMap
keys, addEventListener de-duplication, and `Object.setPrototypeOf`.
Minimal repros (byte-compared against node before/after):
function mk() { return () => K; } // captured arrow
const a = mk(), b = mk(); // perry: a === b (node: false)
Object.setPrototypeOf(a, b); // perry threw Cyclic __proto__
and the same with `() => 1` (captureless). Both now match node.
Fix: gate every closure.rs literal singleton path on
`is_plain_async_step_body` — the file's existing detector for the
compiler-synthesized plain-async step closures (their terminal
`Stmt::ReleaseBoxes` arms cannot appear in user code). Those are the
closures the caches were built for (PerryTS#8269's parallel async-await
pattern re-creates them per resume with the same per-activation box
captures, and their identity never escapes the promise machinery), and
they keep the fast path. Every user-authored arrow and function
expression now mints a fresh closure. Runtime-internal singleton users
(function-declaration references, property_get/i18n/arrays wrapper
thunks) are separate paths and unchanged. A genuine
`setPrototypeOf(x, x)` still throws — the cycle check is untouched.
Perf note: this deliberately gives back the user-arrow closure reuse
from the PerryTS#8269/PerryTS#8291 captured-singleton extension (e.g. ECS
`World.executeEntityCommands`' per-call inner arrow) and the captureless
user-arrow singleton at literal sites; a sound replacement needs
escape-aware caching rather than identity-violating sharing.
Validation: repros above and test-files/
test_gap_9090_closure_literal_identity.ts byte-identical to node;
`cargo test -p perry-runtime --lib -- --test-threads=1` green — 2813
passed, 0 failed with `--skip reserved_floor` (that module's at-scale
tests SIGABRT on this pre-PerryTS#9110 base; known PerryTS#9108/PerryTS#9110, unrelated);
`cargo test -p perry-codegen`: 283+75 passed after updating the four
native_proof_regressions pins from `js_closure_alloc_singleton` to
`js_closure_alloc` (their real subject — the alloc storing the public
wrapper pointer — is preserved); one pre-existing env-leak flake
(`packed_f64_loop_unary_math_store_versions_with_side_exit`) passes in
isolation.
Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
…ity — pi boots (#9128) * fix(runtime): closure-literal singleton caches conflated user function identity — pi boot threw Cyclic __proto__ perry-compiled pi (13MB esbuild bundle) died at startup with `TypeError: Cyclic __proto__ value` out of `js_object_set_prototype_of`, with obj_bits == proto_bits exactly — the two ARGUMENTS were the same pointer — while the chain behind the proto was healthy. None of the bundle's 17 textual `Object.setPrototypeOf(` sites fired a JS logging shim, because the self-set was manufactured upstream of the call: the closure-literal singleton caches handed back ONE ClosureHeader for two evaluations of the same function literal, so `setPrototypeOf(wrapped, original)` (a graceful-fs-style wrap pattern) received one object twice and correctly refused the "cycle". Mechanism: `expr/closure.rs` routed closure literals through `js_closure_alloc_singleton` (captureless arrows) and `js_closure_alloc_with_captures_singleton` (arrows with captures, and non-arrow literals whose captures are all boxes) keyed by (func_ptr, capture bits). Two evaluations of the same literal with bit-identical captures — e.g. an arrow capturing the same constant, or any captureless arrow — came back `===`-equal. ECMA-262 OrdinaryFunctionCreate requires a fresh object per evaluation, and the distinction is observable through `===`, expando properties, WeakMap keys, addEventListener de-duplication, and `Object.setPrototypeOf`. Minimal repros (byte-compared against node before/after): function mk() { return () => K; } // captured arrow const a = mk(), b = mk(); // perry: a === b (node: false) Object.setPrototypeOf(a, b); // perry threw Cyclic __proto__ and the same with `() => 1` (captureless). Both now match node. Fix: gate every closure.rs literal singleton path on `is_plain_async_step_body` — the file's existing detector for the compiler-synthesized plain-async step closures (their terminal `Stmt::ReleaseBoxes` arms cannot appear in user code). Those are the closures the caches were built for (#8269's parallel async-await pattern re-creates them per resume with the same per-activation box captures, and their identity never escapes the promise machinery), and they keep the fast path. Every user-authored arrow and function expression now mints a fresh closure. Runtime-internal singleton users (function-declaration references, property_get/i18n/arrays wrapper thunks) are separate paths and unchanged. A genuine `setPrototypeOf(x, x)` still throws — the cycle check is untouched. Perf note: this deliberately gives back the user-arrow closure reuse from the #8269/#8291 captured-singleton extension (e.g. ECS `World.executeEntityCommands`' per-call inner arrow) and the captureless user-arrow singleton at literal sites; a sound replacement needs escape-aware caching rather than identity-violating sharing. Validation: repros above and test-files/ test_gap_9090_closure_literal_identity.ts byte-identical to node; `cargo test -p perry-runtime --lib -- --test-threads=1` green — 2813 passed, 0 failed with `--skip reserved_floor` (that module's at-scale tests SIGABRT on this pre-#9110 base; known #9108/#9110, unrelated); `cargo test -p perry-codegen`: 283+75 passed after updating the four native_proof_regressions pins from `js_closure_alloc_singleton` to `js_closure_alloc` (their real subject — the alloc storing the public wrapper pointer — is preserved); one pre-existing env-leak flake (`packed_f64_loop_unary_math_store_versions_with_side_exit`) passes in isolation. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP * fix(runtime): name-keyed builtin-member reads must see user overrides — pi boot threw Cyclic __proto__ (part 2) With the closure-literal identity fix in place, pi still died at startup with `TypeError: Cyclic __proto__ value`, obj_bits == proto_bits exactly. The instrumented throw site showed both arguments were ONE closure with `func_ptr = 0xBADD_DEAD` (BOUND_METHOD_FUNC_PTR, capture_count 3) and a healthy 3-link chain behind it — the canonical bound-native callable that `bound_native_callable_export_value` mints once per (module, member). The failing code is graceful-fs's module init, bundled into pi (pi-bundle.mjs:6621/6686/6705): var chdir = process.chdir; process.chdir = function (d) { ... }; if (Object.setPrototypeOf) Object.setPrototypeOf(process.chdir, chdir); and the same wrap for fs.rename / fs.read. Under perry the patch write did not round-trip on the re-read, so setPrototypeOf received the SAME canonical closure for both arguments — a self-set — and the cycle check correctly refused it. The earlier probe of this exact shape passed because it patched a PLAIN object, where writes round-trip; the failure needs a builtin namespace receiver. The JS shim over `Object.setPrototypeOf(` never fired because the conflation happens in the native member-READ, upstream of the call. Root cause: user writes to builtin namespace members are stored in two different places depending on the lowering — computed stores (`process[k] = fn`) go through `nm_field_set_override` into `NATIVE_NAMESPACE_PROP_OVERRIDES`, while static stores (`process.chdir = fn`) reach the generic store path and land as an own dynamic field on the canonical namespace object. The NAME-KEYED read entries carry no object pointer and consulted only the override table: * `js_native_module_property_by_name` (codegen static reads of process.* members) missed own-field stores, so the graceful-fs static patch was invisible to the static re-read; * `js_native_module_esm_export_value` (codegen property reads off a builtin DEFAULT import — `import fs from "node:fs"; fs.rename`) consulted NOTHING (consult_overrides=false plus its own snapshot cache), so no fs patch was ever visible. In Node the default import of a core module is the live mutable CJS exports object, so the patched value must win; the tls DEFAULT_* cache-coherence hack was the ad-hoc version of this for three keys. Fix: `native_namespace_user_value(module, prop)` consults the override table and then the canonical namespace object's own field (never creating a namespace — if none exists, no user store can have landed on one). Both name-keyed read entries call it before any built-in resolution or snapshot cache. Named ESM import bindings of core modules snapshot at module init before user patches run, so their intended snapshot semantics are unaffected in the eager case. Validation: r11-r16 probe matrix (process/fs, static/computed reads and writes) and test-files/test_gap_9091_native_member_patch_roundtrip.ts byte-identical to node; a genuine `setPrototypeOf(x, x)` still throws. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP * style: rustfmt the closure-identity fix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Summary
Route
Object.entriesandObject.valueson native-module namespace objects through the same virtual export surface used byObject.keys, so user code sees real module keys and values instead of the internal__module__sentinel.Changes
perf_hooks.constantsparity fixture withObject.valuesRelated issue
Fixes #8235
Test plan
cargo build --release -p perry-runtimeenv RUST_TEST_THREADS=1 cargo test --lib -p perry-runtime(2557 passed, 0 failed, 4 ignored on rebased head)./scripts/pre-tag-check.sh --quickPERRY_SKIP_BUILD=1 ./run_parity_tests.sh --suite node-suite --module perf_hooks --filter all-gc-values(1/1 pass)cargo fmt --all -- --checkAdditional surface check
for...inalready uses the virtualObject.keyspath, and spread /Object.assignalready use the export-copy hook from #6667.JSON.stringifystill has a separate raw-field serializer path and reproduces the sentinel leak; that broader compact/pretty/replacer serializer work is intentionally left as a follow-up rather than folded into this entries/values fix.Checklist
CLAUDE.md, orCHANGELOG.mdbumpCONTRIBUTING.mdand agree to the Code of Conduct