fix(object): symbol keys + accessor [[Get]] in descriptor/enumeration engine#5177
Merged
Conversation
added 2 commits
June 15, 2026 07:50
…n entries/values/assign Three shared spec-compliance gaps in the property-descriptor / enumeration engine, found via test262 built-ins/Object tail: 1. Object.getOwnPropertyDescriptors omitted symbol keys. It only walked getOwnPropertyNames (the string subset) so symbol-keyed descriptors were never reported. Now also enumerate js_object_get_own_property_symbols and install each descriptor under its symbol key; also skip undefined descriptors per spec. (symbols-included, order-after-define-property.) 2. Object.entries / Object.values read raw index field slots, which (a) did not fire an own accessor's getter (returned the empty data slot) and (b) re-read the live keys_array, so a getter that added/removed/hid a future key during enumeration perturbed the result set. Now snapshot the enumerable key list once, then read each via the name-keyed [[Get]] (fires accessors), re-checking own-ness + enumerability at read time per EnumerableOwnProperties. (entries/getter-adding-key, getter-removing-future-key, getter-making-future-key-nonenumerable.) 3. Object.assign copied symbol values straight from the value side table, which (a) missed accessor-only symbol properties entirely and (b) never ran a symbol getter. Now iterate the full own-symbol-key list (getOwnPropertySymbols, includes accessors) and read each via [[Get]]. (assign/strings-and-symbol-order.)
js_object_get_own_property_symbols appended accessor-only symbols (from a
HashMap) in nondeterministic order, so a defineProperty(o, sym, {get}) pair
came out reversed. Sort the merged own-symbol set by the symbol's monotonic
creation id (source order for symbols created and assigned in sequence) — the
same convention the class-ref symbol path uses. Fixes the symbol-key ordering
observed by getOwnPropertyDescriptors and Object.assign.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughFour property-enumeration functions in the runtime are updated to match ECMA-262 semantics: ChangesECMA-262 Property Enumeration Compliance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 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/object/field_get_set.rs`:
- Around line 1999-2016: The code currently stores key pointers as NaN-boxed f64
values in a heap-allocated Vec<snapshot_keys>, which is not visible to the GC as
a root. When a getter is invoked during the subsequent key iteration, it can
trigger GC and potentially collect keys that are only referenced through the
heap buffer. To fix this, either snapshot the owned key bytes (store the actual
string data instead of just pointers) or explicitly root the key handles before
invoking any getters in the loop that processes keys. This pattern appears at
multiple sites in the file: the main location at lines 1999-2016 where keys are
collected and then iterated to call getters, and also at lines 2030, 2173-2191,
and 2217 where similar patterns exist. Apply the same fix pattern across all
these locations to ensure all key references remain visible to the GC throughout
getter invocations.
- Around line 2009-2012: The enumerability check using
descriptor_marks_non_enumerable is being performed during the snapshot phase,
which is too early and misses cases where getters can change what properties
become enumerable after enumeration has started. Remove the filtering condition
that skips non-enumerable keys from being added to snapshot_keys in
EnumerableOwnProperties, and instead defer the enumerability check to the
per-key read phase when each key is actually visited. This ensures that
descriptor changes caused by getter execution during enumeration are properly
reflected. Apply the same refactoring to all similar snapshot-phase filtering
patterns in the codebase where enumerability checks are currently being done
prematurely on cached descriptor information.
In `@crates/perry-runtime/src/symbol.rs`:
- Around line 2324-2340: The unconditional sort by symbol-creation ID is
reordering all entries including data symbols that are already in correct
insertion order, which violates the own-property-keys guarantee. Instead of
sorting the entire entries list, partition it into two groups: data-valued
symbols (which maintain their insertion order from the entries list) and
accessor-only symbols (which come from the nondeterministic HashMap). Sort only
the accessor-only symbols by their SymbolHeader ID to establish a deterministic
order, then recombine by placing data-valued symbols first followed by the
sorted accessor-only symbols. This preserves insertion order for existing
properties while fixing the nondeterministic ordering of accessor-only symbols
added via defineProperty.
🪄 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: 43084b56-1a6b-4cd5-8abd-954f5c71e67f
📒 Files selected for processing (4)
crates/perry-runtime/src/object/alloc.rscrates/perry-runtime/src/object/descriptors.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/symbol.rs
…data-symbol order Address CodeRabbit review on #5177: - field_get_set.rs (js_object_values / js_object_entries): snapshot own keys as owned bytes instead of NaN-boxed pointers. A getter fired by js_object_get_field_by_name can delete a future key and GC before we visit it; a key kept only inside the Rust-heap Vec is not a stack-visible GC root, so it could dangle. Rematerialize the string at read time instead. - Move the enumerability filter entirely to the per-key read phase (drop the cached has_descriptors / snapshot-phase skip): per EnumerableOwnProperties an earlier getter can create a descriptor or flip a future key's enumerability, which the up-front filter and stale has_descriptors cache both missed. - symbol.rs (js_object_get_own_property_symbols): don't sort the whole merged set by creation id — that reordered data symbols already in insertion order (e.g. obj[b]=…; obj[a]=… with a created before b). Partition into data-valued (kept in insertion order) and the appended accessor-only tail (sorted by creation id for determinism). Validated vs node v26: Object.values/entries getter-add/remove/enumerability cases and data/accessor symbol ordering all match byte-for-byte. perry-runtime object:: tests 29/29.
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
Reduces the built-ins/Object test262 tail by fixing three shared spec-compliance gaps in the property-descriptor / enumeration engine, plus an own-symbol enumeration-order bug they exposed. Found by differential testing against node v26.
Root causes (each clears multiple tests)
Object.getOwnPropertyDescriptorsomitted symbol keys. It walked onlygetOwnPropertyNames(the string subset), so symbol-keyed descriptors were never reported. Now also enumerategetOwnPropertySymbolsand install each descriptor under its symbol key; also skipundefineddescriptors per spec.Object.entries/Object.valuesread raw index field slots. That (a) did not fire an own accessor's getter (returned the empty data slot) and (b) re-read the livekeys_array, so a getter that added/removed/hid a future key during enumeration perturbed the result set. Now snapshot the enumerable key list once, then read each via the name-keyed[[Get]](fires accessors), re-checking own-ness + enumerability at read time per EnumerableOwnProperties.Object.assigncopied symbol values straight from the value side table. That missed accessor-only symbol properties entirely and never ran a symbol getter. Now iterate the full own-symbol-key list (includes accessors) and read each via[[Get]].Own symbol-key enumeration order.
js_object_get_own_property_symbolsappended accessor-only symbols from a HashMap in nondeterministic order, so adefineProperty(o, sym, {get})pair came out reversed. Now sort the merged own-symbol set by the symbol's monotonic creation id (source order for symbols created and assigned in sequence) — the convention the class-ref symbol path already uses.Results (scoped test262, node v26 oracle, before → after)
No regressions
cargo test -p perry-runtime object::: 29 passed, 0 failed.Remaining tail (separate roots, out of scope)
Object.assign([7,8,9],[1])), proxy symbol-order.No new codegen/dispatch entry points (all changes are inside existing
#[no_mangle]runtime functions), so no API_MANIFEST change is required.Summary by CodeRabbit
Object.assignto execute getters for symbol-keyed properties during symbol-keyed copy.Object.getOwnPropertyDescriptorsto reliably include descriptor entries for both string and symbol keys, omitting removed properties.Object.valuesandObject.entriesenumeration to better match ECMAScript behavior, including correct accessor invocation.