fix(runtime): resolve a shadowed inherited field to its most-derived slot

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.

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 {

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);
+        }
+    }
+}

crates/perry-runtime/src/object/shapes.rs

@@ -2027,17 +2027,36 @@ 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<u32> = None;
     for i in shape.slots.candidates(key_hash) {
         if (i as usize) >= slot_len || i >= key_count {
             continue;
         }
         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

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"),
+            }
+        }
+    }
+}

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

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);