fix(runtime): a live-index read inside forEach defers the squeeze instead of shifting the walk

changelog.d/9561-foreach-live-index-read.md

@@ -0,0 +1,24 @@
+**Map/Set: an array-like `map[i]` / `set[i]` read inside a `forEach` callback no longer makes the walk skip entries.**
+
+`#9504` made the array-like indexed read on a collection a *live-index*
+accessor: it squeezes tombstones first so raw index == live index, and never
+hands out a hole. `forEach` walks the raw entries with a counter that is
+protected against the *delete-path* squeeze (the walk registers itself and
+that squeeze defers) — but not against the accessor's. A callback that deleted
+already-visited entries and then read `map[j]` compacted the buffer under the
+walk's counter, shifting the survivors below it: with two earlier entries
+deleted, two later ones were never visited (18 of 20).
+
+While a `forEach` walk is active the accessor now defers the squeeze exactly
+as the delete path does, and resolves the live index by stepping over the
+tombstones (O(idx), on a path that is rare by construction); the outermost
+walk's completion performs the deferred squeeze as before. Outside a walk the
+accessor squeezes as `#9504` specified. The `for…of` fast path is unaffected —
+its cursor rebases through the compaction log (`#9513`), so a squeeze under it
+was already exact.
+
+Found by the automated review on #9513. Regressions: runtime unit tests for
+Map and Set (visit set, live values read mid-walk, layout left alone during
+the walk, squeezed on completion) and
+`test_gap_foreach_live_index_read_no_skip.ts` (single and nested walks),
+node-differential.

crates/perry-runtime/src/map.rs

@@ -2899,13 +2899,48 @@ pub extern "C" fn js_map_entry_key_at(map: *const MapHeader, idx: u32) -> f64 {
         // exactly. The walkers themselves never come here: they read through
         // `js_map_entry_key_raw_at`, which never compacts (a compaction per
         // observed hole is what made delete-during-`for…of` O(n) per delete).
-        compact_if_holey(map as *mut MapHeader);
-        if idx >= (*map).size {
+        //
+        // While a `forEach` walk is active the squeeze is DEFERRED instead —
+        // that walk's raw counter is not epoch-rebased, so compacting under it
+        // (a callback that deletes an earlier entry and then reads `map[j]`)
+        // shifted the survivors below its counter and it skipped entries. The
+        // live index is translated by stepping over holes, and the outermost
+        // walk's completion performs the deferred squeeze as it already does
+        // for the delete path.
+        let Some(raw) = live_index_to_raw(map, idx) else {
             return f64::from_bits(TAG_UNDEFINED);
-        }
+        };
         let entries = entries_ptr(map);
-        ptr::read(entries.add(idx as usize * 2))
+        ptr::read(entries.add(raw as usize * 2))
+    }
+}
+
+/// Resolve a LIVE entry index onto the raw entries buffer for the live-index
+/// accessors: squeeze first when no `forEach` walk is active (so raw == live
+/// and the read is O(1)), otherwise leave the layout alone and step over the
+/// tombstones (O(idx)). `None` past the live size.
+unsafe fn live_index_to_raw(map: *const MapHeader, idx: u32) -> Option<u32> {
+    if idx >= (*map).size {
+        return None;
+    }
+    if !map_foreach_is_active(map) {
+        compact_if_holey(map as *mut MapHeader);
+        return Some(idx);
+    }
+    let used = (*map).used;
+    let entries = entries_ptr(map);
+    let mut live = 0u32;
+    let mut raw = 0u32;
+    while raw < used {
+        if ptr::read(entries.add(raw as usize * 2)).to_bits() != MAP_HOLE_KEY_BITS {
+            if live == idx {
+                return Some(raw);
+            }
+            live += 1;
+        }
+        raw += 1;
     }
+    None
 }
 
 /// RAW twin of `js_map_entry_key_at` for the raw-index walkers (the `for…of`
@@ -2940,12 +2975,11 @@ pub extern "C" fn js_map_entry_value_at(map: *const MapHeader, idx: u32) -> f64
     }
     unsafe {
         // Live-index accessor — see `js_map_entry_key_at`.
-        compact_if_holey(map as *mut MapHeader);
-        if idx >= (*map).size {
+        let Some(raw) = live_index_to_raw(map, idx) else {
             return f64::from_bits(TAG_UNDEFINED);
-        }
+        };
         let entries = entries_ptr(map);
-        ptr::read(entries.add(idx as usize * 2 + 1))
+        ptr::read(entries.add(raw as usize * 2 + 1))
     }
 }
 

