From 5170340dd10bc94bf3683cb503564fb1808d186b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 6 Jun 2026 12:53:11 +0200 Subject: [PATCH] fix(object): retain existing-property attributes in Object.defineProperty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per ES OrdinaryDefineOwnProperty / ValidateAndApplyPropertyDescriptor, redefining an EXISTING own property must retain the attribute fields the descriptor omits — they do NOT reset to the new-property `false` default. `js_object_define_property` previously defaulted every omitted attribute to `false` (writable to `has_accessor`) on every call, so converting an enumerable data property to a getter (or back to a value) silently dropped its enumerability and configurability. This blocked zod v4 under `perry.compilePackages` (discussion #3438): its `$ZodObject` constructor swaps the enumerable `shape` data property for a self-replacing getter via `Object.defineProperty`, expecting `{ ...def }` to still spread `shape`. Pre-fix `shape` became non-enumerable, the spread dropped it, `normalizeDef`'s `value.shape` was undefined, and object parsing crashed with "Cannot read properties of undefined (reading '_zod')". Fix: in both the object and closure define paths, capture the property's current effective attributes before any mutation (all-true when no descriptor side-table entry exists, matching the JS data-property default) and default each omitted descriptor field to the existing value. New properties still default to `false`, preserving prior behavior. Adds test-files/test_issue_defineproperty_retain_attrs.ts (byte-matches `node --experimental-strip-types`). --- crates/perry-runtime/src/object/object_ops.rs | 57 ++++++++++++++--- .../test_issue_defineproperty_retain_attrs.ts | 62 +++++++++++++++++++ 2 files changed, 109 insertions(+), 10 deletions(-) create mode 100644 test-files/test_issue_defineproperty_retain_attrs.ts diff --git a/crates/perry-runtime/src/object/object_ops.rs b/crates/perry-runtime/src/object/object_ops.rs index a9cb97e16fe..db0f49d4740 100644 --- a/crates/perry-runtime/src/object/object_ops.rs +++ b/crates/perry-runtime/src/object/object_ops.rs @@ -1009,6 +1009,18 @@ pub extern "C" fn js_object_define_property( return obj_value; } + // Spec retention: redefining an existing own property keeps the + // attributes the descriptor omits (see the object-path comment). + let existing_attrs: Option = + if super::has_own_helpers::closure_own_key_present(closure_ptr, &key_rust) { + Some( + super::get_property_attrs(closure_ptr, &key_rust) + .unwrap_or_else(|| PropertyAttrs::new(true, true, true)), + ) + } else { + None + }; + let get_key = crate::string::js_string_from_bytes(b"get".as_ptr(), 3); let set_key = crate::string::js_string_from_bytes(b"set".as_ptr(), 3); let get_field = js_object_get_field_by_name(desc_ptr as *const ObjectHeader, get_key); @@ -1059,9 +1071,12 @@ pub extern "C" fn js_object_define_property( Some(crate::value::js_is_truthy(f64::from_bits(v.bits())) != 0) } }; - let writable = read_bool(b"writable").unwrap_or(has_accessor); - 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(|| existing_attrs.map(|a| a.writable()).unwrap_or(has_accessor)); + let enumerable = read_bool(b"enumerable") + .unwrap_or_else(|| existing_attrs.map(|a| a.enumerable()).unwrap_or(false)); + let configurable = read_bool(b"configurable") + .unwrap_or_else(|| existing_attrs.map(|a| a.configurable()).unwrap_or(false)); set_property_attrs( closure_ptr, key_rust, @@ -1219,6 +1234,23 @@ pub extern "C" fn js_object_define_property( return obj_value; } + // Spec (OrdinaryDefineOwnProperty / ValidateAndApplyPropertyDescriptor): + // when the property ALREADY EXISTS as an own property, attribute fields + // the descriptor omits must RETAIN the property's current values — they do + // NOT reset to the new-property `false` default. Capture the current + // attributes before any mutation below. `None` ⇒ the key is new, so the + // historical all-`false` (writable defaults to `has_accessor`) applies. + let existing_attrs: Option = key_rust.as_ref().and_then(|k| { + if super::obj_value_has_own_key(obj_value, key_value) { + Some( + super::get_property_attrs(obj as usize, k) + .unwrap_or_else(|| PropertyAttrs::new(true, true, true)), + ) + } else { + None + } + }); + // Detect accessor descriptor (has `get` and/or `set`) vs. data descriptor (has `value`). // JS disallows mixing them, but we only check for `get`/`set` presence. let get_key = crate::string::js_string_from_bytes(b"get".as_ptr(), 3); @@ -1296,13 +1328,18 @@ pub extern "C" fn js_object_define_property( Some(crate::value::js_is_truthy(f64::from_bits(v.bits())) != 0) } }; - // Accessor descriptors don't have `writable`; we leave it true so data - // lookups that happen before the accessor override don't accidentally - // reject a legitimate fallthrough write. Attrs default to false when - // omitted (JS spec). - let writable = read_bool(b"writable").unwrap_or(has_accessor); - let enumerable = read_bool(b"enumerable").unwrap_or(false); - let configurable = read_bool(b"configurable").unwrap_or(false); + // Omitted attributes default to the EXISTING property's value when + // redefining (spec retention, see `existing_attrs` above), else to + // `false` for a new property. Accessor descriptors don't carry + // `writable`; for a brand-new accessor we leave it `true` (via + // `has_accessor`) so data lookups before the accessor override don't + // reject a legitimate fallthrough write. + let writable = read_bool(b"writable") + .unwrap_or_else(|| existing_attrs.map(|a| a.writable()).unwrap_or(has_accessor)); + let enumerable = read_bool(b"enumerable") + .unwrap_or_else(|| existing_attrs.map(|a| a.enumerable()).unwrap_or(false)); + let configurable = read_bool(b"configurable") + .unwrap_or_else(|| existing_attrs.map(|a| a.configurable()).unwrap_or(false)); if let Some(k) = key_rust { set_property_attrs( diff --git a/test-files/test_issue_defineproperty_retain_attrs.ts b/test-files/test_issue_defineproperty_retain_attrs.ts new file mode 100644 index 00000000000..9899d2370f7 --- /dev/null +++ b/test-files/test_issue_defineproperty_retain_attrs.ts @@ -0,0 +1,62 @@ +// Regression for the `Object.defineProperty` attribute-retention bug that +// blocked zod v4 under `perry.compilePackages` (discussion #3438). +// +// Spec (OrdinaryDefineOwnProperty / ValidateAndApplyPropertyDescriptor): +// when redefining an EXISTING own property, any attribute the descriptor +// omits must RETAIN the property's current value — it does NOT reset to the +// new-property `false` default. Perry previously reset omitted attributes to +// `false` on every define, so converting an enumerable data property to a +// getter (and back to a value) silently dropped its enumerability. +// +// zod's `$ZodObject` relies on this: it replaces the enumerable `shape` data +// property with a self-replacing getter via `Object.defineProperty`, then +// `{ ...def }` is expected to still spread `shape`. Pre-fix `shape` became +// non-enumerable, `{ ...def }.shape` was undefined, and object parsing crashed +// with "Cannot read properties of undefined (reading '_zod')". +// +// Output must match `node --experimental-strip-types` byte-for-byte. + +// 1. Redefine an existing enumerable data property with only { value } — +// enumerable must stay true. +const a: any = { x: 1 }; +Object.defineProperty(a, "x", { value: 2 }); +console.log("redefine value keeps enumerable:", Object.keys(a).includes("x")); + +// 2. Convert an enumerable data property to a getter (only { get }) — +// enumerable retained, then back to a value — still retained. +const b: any = { y: 1 }; +Object.defineProperty(b, "y", { get: () => 99 }); +console.log("data->getter keeps enumerable:", Object.keys(b).includes("y")); +Object.defineProperty(b, "y", { value: 42 }); +console.log("getter->value keeps enumerable:", Object.keys(b).includes("y")); +console.log("spread keeps it:", "y" in { ...b }); + +// 3. Configurable is likewise retained: a configurable property stays +// configurable across a value-only redefine (no "Cannot redefine" throw). +const c: any = { z: 1 }; +Object.defineProperty(c, "z", { value: 2 }); +Object.defineProperty(c, "z", { value: 3 }); +console.log("still configurable z:", c.z); + +// 4. Control: a BRAND-NEW property defined via defineProperty still defaults +// to non-enumerable (omitted attributes are false for new properties). +const d: any = {}; +Object.defineProperty(d, "n", { value: 7 }); +console.log("new prop non-enumerable:", Object.keys(d).includes("n")); + +// 5. The zod-shaped pattern: enumerable data prop -> self-replacing getter -> +// value, then enumerable spread must still see it. +const def: any = { type: "object", shape: { a: 1, b: 2 } }; +const sh = def.shape; +Object.defineProperty(def, "shape", { + get: () => { + const newSh = { ...sh }; + Object.defineProperty(def, "shape", { value: newSh }); + return newSh; + }, +}); +const keys = Object.keys(def.shape); // triggers the getter +const spread = { ...def }; +console.log("normalized keys:", keys.join(",")); +console.log("spread.shape defined:", spread.shape !== undefined); +console.log("spread.shape keys:", Object.keys(spread.shape).join(","));