diff --git a/changelog.d/10607-shadowed-field-most-derived-slot.md b/changelog.d/10607-shadowed-field-most-derived-slot.md new file mode 100644 index 0000000000..c3fc2c6cbd --- /dev/null +++ b/changelog.d/10607-shadowed-field-most-derived-slot.md @@ -0,0 +1,23 @@ +### Fixed + +- **A field a subclass overrides could read as the base class's value from + every read that isn't a compile-time-typed `obj.field`.** `class Sub + extends Base { tag = "sub-tag" }` (with `Base { tag = "base-tag" }`) gave + `Sub` two inline field slots named `"tag"` — the packed class layout + (`crates/perry-codegen/src/codegen/mod.rs`) never deduplicates a name a + subclass re-declares — but only the most-derived one is ever written + (`class_field_global_index` already resolves a compile-time-typed + `obj.field` to that slot, "TS shadowing"). Every *dynamic* by-name lookup — + an inherited `Object.defineProperty` accessor's `this.field`, a computed + `obj[key]`, `Reflect.get`, `hasOwnProperty` — instead returned the + ancestor's never-written slot. Fixed in three runtime lookup sites + (`object/keys_lookup.rs`, `object/shapes.rs`, `object/field_get_set/ic_miss.rs`) + to agree with the compile-time path: `keys_find_slot_by_bytes` / + `keys_find_slot_by_key_ptr` and the IC-miss fast-path scan now walk + back-to-front, and the ≥32-key indexed lookup keeps the highest matching + slot index instead of the first. No storage-layout change; validated with + `test-files/test_gap_10595_inherited_accessor_field_shape.ts` (string and + Symbol keys, getter+setter, a two-level subclass, and a field overridden + with a different runtime type) plus new `perry-runtime` unit tests. Still + open: `Object.keys()`/`for...in` list a shadowed field's name twice, since + enumeration reads the same undeduplicated keys array — a follow-up. diff --git a/crates/perry-runtime/src/object/field_get_set/ic_miss.rs b/crates/perry-runtime/src/object/field_get_set/ic_miss.rs index 9d3b4ea774..4005e85cf2 100644 --- a/crates/perry-runtime/src/object/field_get_set/ic_miss.rs +++ b/crates/perry-runtime/src/object/field_get_set/ic_miss.rs @@ -893,7 +893,8 @@ pub(super) fn get_field_ic_miss_impl( let key_count = shape.logical_key_count as usize; let keys_data = (keys as *const u8).add(8) as *const f64; let alloc_limit = shape.live_inline_slot_count as usize; - for i in 0..key_count { + for i in (0..key_count).rev() { + // #10595: back-to-front so a shadowed field's most-derived slot wins; see keys_lookup.rs. let k_bits = (*keys_data.add(i)).to_bits(); let k_ptr = (k_bits & 0x0000_FFFF_FFFF_FFFF) as *const crate::StringHeader; if !k_ptr.is_null() && crate::string::js_string_equals(k_ptr, key) != 0 { diff --git a/crates/perry-runtime/src/object/keys_lookup.rs b/crates/perry-runtime/src/object/keys_lookup.rs index 50e827606c..81ea301c27 100644 --- a/crates/perry-runtime/src/object/keys_lookup.rs +++ b/crates/perry-runtime/src/object/keys_lookup.rs @@ -100,7 +100,20 @@ pub(crate) unsafe fn keys_find_slot_by_bytes( } let n = (key_count as usize).min(slot_len); let mut sso = [0u8; crate::value::SHORT_STRING_MAX_LEN]; - for i in 0..n { + // #10595: scan back-to-front. A subclass field that re-declares an + // ancestor's field name (`class Sub extends Base { tag = ... }` where + // `Base` also declares `tag`) is NOT deduplicated in the packed keys — + // `codegen/mod.rs` lists ancestor fields first, then the class's own, so + // the array holds one entry per DECLARATION, oldest ancestor first, most + // derived last. `class_field_global_index` (the compile-time-typed read's + // index resolver) already picks the most-derived declaration ("TS + // shadowing"); this dynamic by-name lookup must agree, or a receiver + // whose static type is unknown (an inherited accessor's `this.field`, a + // computed `obj[key]`) sees the ancestor's stale slot instead of the + // override. Scanning in reverse finds that same most-derived match first, + // with no change to storage layout and no cost in the (common, no + // shadowing) case where a name occurs once. + for i in (0..n).rev() { let v = crate::JSValue::from_bits((*slots.add(i)).to_bits()); if let Some(stored) = crate::string::js_string_key_bytes(v, &mut sso) { if stored == key_bytes { @@ -140,7 +153,8 @@ pub(crate) unsafe fn keys_find_slot_by_key_ptr( return None; } let n = (key_count as usize).min(slot_len); - for i in 0..n { + // #10595: same most-derived-wins scan direction as the fast path above. + for i in (0..n).rev() { let v = crate::JSValue::from_bits((*slots.add(i)).to_bits()); if crate::string::js_string_key_matches(v, key) { return Some(i as u32); @@ -185,3 +199,74 @@ pub(crate) fn keys_index_insert( } shapes::shape_note_append(keys, new_count, key_hash, slot); } + +#[cfg(test)] +mod tests_10595 { + use super::*; + + /// #10595: a subclass field that re-declares an ancestor's field name is + /// not deduplicated in the packed keys array built by + /// `crates/perry-codegen/src/codegen/mod.rs` (ancestor fields first, + /// then the class's own) — the array genuinely holds two entries for + /// one logical property, oldest declaration first. Only the LAST + /// (most-derived) slot is ever written, matching + /// `class_field_global_index`'s "TS shadowing" resolution for the + /// compile-time-typed path. A dynamic by-name lookup that returned the + /// first match instead found the never-initialized ancestor slot. + #[test] + fn duplicate_key_name_resolves_to_the_last_occurrence() { + let ancestor_key = crate::string::js_string_from_bytes(b"tag".as_ptr(), 3); + let override_key = crate::string::js_string_from_bytes(b"tag".as_ptr(), 3); + let keys = crate::array::js_array_alloc(4); + let keys = crate::array::js_array_push(keys, JSValue::string_ptr(ancestor_key)); + let keys = crate::array::js_array_push(keys, JSValue::string_ptr(override_key)); + + let lookup_key = crate::string::js_string_from_bytes(b"tag".as_ptr(), 3); + unsafe { + assert_eq!( + keys_find_slot_by_key_ptr(keys, 2, lookup_key), + Some(1), + "must resolve to the most-derived slot (index 1), not the ancestor's (index 0)" + ); + // `keys_find_slot_by_bytes` is the byte-slice twin the pointer + // form delegates to for a valid header; pin it directly too. + assert_eq!( + keys_find_slot_by_bytes(keys, 2, b"tag"), + Some(1), + "byte-slice lookup must agree with the pointer-key lookup" + ); + } + } + + /// The common (non-shadowing) case — a name that occurs exactly once — + /// must be completely unaffected by scanning in reverse. + #[test] + fn single_occurrence_key_is_unaffected_by_scan_direction() { + let a = crate::string::js_string_from_bytes(b"x".as_ptr(), 1); + let b = crate::string::js_string_from_bytes(b"y".as_ptr(), 1); + let keys = crate::array::js_array_alloc(4); + let keys = crate::array::js_array_push(keys, JSValue::string_ptr(a)); + let keys = crate::array::js_array_push(keys, JSValue::string_ptr(b)); + + let lookup_x = crate::string::js_string_from_bytes(b"x".as_ptr(), 1); + let lookup_y = crate::string::js_string_from_bytes(b"y".as_ptr(), 1); + unsafe { + assert_eq!(keys_find_slot_by_key_ptr(keys, 2, lookup_x), Some(0)); + assert_eq!(keys_find_slot_by_key_ptr(keys, 2, lookup_y), Some(1)); + } + } + + /// A key that is genuinely absent must still miss, in both scan + /// directions. + #[test] + fn absent_key_is_not_found() { + let a = crate::string::js_string_from_bytes(b"tag".as_ptr(), 3); + let keys = crate::array::js_array_alloc(4); + let keys = crate::array::js_array_push(keys, JSValue::string_ptr(a)); + + let lookup = crate::string::js_string_from_bytes(b"tagViaGetter".as_ptr(), 12); + unsafe { + assert_eq!(keys_find_slot_by_key_ptr(keys, 1, lookup), None); + } + } +} diff --git a/crates/perry-runtime/src/object/shapes.rs b/crates/perry-runtime/src/object/shapes.rs index 329caeeb07..215a2ea560 100644 --- a/crates/perry-runtime/src/object/shapes.rs +++ b/crates/perry-runtime/src/object/shapes.rs @@ -2027,6 +2027,22 @@ pub(crate) unsafe fn shape_slot_lookup_verdict( }; let mut sso = [0u8; crate::value::SHORT_STRING_MAX_LEN]; let (slots, slot_len) = super::keys_array_dense_slots(keys); + // #10595: keep scanning past the first content match and keep the + // HIGHEST slot index among them, not the probe order's first hit. + // A field name that a subclass re-declares (`class Sub extends Base { + // tag = ... }` where `Base` also declares `tag`) is NOT deduplicated in + // the packed keys — `codegen/mod.rs` lists ancestor fields first, then + // the class's own, so a genuine duplicate always has the most-derived + // declaration at the HIGHER slot index, regardless of this table's probe + // order (which is insertion order for a fresh table, but open-addressing + // growth/rehash can reshuffle it). `class_field_global_index` — the + // compile-time-typed read's index resolver — already picks the + // most-derived declaration ("TS shadowing"); this dynamic by-name lookup + // must agree, or a receiver whose static type is unknown (an inherited + // accessor's `this.field`, a computed `obj[key]`) sees the ancestor's + // stale slot instead of the override. A name with only one candidate + // (the common, non-shadowing case) is unaffected. + let mut found: Option = None; for i in shape.slots.candidates(key_hash) { if (i as usize) >= slot_len || i >= key_count { continue; @@ -2034,10 +2050,13 @@ pub(crate) unsafe fn shape_slot_lookup_verdict( let v = crate::JSValue::from_bits((*slots.add(i as usize)).to_bits()); if let Some(stored) = crate::string::js_string_key_bytes(v, &mut sso) { if stored == key_bytes { - return KeysIndexVerdict::Found(i); + found = Some(found.map_or(i, |prev| prev.max(i))); } } } + if let Some(i) = found { + return KeysIndexVerdict::Found(i); + } // Hash-bucket candidates existed but none matched: with a complete index // that still proves absence (the bucket held colliding OTHER keys). absent diff --git a/crates/perry-runtime/src/object/shapes_tests.rs b/crates/perry-runtime/src/object/shapes_tests.rs index 4325b432f9..5fd8e5fa71 100644 --- a/crates/perry-runtime/src/object/shapes_tests.rs +++ b/crates/perry-runtime/src/object/shapes_tests.rs @@ -1182,3 +1182,40 @@ fn the_ordinary_slot_query_declines_a_class_kind_shape() { ); } } + +#[cfg(test)] +mod issue_10595_tests { + use super::*; + + /// #10595: the >=`KEYS_INDEX_THRESHOLD` indexed lookup must agree with + /// the linear-scan lookups fixed in `object/keys_lookup.rs` — a + /// duplicate key name (a subclass field re-declaring an ancestor's + /// field, never deduplicated in the packed keys) must resolve to the + /// HIGHEST slot index among the candidates a hash bucket returns, not + /// whichever one the open-addressing probe order happens to visit + /// first. + #[test] + fn indexed_lookup_duplicate_key_name_resolves_to_the_highest_slot() { + let ancestor_key = crate::string::js_string_from_bytes(b"tag".as_ptr(), 3); + let override_key = crate::string::js_string_from_bytes(b"tag".as_ptr(), 3); + let keys = crate::array::js_array_alloc(4); + let keys = crate::array::js_array_push(keys, crate::JSValue::string_ptr(ancestor_key)); + let keys = crate::array::js_array_push(keys, crate::JSValue::string_ptr(override_key)); + + let h = crate::object::key_bytes_hash(b"tag".as_ptr(), 3); + unsafe { + // build=true: force the index to cover both slots regardless of + // KEYS_INDEX_THRESHOLD — the verdict function itself does not + // gate on that threshold, only its linear-scan callers do. + let verdict = shape_slot_lookup_verdict(keys, b"tag", h, 2, true); + match verdict { + KeysIndexVerdict::Found(slot) => assert_eq!( + slot, 1, + "must resolve to the most-derived slot (index 1), not the ancestor's (index 0)" + ), + KeysIndexVerdict::Absent => panic!("expected Found(1), got Absent"), + KeysIndexVerdict::Unindexed => panic!("expected Found(1), got Unindexed"), + } + } + } +} diff --git a/scripts/check_file_size.sh b/scripts/check_file_size.sh index f34edab097..27dc952b75 100755 --- a/scripts/check_file_size.sh +++ b/scripts/check_file_size.sh @@ -80,6 +80,12 @@ crates/perry-hir/src/lower_decl/body_stmt.rs # finally-wrapper tail (~790 lines) into a sibling module — is a mechanical # cut deferred to a focused follow-up, same pattern as body_stmt.rs above. crates/perry-runtime/src/promise/then.rs +# #10595 shadowed-field fix: `get_field_ic_miss_impl`'s inline own-key scan +# now walks back-to-front (1 line) with a 1-line rationale comment, matching +# the compile-time-typed path's most-derived-wins rule. The file was already +# exactly at the 2000-line gate; a structural split of the IC-miss ladder +# into a sibling module is deferred to a focused follow-up. +crates/perry-runtime/src/object/field_get_set/ic_miss.rs # --- Representation-aware type lowering (#5466 / #5464) --- # These files crossed the gate on the type-lowering branch (native i32/u32/f64/ # i128/StringRef reps, guarded fast/fallback splits, and the material-evidence diff --git a/test-files/test_gap_10595_inherited_accessor_field_shape.ts b/test-files/test_gap_10595_inherited_accessor_field_shape.ts new file mode 100644 index 0000000000..06e4fc8c68 --- /dev/null +++ b/test-files/test_gap_10595_inherited_accessor_field_shape.ts @@ -0,0 +1,81 @@ +// #10595: a field a subclass overrides must read as the SUBCLASS's value +// from every read path, not just a direct `obj.field` access — including +// from inside an inherited accessor (`Object.defineProperty` getter/setter), +// a dynamic `obj[computedKey]` read, and across a two-level `extends` chain. +// +// Root cause: a subclass field declaration that shares a name with an +// ancestor field is not deduplicated in the class's packed inline-slot +// layout (ancestor fields first, then the class's own), so the object ends +// up with two inline slots for the same logical property. The compile-time +// typed path (`obj.field` on a statically-known class) already resolves to +// the most-derived slot. Every DYNAMIC name-based lookup (an inherited +// accessor's `this.field`, a computed `obj[key]`) used to return the FIRST +// (ancestor's, uninitialized) slot instead. + +class Base { + tag = "base-tag"; +} + +Object.defineProperty(Base.prototype, "tagViaGetter", { + get() { + return (this as any).tag; + }, +}); + +const tagSym = Symbol("tagSym"); +Object.defineProperty(Base.prototype, tagSym, { + get() { + return (this as any).tag; + }, +}); + +let setterLog = ""; +Object.defineProperty(Base.prototype, "tagViaAccessor", { + get() { + return (this as any).tag; + }, + set(v: string) { + setterLog = (this as any).tag + ":" + v; + }, +}); + +class Sub extends Base { + tag = "sub-tag"; +} + +class SubSub extends Sub { + tag = "subsub-tag"; +} + +// A field overridden with a different runtime type than its ancestor. +class SubTyped extends Base { + tag: any = 42; +} + +const base = new Base(); +console.log("base.tag", base.tag); +console.log("base.tagViaGetter", (base as any).tagViaGetter); +console.log("base[tagSym]", (base as any)[tagSym]); + +const sub = new Sub(); +// Direct-read control: the non-accessor path already saw the override +// correctly before this fix, and must keep doing so. +console.log("sub.tag", sub.tag); +console.log("sub.tagViaGetter", (sub as any).tagViaGetter); +console.log("sub[tagSym]", (sub as any)[tagSym]); +const key = "tag"; +console.log("sub[computedKey]", (sub as any)[key]); + +const subsub = new SubSub(); +console.log("subsub.tag", subsub.tag); +console.log("subsub.tagViaGetter", (subsub as any).tagViaGetter); +console.log("subsub[tagSym]", (subsub as any)[tagSym]); + +const typed = new SubTyped(); +console.log("typed.tag", typed.tag); +console.log("typed.tagViaGetter", (typed as any).tagViaGetter); +console.log("typed[tagSym]", (typed as any)[tagSym]); + +(sub as any).tagViaAccessor = "written"; +console.log("setterLog", setterLog); +console.log("sub.tag after setter", sub.tag);