fix(object): property-descriptor engine — named-array merge, accessor delete/set, symbol & non-extensible throws#5146
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 (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughFive runtime object files are updated to improve ECMA-262 compliance: ChangesProperty Descriptor and Accessor Spec Compliance
Sequence DiagramsequenceDiagram
participant Caller
participant js_object_set_field_by_name
participant AccessorDescriptor
participant SetterClosure
participant FrozenWritableCheck
Caller->>js_object_set_field_by_name: obj[key] = value
js_object_set_field_by_name->>AccessorDescriptor: resolve accessor for incoming_key_str
alt accessor with setter
AccessorDescriptor-->>js_object_set_field_by_name: setter closure
js_object_set_field_by_name->>SetterClosure: invoke(this, value)
js_object_set_field_by_name-->>Caller: return
else getter-only accessor
js_object_set_field_by_name->>js_object_set_field_by_name: throw_immutable_write
js_object_set_field_by_name-->>Caller: throw
else no accessor found
AccessorDescriptor-->>js_object_set_field_by_name: none
js_object_set_field_by_name->>FrozenWritableCheck: check frozen/sealed/writable
FrozenWritableCheck-->>js_object_set_field_by_name: result
js_object_set_field_by_name-->>Caller: return/throw
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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/array_object_ops.rs`:
- Around line 686-730: The code unconditionally removes the accessor descriptor
when cur_accessor exists, but it should only remove it when actually converting
from an accessor to a data property. For generic redefinitions (where only
enumerable/configurable are specified without explicit value or writable), the
accessor should be preserved. Add a condition before the
ACCESSOR_DESCRIPTORS.with() call that checks whether this is actually an
accessor-to-data conversion (by verifying has_value is true or writable is
explicitly specified in the descriptor) rather than just a generic attribute
redefinition. Only proceed with the accessor removal when an actual data
property descriptor is being applied. Additionally, update the logic that
determines whether to materialize undefined to properly account for the accessor
state, ensuring that accessor-to-data conversions without an explicit value
produce a data descriptor rather than erasing the property.
In `@crates/perry-runtime/src/object/field_set_by_name.rs`:
- Around line 890-917: The accessor short-circuit check in the
field_set_by_name.rs section (lines 890-917) currently only looks for accessors
on the receiver object itself, missing setters defined on the prototype chain.
Modify the accessor lookup to walk the prototype chain starting from the
receiver object, checking each prototype in order until a setter is found, and
invoke the first setter discovered rather than treating the property as
non-existent when no own accessor exists. Additionally, update the
object_assign_throw_if_set_rejected function in alloc.rs (lines 715-726) to
implement the same prototype-chain traversal logic so that Object.assign
respects prototype setters consistently with the updated field_set_by_name
behavior, preventing writes from being incorrectly rejected when a valid setter
exists on a prototype.
In `@crates/perry-runtime/src/object/object_ops.rs`:
- Around line 1281-1283: The descriptor validation at lines 1281-1283 and
1336-1338 use `value_is_object_like` which rejects valid descriptor objects like
Proxy handles and INT32-tagged class refs. Remove or relax the
`value_is_object_like` check in both locations to allow these object kinds, and
refactor all subsequent descriptor property reads (such as accessing properties
like `value`, `writable`, `get`, `set`, `enumerable`, `configurable`) to use
value-level operations like `HasProperty` and `Get` instead of direct
`ObjectHeader` access, ensuring the code works with both ObjectHeader-backed
objects and Proxy/class-ref descriptors.
🪄 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: 0991c0b5-df5f-45db-8508-69d7e4bc1895
📒 Files selected for processing (5)
crates/perry-runtime/src/object/alloc.rscrates/perry-runtime/src/object/array_object_ops.rscrates/perry-runtime/src/object/delete_rest.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/object_ops.rs
| // Accessor short-circuit — must precede the frozen/sealed and | ||
| // writable checks below: a property defined with a setter is invoked | ||
| // via [[Set]] regardless of the object's frozen/sealed state (freezing | ||
| // an accessor only clears [[Configurable]]; the setter still runs). A | ||
| // getter-only accessor is read-only. Hoisted above the sidecar + the | ||
| // linear-scan blocks so BOTH key-lookup paths honor it — previously the | ||
| // frozen check at the top of each block threw before the accessor was | ||
| // consulted (test262 | ||
| // assign/target-is-frozen-accessor-property-set-succeeds). | ||
| if ACCESSORS_IN_USE.with(|c| c.get()) { | ||
| if let Some(ref k) = incoming_key_str { | ||
| if let Some(acc) = get_accessor_descriptor(obj as usize, k) { | ||
| if acc.set != 0 { | ||
| let closure = (acc.set & crate::value::POINTER_MASK) | ||
| as *const crate::closure::ClosureHeader; | ||
| if !closure.is_null() { | ||
| let receiver = crate::value::js_nanbox_pointer(obj as i64); | ||
| let previous_this = super::js_implicit_this_set(receiver); | ||
| crate::closure::js_closure_call1(closure, value); | ||
| super::js_implicit_this_set(previous_this); | ||
| } | ||
| } else { | ||
| crate::error::throw_immutable_write(0, k); | ||
| } | ||
| return; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Prototype-chain setter lookup is still missing from the ordinary write paths. These updates fix own-accessor ordering, but both sites still treat accessors as receiver-local only. Ordinary objects whose setter lives on a prototype recorded via Object.create / Object.setPrototypeOf will still behave like the property is missing, which turns valid setter dispatch into either a rejected write or an unintended own data property.
crates/perry-runtime/src/object/field_set_by_name.rs#L890-L917: walk the ordinary prototype chain before frozen/writable/new-property checks and invoke the first setter found.crates/perry-runtime/src/object/alloc.rs#L715-L726: mirror that same prototype-chain lookup inobject_assign_throw_if_set_rejectedsoObject.assigndoes not reject a write that the shared setter path would accept.
📍 Affects 2 files
crates/perry-runtime/src/object/field_set_by_name.rs#L890-L917(this comment)crates/perry-runtime/src/object/alloc.rs#L715-L726
🤖 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/object/field_set_by_name.rs` around lines 890 - 917,
The accessor short-circuit check in the field_set_by_name.rs section (lines
890-917) currently only looks for accessors on the receiver object itself,
missing setters defined on the prototype chain. Modify the accessor lookup to
walk the prototype chain starting from the receiver object, checking each
prototype in order until a setter is found, and invoke the first setter
discovered rather than treating the property as non-existent when no own
accessor exists. Additionally, update the object_assign_throw_if_set_rejected
function in alloc.rs (lines 715-726) to implement the same prototype-chain
traversal logic so that Object.assign respects prototype setters consistently
with the updated field_set_by_name behavior, preventing writes from being
incorrectly rejected when a valid setter exists on a prototype.
| if !value_is_object_like(descriptor_value) | ||
| || crate::symbol::js_is_symbol(descriptor_value) != 0 | ||
| { |
There was a problem hiding this comment.
Don’t reject valid descriptor objects that aren’t ObjectHeader-backed.
Both guards still hinge on value_is_object_like, which intentionally excludes Proxy handles and INT32-tagged class refs. That means Object.defineProperty(obj, k, proxyDesc) and class-ref descriptors still fail here with “Property description must be an object” even though they are valid objects. If this path is meant to tighten ToPropertyDescriptor, it needs to admit those object kinds and keep all subsequent descriptor reads on the value-level HasProperty/Get path instead of ObjectHeader-only access.
Also applies to: 1336-1338
🤖 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/object/object_ops.rs` around lines 1281 - 1283, The
descriptor validation at lines 1281-1283 and 1336-1338 use
`value_is_object_like` which rejects valid descriptor objects like Proxy handles
and INT32-tagged class refs. Remove or relax the `value_is_object_like` check in
both locations to allow these object kinds, and refactor all subsequent
descriptor property reads (such as accessing properties like `value`,
`writable`, `get`, `set`, `enumerable`, `configurable`) to use value-level
operations like `HasProperty` and `Get` instead of direct `ObjectHeader` access,
ensuring the code works with both ObjectHeader-backed objects and
Proxy/class-ref descriptors.
… delete/set, symbol & non-extensible throws Closes a dense test262 built-ins/Object descriptor cluster (+15, 0 regressions; Reflect/Proxy byte-identical). Five shared root causes in the ToPropertyDescriptor / ValidateAndApplyPropertyDescriptor paths: 1. Named (non-index) array property define did not merge with the existing property: it reset every omitted attribute to false and overwrote the value with `undefined`. Now retains omitted attrs/value on redefine (mirrors the index path), defaulting to false only for new properties. (array_object_ops) 2. `delete` of an array's named property only dropped the value store entry, so a named accessor (which lives only in the side tables) survived — hasOwnProperty kept reporting it. Now clears the accessor + attrs side-table state too. (delete_rest) 3. `[[Set]]` on a frozen/sealed object's accessor threw instead of invoking the setter (the frozen check ran before the accessor short-circuit). Hoisted the accessor short-circuit above the frozen/sealed/writable checks in both the sidecar and linear-scan key-lookup paths; freezing an accessor only clears [[Configurable]], the setter still runs. (field_set_by_name, alloc) 4. A Symbol passed as a descriptor was accepted (Symbol is pointer-tagged); ToPropertyDescriptor(Symbol) now throws "Property description must be an object". (object_ops) 5. Defining a NEW own property on a non-extensible array did not throw; added the extensible check to the array define path. (array_object_ops)
proggeramlug
force-pushed
the
fix/object-descriptor-cluster
branch
from
cb77f6b to
2fa3da2
Compare
|
Thanks @coderabbitai — addressed in the pushed update (also fixed the rustfmt lint failure): 1. Generic descriptor on an existing named accessor (valid — fixed). You're right: the named-property data path is also reached by generic descriptors (attrs-only, no value/writable/get/set), and it would have stripped the accessor. Added the same guard the index path already has — a generic descriptor on an existing accessor now only updates 2. Prototype-chain setters (pre-existing, out of scope). This PR doesn't change prototype-setter coverage: the hoisted short-circuit calls 3. Validation after the update: |
|
Tip For best results, initiate chat on the files or code changes.
The +15/0 result with a byte-identical Reflect/Proxy failure set against 🐇 ✨ |
1 participant
Summary
Closes a dense test262
built-ins/Objectproperty-descriptor failure cluster by fixing shared root causes in theToPropertyDescriptor/ValidateAndApplyPropertyDescriptorengine. +15 tests, 0 regressions across the fullbuilt-ins/Objectsuite (3062 → 3077 passing);built-ins/Reflectandbuilt-ins/Proxyresults are byte-identical to pristineorigin/main(no regressions in adjacent descriptor consumers).Root causes fixed
Named (non-index) array property define didn't merge with the existing property.
Object.defineProperty(arr, "p", {writable:false})reset every omitted attribute tofalseand overwrote the value withundefined. Now retains omitted attrs/value on redefine (mirroring the index path); defaults omitted fields tofalseonly for genuinely new properties. (array_object_ops.rs)deleteof an array's named property left accessors behind. It only dropped the value-store entry, so a named accessor (which lives only in the side tables) survived andhasOwnPropertykept reporting it. Now also clears the accessor + attribute side-table state. (delete_rest.rs)[[Set]]on a frozen/sealed object's accessor threw instead of calling the setter. The frozen check ran before the accessor short-circuit. Hoisted the accessor short-circuit above the frozen/sealed/writable checks in both the O(1) sidecar and the linear-scan key-lookup paths — freezing an accessor only clears[[Configurable]], the setter still runs. (field_set_by_name.rs,alloc.rs)A Symbol accepted as a property descriptor. A Symbol is pointer-tagged, so it passed the "descriptor must be an object" check.
ToPropertyDescriptor(Symbol())now throws "Property description must be an object". (object_ops.rs)Defining a new own property on a non-extensible array didn't throw. Added the extensibility check to the array define path. (
array_object_ops.rs)Per-sub-area improvement (built-ins/Object)
defineProperty: +9defineProperties: +5assign: +1Verification
built-ins/Object: 3062 → 3077 pass, 0 regressions (scripts/test262_subset.py).built-ins/Reflect(97/102) andbuilt-ins/Proxy(207/240): failure sets identical to pristineorigin/main— no regressions.cargo test --release -p perry-runtime -p perry-stdlib -p perry-codegengreen (the lone parallel failure,object::tests::builtin_prototype_methods_reject_dynamic_new, is a pre-existing shared-global isolation flake — passes single-threaded and on pristinemain).Remaining (separate root causes, out of scope)
Distinct subsystems, not part of the descriptor engine:
Array.prototype[N]accessor reads on the special built-in proto;Object/Function/Date.prototypemutation observed via inheritance; hugearray.length(dense-array / sparse limitation — surfaces as crashes on ~4e9 lengths);Object.creategetPrototypeOfidentity (known prototype-observability limitation);Object.assignwith Proxy sources / symbol-accessor enumeration order.Summary by CodeRabbit
Bug Fixes
Object.definePropertyand property assignment on frozen/non-extensible objects to prevent incorrect “forbidden new-property add” and related errors.Object.definePropertydescriptor processing for arrays and named properties to better match expected spec behavior forenumerable,configurable, and accessor/data transitions.obj[key] = value.