diff --git a/changelog.d/8130-fused-foreach-collection-reroute.md b/changelog.d/8130-fused-foreach-collection-reroute.md new file mode 100644 index 0000000000..e4477784c6 --- /dev/null +++ b/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). diff --git a/crates/perry-runtime/src/array/collection_tag_tests.rs b/crates/perry-runtime/src/array/collection_tag_tests.rs index b14bf68e99..e429f2be20 100644 --- a/crates/perry-runtime/src/array/collection_tag_tests.rs +++ b/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 `.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> = 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 { + FOREACH_VISITS.with(|v| { + v.borrow_mut() + .drain(..) + .map(f64::from_bits) + .collect::>() + }) +} + +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" + ); +} diff --git a/crates/perry-runtime/src/array/iter_methods.rs b/crates/perry-runtime/src/array/iter_methods.rs index 944a897a68..cf4c97ae93 100644 --- a/crates/perry-runtime/src/array/iter_methods.rs +++ b/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();