diff --git a/changelog.d/7134-computed-key-class-proto.md b/changelog.d/7134-computed-key-class-proto.md new file mode 100644 index 0000000000..a87967f00e --- /dev/null +++ b/changelog.d/7134-computed-key-class-proto.md @@ -0,0 +1,30 @@ +**runtime:** computed / dynamic object-key property access on class prototypes +and class constructors now matches Node (#6945). + +Three cooperating gaps: + +1. `js_dyn_index_get` treated non-string, non-numeric keys as floats + (`format!("{}", f64)`), so `obj[{toString(){return "k"}}]` never ran user + ToPropertyKey. Object / boolean / null / undefined / bigint keys now go + through `js_to_property_key` (with receiver rooting) before the by-name + get — matching the set-side path in `js_dyn_index_set`. + +2. Class-instance field get walked the reflective decl-proto object only for + *accessors*, deliberately skipping data reads to avoid class-id re-entry. + Runtime `C.prototype[k] = v` stores an own data field there, so + `(new C()).name` missed it while `C.prototype.name` saw it. Own data is + now read via `own_data_field_by_name` (no re-walk). + +3. Codegen's IndexGet last-resort path routes non-string keys on a known + ClassRef through `js_object_get_index_polymorphic`, which rejected every + INT32-tagged receiver as a primitive. Registered class-ids now forward to + `js_dyn_index_get`'s class-ref arm so `C[k]` / `C[objectKey]` resolve + statics and `CLASS_DYNAMIC_PROPS`. + +Regression: `test-files/test_gap_computed_key_class_proto_6945.ts` +(byte-for-byte vs Node 26.5). + +Follow-up (CodeRabbit): set-side dynamic-index fallback and polymorphic +`rooted_property_key_{get,set}` now use `js_to_property_key` and route a +Symbol-yielding `@@toPrimitive` through the symbol store (was silently +undefined / stringified). diff --git a/crates/perry-runtime/src/object/class_registry/prototype_objects.rs b/crates/perry-runtime/src/object/class_registry/prototype_objects.rs index 880a81f582..92b066ed70 100644 --- a/crates/perry-runtime/src/object/class_registry/prototype_objects.rs +++ b/crates/perry-runtime/src/object/class_registry/prototype_objects.rs @@ -352,20 +352,36 @@ unsafe fn resolve_proto_chain_field_inner( // getter was invisible to `instance.name` (winston: // `Object.defineProperty(Logger.prototype, 'transports', { get })`, // read as `this.transports`, came back `undefined` → `.length` threw). - // Check the decl-proto object for an ACCESSOR only: it is allocated - // WITH this `class_id` (`js_object_alloc(class_id, 0)`), so routing its - // DATA reads back through `js_object_get_field_by_name` would re-enter - // this same walk for the same id and recurse infinitely (a Transform - // subclass's `_read` lookup stack-overflowed → SIGSEGV). Class methods / - // data are already covered by the vtable + `class_prototype_object` - // path below, so the accessor-only probe here is sufficient. - if let Some(receiver) = receiver { - let decl_proto = class_decl_prototype_object(cid); - if !decl_proto.is_null() { + // Decl-proto object (`CLASS_DECL_PROTOTYPE_OBJECTS`) is where a user + // `Object.defineProperty(ClassName.prototype, name, { get })` installs + // its accessor AND where a runtime `C.prototype[k] = v` (computed / + // dynamic index write) stores an OWN data field. It is allocated WITH + // this `class_id` (`js_object_alloc(class_id, 0)`), so routing DATA + // reads back through `js_object_get_field_by_name` would re-enter this + // same walk for the same id and recurse infinitely (a Transform + // subclass's `_read` lookup stack-overflowed → SIGSEGV). + // + // Accessors: fire with the instance as receiver. + // Own data fields: read via `own_data_field_by_name` only (no class-id + // re-walk) so a computed prototype write is visible as + // `(new C()).name` (#6945). Class methods / vtable data still come + // from the vtable + `class_prototype_object` path below. + let decl_proto = class_decl_prototype_object(cid); + if !decl_proto.is_null() { + if let Some(receiver) = receiver { if let Some(value) = inherited_proto_accessor_value(decl_proto, key, receiver) { return Some(value); } } + // #6945: own data property written onto the reflective prototype + // (computed / dynamic-key set) — never re-enter by-name get. + if let Some(value) = + unsafe { super::super::field_get_set::own_data_field_by_name(decl_proto, key) } + { + if !value.is_undefined() { + return Some(value); + } + } } let proto_obj = class_prototype_object(cid); if !proto_obj.is_null() { diff --git a/crates/perry-runtime/src/object/polymorphic_index.rs b/crates/perry-runtime/src/object/polymorphic_index.rs index b19c137ad9..4536b52e37 100644 --- a/crates/perry-runtime/src/object/polymorphic_index.rs +++ b/crates/perry-runtime/src/object/polymorphic_index.rs @@ -10,14 +10,6 @@ use super::*; -unsafe fn property_key_string_ptr(value: f64) -> *mut crate::StringHeader { - let key = crate::object::js_to_property_key(value); - if crate::symbol::js_is_symbol(key) != 0 { - return std::ptr::null_mut(); - } - crate::value::js_jsvalue_to_string(key) -} - /// `obj[key]` READ through a non-canonical (object / exotic) key, with the /// receiver rooted across the coercion (#6935). /// @@ -27,14 +19,27 @@ unsafe fn property_key_string_ptr(value: f64) -> *mut crate::StringHeader { /// `valueOf`, allocates, and can trigger a GC that **evacuates** the receiver. /// `raw` is a bare `u64` address in a Rust local — not a GC root and not a /// shadow slot — so it has to be re-read through a handle afterwards. +/// +/// #6945 / CodeRabbit: if ToPropertyKey yields a Symbol (e.g. `@@toPrimitive` +/// returns one), route through the symbol side-table — never stringify the +/// Symbol and never treat the Symbol case as "no key" (the old +/// `property_key_string_ptr` null-out silently returned `undefined`). unsafe fn rooted_property_key_get(raw: u64, idx: f64) -> f64 { let scope = crate::gc::RuntimeHandleScope::new(); let recv = scope.root_raw_mut_ptr(raw as *mut ObjectHeader); - let key = property_key_string_ptr(idx); - if key.is_null() { + let key = crate::object::js_to_property_key(idx); + let key_h = scope.root_nanbox_f64(key); + let key = key_h.get_nanbox_f64(); + let recv_bits = + crate::value::js_nanbox_pointer(recv.get_raw_const_ptr::() as i64); + if crate::symbol::js_is_symbol(key) != 0 { + return crate::symbol::js_object_get_symbol_property(recv_bits, key); + } + let key_ptr = crate::value::js_get_string_pointer_unified(key) as *const crate::StringHeader; + if key_ptr.is_null() { return f64::from_bits(crate::value::TAG_UNDEFINED); } - let key_handle = scope.root_string_ptr(key); + let key_handle = scope.root_string_ptr(key_ptr); let v = js_object_get_field_by_name( recv.get_raw_mut_ptr::(), key_handle.get_raw_const_ptr::(), @@ -47,20 +52,31 @@ unsafe fn rooted_property_key_get(raw: u64, idx: f64) -> f64 { /// This is the corruption half: the coercion sits between the receiver/value /// arriving and the store, so pre-fix a stale receiver dropped the write onto a /// forwarding stub and a stale `value` planted a dangling pointer *inside* a -/// live object, outliving the call. +/// live object, outliving the call. Symbol-yielding ToPropertyKey routes to +/// the symbol store (#6945 / CodeRabbit). unsafe fn rooted_property_key_set(raw: u64, idx: f64, value: f64) { let scope = crate::gc::RuntimeHandleScope::new(); let recv = scope.root_raw_mut_ptr(raw as *mut ObjectHeader); let value_handle = scope.root_nanbox_f64(value); - let key = property_key_string_ptr(idx); - if key.is_null() { + let key = crate::object::js_to_property_key(idx); + let key_h = scope.root_nanbox_f64(key); + let key = key_h.get_nanbox_f64(); + let value = value_handle.get_nanbox_f64(); + let recv_bits = + crate::value::js_nanbox_pointer(recv.get_raw_const_ptr::() as i64); + if crate::symbol::js_is_symbol(key) != 0 { + crate::symbol::js_object_set_symbol_property(recv_bits, key, value); + return; + } + let key_ptr = crate::value::js_get_string_pointer_unified(key) as *const crate::StringHeader; + if key_ptr.is_null() { return; } - let key_handle = scope.root_string_ptr(key); + let key_handle = scope.root_string_ptr(key_ptr); js_object_set_field_by_name( recv.get_raw_mut_ptr::(), key_handle.get_raw_const_ptr::(), - value_handle.get_nanbox_f64(), + value, ); } @@ -109,17 +125,28 @@ fn numeric_key_i32_index(value: f64) -> Option { pub extern "C" fn js_object_get_index_polymorphic(obj_handle: i64, idx: f64) -> f64 { let raw = if (obj_handle as u64) >> 48 >= 0x7FF8 { // NaN-boxed: only POINTER_TAG (0x7FFD) and STRING_TAG (0x7FFF) carry a - // heap pointer in the low 48 bits. INT32 (0x7FFE), BIGINT (0x7FFA) and - // the undefined/null/bool tags (0x7FFC) are PRIMITIVES — indexing them - // yields `undefined` per JS (`(983055)[0] === undefined`). Treating an - // INT32's integer payload as a pointer derefs a wild address → SIGSEGV. - // This is the Next.js app-page-turbo render crash: a NaN-boxed-int - // receiver (0xf000f = 983055) indexed inside a class `get` method - // (js_object_get_index_polymorphic read its GcHeader at raw-8). Reject - // non-pointer/non-string NaN-boxed receivers up front (cross-platform — - // not dependent on a heap-address floor). + // heap pointer in the low 48 bits. INT32 (0x7FFE) is usually a primitive + // number — BUT a registered **class-ref** is also INT32-tagged, and + // `C[k]` with a non-string key (object ToPropertyKey, numeric static + // name, …) reaches this helper from codegen's IndexGet last-resort + // path. Class refs must NOT be rejected as primitives: route them + // through `js_dyn_index_get`, which has the dedicated class-ref arm + // (static methods / CLASS_DYNAMIC_PROPS / ToString key). (#6945) + // BIGINT (0x7FFA) and the undefined/null/bool tags (0x7FFC) remain + // primitives — indexing them yields `undefined` per JS. Treating an + // INT32 *number* payload as a pointer would SIGSEGV (Next.js + // app-page-turbo: 0xf000f indexed inside a class `get`); only a + // *registered* class-id takes the class-ref arm. match (obj_handle as u64) >> 48 { 0x7FFD | 0x7FFF => (obj_handle as u64) & 0x0000_FFFF_FFFF_FFFF, + 0x7FFE => { + let class_id = (obj_handle as u64 & 0xFFFF_FFFF) as u32; + if class_id != 0 && crate::object::class_registry::is_class_id_registered(class_id) + { + return crate::value::js_dyn_index_get(f64::from_bits(obj_handle as u64), idx); + } + return f64::from_bits(crate::value::TAG_UNDEFINED); + } _ => return f64::from_bits(crate::value::TAG_UNDEFINED), } } else { diff --git a/crates/perry-runtime/src/value/dyn_index.rs b/crates/perry-runtime/src/value/dyn_index.rs index 58544327fe..524855baaa 100644 --- a/crates/perry-runtime/src/value/dyn_index.rs +++ b/crates/perry-runtime/src/value/dyn_index.rs @@ -269,6 +269,45 @@ pub extern "C" fn js_dyn_index_get(value: f64, index: f64) -> f64 { } return f64::from_bits(TAG_UNDEFINED); } + // #6945: a non-string, non-numeric index must run ToPropertyKey (object + // keys invoke `toString`/`valueOf`/`@@toPrimitive`; booleans/null/ + // undefined/bigint stringify) before the by-name get. The arms below + // cast `index as i32` / `format!("{}", index)`, which treat an object + // NaN-box as a float and never call user coercion — so + // `proto[{toString(){return "k"}}]` missed a write that + // `proto.k` / `proto["k"]` could see. Mirrors the set-side + // `js_jsvalue_to_string` path in `js_dyn_index_set`. + { + let idx_js = JSValue::from_bits(idx_bits); + // INT32-tagged keys are integer property names (and class-ref values + // used as keys, rare); pure f64 numbers keep the element path. Every + // other tag is a ToPropertyKey case. + if !idx_js.is_number() && !idx_js.is_int32() { + let scope = crate::gc::RuntimeHandleScope::new(); + let recv = scope.root_raw_mut_ptr(raw_ptr as *mut crate::object::ObjectHeader); + // Prefer `js_to_property_key` so a Symbol-returning toString is + // preserved (and then routed via the symbol arm). Root the + // coerced key: ToPropertyKey can allocate / run user JS. + let key = unsafe { crate::object::js_to_property_key(index) }; + let key_h = scope.root_nanbox_f64(key); + let key = key_h.get_nanbox_f64(); + if unsafe { crate::symbol::js_is_symbol(key) } != 0 { + let recv_bits = crate::value::js_nanbox_pointer( + recv.get_raw_const_ptr::() as i64, + ); + return unsafe { crate::symbol::js_object_get_symbol_property(recv_bits, key) }; + } + let key_ptr = + crate::value::js_get_string_pointer_unified(key) as *const crate::StringHeader; + if key_ptr.is_null() { + return f64::from_bits(TAG_UNDEFINED); + } + return crate::object::js_object_get_field_by_name_f64( + recv.get_raw_const_ptr::(), + key_ptr, + ); + } + } let idx_i32 = if index.is_nan() || index.is_infinite() { return f64::from_bits(TAG_UNDEFINED); } else { @@ -582,18 +621,36 @@ pub extern "C" fn js_dyn_index_set(obj: f64, index: f64, value: f64) -> f64 { ); return value; } - // #6935: this is the corruption case. `js_jsvalue_to_string(index)` runs a - // user `toString` / `valueOf` for an object index (`obj[{toString(){...}}] = v`) - // and allocates for every other shape, so it can GC and EVACUATE. Both the + // #6935: ToPropertyKey (below) runs a user `toString` / `valueOf` / + // `@@toPrimitive` for an object index (`obj[{toString(){...}}] = v`) and + // allocates for every other shape, so it can GC and EVACUATE. Both the // receiver `raw_ptr` and the `value` being stored were raw Rust locals // across it: a stale receiver dropped the write onto a forwarding stub, and // a stale `value` wrote a dangling pointer INTO a live object, where it // outlives the call. + // + // #6945 / CodeRabbit: use `js_to_property_key` (not `js_jsvalue_to_string`) + // so an `@@toPrimitive` that returns a Symbol is preserved and routed to + // the symbol store — matching the get-side fallback. Stringifying that + // Symbol would miss the target property (and Spec ToPropertyKey must not + // turn a Symbol result into a string). let scope = crate::gc::RuntimeHandleScope::new(); let recv = scope.root_raw_mut_ptr(raw_ptr as *mut crate::object::ObjectHeader); let value_handle = scope.root_nanbox_f64(value); - let key_ptr = crate::value::js_jsvalue_to_string(index); + let key = unsafe { crate::object::js_to_property_key(index) }; + let key_h = scope.root_nanbox_f64(key); + let key = key_h.get_nanbox_f64(); let value = value_handle.get_nanbox_f64(); + if unsafe { crate::symbol::js_is_symbol(key) } != 0 { + let recv_bits = crate::value::js_nanbox_pointer( + recv.get_raw_const_ptr::() as i64, + ); + unsafe { + crate::symbol::js_object_set_symbol_property(recv_bits, key, value); + } + return value; + } + let key_ptr = crate::value::js_get_string_pointer_unified(key) as *const crate::StringHeader; if key_ptr.is_null() { return value; } diff --git a/test-files/test_gap_computed_key_class_proto_6945.ts b/test-files/test_gap_computed_key_class_proto_6945.ts new file mode 100644 index 0000000000..f22e1b3070 --- /dev/null +++ b/test-files/test_gap_computed_key_class_proto_6945.ts @@ -0,0 +1,76 @@ +// #6945: a computed object-key write onto a class prototype (or class +// constructor) must be visible to: +// - the computed read with the same key object (ToPropertyKey), +// - the plain string-key read, +// - instance inheritance through the prototype chain. +// Pre-fix, the write landed somewhere the dotted `C.prototype.name` read +// found, but `C.prototype[k]` and `(new C()).name` returned undefined because +// `js_dyn_index_get` treated object indices as floats (`format!("{}", f64)`) +// instead of running ToPropertyKey / user `toString`. + +class C { + m(): number { + return 1; + } +} +const k: any = { + toString(): string { + return "protoKey"; + }, +}; +(C.prototype as any)[k] = { tag: 6 }; +console.log("computed via instance:", JSON.stringify((new C() as any).protoKey)); +console.log("computed direct:", JSON.stringify((C.prototype as any).protoKey)); +console.log("computed via key obj:", JSON.stringify((C.prototype as any)[k])); + +const plain: any = {}; +plain[k] = { tag: 7 }; +console.log("plain:", JSON.stringify(plain.protoKey)); +console.log("plain via key obj:", JSON.stringify(plain[k])); + +// class-constructor (static) side: same ToPropertyKey obligation +class D { + static s(): number { + return 1; + } +} +const ks: any = { + toString(): string { + return "statKey"; + }, +}; +(D as any)[ks] = { tag: 6 }; +console.log("static via name:", JSON.stringify((D as any).statKey)); +console.log("static via key obj:", JSON.stringify((D as any)[ks])); + +// coercion is observable even when the property is absent +let calls = 0; +const absent: any = { + toString(): string { + calls++; + return "nope"; + }, +}; +console.log("absent via key obj:", JSON.stringify((C.prototype as any)[absent])); +console.log("absent coercion count:", calls); + +// boolean / null keys also go through ToPropertyKey +const mixed: any = {}; +mixed[true as any] = "t"; +mixed[null as any] = "n"; +console.log("bool key:", mixed["true"]); +console.log("null key:", mixed["null"]); +console.log("bool via true:", mixed[true as any]); + +// @@toPrimitive returning a Symbol must use the symbol store (get + set), +// not stringify the Symbol (get-side ToPropertyKey parity on set — #7134 CR). +const sym = Symbol("viaPrim"); +const viaSym: any = { + [Symbol.toPrimitive](_hint: string): symbol { + return sym; + }, +}; +const holder: any = {}; +holder[viaSym] = { tag: 9 }; +console.log("viaPrim set+get:", JSON.stringify(holder[viaSym])); +console.log("viaPrim symbol key:", JSON.stringify(holder[sym]));