fix(runtime): improve test262 built-ins/Object parity (#5588)#5763
Merged
Conversation
Three targeted fixes that close a cluster of test262 built-ins/Object failures found in issue #5588: 1. `closure_dynamic_enumerable_props` (enumeration.rs): `Object.keys/ values/entries` on a function now includes `length` and `name` when they have been explicitly made enumerable via `Object.defineProperty`. Previously those built-in function properties were only surfaced via the direct field-read path, so a descriptor-only redefinition (no `value` written to the dynamic-props side table) was silently dropped. Fix: check the descriptor side table for `length`/`name` first; if enumerable, compute the value from the same cascade as the field-read path (dynamic-prop override → bound arity → builtin length → closure arity) and prepend them before user-added dynamic props. 2. `js_object_default_to_locale_string` (object_proto.rs): spec step 2 of `Object.prototype.toLocaleString` is `Invoke(O, "toString")` where O is the raw `this` (primitive, not ToObject'd). A strict callee on the patched prototype must receive the original primitive as `this` (`typeof this === "boolean"` for `toLocaleString.call(true)` after patching `Boolean.prototype.toString`). The old code called the generic `js_native_call_method` which hard-codes the native toString result. Fix: check `builtin_proto_user_method` for a user patch first; if present, dispatch via `call_primitive_closure_value` which correctly applies strict/non-strict `this` binding. 3. `js_object_set_prototype_of` (define_properties.rs): add the OrdinarySetPrototypeOf step-7 prototype-cycle check. Walking the chain of the proposed new prototype and comparing each ancestor against the target object throws `TypeError: Cyclic __proto__ value` when a cycle would be created. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JWENcSJa3zenoQyBXXCF5j
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThree spec-compliance fixes update JS object enumeration, primitive ChangesJS Object Model Spec Compliance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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: 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/enumeration.rs`:
- Around line 269-272: The override lookup in enumeration.rs is using
closure_get_dynamic_prop with TAG_UNDEFINED as a sentinel, which conflates a
missing override with an explicit undefined value. Update the enumeration logic
to separate “property exists” detection from reading the override value in the
Object.values/Object.entries path, and use the computed length/name fallback
only when the override is truly absent. Also adjust the Object.keys path so it
does not call [[Get]] via closure_get_dynamic_prop, preventing enumerable
length/name accessors from being invoked while only collecting keys.
In `@crates/perry-runtime/src/object/native_call_method/object_proto.rs`:
- Around line 97-115: The Symbol primitive branch in object_proto.rs is
currently unreachable because the check uses js_is_symbol(receiver) inside the
!jsval.is_pointer() path, so Symbol.prototype.toString still won’t dispatch
through the primitive path. Update the primitive-type detection in this
native_call_method::object_proto logic so Symbols are identified from the same
non-pointer receiver value as the other primitives, and ensure
call_primitive_closure_value can be reached for Symbol just like
Boolean/BigInt/String.
In `@crates/perry-runtime/src/object/object_ops/define_properties.rs`:
- Around line 205-215: The prototype chain walk in define_properties.rs only
checks for null and self-edges, so it can hang on pre-existing multi-node
cycles. Update the loop in the Object.setPrototypeOf path to detect any repeated
prototype node, either by tracking visited bits or using tortoise/hare cycle
detection in the prototype traversal. When a cycle is found, throw the same
TypeError used for cyclic __proto__ values via throw_object_type_error, and keep
the existing behavior for null termination and direct self-cycles.
🪄 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: b0fc7e91-9ed9-4d5a-b677-cd99c548484d
📒 Files selected for processing (3)
crates/perry-runtime/src/object/field_get_set/enumeration.rscrates/perry-runtime/src/object/native_call_method/object_proto.rscrates/perry-runtime/src/object/object_ops/define_properties.rs
Three correctness fixes raised in the code review: 1. enumeration.rs: use `closure_has_own_dynamic_prop` to separate presence from value in `closure_dynamic_enumerable_props`. The previous `closure_get_dynamic_prop` sentinel (TAG_UNDEFINED) conflated "no dynamic override" with "dynamic override whose value is undefined", and also invoked getters in a keys-only path. 2. object_proto.rs: symbols are POINTER-tagged, so the `!jsval.is_pointer()` guard made the `js_is_symbol` branch inside it dead code. Compute `is_symbol` before the guard and extend the condition to `!jsval.is_pointer() || is_symbol` so Symbol.prototype.toString dispatch is reachable. 3. define_properties.rs: replace the single-step self-edge cycle check in `js_object_set_prototype_of` with Floyd's tortoise-and-hare algorithm so a pre-existing multi-node cycle (A→B→A) terminates the walk instead of looping forever.
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
2 participants
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.
Closes a cluster of test262
built-ins/Objectfailures tracked in #5588.Root causes and fixes
1.
Object.keys/values/entrieson functions —length/namemissing when made enumerable (enumeration.rs)closure_dynamic_enumerable_propsiterated onlyclosure_dynamic_props_snapshot, which holds user-added dynamic properties. The built-in function slotslengthandnamelive outside that snapshot (they're computed on the fly from the closure registry), so aObject.defineProperty(fn, "length", { enumerable: true })call correctly recorded the descriptor in the side table but the key never appeared inObject.keys(fn).Fix: Before iterating user-added props, check the descriptor side table for
lengthandname. If either is explicitly enumerable, compute its value through the same cascade as the direct field-read path (dynamic-prop override → bound arity → builtin length → closure arity) and prepend it.length/nameare then skipped in the user-props pass to avoid double-counting.2.
Object.prototype.toLocaleStringwith a patched primitivetoString(object_proto.rs)The spec (20.1.3.6) says step 1 is
Let O = this value(notToObject) and step 2 isReturn ? Invoke(O, "toString"). FortoLocaleString.call(true)with a user-patchedBoolean.prototype.toString = function() { return typeof this; }(strict),thismust be the raw primitivetrue;typeof thismust return"boolean". The old code calledjs_native_call_methodwhich hard-codes the native result without checking prototype patches.Fix: Before falling back to the native path, call
builtin_proto_user_methodto detect a user patch on the primitive's prototype (Boolean/Number/BigInt/String/Symbol). If found, dispatch viacall_primitive_closure_value, which already implementsOrdinaryCallBindThis(strict callee gets raw primitive; sloppy callee gets the boxed wrapper).3. Prototype cycle detection in
Object.setPrototypeOf(define_properties.rs)OrdinarySetPrototypeOfstep 7 requires walking the prototype chain of the proposed new prototype and throwingTypeError: Cyclic __proto__ valueif any ancestor equals the target object. This check was absent, soObject.setPrototypeOf(a, b); Object.setPrototypeOf(b, a)silently created a cycle instead of throwing on the second call.Fix: Walk the chain of the proposed prototype (bounded by null or a stable fixed-point); throw on the first ancestor that equals the target.
Before / after (counts estimated from issue #5588 analysis)
keys/entriesfunction property orderingtoLocaleStringwith patched primitive toStringsetPrototypeOfcycle detectionValidation
cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static— cleancargo fmt --all -- --check— cleanbash scripts/check_file_size.sh— clean (all files under 2000 lines)No version bump, no CHANGELOG entry — maintainer folds those at merge per contributor guidelines.
Generated by Claude Code
Summary by CodeRabbit
length/nameappear in spec order, respect deletions/enumerability rules, and aren’t double-counted.toLocaleStringfor primitive values to align withObject.prototype.toLocaleStringsemantics and to honor user-customizedtoStringon relevant built-in prototypes.__proto__changes that would create inheritance loops.