crates/perry-runtime/src/map_tombstone_tests.rs

@@ -485,3 +485,72 @@ fn clear_truncates_the_squeeze_history() {
     );
     assert_eq!(js_map_entry_key_raw_at(map, 0), 7.0);
 }
+
+extern "C" fn delete_earlier_then_live_read(
+    _closure: *const crate::closure::ClosureHeader,
+    value: f64,
+    key: f64,
+    collection: f64,
+) -> f64 {
+    FOREACH_DELETE_VISITS.with(|visits| visits.borrow_mut().push((key.to_bits(), value.to_bits())));
+    if key == 5.0 {
+        let map = crate::value::js_nanbox_get_pointer(collection) as *mut MapHeader;
+        js_map_delete(map, 1.0);
+        js_map_delete(map, 2.0);
+        // The array-like `map[0]` read: a LIVE-index accessor. It must answer
+        // the live element without squeezing the layout under the walk.
+        assert_eq!(js_map_entry_key_at(map, 0), 0.0, "live index 0 is key 0");
+        assert_eq!(
+            js_map_entry_key_at(map, 1),
+            3.0,
+            "live index 1 skips the two holes"
+        );
+        assert_eq!(js_map_entry_value_at(map, 1), 30.0);
+        unsafe {
+            assert_ne!(
+                (*map).used,
+                (*map).size,
+                "the walk's raw layout was left alone"
+            );
+        }
+    }
+    f64::from_bits(crate::value::TAG_UNDEFINED)
+}
+
+#[test]
+fn a_live_index_read_inside_foreach_defers_the_squeeze_and_skips_nothing() {
+    // A callback that deletes already-visited entries and then reads `map[j]`
+    // used to compact the entries under forEach's raw counter, shifting the
+    // survivors below it: keys 6 and 7 were never visited.
+    let map = js_map_alloc(32);
+    let expected = (0..20)
+        .map(|key| (key as f64, (key * 10) as f64))
+        .collect::<Vec<_>>();
+    for &(key, value) in &expected {
+        js_map_set(map, key, value);
+    }
+    js_map_foreach(
+        map,
+        foreach_callback(delete_earlier_then_live_read as *const u8),
+        f64::from_bits(crate::value::TAG_UNDEFINED),
+    );
+    assert_eq!(
+        take_foreach_delete_visits(),
+        expected,
+        "every entry visited exactly once"
+    );
+    assert_eq!(js_map_size(map), 18);
+    unsafe {
+        assert_eq!(
+            (*map).used,
+            (*map).size,
+            "the outermost walk's completion squeezed"
+        );
+    }
+    // Outside a walk the accessor squeezes as before (#9504 contract).
+    js_map_delete(map, 3.0);
+    assert_eq!(js_map_entry_key_at(map, 1), 4.0);
+    unsafe {
+        assert_eq!((*map).used, (*map).size);
+    }
+}

crates/perry-runtime/src/set.rs

@@ -1879,13 +1879,43 @@ pub extern "C" fn js_set_value_at(set: *const SetHeader, i: u32) -> f64 {
         // those paths only, RECORDED in the compaction log so an open
         // `for…of` cursor rebases exactly. The walkers read through
         // `js_set_value_raw_at`, which never compacts.
-        compact_if_holey_set(set as *mut SetHeader);
-        if i >= (*set).size {
+        //
+        // While a `forEach` walk is active the squeeze is DEFERRED (its raw
+        // counter is not epoch-rebased; compacting under it skipped entries)
+        // and the live index is translated by stepping over holes; the
+        // outermost walk's completion performs the deferred squeeze.
+        let Some(raw) = live_index_to_raw_set(set, i) else {
             return f64::from_bits(UNDEF);
-        }
+        };
         let elements = (*set).elements as *const f64;
-        ptr::read(elements.add(i as usize))
+        ptr::read(elements.add(raw as usize))
+    }
+}
+
+/// Resolve a LIVE element index onto the raw elements buffer — see
+/// `map::live_index_to_raw`.
+unsafe fn live_index_to_raw_set(set: *const SetHeader, idx: u32) -> Option<u32> {
+    if idx >= (*set).size {
+        return None;
+    }
+    if !set_foreach_is_active(set) {
+        compact_if_holey_set(set as *mut SetHeader);
+        return Some(idx);
+    }
+    let used = (*set).used;
+    let elements = elements_ptr(set);
+    let mut live = 0u32;
+    let mut raw = 0u32;
+    while raw < used {
+        if ptr::read(elements.add(raw as usize)).to_bits() != SET_HOLE_VALUE_BITS {
+            if live == idx {
+                return Some(raw);
+            }
+            live += 1;
+        }
+        raw += 1;
     }
+    None
 }
 
 /// RAW twin of `js_set_value_at` for the raw-index walkers (the `for…of`

