runtime: serve Object.prototype-inherited globalThis members on bare-identifier dispatch (#6652) - #6656
Merged
proggeramlug merged 2 commits intoJul 19, 2026
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 (1)
📝 WalkthroughWalkthroughUnknown identifiers in member-object position now resolve at runtime through ChangesGlobal resolution
Diagnostic preview safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 3
🧹 Nitpick comments (2)
crates/perry/tests/issue_6652_global_proto_inherited_members.rs (1)
1-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving this coverage to a
cargo-test-visible unit test.As per path instructions, prefer acceptance coverage in
cargo-test-visible unit tests because integration suites undercrates/*/tests/*.rsdo not run on every PR. While retrieved learnings suggest that tests executing the full compile → link → run path belong in integration tests, please verify if this coverage can be adequately represented as a unit test to ensure it runs on every PR.🤖 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/tests/issue_6652_global_proto_inherited_members.rs` around lines 1 - 135, Move the regression coverage from the integration test functions object_prototype_inherited_globals_match_node and runtime_created_global_member_access_matches_node into a cargo-test-visible unit-test location, if the behavior can be represented without the full compile-and-run harness. Preserve assertions for inherited global lookup, runtime-created global member access, receiver semantics, and unresolved-identifier errors; retain integration coverage only where unit testing cannot exercise the required compiler/runtime path.Sources: Path instructions, Learnings
tests/test_class_expr_capture_refresh_6604.sh (1)
154-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCapture
test_bin's exit status explicitly before comparing output.
RUN_OUTPUT=$(./test_bin 2>&1)doesn't check the run's exit code. Withset -eactive, a crash (segfault, panic, etc. — the very failure mode this regression guards against, per the header's "TypeError: value is not a function" note) aborts the whole script at that line, skipping the descriptiveFAIL: ...block and leaving a bare non-zero exit with no diagnostic. Capturing$?(or temporarily disablingset -earound the call) would let the existing FAIL branch report a crash the same way it reports a stale-snapshot mismatch.🧪 Suggested fix
-RUN_OUTPUT=$(./test_bin 2>&1) +set +e +RUN_OUTPUT=$(./test_bin 2>&1) +RUN_EXIT=$? +set -e +if [ "$RUN_EXIT" -ne 0 ]; then + echo "FAIL: test_bin exited with status $RUN_EXIT" + echo "$RUN_OUTPUT" + exit 1 +fiAs per path instructions,
**/*.shfiles should check the harness exit code directly rather than relying on output alone.🤖 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 `@tests/test_class_expr_capture_refresh_6604.sh` around lines 154 - 166, Capture test_bin’s exit status immediately after the RUN_OUTPUT command while avoiding set -e aborting the script, then include that status in the existing PASS/FAIL decision. Update the comparison around RUN_OUTPUT and EXPECTED so a nonzero test_bin exit follows the descriptive FAIL path, while successful matching output still reports PASS.Source: Path instructions
🤖 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/value/dynamic_arith.rs`:
- Around line 45-46: Update the string limiting logic in the dynamic arithmetic
diagnostic path to cap the value at 80 Unicode characters rather than truncating
at byte offset 80. Preserve the existing string conversion and ensure multi-byte
UTF-8 content cannot trigger a panic.
- Around line 59-67: Update throw_mix_bigint and its diagnostic path to root
both NaN-boxed operands with crate::gc::RuntimeHandleScope before calling
describe_mix_operand. After each potentially allocating description call, reload
the corresponding operand from its rewritten handle so the second description
cannot use an evacuated pointer; preserve the existing diagnostic output and
non-diagnostic behavior.
- Around line 718-721: Update js_dynamic_ushr to use
crate::gc::RuntimeHandleScope and root the original operands and the result of
to_numeric(a) across calls that may execute user code. Ensure b remains rooted
before calling to_numeric(a), and both converted values remain rooted before
both_bigint_or_throw(a, b) and subsequent arithmetic.
---
Nitpick comments:
In `@crates/perry/tests/issue_6652_global_proto_inherited_members.rs`:
- Around line 1-135: Move the regression coverage from the integration test
functions object_prototype_inherited_globals_match_node and
runtime_created_global_member_access_matches_node into a cargo-test-visible
unit-test location, if the behavior can be represented without the full
compile-and-run harness. Preserve assertions for inherited global lookup,
runtime-created global member access, receiver semantics, and
unresolved-identifier errors; retain integration coverage only where unit
testing cannot exercise the required compiler/runtime path.
In `@tests/test_class_expr_capture_refresh_6604.sh`:
- Around line 154-166: Capture test_bin’s exit status immediately after the
RUN_OUTPUT command while avoiding set -e aborting the script, then include that
status in the existing PASS/FAIL decision. Update the comparison around
RUN_OUTPUT and EXPECTED so a nonzero test_bin exit follows the descriptive FAIL
path, while successful matching output still reports PASS.
🪄 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: 2062d353-7a4c-4b21-8c1b-83d36e9cf4a5
📒 Files selected for processing (24)
crates/perry-codegen/src/codegen/module_globals_emit.rscrates/perry-codegen/src/lower_call/native_table/node_core/module_sea_tls_test.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/expr_function.rscrates/perry-hir/src/lower/lower_expr/arm_class.rscrates/perry-hir/src/lower/lower_expr/arm_ident.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_patterns.rscrates/perry-runtime/src/module_require.rscrates/perry-runtime/src/node_submodules/mod.rscrates/perry-runtime/src/process.rscrates/perry-runtime/src/process/node_module.rscrates/perry-runtime/src/typedarray/bigint.rscrates/perry-runtime/src/value/dynamic_arith.rscrates/perry/tests/createrequire_builtin_modules.rscrates/perry/tests/issue_6652_global_proto_inherited_members.rscrates/perry/tests/module_destructured_globals.rstest-parity/node-suite/bigint/arithmetic/mixed-operand-errors.tstest-parity/node-suite/globals/global-object-prototype-inherited-members.tstest-parity/node-suite/node-core/module-destructure-function-visibility.tstest-parity/node-suite/object/class-expr-capture-refresh.tstests/test_class_expr_capture_refresh_6604.sh
…identifier dispatch (PerryTS#6652) Sloppy/global bare identifiers that resolve to Object.prototype-inherited members of the global object (hasOwnProperty, toString, valueOf, ...) threw 'TypeError: value is not a function' in member-object position: arm_ident's unknown-identifier lowering collapsed the ident to the bare GlobalGet(0) sentinel (which IS globalThis), discarding the identifier name, so hasOwnProperty.call(o, k) dispatched '.call' against globalThis. Node resolves the bare ident through the scope chain to the global object, which inherits Object.prototype.hasOwnProperty. Fix: route ALL unknown identifiers through the existing js_global_get_or_throw_unresolved by-name runtime lookup — member-object position included — which serves globalThis' own and inherited members with identity preserved and still throws the spec ReferenceError on a true miss. Known globals keep the GlobalGet(0) sentinel and their intrinsic member routing. The compile warning text now describes the new behavior. Also fixes runtime-created globals in member position (myGlobal.prop read globalThis.prop pre-fix) as the same lost-name bug. Trigger: @babel/types/lib/definitions/placeholders.js (hasOwnProperty.call(o, t4), 14 sites + 2 bare-toString sites in the pi bundle) during pi-native module init — pi bring-up wall PerryTS#6. Fixes PerryTS#6652 Claude-Session: https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9
proggeramlug
force-pushed
the
fix/6652-global-proto-members
branch
from
July 19, 2026 02:38
8dde6fb to
27e7342
Compare
…tics (CodeRabbit on PerryTS#6656) Claude-Session: https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9
Contributor
Author
|
CodeRabbit findings: the two dynamic_arith GC-rooting items were already fixed (169456b, merged via #6650 — this branch picked them up on rebase, hence the ✅ marks). The remaining live one — byte-index |
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.
Wall #6 of the pi coding-agent bring-up (tracker #6564; prior walls #6593, #6604, #6644, #6649, #6651 all fixed).
Root cause
Node resolves a bare
hasOwnPropertythrough the scope chain to the global object, which inheritsObject.prototype.hasOwnProperty— sohasOwnProperty.call(o, k)works. In perry,arm_ident.rs's unknown-identifier lowering behaved differently depending on position:js_global_get_or_throw_unresolvedby-name runtime lookup — already correct (serves inherited members, identity preserved).ctx.unresolved_ident_as_global, set bylower_member): collapsed the ident to the bareGlobalGet(0)sentinel — which IS globalThis — discarding the identifier name entirely.hasOwnProperty.call(o, k)therefore readglobalThis.call(undefined) and threwTypeError: value is not a function. Same lost-name bug made a runtime-created global's member access read the member off globalThis instead (myGlobal.prop→globalThis.prop).Trigger:
@babel/types/lib/definitions/placeholders.js(hasOwnProperty.call(o, t4) || (o[t4] = []), pi-bundle.mjs:203852; 14hasOwnProperty+ 2 bare-toStringsites) during pi-native module init.Fix
crates/perry-hir/src/lower/lower_expr/arm_ident.rs: route all unknown identifiers through the existingjs_global_get_or_throw_unresolvedruntime lookup — member-object position included. The lookup resolves throughjs_object_get_field_by_nameon globalThis, which serves own AND Object.prototype-inherited members with identity preserved, and still throws the specReferenceError: X is not definedon a true miss (now correctly localized to the base identifier instead of a wrong member read). Known globals keep theGlobalGet(0)sentinel and their intrinsic member routing (Math.max,Date.now, …) — untouched.The compile warning now describes the real behavior:
unknown identifier 'X' — assuming global; resolved by name on globalThis (incl. Object.prototype-inherited members) at runtime.Receiver semantics (verified against node v26.3.0, both ESM/strict and sloppy CJS): a bare CALL of such an identifier gets
this = undefined— the global environment record's WithBaseObject is undefined; builtins don't coerce.toString()→"[object Undefined]",hasOwnProperty("x")→TypeError: Cannot convert undefined or null to object. Perry already matched this on the call-callee path; unchanged."bare reads lower to 0": no longer true for unknown identifiers — they never lower to the bare sentinel anymore. (A bare VALUE read of a known global that has no value-form, e.g.
const q = queueMicrotask, keeps its pre-existing behavior — out of scope here.)Tests
crates/perry/tests/issue_6652_global_proto_inherited_members.rs— the @babel/types shape + extraction/identity/typeof/bare-call-this-undefined/in-function/ReferenceError-miss, and the runtime-created-global member-access case; expected output is node v26's byte-for-byte.test-parity/node-suite/globals/global-object-prototype-inherited-members.ts— live node-vs-perry parity fixture (10 sections incl.valueOf/isPrototypeOf/propertyIsEnumerable); verified stdout+stderr+rc identical tonode --experimental-strip-types.cargo test -p perry-runtime --lib -- --test-threads=1: 1426 passed, 0 failed.perry-hir/perry-codegenlib tests green; at-risk integration tests (with_implicit_global_fallback,issue_5253_construct_reference_source_location,issue_forward_captured_lets_comma_decl,issue_4858,issue_5459,issue_5989,issue_6560_bun_globals,module_destructured_globals) all green. Pre-existing failures NOT from this change (fail identically on the branch base):c262_parity::logical_property_assignment_short_circuits_the_store_4586, and two perry-codegen test targets that don't compile (i64_spec_ternary_recursion,perry_builtin_name_collision— staleCompileOptions.namespace_reexport_named_importsfield).pi bring-up status
Full pi recompile with this fix: wall #6 is down —
pi-native --version/--helpget past pi-bundle.mjs:203852 (the pre-fix binary still throwsvalue is not a functionthere). Module init now advances to a NEW, later wall #7:TypeError: Method Set.prototype.add called on incompatible receiver(builtin-subclass family — the bundle'svar GaxiosInterceptorManager = class extends Set {}CJS-factory class expression is the prime suspect, adjacent to the knownclass extends Arraygap #6232). Will be filed separately with the debug-symbols line.Stacks on #6653 (branched from
fix/6651-createrequire-allowlist-family); the diff shown includes those commits until #6653 (and its parents) merge.Fixes #6652
https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9
Summary by CodeRabbit
Object.prototypemembers (method identity, dispatch, and error behavior) to match Node.ReferenceError.Object.prototypeinherited members, function-scope behavior, runtime-created globals, and exact error messaging parity.