fix(runtime): computed object keys on class proto and ClassRef (#6945) - #7134
Merged
Conversation
added 2 commits
July 31, 2026 11:33
Make object-key and dynamic-key property access match Node on class
prototypes and class constructors:
- js_dyn_index_get: ToPropertyKey for non-numeric keys (was format!("{}", f64))
- class-instance get: own data fields on the reflective decl-proto
- js_object_get_index_polymorphic: registered ClassRef receivers forward
to js_dyn_index_get instead of returning undefined
Regression: test_gap_computed_key_class_proto_6945.ts
📝 WalkthroughWalkthroughThe runtime now applies ChangesComputed key access
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant js_dyn_index_get
participant js_to_property_key
participant js_object_get_index_polymorphic
participant resolve_proto_chain_field_inner
Caller->>js_dyn_index_get: read computed property
js_dyn_index_get->>js_to_property_key: coerce object or primitive key
js_to_property_key-->>js_dyn_index_get: string or symbol key
js_dyn_index_get->>js_object_get_index_polymorphic: dispatch receiver and key
js_object_get_index_polymorphic->>resolve_proto_chain_field_inner: resolve class instance property
resolve_proto_chain_field_inner-->>Caller: property value
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 1
🤖 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/dyn_index.rs`:
- Around line 272-310: The set-side dynamic index fallback should match
js_dyn_index_get by using js_to_property_key instead of js_jsvalue_to_string for
non-native keys. Update js_dyn_index_set so Symbol results are preserved and
routed through the symbol-property path, allowing the required assignment
behavior to occur rather than stringifying the Symbol.
🪄 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: 8af455c6-28d5-490e-a0d1-0cb04be0042d
📒 Files selected for processing (5)
changelog.d/7134-computed-key-class-proto.mdcrates/perry-runtime/src/object/class_registry/prototype_objects.rscrates/perry-runtime/src/object/polymorphic_index.rscrates/perry-runtime/src/value/dyn_index.rstest-files/test_gap_computed_key_class_proto_6945.ts
Address CodeRabbit: align set-side key coercion with the get-side
js_to_property_key path, and fix rooted_property_key_{get,set} so a
Symbol-yielding @@toPrimitive is not dropped as a null string key.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/dyn_index.rs`:
- Around line 632-653: Update the ClassRef assignment branch around its existing
js_string_coerce handling to use js_to_property_key, preserving Symbol results
instead of converting them to strings. When the coerced key is a Symbol, route
the write through the existing js_object_set_symbol_property path before the
string-property logic. Add a regression covering ClassRef[viaSym] assignment and
retrieval, including a Symbol returned by Symbol.toPrimitive.
🪄 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: f303a4f2-269a-468b-95b4-4c742cb53e55
📒 Files selected for processing (4)
changelog.d/7134-computed-key-class-proto.mdcrates/perry-runtime/src/object/polymorphic_index.rscrates/perry-runtime/src/value/dyn_index.rstest-files/test_gap_computed_key_class_proto_6945.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- changelog.d/7134-computed-key-class-proto.md
- crates/perry-runtime/src/object/polymorphic_index.rs
Comment on lines
+632
to
+653
| // #6945 / CodeRabbit: use `js_to_property_key` (not `js_jsvalue_to_string`) | ||
| // so an `@@toPrimitive` that returns a Symbol is preserved and routed to | ||
| // the symbol store — matching the get-side fallback. Stringifying that | ||
| // Symbol would miss the target property (and Spec ToPropertyKey must not | ||
| // turn a Symbol result into a string). | ||
| let scope = crate::gc::RuntimeHandleScope::new(); | ||
| let recv = scope.root_raw_mut_ptr(raw_ptr as *mut crate::object::ObjectHeader); | ||
| let value_handle = scope.root_nanbox_f64(value); | ||
| let key_ptr = crate::value::js_jsvalue_to_string(index); | ||
| let key = unsafe { crate::object::js_to_property_key(index) }; | ||
| let key_h = scope.root_nanbox_f64(key); | ||
| let key = key_h.get_nanbox_f64(); | ||
| let value = value_handle.get_nanbox_f64(); | ||
| if unsafe { crate::symbol::js_is_symbol(key) } != 0 { | ||
| let recv_bits = crate::value::js_nanbox_pointer( | ||
| recv.get_raw_const_ptr::<crate::object::ObjectHeader>() as i64, | ||
| ); | ||
| unsafe { | ||
| crate::symbol::js_object_set_symbol_property(recv_bits, key, value); | ||
| } | ||
| return value; | ||
| } | ||
| let key_ptr = crate::value::js_get_string_pointer_unified(key) as *const crate::StringHeader; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve symbol keys in the ClassRef write branch.
Line 640 fixes only the non-array object fallback. ClassRef receivers return through lines 480-509 first. That branch still uses js_string_coerce for a non-string key.
If key[Symbol.toPrimitive]() returns a Symbol, C[key] = value cannot reach js_object_set_symbol_property. Route the ClassRef coercion through js_to_property_key, then dispatch Symbol results through the existing symbol setter.
Add a regression for ClassRef[viaSym] assignment and retrieval. As per PR objectives, computed class constructor writes must use the same property-key semantics as other computed writes.
🤖 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/dyn_index.rs` around lines 632 - 653, Update
the ClassRef assignment branch around its existing js_string_coerce handling to
use js_to_property_key, preserving Symbol results instead of converting them to
strings. When the coerced key is a Symbol, route the write through the existing
js_object_set_symbol_property path before the string-property logic. Add a
regression covering ClassRef[viaSym] assignment and retrieval, including a
Symbol returned by Symbol.toPrimitive.
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
Fixes #6945 — computed / dynamic object-key property access on class prototypes and class constructors.
Three cooperating gaps (none GC-related):
js_dyn_index_gettreated non-string, non-numeric keys as floats (format!("{}", f64)), soobj[{toString(){return "k"}}]never ran ToPropertyKey. Object / boolean / null / undefined / bigint keys now go throughjs_to_property_key(with receiver rooting) before the by-name get.Class-instance field get walked the reflective decl-proto object only for accessors (to avoid class-id re-entry). Runtime
C.prototype[k] = vstores an own data field there, so(new C()).namemissed it whileC.prototype.namesaw it. Own data is now read viaown_data_field_by_name.Codegen IndexGet last-resort routes non-string keys on a known ClassRef through
js_object_get_index_polymorphic, which rejected every INT32-tagged receiver as a primitive. Registered class-ids now forward tojs_dyn_index_get's class-ref arm soC[objectKey]resolves statics /CLASS_DYNAMIC_PROPS.Test plan
test-files/test_gap_computed_key_class_proto_6945.ts— byte-for-byte vs Node 26.5 (prototype instance inheritance, computed key read/write, static ClassRef computed key, boolean/null keys, observable toString on absent key)cargo check -p perry-runtimeperry-runtime-static+ compiled gap binary parityNotes
No version bump (maintainer bumps at merge). Changelog fragment provisionally named on the issue number; can be renamed to the PR number if needed.
Summary by CodeRabbit
Bug Fixes
Tests