crates/perry-runtime/src/set_tombstone_tests.rs

@@ -394,3 +394,63 @@ fn clear_truncates_the_squeeze_history() {
     );
     assert_eq!(js_set_value_raw_at(set, 0), 7.0);
 }
+
+extern "C" fn delete_earlier_then_live_read(
+    _closure: *const crate::closure::ClosureHeader,
+    value: f64,
+    _value_again: f64,
+    collection: f64,
+) -> f64 {
+    FOREACH_DELETE_VISITS.with(|visits| visits.borrow_mut().push(value.to_bits()));
+    if value == 5.0 {
+        let set = crate::value::js_nanbox_get_pointer(collection) as *mut SetHeader;
+        js_set_delete(set, 1.0);
+        js_set_delete(set, 2.0);
+        assert_eq!(js_set_value_at(set, 0), 0.0, "live index 0 is value 0");
+        assert_eq!(
+            js_set_value_at(set, 1),
+            3.0,
+            "live index 1 skips the two holes"
+        );
+        unsafe {
+            assert_ne!(
+                (*set).used,
+                (*set).size,
+                "the walk's raw layout was left alone"
+            );
+        }
+    }
+    f64::from_bits(crate::value::TAG_UNDEFINED)
+}
+
+#[test]
+fn a_live_index_read_inside_foreach_defers_the_squeeze_and_skips_nothing() {
+    let expected = (0..20).map(|value| value as f64).collect::<Vec<_>>();
+    let set = js_set_alloc(32);
+    for &value in &expected {
+        js_set_add(set, value);
+    }
+    js_set_foreach(
+        set,
+        foreach_callback(delete_earlier_then_live_read as *const u8),
+        f64::from_bits(crate::value::TAG_UNDEFINED),
+    );
+    assert_eq!(
+        take_foreach_delete_visits(),
+        expected,
+        "every element visited exactly once"
+    );
+    assert_eq!(js_set_size(set), 18);
+    unsafe {
+        assert_eq!(
+            (*set).used,
+            (*set).size,
+            "the outermost walk's completion squeezed"
+        );
+    }
+    js_set_delete(set, 3.0);
+    assert_eq!(js_set_value_at(set, 1), 4.0);
+    unsafe {
+        assert_eq!((*set).used, (*set).size);
+    }
+}

test-files/test_gap_foreach_live_index_read_no_skip.ts

@@ -0,0 +1,43 @@
+// A forEach callback that deletes already-visited entries and then performs an
+// array-like indexed read on the collection (`set[0]` / `map[0]` — Perry's
+// live-index accessor, which squeezes tombstones so raw index == live index)
+// must not shift the raw layout under the walk's own counter: every entry not
+// deleted before its visit is visited exactly once. Node has no array-like
+// read on collections (it yields undefined), so only visit ORDER is printed.
+function walkSet(): string {
+  const s = new Set<number>();
+  for (let i = 0; i < 20; i++) s.add(i);
+  const seen: number[] = [];
+  s.forEach((v) => {
+    seen.push(v);
+    if (v === 5) { s.delete(1); s.delete(2); void (s as any)[0]; void (s as any)[1]; }
+  });
+  return `set ${seen.length} ${seen.join(",")} size=${s.size}`;
+}
+function walkMap(): string {
+  const m = new Map<number, number>();
+  for (let i = 0; i < 20; i++) m.set(i, i * 10);
+  const seen: number[] = [];
+  m.forEach((_v, k) => {
+    seen.push(k);
+    if (k === 5) { m.delete(1); m.delete(2); void (m as any)[0]; void (m as any)[1]; }
+  });
+  return `map ${seen.length} ${seen.join(",")} size=${m.size}`;
+}
+// Nested walks: the inner walk's read must not squeeze under the outer walk.
+function nested(): string {
+  const m = new Map<number, number>();
+  for (let i = 0; i < 12; i++) m.set(i, i);
+  const outer: number[] = [];
+  m.forEach((_v, k) => {
+    outer.push(k);
+    if (k === 4) {
+      m.delete(0);
+      m.forEach((_iv, ik) => { if (ik === 6) void (m as any)[0]; });
+    }
+  });
+  return `nested ${outer.length} ${outer.join(",")}`;
+}
+console.log(walkSet());
+console.log(walkMap());
+console.log(nested());