fix(ir,codegen): fetch by-ref foreach element sources for writing (#580)

[mirchaemanuel]

Fixes #580 for indexed receivers. Based on current main 9f74f5300.

Root cause

Measured rather than guessed. Allocation counts under --heap-debug isolate which step copies:

program	allocs
$a = [[1,2]]; (baseline)	2
$b = $a[0];	2 — no copy
foreach ($a[0] as $v) {} by value	2 — no copy
foreach ($a[0] as &$v) {} by ref, empty body	3 — one copy

Op::ArrayGet does not copy: it returns the parent's own container with a retain. That retain leaves the element at refcount 2 (parent + temporary), so the by-ref iteration copy-on-writes it, mutates the private copy, and releases it at the end of the loop. Every write is lost — no crash, no diagnostic.

The working plain-local form differs by exactly one instruction, and the loop bodies are byte-identical:

# works:   $plain
v4: array<int> own=maybe_owned = load_local slot[0]   # no retain -> refcount 1 -> no COW
# broken:  $a[0]
v7: array<int> own=owned      = array_get v5 v6       # retain   -> refcount 2 -> COW

The report guessed a missing write-back. It is upstream of that: the loop never shares the parent's storage in the first place.

Fix

Op::ArrayGetForWrite performs the copy-on-write split up front and republishes the unique container into the parent's element slot, so the loop iterates storage the parent genuinely shares.

Its effects are deliberately broad — READS_HEAP | WRITES_HEAP | WRITES_LOCAL | ALLOC_HEAP | REFCOUNT_OP | MAY_WARN. Despite the name it is not a pure read: the split rewrites the receiver's element slot and the receiver's own local slot, so it must never be reordered or folded against the plain reads around it. MAY_WARN matters too — a missing index must still emit Undefined array key and yield the null-container sentinel the loop knows how to skip, which is what keeps #556 intact.

By-value foreach keeps the ordinary retaining read: that extra reference is exactly what makes the runtime copy, and copying is precisely its semantics.

PHP's mid-loop visibility is satisfied, which is the discriminator a write-back-only fix would fail:

foreach ($a[0] as &$v) { $v = $v * 2; echo $a[0][0], ';'; }
// php: 2;2;   this branch: 2;2;   (before: 1;1;)

Scope

Indexed receivers, including elements that are themselves hashes ($a[0] holding ['x' => 1]), and nested receivers ($a[0][0]).

A hash receiver is not covered ($h['a']). Reference-binding a hash element is unimplemented in elephc — an explicit $r = &$h['k']; is rejected by the checker with "Reference assignment to an array element requires an indexed array" — so there is no alias to iterate. Those keep their previous behaviour rather than starting to fail differently.

Tests

13 regressions in tests/codegen/types/iterable/foreach.rs, covering the indexed element source, mid-loop parent visibility, a nested element source, the keyed form, a hash-valued element of an indexed receiver, a source shared with another owner, an aliased parent, a missing index (warns and skips), plus guards that by-value must NOT mutate and that the plain-local form still does, and a heap-debug leak guard.

Verification (macOS ARM64, f72d0e3c7)

filter	result
the 13 new tests	13 passed, 0 failed
same, ELEPHC_IR_OPT=off	13 passed, 0 failed
iterable::foreach	41 passed, 0 failed
byref_foreach (the #556 guards)	9 passed, 0 failed
repro under --heap-debug	leak summary: clean
cargo build / git diff --check	clean, zero warnings

Narrow filters only, per the project's test policy; CI owns the full matrix. Linux targets not run locally (no Docker on this machine).

The byref_foreach row is the one worth checking first: an earlier attempt of mine, aliasing the element through a hidden ref-cell, made the indexed cases pass while regressing exactly those three #556 tests — LoadArrayElemRefCell yields a null pointer for a missing index and skips the warning, so foreach ($a[7] as &$v) crashed instead of warning and skipping. That approach was discarded for this one.

Related, not fixed here

Two strictly worse defects found while diagnosing this, both pre-existing and filed separately: #642 (by-ref foreach over an object property corrupts the property — count() returns a heap address) and #643 (by-ref foreach inside a by-ref array parameter fails to compile with EIR PhpTypeMismatch).

[greptile-apps]

Greptile Summary

This PR fixes a silent mutation-loss bug in by-reference foreach over indexed and hash array element sources (issue #580). The root cause was that the plain array_get retain left the element at refcount 2 when the loop started, so IterStart's __rt_array_ensure_unique copy-on-wrote it and every write went into a private copy that was then dropped.

• Introduces Op::ArrayGetForWrite and Op::HashGetForWrite — new opcodes that perform the COW split themselves (receiver first, then element), publishing each unique container back into its parent slot and returning the element borrowed so the loop mutates the parent's own storage.
• Adds a lifetime pin (Acquire with Bool(true) immediate, emitted after IterStart) to keep the borrowed element alive across the loop body even if the body drops the parent variable; the pin is released on every exit path including break N, return, and throw.
• Adds a Bool-immediate exemption to the paired acquire/release peephole pass, preventing it from cancelling a pin whose raised refcount is the whole point of its existence.

Confidence Score: 5/5

• This PR is safe to merge. The fix is narrowly scoped to by-reference foreach over array/hash element sources and does not touch any other language construct.
• The root cause is clearly identified and measured, the fix addresses it at the correct level (the COW split happens before IterStart, not after), all exit paths release the lifetime pin, the peephole exemption is tightly gated on a new immediate marker that no existing instruction uses, and 13 regression tests cover the fixed case, the hash variant, nested access, mid-loop visibility, the by-value guard, the plain-local guard, the missing-index warning, and heap-debug cleanliness.
• No files require special attention.

Important Files Changed

Filename	Overview
src/ir_lower/stmt/typed_foreach.rs	Core fix site: splits lower_foreach_source from the old plain lower_expr call; emits ArrayGetForWrite/HashGetForWrite for by-ref element sources; adds lifetime pin after IterStart and releases it on all exit paths. Logic is correct and well-documented.
src/ir_lower/expr/array_access.rs	Adds lower_by_ref_foreach_element_source and helpers. The element_fetch_for_write_op guard correctly rejects nullable receivers, non-integer keys for indexed arrays, Mixed elements, and non-variable-rooted chains before opting into the fetch-for-write path. Recursive receiver walk handles nested chains correctly.
src/codegen/lower_inst/arrays.rs	Adds lower_array_get_for_write, separate_get_for_write_receiver, array_get_for_write_cow_helper, and target-specific in-bounds emitters. Both AArch64 and x86_64 paths correctly spill the element slot address across the COW helper call, then store the result back unconditionally.
src/codegen/lower_inst/hashes.rs	Adds lower_hash_get_for_write and per-target helpers. Uses __rt_hash_get's new entry-address output (x4/r8) to locate the value slot, then separates it in place. Miss and null-receiver paths correctly reuse the existing sentinel/warning infrastructure.
src/codegen_support/runtime/arrays/hash_get.rs	Adds entry-address output (x4 on AArch64, r8 on x86_64, 0 on miss) to __rt_hash_get. Both targets are handled; the miss path explicitly zeroes the register, and the found path exposes an address the probe already computed at no extra cost.
src/ir_lower/ownership.rs	Adds acquire_lifetime_pin_if_refcounted — same as acquire_if_refcounted but passes Immediate::Bool(true) to mark the pair as exempt from the peephole cancellation pass. Returns source value unchanged for non-refcounted types so callers can detect "nothing to pin" by comparing value IDs.
src/ir_passes/peephole/acquire_release.rs	Correctly skips any Acquire with a non-None immediate, preventing cancellation of lifetime pins. The guard is narrow enough not to affect normal acquires (which always have None immediate).
src/ir/instr.rs	Adds ArrayGetForWrite and HashGetForWrite opcodes with correct broad effect flags (`READS_HEAP
src/ir/validator.rs	ArrayGetForWrite validates both operands (array + I64 key). HashGetForWrite validates operand count and the hash operand only — intentionally consistent with the existing HashGet pattern, since hash keys are polymorphic (string or int) and the hash lookup normalizes them.
src/ir_lower/context.rs	Adds source_pin: Option<LoopCleanup> to LoopFrame. All non-foreach loop constructors correctly initialize it to None.
tests/codegen/types/iterable/foreach.rs	13 new regression tests with good coverage: basic indexed/hash element mutation, mid-loop parent visibility, nested access, keyed form, hash-valued element, shared-owner scenario, missing-index warning+skip, by-value guard, plain-local guard, and heap-debug leak guards.
src/ir_passes/tests/peephole_test.rs	Adds lifetime_pin_acquire_release_pair_is_not_cancelled test that directly constructs a pinned acquire/release pair and asserts the peephole pass leaves it intact.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["lower_foreach_source(array, value_by_ref)"] --> B{value_by_ref AND\narray is ArrayAccess?}
    B -- No --> C["lower_expr(array)\nsource_is_borrowed = false"]
    B -- Yes --> D["lower_by_ref_foreach_element_source\n(receiver, index, expr)"]
    D --> E["lower_by_ref_foreach_source_receiver(receiver)"]
    E --> F{receiver is\nArrayAccess?}
    F -- Yes --> D
    F -- No --> G["lower_expr(receiver)\n(base local)"]
    G --> H["element_fetch_for_write_op(array_value, index)"]
    H --> I{Nullable receiver?\nOr non-container element?\nOr not variable-rooted?}
    I -- Yes --> J["fallback: lower_array_access_from_value\n(retaining read)"]
    I -- No --> K{Indexed array\nor Hash?}
    K -- "Array (int key)" --> L["emit Op::ArrayGetForWrite\nresult = Borrowed"]
    K -- Hash --> M["emit Op::HashGetForWrite\nresult = Borrowed"]
    L --> N["source_is_borrowed = true"]
    M --> N
    N --> O["emit Op::IterStart\n(__rt_array_ensure_unique — no-op: element already unique)"]
    O --> P["pin_by_ref_foreach_element_source\nemit Acquire+Bool(true) pin\nset pin = Owned"]
    P --> Q["Loop body runs"]
    Q --> R{Loop exit path}
    R -- "normal termination\n(exit block)" --> S["release_if_owned(pin)"]
    R -- "break / break N\nreturn / throw" --> T["emit_innermost_loop_cleanups\nrelease_if_owned(source_pin)"]

Loading

_{Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile}

[nahime0]

Requesting changes for one blocking lifetime regression plus two scope/documentation corrections.

The COW split fixes the covered cases, and the focused regression suites are green, but the borrowed element is not kept alive when the loop body replaces its parent. I reproduced this against the exact head: PHP and base 9f74f5300 print 1,2,done, while this PR prints 1,done with IR optimization both on and off. The inline comment includes the reproducer and the required lifetime invariant.

Please also avoid auto-closing #580 while its hash-receiver case remains silently wrong, and align the IR effects table with the implementation.

Operationally, the branch currently conflicts with live main in CHANGELOG.md; after the code changes and base refresh, CI needs to be terminal on the new exact head.

[mirchaemanuel]

Thanks for the review — taking all three points. A short status so this does not sit silent while I work on them.

The branch is rebased onto current main (dd5de0699). The only conflict was CHANGELOG.md; every source file merged clean, and cargo check --all-targets is at 0 errors / 0 warnings on the new base — including against the ReleaseUnlessAliases op that landed meanwhile. I have not pushed it yet: the rebase will land together with the fixes below rather than as a separate force-push.

• Blocking lifetime. Reproducing your case first, then carrying an explicit owner acquired after the COW separation and releasing it on normal and early exits, with the heap-debug regression you asked for. I will check the mirror-image failure too — an owner taken and not released is exactly the symmetric bug.
• Hash receiver. I am going to diagnose and fix that shape rather than file a follow-up, so the closing keyword can stay honest and By-reference foreach writes through an array-element source are silently lost #580 closes on a complete fix. If it turns out to need a mechanism this PR should not grow into, I will say so and switch to the follow-up route instead of quietly narrowing the scope. Either way the test will stop pinning 1,2 as correct.
• IR effects table. Aligning it with Op::default_effects().

I will come back with the reproducer output before and after, and the test numbers.

[mirchaemanuel]

All three points are addressed. The branch is rebased onto current main (e3959fe30) and the head is 39db03fcc.

Blocking lifetime

Your reproducer, measured on the exact head, --ir-opt both ways:

	before	after	php 8.4
--ir-opt=on	1,done	1,2,done	1,2,done
--ir-opt=off	1,done	1,2,done	1,2,done

The loop now takes a lifetime reference of its own on the source and drops it on every way out: its exit block covers normal termination and a plain break, and emit_innermost_loop_cleanups covers break N, return, and throw, where the pre-existing source cleanup is already released. A program exercising all four exits reports allocs=19 frees=19, live_blocks=0, leak summary: clean under --heap-debug.

Two details worth flagging, because both changed the shape of the fix:

The acquire has to come after IterStart, not after the split in general. IterStart is what runs __rt_array_ensure_unique for a by-reference source, so a reference held across it makes that split fire and hands the loop exactly the private copy this path exists to avoid.

The pin does not survive the optimizer unmarked. The paired acquire/release peephole cancels an Acquire whose only use is its Release, on the premise that the raised refcount in between is unobservable — the one premise a pin breaks by construction. That is why --ir-opt=on kept miscompiling after the pin was first added. Acquire now carries a Bool immediate marking it as a lifetime pin, and the peephole skips those pairs; there is a peephole unit test for it.

Hash receiver

Fixed here rather than deferred, so the closing keyword stays honest.

The reason the hash half was excluded does not hold up: the checker's restriction is on $r = &$h['k'], which binds a hash slot to a local through LoadArrayElemRefCell. Fetch-for-write binds nothing — it separates the element and iterates the storage the parent already owns — so that unimplemented feature was never a blocker.

Measured the same way as the indexed case, on $h = ['a' => [1, 2]]: baseline 3 allocations, by-value foreach 3, by-reference foreach with an empty body 4. One copy, no user write involved.

Op::HashGetForWrite is the hash counterpart of ArrayGetForWrite; the two differ only in how the element slot is addressed. __rt_hash_get already builds the matching entry's address while probing and discarded it, so it now returns it (x4 on AArch64, r8 on x86_64, null on a miss). Both registers are caller-saved and were already clobbered by the helper's own probing and key comparisons, and no caller reads them after the call, so no existing contract changes. Everything downstream is shared with the indexed path.

End-to-end, $h = ['a' => [1,2]]; $n = ['k' => ['x'=>1,'y'=>2]]; $i = [7 => [1,2]] with a by-reference loop over each: before 1,2|1,2|1,2, after 2,4|3,6|11,12, which is what php prints. Heap clean.

The test that pinned 1,2 as expected no longer does: it asserts the mutation, and the section comment and docstring that declared the shape out of scope are corrected.

Scope left out: a hash whose value type is Mixed keeps the retaining read, symmetrically with the indexed Mixed element — that slot can hold an invoker ref-cell marker whose read materialises a fresh box rather than the slot's own storage.

IR effects table

ArrayGetForWrite and HashGetForWrite now list exactly what Op::default_effects() assigns them: reads_heap, writes_heap, writes_local, alloc_heap, refcount_op, may_warn. The Acquire row documents the pin immediate, and the peephole section documents why a pin is exempt from cancellation.

Tests

cargo test --test codegen_tests by_ref_foreach → 31 passed, 0 failed on 39db03fcc. That includes the new parent-replacement regressions (plain and nested), one per early exit, the hash regressions, and the pre-existing #556 guards. CI is the terminal check on this exact head.

[nahime0]

Approve