fix(runtime): a Map/Set subclass in a base-typed binding was read as a raw header (#7570) - #7573
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRuntime Map and Set entry points now resolve subclass receivers to hidden backing collections. Mutations preserve subclass identity. Iterators and collection operations use resolved headers. Regression tests cover annotated bindings, invalid receivers, chaining, iteration, and specialized Map setters. ChangesMap and Set receiver resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AnnotatedBinding
participant RawCollectionEntryPoint
participant redirect_collection_receiver
participant HiddenBackingCollection
AnnotatedBinding->>RawCollectionEntryPoint: invoke Map or Set operation
RawCollectionEntryPoint->>redirect_collection_receiver: resolve receiver
redirect_collection_receiver->>HiddenBackingCollection: select matching hidden backing
HiddenBackingCollection-->>RawCollectionEntryPoint: return result or original receiver
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test-files/test_gap_7570_map_set_declared_base_type.ts (1)
120-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
Set.prototype.entries()coverage for the base-typed subclass binding.Section 6 exercises
values(),keys(), spread,forEach, andfor-ofons6, but notentries().entries()reachesjs_set_entries_iter_obj, which is one of theset_iter_obj_rawcallers changed in this PR. The Map side coversentries()at line 108. Add the Set equivalent so both iterator kinds are exercised through an annotated subclass receiver.💚 Proposed addition
console.log("6 set keys:", [...s6.keys()].join(",")); +console.log("6 set entries:", JSON.stringify([...s6.entries()])); console.log("6 set spread:", [...s6].join(","));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-files/test_gap_7570_map_set_declared_base_type.ts` around lines 120 - 131, Add Set.prototype.entries() coverage to section 6 using the annotated subclass receiver s6, matching the existing Map entries() test and the style of the other s6 iterator checks. Record and log the returned key-value pairs so js_set_entries_iter_obj and the modified set iterator path are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test-files/test_gap_7570_map_set_declared_base_type.ts`:
- Around line 120-131: Add Set.prototype.entries() coverage to section 6 using
the annotated subclass receiver s6, matching the existing Map entries() test and
the style of the other s6 iterator checks. Record and log the returned key-value
pairs so js_set_entries_iter_obj and the modified set iterator path are
exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 47b6507c-47ac-4129-baad-2e59d6eb41b1
📒 Files selected for processing (6)
changelog.d/7573-map-set-declared-base-type-receiver.mdcrates/perry-runtime/src/collection_iter_object.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/object/map_set_subclass.rscrates/perry-runtime/src/set.rstest-files/test_gap_7570_map_set_declared_base_type.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-runtime/src/object/map_set_subclass.rs (1)
159-161: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the raw address at the resolver boundary.
Call
crate::value::addr_class::is_plausible_heap_addr(addr)before boxingaddr. Return0when the address is implausible. Do not rely on ad hoc address-floor checks in downstream receiver classification.Based on learnings: receiver and address classifiers must use
crate::value::addr_class::is_plausible_heap_addrfor the handle-band and heap-floor check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/object/map_set_subclass.rs` around lines 159 - 161, Update redirect_collection_receiver to validate addr with crate::value::addr_class::is_plausible_heap_addr before converting it through JSValue::pointer; return 0 immediately when validation fails. Remove or avoid relying on downstream ad hoc address-floor checks for this resolver boundary.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/object/map_set_subclass.rs`:
- Around line 102-165: Update redirect_collection_receiver and
subclass_backing_of so the boxed receiver is rooted before the backing-key
allocation, then reloaded after js_string_from_bytes completes before deriving
the ObjectHeader pointer used by js_object_get_field_by_name_f64. Ensure the
root remains valid across moving GC and add a regression test that forces
evacuation during this redirect path.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/map_set_subclass.rs`:
- Around line 159-161: Update redirect_collection_receiver to validate addr with
crate::value::addr_class::is_plausible_heap_addr before converting it through
JSValue::pointer; return 0 immediately when validation fails. Remove or avoid
relying on downstream ad hoc address-floor checks for this resolver boundary.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a67a4a8b-adcf-441c-bf6c-164893a6e889
📒 Files selected for processing (6)
changelog.d/7573-map-set-declared-base-type-receiver.mdcrates/perry-runtime/src/collection_iter_object.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/object/map_set_subclass.rscrates/perry-runtime/src/set.rstest-files/test_gap_7570_map_set_declared_base_type.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/perry-runtime/src/collection_iter_object.rs
- changelog.d/7573-map-set-declared-base-type-receiver.md
- test-files/test_gap_7570_map_set_declared_base_type.ts
- crates/perry-runtime/src/map.rs
- crates/perry-runtime/src/set.rs
| /// #7570 — resolve a raw Map/Set RECEIVER address that is NOT a genuine | ||
| /// `MapHeader`/`SetHeader` to the collection the operation must actually run | ||
| /// on. `want` selects which backing kind the caller can use. | ||
| /// | ||
| /// Why this exists: codegen decides "this receiver is a Map" from the | ||
| /// **declared** TypeScript type of the binding (`is_map_expr` / | ||
| /// `Type::Generic { base: "Map" }`), then emits a raw `js_map_*` call whose | ||
| /// first act is to dereference the receiver as a `MapHeader`. A declared type | ||
| /// is a hint, never a layout fact (CLAUDE.md, *Known Limitations*: annotations | ||
| /// are erased, nothing validates them at runtime), so any binding annotated | ||
| /// with the BASE type — `const m: Map<K, V> = new MyMap()`, a parameter, a | ||
| /// class field, a return type, an `as Map<…>` cast — can be holding a | ||
| /// SUBCLASS instance, which perry models as a plain `ObjectHeader`. The two | ||
| /// headers overlay field-for-field, so `entries: *mut f64` reads | ||
| /// `parent_class_id ‖ field_count` — two `u32` class ids glued into a pointer | ||
| /// — and the first `.set()` stores through it (SIGBUS). | ||
| /// | ||
| /// The unannotated path never had this problem because it dispatches through | ||
| /// [`subclass_backing_of`]. This is the same redirect, performed at the raw | ||
| /// runtime entry points so it is **fail-closed**: it covers every binding form | ||
| /// and every future caller, rather than one predicate at a time. | ||
| /// | ||
| /// Returns `0` for an object that is not a Map/Set subclass instance (a plain | ||
| /// object mis-annotated as a native collection), so the caller degrades to its | ||
| /// existing null handling — `undefined` / `0` / `false` — instead of | ||
| /// dereferencing a forged pointer. | ||
| /// | ||
| /// Marked `#[cold]`/`#[inline(never)]`: the genuine-header fast path never | ||
| /// reaches here, and keeping the body out of line preserves the inlined | ||
| /// receiver check at the ~57 `js_map_*` / `js_set_*` entry points. | ||
| /// | ||
| /// # This ALLOCATES, and its callers hold unrooted JSValue args | ||
| /// | ||
| /// [`subclass_backing_of`] builds the hidden field's key with | ||
| /// `js_string_from_bytes`, so reaching this arm is a collection point — and it | ||
| /// runs at the TOP of e.g. `js_map_set`, before that function roots its `key` / | ||
| /// `value` params. The exposure is the #7213 shape, and it is closed by the same | ||
| /// accident described in `string/alloc.rs`: an allocation here reaches the | ||
| /// alloc-point arm of `gc_check_trigger`, which takes | ||
| /// `ManualGcScanGuard::force_full_scan`, and a forced conservative stack scan | ||
| /// makes the copying minor ineligible. So the collection this can cause never | ||
| /// MOVES anything, and the same conservative scan finds the raw args on the | ||
| /// native stack. | ||
| /// | ||
| /// Recorded rather than pre-emptively fixed, for two reasons. The shape is | ||
| /// already load-bearing on hotter paths — `native_call_method`'s | ||
| /// `collection_methods.rs` calls `subclass_backing_of` on every native method | ||
| /// call on an object, and `field_get_set/get_field_by_name.rs` on every `.size` | ||
| /// read — so this adds no NEW class of exposure. And the obvious fix (a | ||
| /// thread-local caching the interned key `StringHeader`) is itself an unrooted | ||
| /// runtime cache of a heap pointer, the invisible-root hazard CLAUDE.md warns | ||
| /// about, which would have to be registered with | ||
| /// `gc_register_mutable_root_scanner` to be sound. If #7213's premise ever | ||
| /// changes — if the alloc-point arm stops forcing a conservative scan — this | ||
| /// call site must be revisited together with the two above. | ||
| #[cold] | ||
| #[inline(never)] | ||
| pub(crate) fn redirect_collection_receiver(addr: usize, want: CollectionKind) -> usize { | ||
| let boxed = f64::from_bits(JSValue::pointer(addr as *const u8).bits()); | ||
| match (subclass_backing_of(boxed), want) { | ||
| (Some(CollectionBacking::Map(m)), CollectionKind::Map) => m as usize, | ||
| (Some(CollectionBacking::Set(s)), CollectionKind::Set) => s as usize, | ||
| _ => 0, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Root the receiver across the backing-key allocation.
redirect_collection_receiver calls subclass_backing_of, which derives obj before js_string_from_bytes can allocate. If that allocation evacuates the object, js_object_get_field_by_name_f64 can dereference a stale ObjectHeader pointer.
Root the boxed receiver inside subclass_backing_of. Reload it after key creation. Derive obj only after the reload. Add a regression test that forces moving evacuation in this path.
Based on learnings: production GC does not conservatively scan Rust stack locals, and NaN-boxed values must be rooted and reloaded across allocating operations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/object/map_set_subclass.rs` around lines 102 - 165,
Update redirect_collection_receiver and subclass_backing_of so the boxed
receiver is rooted before the backing-key allocation, then reloaded after
js_string_from_bytes completes before deriving the ObjectHeader pointer used by
js_object_get_field_by_name_f64. Ensure the root remains valid across moving GC
and add a regression test that forces evacuation during this redirect path.
Source: Learnings
…a raw header (#7570) `const m: Map<string, number> = new MyMap<string, number>(); m.set("a", 1)` took SIGBUS (exit 138) before printing anything. Node prints `size: 1`. Codegen decides "this receiver is a Map" from the DECLARED TypeScript type (`is_map_expr` <= `Type::Generic { base: "Map" }`, plus the matching HIR folds and for-of fast paths), then emits a raw `js_map_*` call that dereferences the receiver as a `MapHeader`. A declared type is a hint, never a layout fact. Perry models a Map/Set subclass instance as a plain `ObjectHeader`, and the two headers overlay field-for-field, so `entries: *mut f64` reads `parent_class_id || field_count` -- two u32 class ids glued into a pointer -- and the first store through it faults. Resolve the receiver at the raw runtime entry points instead of tightening one codegen predicate at a time. `clean_map_ptr` / `clean_set_ptr` -- the funnels 27 and 30 `js_map_*` / `js_set_*` entries already share -- now brand-check what they are handed: a genuine `GC_TYPE_MAP`/`GC_TYPE_SET` header passes through (one `GcHeader.obj_type` load plus a compare), a subclass instance is redirected onto its hidden backing, and a plain object merely annotated `Map<K, V>` resolves to null so each entry degrades through its existing null branch instead of dereferencing a forged pointer. Anything with no readable `GcHeader` is passed through exactly as before. Three sites needed more than the funnel: `js_set_add`/`has`/`delete`/`clear`/ `to_array` never called `clean_set_ptr` at all; the iterator-object constructors STORE the pointer rather than using it immediately; and `Map.prototype.set` / `Set.prototype.add` return their RECEIVER, which for a subclass is the instance and not the backing, so it is rooted across the store and handed back. Adds `test_gap_7570_map_set_declared_base_type.ts` (all five binding forms, the whole iteration surface, receiver identity, indirect subclasses, and non-subclass controls) plus four sabotage-shaped unit tests that assert the misread header byte is still present before asserting the resolved answer. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
…7570) `js_map_foreach` / `js_set_foreach` derive the collection they report as the callback's 3rd argument (and the `self === m` identity) from the map being iterated. After the receiver resolution that is the hidden backing, not the instance, for a base-typed binding holding a `class X extends Map | Set`. Pass the receiver through as the collection override -- the same contract `js_map_foreach_with_collection` already serves for the unannotated path -- and only when the resolution actually moved, so a plain Map keeps `has_override == false` and behaves exactly as before. Gap test now asserts `self === m` inside forEach for the subclass, the Set twin, and the plain-Map control. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
787f45a to
5505db4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Audit before merge — verified, merged as v0.5.1335Sabotage-verified decisively. Rather than reverting the runtime wholesale Verified all five binding forms plus the controls, with a probe I wrote The The PR's own gap test is byte-identical too (41 lines, exit 0). Gates re-run On the full gap sweep — merged without it, deliberatelyThe report offered to run the full 495-test sweep (~6 hours) before merging. I
If a Map/Set regression does surface later, this is the assumption to re-examine The framing worth keepingThis is CLAUDE.md's "TS types don't drive layout" rule violated in its most The follow-ups filed rather than silently absorbed are the other half of the Also: the |
1 participant
Fixes #7570.
The bug
Node prints
size: 1. Reproduced onmainat v0.5.1323, exit 138 with zero output.Annotating a binding with a base type is everyday TypeScript. A parameter
(
function f(m: Map<string, number>)) is the more likely way to hit this in realcode — NestJS's
ModulesContainer extends Mapis exactly this shape. All fivebinding forms are affected:
const, parameter, class field, return type, andas Map<…>cast.Verified root cause
Perry models a Map/Set subclass instance as a plain
ObjectHeadercarrying thereal collection under a hidden field (
object/map_set_subclass.rs). The twoheaders overlay field-for-field:
MapHeaderfieldObjectHeadersize: u32object_type(= 1)capacity: u32class_identries: *mut f64parent_class_id‖field_countentriesis twou32class ids glued into a pointer, and the first storethrough it faults (
map_set_string_key_value + 708,str x21, [x20], #0x8).The instance reaches those raw entry points because "is a Map" was decided
from the declared TypeScript type — at three layers, all keyed on the same
fact:
crates/perry-codegen/src/type_analysis/strings.rs:135(is_map_expr) and:13(is_set_expr) — satisfied byType::Generic { base: "Map" }, nosubclass or brand check. Feed
lower_call/property_get/map_set.rs:22,131,289,317and
expr/property_get.rs:486,495(.size).crates/perry-hir/src/lower/expr_call/local_array_methods.rs:915-1032— theHIR fold to
Expr::MapSet/MapGet/MapHas/SetAdd/ … onctx.lookup_local_type(...) == Generic { base: "Map" | "Set" }.crates/perry-hir/src/lower/stmt_loops.rs:1306,1326andlower/for_head.rs:303— thefor…ofindex fast paths(
js_map_size+js_map_entry_value_at,js_set_value_at).A declared type is a hint, never a layout fact — CLAUDE.md's Known
Limitations says annotations are erased and nothing validates them at runtime.
The unannotated form (
const m = new MyMap(), covered bytest_gap_6325_map_set_subclass.ts) always worked precisely because it types asthe subclass and dispatches through
subclass_backing_of. The annotation iswhat routes the value onto the raw lowering.
Confirmed by reading the emitted IR for a probe covering every binding form:
raw
js_map_set/js_map_get_string_key/js_map_size/js_map_values_iter_obj/js_set_add/js_set_value_atcalls on all of them.Fix, and why this shape
Resolve the receiver at the raw runtime entry points, not by tightening one
codegen predicate at a time.
map::clean_map_ptrandset::clean_set_ptr— thefunnels that 27 and 30
js_map_*/js_set_*entries already share — nowbrand-check what they are handed:
GC_TYPE_MAP/GC_TYPE_SET) passes straight through. Thisis the only case that costs anything: one
GcHeader.obj_typeload — the 8bytes immediately preceding the header, so normally the same cache line — plus
a compare;
class X extends Map | Setinstance is redirected onto its hidden backing(
redirect_collection_receiver,#[cold]/#[inline(never)]);Map<K, V>resolves to null, so every entrydegrades through its existing null branch (
undefined/0/false)instead of dereferencing a forged pointer;
GcHeader(handle-band ids, tag remnants,non-pointer garbage) is passed through unchanged — exactly the pre-fix
behaviour. Narrowing that is a separate, riskier change.
Why not "refuse the raw lowering when the static type is a subclassable native
base": that costs the fast path for every
Map<K, V>-annotated binding inevery program, including the overwhelming majority that never declare a
subclass, and "does this program declare a Map subclass?" is a whole-program
question that per-module parallel codegen cannot answer soundly.
Why not a codegen-emitted runtime guard: it would have to be repeated at
~20 lowering sites across two crates, and each new site is a new way to forget.
The runtime funnel is fail-closed — it covers every binding form and every
future caller. Same precedent as
promise/checked_dispatch.rs, which alreadydoes exactly this for
class X extends Promise.Four sites needed more than the funnel:
js_set_add/js_set_has/js_set_delete/js_set_clear/js_set_to_arraynever calledclean_set_ptrat all — they went straight tofind_value_indexon the raw pointer.collection_iter_object::{map,set}_iter_obj_rawstore the pointer into theiterator object rather than using it immediately, so the redirect has to happen
before capture.
Map.prototype.setandSet.prototype.addreturn their receiver. For asubclass the receiver and the collection differ, so the write goes to the
backing while the instance comes back — otherwise
m.set(k, v) === mwasfalse and chaining handed out the backing. The receiver is rooted across the
store (
RuntimeHandle::across_mut): it is a movableObjectHeaderand thestore allocates.
js_map_foreach/js_set_foreachderive the collection they report as thecallback's 3rd argument (and the
self === midentity) from the map beingiterated, which after resolution is the backing. They now pass the receiver
through as the collection override — the same contract
js_map_foreach_with_collectionalready serves for the unannotated path — andonly when the resolution actually moved, so a plain Map keeps
has_override == falseand behaves exactly as before.Validation (local; CI has a deep backlog and may not report)
test-files/test_gap_7570_map_set_declared_base_type.ts— byte-identical tonode --experimental-strip-types(v26.5.1, the.node-versionpin), exit 0.Covers all five binding forms, the whole iteration surface (
for-of, spread,Array.from,forEach,.entries()/.keys()/.values()),size/get/has/delete/clear, receiver identity and chaining, indirectsubclasses, a subclass with its own constructor and fields, and non-subclass
controls including the specialized numeric- and string-keyed entry points.
crates/perry-runtime/reverted in full to the parent commit and the testfile untouched, the same file exits 138 with zero output; with the fix
restored it exits 0, byte-identical.
LLVM IR for a probe exercising every affected form is byte-identical before
and after (
diffover--trace llvm, 8,910 lines, zero differences). Plainnew Map()still lowers tojs_map_set_string_number/js_map_get_string_key/js_map_sizeexactly as before. Additionally, theunit test
a_genuine_map_takes_the_fast_path_and_is_never_redirectedassertsredirect_collection_receiverreturns 0 for a realMapHeader, so thefast-path identity cannot have come from a redirect that happened to agree.
object/map_set_subclass.rs: each firstasserts the header byte the pre-fix code misread is still sitting there
(
object_type == 1atMapHeader.size's offset), and only then that the entrypoint returns the resolved answer — so a green run proves the redirect fired,
not merely that nothing threw.
mainat v0.5.1334): themap/set/iter/weak/collection/foreach/spreadsweeps(~110 tests) produce the identical failure set with the fix and with
crates/perry-runtime/reverted in full —test_effect_pipe_map,test_gap_2514_settracesigint,test_phase2v3_3_show_toast_set_text,test_gap_ratelimiter_memory,test_issue_4034_object_literal_semantics,test_issue_2656_weakref_finalization_gc,test_issue_610_foreach. Every oneis pre-existing and none references
Map/Set. Zero regressions.cargo test -p perry-runtime: 1842 passed, 0 failed.cargo test -p perry-codegen --lib: 672 passed, 0 failed.python3 scripts/raw_handle_debt.py: 998 (baseline 998), per-moduleceilings held.
python3 scripts/addr_class_inventory.py,python3 scripts/class_id_collisions.py,./scripts/check_file_size.sh,cargo fmt --all -- --check: all clean.Scope — declared-type-keyed lowerings NOT fixed here
I swept every
is_<native>_exprpredicate and every declared-type HIR fold.Two families remain, both filed separately rather than folded into this PR:
class X extends Array(class X extends Array in anT[]-annotated binding takes the raw ArrayHeader fast paths (sibling of #7570) #7574) is the same hazard on a different family. An Arraysubclass instance is also a plain
ObjectHeader(array/subclass.rs;js_array_subclass_initinstalls noArrayHeaderbacking), andis_array_exprgates three tiers that do not brand-check: the bounded-index element get/set
(
expr/index_get.rs:531), the inlinearr.lengthload(
expr/property_get.rs:236), andlower_array_method(
lower_call/property_get.rs:215). The general index-get tier already guards(
expr/index_get/guarded_array.rstestsGC_TYPE_ARRAYbefore the slot load).m instanceof MyMap(instanceofa Map/Set SUBCLASS is false (m instanceof MyMap); only the native base edge survives #7575) is false for a Map/Set subclass instance with orwithout the annotation — a pre-existing gap in the class-registry parent
edge, unrelated to this change.
Everything else in the sweep already re-validates at the runtime boundary:
Promise (
subclass_backing_promise), RegExp (is_valid_regex_ptr), Error(
object_type == OBJECT_TYPE_ERROR), Date, DataView / typed arrays / Buffer(
lookup_typed_array_kind,is_registered_buffer), URLSearchParams(
resolve_search_params_receiver), WeakMap/WeakSet (object-backed, no raw fold).No version bump (maintainer bumps at merge).
https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
Summary by CodeRabbit
Bug Fixes
forEach.Tests