From 2fa3da2407532f142bb0a1c9d294aa1ee9e1f6fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 14 Jun 2026 17:18:27 +0200 Subject: [PATCH] =?UTF-8?q?fix(object):=20property-descriptor=20engine=20?= =?UTF-8?q?=E2=80=94=20named-array=20merge,=20accessor=20delete/set,=20sym?= =?UTF-8?q?bol=20&=20non-extensible=20throws?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- crates/perry-runtime/src/object/alloc.rs | 19 +-- .../src/object/array_object_ops.rs | 112 +++++++++++++++++- .../perry-runtime/src/object/delete_rest.rs | 14 +++ .../src/object/field_set_by_name.rs | 54 +++++---- crates/perry-runtime/src/object/object_ops.rs | 11 +- 5 files changed, 173 insertions(+), 37 deletions(-) diff --git a/crates/perry-runtime/src/object/alloc.rs b/crates/perry-runtime/src/object/alloc.rs index 336cfa9ece..eb1f96c64a 100644 --- a/crates/perry-runtime/src/object/alloc.rs +++ b/crates/perry-runtime/src/object/alloc.rs @@ -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() { diff --git a/crates/perry-runtime/src/object/array_object_ops.rs b/crates/perry-runtime/src/object/array_object_ops.rs index 7525b0cb73..92a81daaf9 100644 --- a/crates/perry-runtime/src/object/array_object_ops.rs +++ b/crates/perry-runtime/src/object/array_object_ops.rs @@ -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); @@ -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(), @@ -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(), @@ -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 = 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)); set_property_attrs( obj as usize, key_name.to_string(), diff --git a/crates/perry-runtime/src/object/delete_rest.rs b/crates/perry-runtime/src/object/delete_rest.rs index 068dccfddb..be1983b9c4 100644 --- a/crates/perry-runtime/src/object/delete_rest.rs +++ b/crates/perry-runtime/src/object/delete_rest.rs @@ -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, diff --git a/crates/perry-runtime/src/object/field_set_by_name.rs b/crates/perry-runtime/src/object/field_set_by_name.rs index e16f604e10..1f1017d08e 100644 --- a/crates/perry-runtime/src/object/field_set_by_name.rs +++ b/crates/perry-runtime/src/object/field_set_by_name.rs @@ -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; + } + } + } + // 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; @@ -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()) { diff --git a/crates/perry-runtime/src/object/object_ops.rs b/crates/perry-runtime/src/object/object_ops.rs index c19be3f18a..55d4d586ce 100644 --- a/crates/perry-runtime/src/object/object_ops.rs +++ b/crates/perry-runtime/src/object/object_ops.rs @@ -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 + { let desc = describe_value_for_type_error(descriptor_value); throw_object_type_error_with_suffix( "Property description must be an object: ", @@ -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); }