Skip to content

fix(object): property-descriptor engine — named-array merge, accessor delete/set, symbol & non-extensible throws #5146

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
proggeramlug merged 1 commit into from
Jun 14, 2026
+173 −37
Merged

fix(object): property-descriptor engine — named-array merge, accessor delete/set, symbol & non-extensible throws #5146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions crates/perry-runtime/src/object/alloc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,15 +712,20 @@ unsafe fn object_assign_throw_if_set_rejected(
if target.is_null() || (target as usize) <= 0x10000 {
return;
}
// Accessor own property: a setter must exist, else the write fails. Check
// this BEFORE `own_key_present`: an accessor-only property (`{ set foo(){} }`)
// lives in the accessor side table and may have no `keys_array` entry, so
// `own_key_present` can report it absent — which on a frozen/non-extensible
// target would mis-classify the setter call as a forbidden new-property add
// (test262 assign/target-is-frozen-accessor-property-set-succeeds).
if let Some(acc) = super::get_accessor_descriptor(target as usize, name) {
if acc.set == 0 {
throw_object_assign_readonly(name);
}
return;
}
let exists = own_key_present(target, key_ptr);
if exists {
// Accessor own property: a setter must exist, else the write fails.
if let Some(acc) = super::get_accessor_descriptor(target as usize, name) {
if acc.set == 0 {
throw_object_assign_readonly(name);
}
return;
}
// Data own property: must be writable.
if let Some(attrs) = super::get_property_attrs(target as usize, name) {
if !attrs.writable() {
Expand Down
112 changes: 106 additions & 6 deletions crates/perry-runtime/src/object/array_object_ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,29 @@ pub(crate) unsafe fn define_array_property(
Some(crate::value::js_is_truthy(f64::from_bits(v.bits())) != 0)
};

// A NEW own property on a non-extensible array is forbidden (ECMA-262
// OrdinaryDefineOwnProperty: `extensible` false + no current property ⇒
// reject). `Object.preventExtensions`/`seal`/`freeze` set the flag; an
// existing index/named property can still be redefined within the spec
// bounds. (test262 defineProperty/15.2.3.6-4-198 and friends.)
{
let named_exists = super::canonical_array_index(key_name).is_none()
&& (super::get_accessor_descriptor(obj as usize, key_name).is_some()
|| crate::array::array_named_property_get_by_name(arr, key_name).is_some());
let index_exists = super::canonical_array_index(key_name)
.map(|_| super::has_own_helpers::array_own_key_present(arr, key_str))
.unwrap_or(false);
if !named_exists && !index_exists {
let gc = gc_header_for(obj);
if (*gc)._reserved & crate::gc::OBJ_FLAG_NO_EXTEND != 0 {
super::throw_object_type_error_with_suffix(
"Cannot define property ",
&format!("{key_name}, object is not extensible"),
);
}
}
}

if let Some(index) = super::canonical_array_index(key_name) {
let exists = super::has_own_helpers::array_own_key_present(arr, key_str);

Expand Down Expand Up @@ -622,6 +645,22 @@ pub(crate) unsafe fn define_array_property(
} else {
prior.map(|a| a.set).unwrap_or(0)
};
// Retain attrs the descriptor omits when redefining; a new accessor
// (or one replacing a data property) defaults to
// non-enumerable/non-configurable. An existing data property with no
// side-table entry has default all-true attributes, so a
// data→accessor conversion keeps `enumerable`/`configurable`.
let existed = prior.is_some()
|| super::get_property_attrs(obj as usize, key_name).is_some()
|| crate::array::array_named_property_get_by_name(arr, key_name).is_some();
let cur = if existed {
Some(
super::get_property_attrs(obj as usize, key_name)
.unwrap_or_else(|| PropertyAttrs::new(true, true, true)),
)
} else {
None
};
set_accessor_descriptor(
obj as usize,
key_name.to_string(),
Expand All @@ -630,8 +669,10 @@ pub(crate) unsafe fn define_array_property(
set: set_bits,
},
);
let enumerable = read_bool(b"enumerable").unwrap_or(false);
let configurable = read_bool(b"configurable").unwrap_or(false);
let enumerable = read_bool(b"enumerable")
.unwrap_or_else(|| cur.map(|a| a.enumerable()).unwrap_or(false));
let configurable = read_bool(b"configurable")
.unwrap_or_else(|| cur.map(|a| a.configurable()).unwrap_or(false));
set_property_attrs(
obj as usize,
key_name.to_string(),
Expand All @@ -642,11 +683,70 @@ pub(crate) unsafe fn define_array_property(
}
}

crate::array::array_named_property_set(arr, key_str, value);
// ValidateAndApplyPropertyDescriptor merge for a named (non-index) data
// property: a redefine keeps the attributes (and the value) the descriptor
// omits, while a brand-new property defaults omitted fields to false. The
// index path above already does this; the named path historically reset
// every omitted field to false and overwrote the value with `undefined`
// (dropping `arr.prop = 12` then `defineProperty(arr,"prop",{writable:false})`
// back to `{value: undefined}`). Mirror the index-path merge here.
let cur_accessor = super::get_accessor_descriptor(obj as usize, key_name);
let exists = cur_accessor.is_some()
|| crate::array::array_named_property_get_by_name(arr, key_name).is_some();
let cur_attrs: Option<PropertyAttrs> = if exists {
Some(
super::get_property_attrs(obj as usize, key_name)
.unwrap_or_else(|| PropertyAttrs::new(true, true, true)),
)
} else {
None
};

// A GENERIC descriptor (only enumerable/configurable — no value/writable,
// and no get/set, since accessor descriptors returned above) on an existing
// ACCESSOR property must only update the attributes; it must NOT convert the
// accessor back to a data property (spec ValidateAndApplyPropertyDescriptor:
// IsGenericDescriptor leaves [[Get]]/[[Set]] intact). Mirrors the index path.
if cur_accessor.is_some() && !has_value && !super::desc_has_field(descriptor_value, b"writable")
{
let cur = cur_attrs.unwrap_or_else(|| PropertyAttrs::new(false, false, false));
let enumerable = read_bool(b"enumerable").unwrap_or_else(|| cur.enumerable());
let configurable = read_bool(b"configurable").unwrap_or_else(|| cur.configurable());
set_property_attrs(
obj as usize,
key_name.to_string(),
PropertyAttrs::new(false, enumerable, configurable),
);
let _ = obj_value;
return Some(true);
}

// Redefining a former accessor back to a data property drops the stale
// accessor entry (the non-configurable case already threw above).
if cur_accessor.is_some() {
ACCESSOR_DESCRIPTORS.with(|m| {
m.borrow_mut().remove(&(obj as usize, key_name.to_string()));
});
}

// Write the value: an explicit `value` wins; a NEW property with no value
// defaults to `undefined`; a redefine that omits `value` keeps the current.
if has_value {
crate::array::array_named_property_set(arr, key_str, value);
} else if !exists {
crate::array::array_named_property_set(
arr,
key_str,
f64::from_bits(crate::value::TAG_UNDEFINED),
);
}

let writable = read_bool(b"writable").unwrap_or(false);
let enumerable = read_bool(b"enumerable").unwrap_or(false);
let configurable = read_bool(b"configurable").unwrap_or(false);
let writable =
read_bool(b"writable").unwrap_or_else(|| cur_attrs.map(|a| a.writable()).unwrap_or(false));
let enumerable = read_bool(b"enumerable")
.unwrap_or_else(|| cur_attrs.map(|a| a.enumerable()).unwrap_or(false));
let configurable = read_bool(b"configurable")
.unwrap_or_else(|| cur_attrs.map(|a| a.configurable()).unwrap_or(false));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
set_property_attrs(
obj as usize,
key_name.to_string(),
Expand Down
14 changes: 14 additions & 0 deletions crates/perry-runtime/src/object/delete_rest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,20 @@ pub extern "C" fn js_object_delete_field(
index,
);
}
// Named (non-index) property: drop the value-store entry AND
// any accessor / attribute side-table state. A named
// accessor (`Object.defineProperty(arr, "p", {get})`) lives
// ONLY in the side tables, so without these clears the
// delete was a no-op and `hasOwnProperty("p")` stayed true
// (test262 verifyProperty's configurable check deletes then
// asserts the key is gone).
crate::array::array_named_property_delete(
obj as *const crate::array::ArrayHeader,
key,
);
super::clear_accessor_descriptor(obj as usize, name);
super::clear_property_attrs(obj as usize, name);
return 1;
}
crate::array::array_named_property_delete(
obj as *const crate::array::ArrayHeader,
Expand Down
54 changes: 32 additions & 22 deletions crates/perry-runtime/src/object/field_set_by_name.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,6 +887,35 @@ pub extern "C" fn js_object_set_field_by_name(
None
};

// 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;
}
}
}
Comment on lines +890 to +917

@coderabbitai coderabbitai Bot Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

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 in object_assign_throw_if_set_rejected so Object.assign does 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.


// Search through the keys array for a match
let key_count = crate::array::js_array_length(keys) as usize;
let alloc_limit = std::cmp::max((*obj).field_count, 8) as usize;
Expand Down Expand Up @@ -1038,32 +1067,13 @@ pub extern "C" fn js_object_set_field_by_name(
// Found it - update the field. Frozen objects must
// throw a TypeError on writes to existing keys
// (issue #615 — strict-mode behavior, default for TS).
// Accessors were already handled by the hoisted short-circuit
// above; a key found here is a data property, so a frozen object
// throws on the write (issue #615 — strict-mode default for TS).
if is_frozen {
let key_str = key_to_str_for_diag(key);
crate::error::throw_immutable_write(0, &key_str);
}
// Accessor short-circuit: if a setter is registered, invoke
// it instead of writing the slot. A getter-only accessor is
// read-only under Perry's strict-by-default TS semantics.
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;
}
}
}
// Per-property writable check (set by Object.defineProperty / freeze).
// Issue #615 — strict-mode throw on read-only assign.
if PROPERTY_ATTRS_IN_USE.with(|c| c.get()) {
Expand Down
11 changes: 9 additions & 2 deletions crates/perry-runtime/src/object/object_ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1278,7 +1278,9 @@ pub extern "C" fn js_object_define_property(
// `[[DefineOwnProperty]]` trap, and throw a TypeError if it reports
// failure. (Proxy crash cluster.)
if crate::proxy::js_proxy_is_proxy(obj_value) != 0 {
if !value_is_object_like(descriptor_value) {
if !value_is_object_like(descriptor_value)
|| crate::symbol::js_is_symbol(descriptor_value) != 0
{
Comment on lines +1281 to +1283

@coderabbitai coderabbitai Bot Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

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.

let desc = describe_value_for_type_error(descriptor_value);
throw_object_type_error_with_suffix(
"Property description must be an object: ",
Expand Down Expand Up @@ -1328,7 +1330,12 @@ pub extern "C" fn js_object_define_property(
if !target_is_class_ref && !value_is_object_like(obj_value) {
throw_object_type_error(b"Object.defineProperty called on non-object");
}
if !value_is_object_like(descriptor_value) {
// A descriptor must be an Object; a Symbol is pointer-tagged but not an
// object, so `ToPropertyDescriptor(Symbol())` throws (test262
// property-description-must-be-an-object-not-symbol).
if !value_is_object_like(descriptor_value)
|| crate::symbol::js_is_symbol(descriptor_value) != 0
{
let desc = describe_value_for_type_error(descriptor_value);
throw_object_type_error_with_suffix("Property description must be an object: ", &desc);
}
Expand Down