From 350a1b9590203856e4bb19ba2b48c18e5ab96e3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 10 Aug 2026 23:05:29 +0200 Subject: [PATCH] fix(runtime): Object.defineProperty on a class installs a STATIC own property (#7190) --- .../7798-class-static-define-property.md | 16 ++++ .../src/object/class_registry.rs | 8 +- .../src/object/class_registry/state.rs | 34 +++++++++ .../perry-runtime/src/object/descriptors.rs | 20 ++++- crates/perry-runtime/src/object/mod.rs | 9 +++ crates/perry-runtime/src/object/object_ops.rs | 11 +-- .../src/object/object_ops/define_property.rs | 60 +++++++++++++++ .../object/object_ops/descriptor_helpers.rs | 10 +++ ...t_gap_class_static_define_property_7190.ts | 73 +++++++++++++++++++ 9 files changed, 232 insertions(+), 9 deletions(-) create mode 100644 changelog.d/7798-class-static-define-property.md create mode 100644 test-files/test_gap_class_static_define_property_7190.ts diff --git a/changelog.d/7798-class-static-define-property.md b/changelog.d/7798-class-static-define-property.md new file mode 100644 index 0000000000..1c1b83d6c5 --- /dev/null +++ b/changelog.d/7798-class-static-define-property.md @@ -0,0 +1,16 @@ +**`Object.defineProperty(SomeClass, key, descriptor)` now installs a static own property** (#7190). It was silently dropped — not misfiled, dropped: `C.zzz` came back `undefined` and so did `new C().zzz`, so the value went nowhere at all. + +The cause is that `C` and `C.prototype` answer `class_ref_id` with the **same class id** — Perry maps a prototype ref back to its class — so the define path could not tell the two receivers apart and treated every one as a prototype install. That is correct for `Object.defineProperty(C.prototype, …)`, the drizzle `applyMixins` case the arm was written for, and wrong for the class itself. `class_prototype_ref_id` is the discriminator, and `descriptors.rs` was already using it to tell the two apart when reporting descriptors; the define path now does the same and routes a bare class ref into `CLASS_DYNAMIC_PROPS`, the table `static x = …` already writes to, so the existing static read path finds it with no new lookup. + +The user-visible form was zod: it renames constructors with `Object.defineProperty(Cls, "name", { value })`, and Perry kept resolving `.name` through the class registry, so class errors reported `constructor.name === "Definition"`. + +Two things that had to come with it, both found by the oracle rather than by reasoning: + +* **Attributes.** A declared `static x = …` is writable and enumerable (CreateDataPropertyOrThrow); a `defineProperty` data descriptor is neither. Both now live in one table, so the descriptor-installed ones carry their `(writable, enumerable, configurable)` bits and an *absent* entry keeps the previous `(true, true, true)` reporting for declared fields. Without this, `Object.keys(C)` gained a key Node does not report — the first cut of this fix did exactly that, leaking a non-enumerable `hidden` into both `Object.keys` and `for…in`. +* **`configurable` is retain-or-default, not default.** ECMA-262 `[[DefineOwnProperty]]` defaults an omitted field to `false` on a NEW property but RETAINS it on an existing one. The built-in `name`/`length` slots are `configurable: true`, so redefining `name` without saying `configurable` must stay configurable while a brand-new key must not. Hardcoding either answer fails one of the two, and both appear in the same test. + +`getOwnPropertyDescriptor(C, "name")` now agrees with `C.name` too — previously the value read reported the redefined string while the descriptor still reported the declared one, which is the state that makes a define look like it never happened. + +Verified against Node v26.5.1: the new gap test `test_gap_class_static_define_property_7190.ts` passes byte-for-byte, covering all three receivers (function, class, class prototype), an arbitrary key as well as `name`, subclasses, class expressions, and the enumerability/descriptor bits. `test_gap_class` (25) and `test_gap_static` (3) stay green. + +Two pre-existing failures were checked rather than assumed: `test_gap_2159_defineproperty_class_prototype` fails on clean `main` in the release sweep and its diff is an unsettled top-level await, not a descriptor; and the runtime lib suite's intermittent failure is #7365 — `obj_dispatch_ic_tests::a_hit_requires_matching_name_bytes_not_a_matching_address` fails **10 of 12** isolated runs on clean `main` against 7 of 12 with this change, so it is order-dependent flake and not fallout here. diff --git a/crates/perry-runtime/src/object/class_registry.rs b/crates/perry-runtime/src/object/class_registry.rs index 33d0a44f15..6827bd0891 100644 --- a/crates/perry-runtime/src/object/class_registry.rs +++ b/crates/perry-runtime/src/object/class_registry.rs @@ -57,9 +57,11 @@ pub(crate) use state::{ class_object_value_root_store, class_own_enumerable_field_names, class_own_static_field_value, class_parent_closure, class_parent_closure_root_store, class_prototype_method_is_enumerable, class_prototype_method_set_enumerable, class_prototype_method_value_cache_root_store, - class_prototype_object_root_store, global_object_prototype_bits, - is_bound_native_method_closure_value, is_non_constructable_builtin_function_value, - parent_closure_in_chain, throw_non_constructable_builtin_function, + class_prototype_object_root_store, class_static_defined_attrs, + class_static_key_is_non_enumerable, class_static_set_defined_attrs, + global_object_prototype_bits, is_bound_native_method_closure_value, + is_non_constructable_builtin_function_value, parent_closure_in_chain, + throw_non_constructable_builtin_function, }; pub use state::{ ClassVTable, VTableMethodEntry, CLASS_DECL_PROTOTYPE_OBJECTS, CLASS_DYNAMIC_PARENT_VALUE, diff --git a/crates/perry-runtime/src/object/class_registry/state.rs b/crates/perry-runtime/src/object/class_registry/state.rs index e89c1437e9..6afa63b151 100644 --- a/crates/perry-runtime/src/object/class_registry/state.rs +++ b/crates/perry-runtime/src/object/class_registry/state.rs @@ -97,6 +97,10 @@ pub(crate) fn class_own_enumerable_field_names(class_id: u32) -> Vec { props .keys() .filter(|k| !k.starts_with('#')) + // #7190: a key installed by `Object.defineProperty` without + // `enumerable: true` shares this table with static fields + // but is NOT enumerable. + .filter(|k| !class_static_key_is_non_enumerable(class_id, k)) .cloned() .collect() }) @@ -104,6 +108,36 @@ pub(crate) fn class_own_enumerable_field_names(class_id: u32) -> Vec { }) } +/// #7190: record a `defineProperty`-installed static key's attributes. Called +/// only from the define path; `static x = …` never touches it, so a declared +/// field keeps its CreateDataPropertyOrThrow `(writable, enumerable) = (true, +/// true)` reporting. +pub(crate) fn class_static_set_defined_attrs( + class_id: u32, + name: &str, + writable: bool, + enumerable: bool, + configurable: bool, +) { + crate::object::CLASS_STATIC_DEFINED_ATTRS.with(|m| { + m.borrow_mut() + .entry(class_id) + .or_default() + .insert(name.to_string(), (writable, enumerable, configurable)); + }); +} + +/// `(writable, enumerable)` if this static key was installed by +/// `Object.defineProperty`; `None` for a declared `static x = …` field. +pub(crate) fn class_static_defined_attrs(class_id: u32, name: &str) -> Option<(bool, bool, bool)> { + crate::object::CLASS_STATIC_DEFINED_ATTRS + .with(|m| m.borrow().get(&class_id).and_then(|k| k.get(name)).copied()) +} + +pub(crate) fn class_static_key_is_non_enumerable(class_id: u32, name: &str) -> bool { + class_static_defined_attrs(class_id, name).is_some_and(|(_, enumerable, _)| !enumerable) +} + /// True when `name` is an own static data property (a static field, or a /// runtime `C.x = …` assignment) recorded in `CLASS_DYNAMIC_PROPS`. Presence /// only — does not read the value, so it never invokes a static getter. Used by diff --git a/crates/perry-runtime/src/object/descriptors.rs b/crates/perry-runtime/src/object/descriptors.rs index b54f3929bf..e802184b4f 100644 --- a/crates/perry-runtime/src/object/descriptors.rs +++ b/crates/perry-runtime/src/object/descriptors.rs @@ -379,6 +379,15 @@ pub extern "C" fn js_object_get_own_property_descriptor(obj_value: f64, key_valu && super::class_prototype_ref_id(obj_value).is_none() && super::class_registry::lookup_static_method_in_chain(class_id, "name") .is_none() + // #7190: an own static `name` — installed by + // `Object.defineProperty(C, "name", { value })` — must win + // over the class-registry name, the same way it already + // wins for `C.name` itself. Without this the VALUE read and + // the DESCRIPTOR disagreed: `C.name` reported the redefined + // string while `getOwnPropertyDescriptor(C, "name")` still + // reported the declared one, which is the state that makes + // a mismatch look like the define never happened. + && !super::class_registry::class_has_own_dynamic_prop(class_id, "name") { if let Some(class_name) = super::class_registry::class_name_for_id(class_id) { let s = crate::string::js_string_from_bytes( @@ -469,7 +478,16 @@ pub extern "C" fn js_object_get_own_property_descriptor(obj_value: f64, key_valu if let Some(v) = super::class_registry::class_own_static_field_value(class_id, &method_name) { - return build_data_descriptor(v, true, true, true); + // #7190: a key installed by `Object.defineProperty` + // reports the attributes it was defined with; a + // declared `static x = …` field keeps (true, true, true). + let (writable, enumerable, configurable) = + super::class_registry::class_static_defined_attrs( + class_id, + &method_name, + ) + .unwrap_or((true, true, true)); + return build_data_descriptor(v, writable, enumerable, configurable); } } } diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index a2c1fee463..24de8badbc 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -567,6 +567,15 @@ crate::perry_thread_local! { /// which are tagged integers rather than ObjectHeader/ClosureHeader values. pub(crate) static CLASS_DELETED_KEYS: std::cell::RefCell>> = std::cell::RefCell::new(std::collections::HashMap::new()); + + /// #7190: `(writable, enumerable)` for static own keys installed by + /// `Object.defineProperty(C, k, desc)`. They live in `CLASS_DYNAMIC_PROPS` + /// next to `static x = …` fields, which are writable AND enumerable by + /// CreateDataPropertyOrThrow — a data descriptor defaults to neither. An + /// ABSENT entry therefore means "declared static field", and keeps the + /// previous `(true, true)` reporting untouched. + pub(crate) static CLASS_STATIC_DEFINED_ATTRS: std::cell::RefCell>> = + std::cell::RefCell::new(std::collections::HashMap::new()); } // Storage: `ObjectHotTables::{shape_inline_cache, shape_cache_overflow}`. diff --git a/crates/perry-runtime/src/object/object_ops.rs b/crates/perry-runtime/src/object/object_ops.rs index b4ddf8705a..3fd60443e5 100644 --- a/crates/perry-runtime/src/object/object_ops.rs +++ b/crates/perry-runtime/src/object/object_ops.rs @@ -35,11 +35,12 @@ pub use prototype::{ // Internal `pub(crate)` helpers shared between siblings / the rest of the crate. pub(crate) use descriptor_helpers::{ define_property_force_store_value, desc_has_field, desc_read_field, - describe_value_for_type_error, descriptor_enumerable, enforce_define_property_invariants, - registered_buffer_index_own_property_present, throw_object_type_error, - throw_object_type_error_with_suffix, try_decode_descriptor, validate_nonconfigurable_redefine, - validate_property_descriptor, validate_property_descriptor_view, value_is_object_like, - DESC_CONFIGURABLE, DESC_ENUMERABLE, DESC_GET, DESC_SET, DESC_VALUE, DESC_WRITABLE, + describe_value_for_type_error, descriptor_enumerable, descriptor_writable, + enforce_define_property_invariants, registered_buffer_index_own_property_present, + throw_object_type_error, throw_object_type_error_with_suffix, try_decode_descriptor, + validate_nonconfigurable_redefine, validate_property_descriptor, + validate_property_descriptor_view, value_is_object_like, DESC_CONFIGURABLE, DESC_ENUMERABLE, + DESC_GET, DESC_SET, DESC_VALUE, DESC_WRITABLE, }; // Module-private `unsafe fn value_is_callable` (descriptor_helpers): used by the // object_ops children (`accessors.rs`, `descriptor_helpers.rs`) but NOT diff --git a/crates/perry-runtime/src/object/object_ops/define_property.rs b/crates/perry-runtime/src/object/object_ops/define_property.rs index 2e3951353e..fb76647dee 100644 --- a/crates/perry-runtime/src/object/object_ops/define_property.rs +++ b/crates/perry-runtime/src/object/object_ops/define_property.rs @@ -516,6 +516,66 @@ pub extern "C" fn js_object_define_property( let value_field = js_object_get_field_by_name(desc_ptr as *const ObjectHeader, value_key); if !value_field.is_undefined() { + // #7190: `C` and `C.prototype` both answer + // `class_ref_id` with the SAME class id — the arm this + // sits in exists because `C.prototype` maps back to the + // class in Perry. So the two receivers were + // indistinguishable here, and every define took the + // prototype route. That is right for + // `Object.defineProperty(C.prototype, …)` (the drizzle + // `applyMixins` case this arm was written for) and wrong + // for `Object.defineProperty(C, …)`, which is a STATIC + // own property: `C.zzz` came back `undefined`, and so + // did `new C().zzz`, so the value went nowhere at all. + // + // `class_prototype_ref_id` is the discriminator — it + // answers only for the prototype ref — and it is what + // `descriptors.rs` already uses to tell the two apart + // when reporting descriptors. + if super::super::class_prototype_ref_id(obj_value).is_none() { + // A static own property lands in CLASS_DYNAMIC_PROPS, + // the same table `static x = …` and + // `js_class_register_static_field` write to, so the + // existing static read path finds it with no new + // lookup. + super::super::class_registry::class_dynamic_prop_root_store( + target_cid, + name.clone(), + f64::from_bits(value_field.bits()), + ); + // A data descriptor is non-enumerable unless it + // says otherwise; a `static x = …` field IS + // enumerable, and both share CLASS_DYNAMIC_PROPS. + // Record which this was, or `Object.keys(C)` gains + // a key node does not report. + // ECMA-262 [[DefineOwnProperty]]: a field the + // descriptor omits DEFAULTS to false on a new + // property but is RETAINED on an existing one. The + // built-in `name`/`length` slots are + // `configurable: true`, which is why redefining + // `name` without saying `configurable` must stay + // configurable — while a brand-new key must not. + let has_cfg = desc_has_field(descriptor_value, b"configurable"); + let configurable = if has_cfg { + crate::value::js_is_truthy(f64::from_bits( + desc_read_field(descriptor_value, b"configurable").bits(), + )) != 0 + } else { + super::super::class_registry::class_static_defined_attrs( + target_cid, &name, + ) + .map(|(_, _, cfg)| cfg) + .unwrap_or(matches!(name.as_str(), "name" | "length")) + }; + super::super::class_registry::class_static_set_defined_attrs( + target_cid, + &name, + descriptor_writable(descriptor_value), + descriptor_enumerable(descriptor_value), + configurable, + ); + return obj_value; + } // #5024 followup: a `defineProperty` data descriptor is // non-enumerable unless it explicitly sets // `enumerable: true`. Record that so the prototype-object diff --git a/crates/perry-runtime/src/object/object_ops/descriptor_helpers.rs b/crates/perry-runtime/src/object/object_ops/descriptor_helpers.rs index 749b7f9aa8..ea90f4a99c 100644 --- a/crates/perry-runtime/src/object/object_ops/descriptor_helpers.rs +++ b/crates/perry-runtime/src/object/object_ops/descriptor_helpers.rs @@ -418,6 +418,16 @@ pub(crate) unsafe fn descriptor_enumerable(descriptor_value: f64) -> bool { )) != 0 } +/// #7190: same rule for `writable`. A data descriptor that omits the field +/// defines a non-writable property, so absence is `false` rather than "keep the +/// default" — the caller records this alongside `enumerable` for class statics. +pub(crate) unsafe fn descriptor_writable(descriptor_value: f64) -> bool { + desc_has_field(descriptor_value, b"writable") + && crate::value::js_is_truthy(f64::from_bits( + desc_read_field(descriptor_value, b"writable").bits(), + )) != 0 +} + /// Validate a property descriptor object per ES `ToPropertyDescriptor` /// invariants that Node surfaces as `TypeError`s (#2817). Assumes /// `descriptor_value` is already known to be an object. Throws on: diff --git a/test-files/test_gap_class_static_define_property_7190.ts b/test-files/test_gap_class_static_define_property_7190.ts new file mode 100644 index 0000000000..39ff907172 --- /dev/null +++ b/test-files/test_gap_class_static_define_property_7190.ts @@ -0,0 +1,73 @@ +// Gap: `Object.defineProperty(SomeClass, key, descriptor)` (#7190). +// +// A static define on a CLASS was silently dropped — not misfiled, dropped: +// `C.zzz` was `undefined` and so was `new C().zzz`, so the value went nowhere. +// The cause is that `C` and `C.prototype` answer `class_ref_id` with the SAME +// class id (Perry maps the prototype ref back to its class), so the define path +// could not tell the two receivers apart and treated every one as a prototype +// install. That is right for `defineProperty(C.prototype, …)` — the drizzle +// `applyMixins` case that arm was written for — and wrong for `defineProperty(C, …)`. +// +// The user-visible form was zod: class errors reported +// `constructor.name === "Definition"` because the library renames constructors +// with `Object.defineProperty(Cls, "name", { value })`, and Perry kept +// resolving `.name` through the class registry. +// +// This test asserts all three receivers stay distinct — function, class, +// class prototype — and covers the attribute bits, because a static field and a +// `defineProperty` data descriptor share one side table but have opposite +// defaults: `static x = …` is writable+enumerable (CreateDataPropertyOrThrow), +// a data descriptor is neither. Getting that wrong does not show up in the +// value, only in `Object.keys` and the descriptor — which is exactly the shape +// that hid the original bug. + +class D {} +Object.defineProperty(D, "name", { value: "Renamed" }); +console.log("class-name:", D.name); +console.log("class-desc:", JSON.stringify(Object.getOwnPropertyDescriptor(D, "name"))); + +// A plain function was always correct; it must stay correct. +function f() {} +Object.defineProperty(f, "name", { value: "RenamedFn" }); +console.log("fn-name:", f.name); + +// Subclass, and the instance's view of it. +class Base {} +class Sub extends Base {} +Object.defineProperty(Sub, "name", { value: "SubRenamed" }); +console.log("sub-name:", Sub.name); +console.log("ctor-name:", (new (Sub as any)() as any).constructor.name); + +// Class expression. +const E = class {}; +Object.defineProperty(E, "name", { value: "ExprRenamed" }); +console.log("expr-name:", (E as any).name); + +// An arbitrary key, not just `name` — the drop was general. +class G {} +Object.defineProperty(G, "zzz", { value: 7, configurable: true }); +console.log("static-zzz:", (G as any).zzz); +// ...and it must NOT have landed on the prototype. +console.log("instance-zzz:", (new G() as any).zzz); + +// Defining on the prototype still installs an instance member. +class H {} +Object.defineProperty(H.prototype, "pm", { value: 5, configurable: true }); +console.log("proto-pm:", (new H() as any).pm); + +// Enumerability: a data descriptor defaults to non-enumerable, a declared +// static field is enumerable, and both live in the same table. +class K { + static declared = 1; +} +Object.defineProperty(K, "hidden", { value: 7 }); +Object.defineProperty(K, "shown", { value: 8, enumerable: true }); +console.log("keys:", JSON.stringify(Object.keys(K).sort())); +const seen: string[] = []; +for (const k in K) seen.push(k); +console.log("forin:", JSON.stringify(seen.sort())); +console.log("values:", (K as any).hidden, (K as any).shown, K.declared); +console.log("hidden-desc:", JSON.stringify(Object.getOwnPropertyDescriptor(K, "hidden"))); +console.log("shown-desc:", JSON.stringify(Object.getOwnPropertyDescriptor(K, "shown"))); +console.log("declared-desc:", JSON.stringify(Object.getOwnPropertyDescriptor(K, "declared"))); +console.log("names:", JSON.stringify(Object.getOwnPropertyNames(K).sort()));