fix(array): route a Map/Set receiver before the array-only funnel in the fused forEach (#8117)

changelog.d/8130-fused-foreach-collection-reroute.md

@@ -0,0 +1,11 @@
+### Fixed
+
+- A 1-argument `.forEach(cb)` on a `Map` or `Set` no longer iterates nothing
+  when codegen could not statically prove the receiver was a collection
+  (`obj.someSet.forEach(cb)`, react-server-dom's `request.abortableTasks`).
+  Codegen fuses that shape to the array entry point `js_array_forEach`, whose
+  #5989 collection reroute sat behind `normalize_array_receiver`; #8041 widened
+  `clean_arr_ptr` to reject every tracked non-array, which nulls a
+  `GC_TYPE_SET`/`GC_TYPE_MAP` receiver and left the reroute unreachable. The
+  reroute now runs first, receiver-tag gated so an ordinary array still never
+  reaches a registry probe (#8117).

crates/perry-runtime/src/array/collection_tag_tests.rs

@@ -328,3 +328,129 @@ fn every_registered_collection_address_carries_its_own_type_tag() {
         assert!(crate::set::is_registered_set(set as usize));
     }
 }
+
+// ---------------------------------------------------------------------------
+// #8117: the fused ARRAY `forEach` entry point must still reach a Map/Set.
+//
+// Codegen fuses a 1-argument `<expr>.forEach(cb)` to `js_array_forEach`
+// whenever it cannot prove the receiver is a collection (`obj.someSet` is the
+// ordinary shape). #5989 put a Set/Map reroute inside that helper, but AFTER
+// `normalize_array_receiver`. #8041 then widened `clean_arr_ptr` from "reject
+// GC_TYPE_OBJECT / GC_TYPE_CLOSURE" to "reject every tracked non-array", which
+// nulls a Set/Map receiver — so the reroute became unreachable and the fused
+// call silently iterated nothing.
+//
+// These assert THE SUBJECT the way this file's #7765 tests do: the Set/Map case
+// asserts the visited VALUES (an empty visit list is exactly the bug), and the
+// plain-array case asserts the registry probe counters do not move, so deleting
+// the tag gate fails even though the answer would stay correct.
+// ---------------------------------------------------------------------------
+
+use crate::closure::{js_closure_alloc, ClosureHeader};
+use std::cell::RefCell;
+
+thread_local! {
+    static FOREACH_VISITS: RefCell<Vec<u64>> = const { RefCell::new(Vec::new()) };
+}
+
+extern "C" fn record_first_arg(
+    _closure: *const ClosureHeader,
+    value: f64,
+    _index: f64,
+    _receiver: f64,
+) -> f64 {
+    FOREACH_VISITS.with(|v| v.borrow_mut().push(value.to_bits()));
+    f64::from_bits(crate::value::TAG_UNDEFINED)
+}
+
+fn take_visits() -> Vec<f64> {
+    FOREACH_VISITS.with(|v| {
+        v.borrow_mut()
+            .drain(..)
+            .map(f64::from_bits)
+            .collect::<Vec<_>>()
+    })
+}
+
+fn recording_callback() -> *const ClosureHeader {
+    take_visits();
+    js_closure_alloc(record_first_arg as *const u8, 0)
+}
+
+#[test]
+fn the_fused_array_foreach_still_visits_a_set_receiver() {
+    let (_map, _set) = arm_both_registries();
+    let set = js_set_alloc(4);
+    js_set_add(set, 10.0);
+    js_set_add(set, 20.0);
+    assert_eq!(js_set_size(set), 2);
+
+    // The precondition that made this a regression rather than a latent gap:
+    // the array-only funnel refuses this receiver, so any reroute placed after
+    // it is dead code.
+    assert!(
+        normalize_array_receiver(set as *const ArrayHeader).is_null(),
+        "#8041's array-only funnel must still refuse a Set receiver — if this \
+         starts passing the reroute is no longer the thing under test"
+    );
+
+    let cb = recording_callback();
+    js_array_forEach(set as *const ArrayHeader, cb);
+    assert_eq!(
+        take_visits(),
+        vec![10.0, 20.0],
+        "a Set reaching the fused array forEach must run Set.prototype.forEach; \
+         an EMPTY list is #8117 — the reroute ran after clean_arr_ptr nulled it"
+    );
+}
+
+#[test]
+fn the_fused_array_foreach_still_visits_a_map_receiver() {
+    let (_map, _set) = arm_both_registries();
+    let map = js_map_alloc(4);
+    js_map_set(map, 1.0, 100.0);
+    js_map_set(map, 2.0, 200.0);
+    assert_eq!(js_map_size(map), 2);
+
+    assert!(
+        normalize_array_receiver(map as *const ArrayHeader).is_null(),
+        "#8041's array-only funnel must still refuse a Map receiver"
+    );
+
+    let cb = recording_callback();
+    js_array_forEach(map as *const ArrayHeader, cb);
+    // Map.prototype.forEach passes (value, key, map) — the first argument is
+    // the VALUE.
+    assert_eq!(
+        take_visits(),
+        vec![100.0, 200.0],
+        "a Map reaching the fused array forEach must run Map.prototype.forEach"
+    );
+}
+
+#[test]
+fn a_plain_array_foreach_iterates_without_probing_the_collection_registries() {
+    let (_map, _set) = arm_both_registries();
+    let arr = dense(&[1.0, 2.0, 3.0]);
+    let cb = recording_callback();
+
+    // Prime anything lazily built on first touch, then measure.
+    js_array_forEach(arr, cb);
+    let _ = take_visits();
+
+    let before = probes();
+    js_array_forEach(arr, cb);
+    let after = probes();
+
+    assert_eq!(
+        take_visits(),
+        vec![1.0, 2.0, 3.0],
+        "the control receiver must keep iterating its own elements"
+    );
+    assert_eq!(
+        after, before,
+        "a GC_TYPE_ARRAY receiver must never reach is_registered_map / \
+         is_registered_set — delete the receiver-tag gate in \
+         collection_foreach_reroute and this is what fails"
+    );
+}

crates/perry-runtime/src/array/iter_methods.rs

@@ -103,10 +103,56 @@ impl Drop for DenseThisGuard {
     }
 }
 
+/// #5989/#8117: `.forEach` on a receiver codegen could not prove is a
+/// collection is statically fused to the ARRAY entry point below, so a native
+/// `Set`/`Map` arrives there. Run the collection's own `forEach` and report
+/// `true`; a genuine array-like receiver reports `false` and falls through.
+///
+/// This MUST run before `normalize_array_receiver`. #8041 made `clean_arr_ptr`
+/// — which `normalize_array_receiver` funnels into — reject every *tracked
+/// non-array*, where it previously rejected only `GC_TYPE_OBJECT` /
+/// `GC_TYPE_CLOSURE`. That is correct for the array layout question, but it
+/// nulls a `GC_TYPE_SET` / `GC_TYPE_MAP` receiver, and #5989's reroute sat
+/// AFTER the normalize call, behind `if arr.is_null() { return; }`. The reroute
+/// therefore became unreachable and every fused `set.forEach(cb)` /
+/// `map.forEach(cb)` silently iterated nothing. Same ordering fix #8060 applied
+/// to the indexed read and #8090/#8119 applied to the typed-array question.
+///
+/// Tag-gated exactly as `js_array_get_f64` is (#7765): every registered
+/// `Map`/`Set` IS its `arena_alloc_gc(_, _, GC_TYPE_MAP|GC_TYPE_SET)` header,
+/// so an ordinary array is excluded by one already-warm header byte and never
+/// reaches a registry probe. The registry remains the liveness/layout proof.
+#[inline]
+fn collection_foreach_reroute(arr: *const ArrayHeader, callback: *const ClosureHeader) -> bool {
+    let addr = crate::array::array_receiver_addr(arr as *mut ArrayHeader);
+    let tag = crate::array::array_receiver_gc_tag(addr as *const ArrayHeader).0;
+    if tag != crate::gc::GC_TYPE_SET && tag != crate::gc::GC_TYPE_MAP {
+        return false;
+    }
+    let cb_value = f64::from_bits(crate::value::JSValue::pointer(callback as *const u8).bits());
+    let undef = undefined_value();
+    if tag == crate::gc::GC_TYPE_SET && crate::set::is_registered_set(addr) {
+        crate::set::js_set_foreach(addr as *mut crate::set::SetHeader, cb_value, undef);
+        return true;
+    }
+    if tag == crate::gc::GC_TYPE_MAP && crate::map::is_registered_map(addr) {
+        crate::map::js_map_foreach(addr as *mut crate::map::MapHeader, cb_value, undef);
+        return true;
+    }
+    false
+}
+
 /// forEach - call callback(element, index) for each element
 /// Returns nothing (void)
 #[no_mangle]
 pub extern "C" fn js_array_forEach(arr: *const ArrayHeader, callback: *const ClosureHeader) {
+    // #5989: a native Set/Map reaching this fused array entry point runs its
+    // own `forEach`. Ordered before `normalize_array_receiver` because that
+    // funnel nulls every tracked non-array (#8041) — see
+    // `collection_foreach_reroute`.
+    if collection_foreach_reroute(arr, callback) {
+        return;
+    }
     // #7574: `normalize_array_receiver` materializes an array-like OBJECT
     // receiver — a `class X extends Array` instance among them — into a fresh
     // dense snapshot. The spec passes the RECEIVER as the callback's 3rd
@@ -130,26 +176,6 @@ pub extern "C" fn js_array_forEach(arr: *const ArrayHeader, callback: *const Clo
         );
         return;
     }
-    // #5989: `.forEach` on an unknown-typed receiver is statically fused to
-    // this array entry point, but the receiver may be a native Set/Map —
-    // react-server-dom iterates `request.abortableTasks` (a Set read back off
-    // the request object) exactly this way. Treating a SetHeader as an
-    // ArrayHeader feeds hash-table internals to the callback as elements and
-    // segfaults on the first property read. `forEach` is the ONLY method name
-    // the fused array methods share with Set/Map, so this single reroute —
-    // mirroring the typed-array reroute above — covers the hazard class.
-    {
-        let cb_value = f64::from_bits(crate::value::JSValue::pointer(callback as *const u8).bits());
-        let undef = f64::from_bits(crate::value::TAG_UNDEFINED);
-        if crate::set::is_registered_set(arr as usize) {
-            crate::set::js_set_foreach(arr as *mut crate::set::SetHeader, cb_value, undef);
-            return;
-        }
-        if crate::map::is_registered_map(arr as usize) {
-            crate::map::js_map_foreach(arr as *mut crate::map::MapHeader, cb_value, undef);
-            return;
-        }
-    }
     unsafe {
         let length = (*arr).length;
         let scope = crate::gc::RuntimeHandleScope::new();