fix(ir,codegen): fetch by-ref foreach property sources for writing (#642) — stacked on #648

[mirchaemanuel]

Fixes #642.

Stacked on #648. This branch is based on fix/580-byref-foreach-borrowed-source (f72d0e3c7), not on main, and adds one branch to the lower_foreach_source dispatcher that PR introduces. Please merge #648 first. Building this on main instead would have created a second, competing dispatcher on the same line — a semantic merge rather than a textual one — so I deliberately stacked it.

Root cause: one reference consumed twice

Reading $o->x lowers to prop_get plus acquire, leaving the container at refcount 2. A plain local is different: load_local is borrowed, refcount 1. That asymmetry is the whole bug.

At refcount 2, __rt_array_to_mixed's leading ensure_unique clones the container and drops the caller's reference. The clone reaches only the iterator's private slot, because the existing write-back (store_iter_source_to_origin_if_local → source_load_local_slot) returns None for any producer that is not Op::LoadLocal — and here it is Op::Acquire. So:

• the loop mutates the clone → writes lost
• the exit path's release still targets the original → refcount 0 → the property's container is freed while the property still points at it

count($o->x) returning a heap address is the direct consequence: it reads the first word of the freed block's header, where the allocator has written its freelist pointer.

The measurements match the mechanism exactly:

fixture	result	GC allocs/frees
by-ref foreach on property, empty body	corrupted	5/4
by-ref foreach on a local	correct	3/3 (no copy at all)
by-value foreach on property	correct	5/5
with an extra live reference ($keep = $o->x;)	corruption disappears, writes still lost	—

The empty body corrupting already proves the damage is in setup and exit, not in the writes. The extra live reference masking it is the refcount arithmetic: 3 → ensure_unique drops to 2 → the final release leaves 1, so it survives.

Fix

Op::PropGetForWrite — the property-side twin of #648's ArrayGetForWrite. It splits the container up front, republishes the unique container into the property's slot, and returns it Borrowed.

The two-part fix I initially expected collapsed into one, and the reason is worth recording: with refcount already 1, ensure_unique does not clone — it converts in place and returns the same pointer. So the local-only write-back becomes irrelevant rather than insufficient. And because the result is Borrowed, the exit-path release disappears too — that was the second consumption. One change closes both defects.

Emitted AArch64:

; op=prop_get_for_write
ldur x9, [x29, #-168]        ; receiver
ldr  x0, [x9, #8]            ; container from the property slot
bl   __rt_array_ensure_unique
ldur x9, [x29, #-168]        ; reload: the helper clobbers scratch on both targets
str  x0, [x9, #8]            ; republish the split container INTO the slot

No acquire, no release.

Tests

12 regressions. Under TDD, 9 failed before the change and 2 passed (the static and by-value guards, which already worked).

Shapes covered, all matching PHP: the report's repro, an empty body, a hash-valued property, an untyped property, a static property, $this->x inside a method, a property of strings, by-value on the same property (must NOT mutate), mid-loop visibility, and the extra-live-reference case.

That last one is the one that proves both defects are closed rather than just the refcount:

$keep = $o->x;
foreach ($o->x as &$v) { $v = $v * 2; }
echo implode(',', $o->x), '|', implode(',', $keep);   // 2,4|1,2 — heap clean

The property is split into its own unique container and receives the writes; $keep keeps PHP's by-value copy semantics.

The assembly test asserts the structural invariant — load the slot → split → republish into the same slot — with per-target mnemonics, and was validated with a negative control: reverting the six source files makes it fail on the host and under ELEPHC_TEST_TARGET=linux-x86_64 and linux-aarch64 with missing prop_get_for_write lowering.

Verification (macOS ARM64, 7595113fa)

filter	result
the 12 new tests	12 passed, 0 failed
regression_580 (#648 intact)	13 passed, 0 failed
foreach::	53 passed, 0 failed
iterable	79 passed, 0 failed
typed_property	21 passed, 0 failed
property_array	24 passed, 0 failed
byref	13 passed, 0 failed
asm test, linux-x86_64 / linux-aarch64	1 passed each
repro under --heap-debug	leak summary: clean
cargo build / git diff --check	clean, zero warnings

Narrow filters only, per the project's test policy. Linux not executed locally (no Docker), but both lowerings are covered by the assembly test.

An independent confirmation of the diagnosis

An untyped property (public $x) lost its writes pre-fix but did not corrupt. Reason, measured: array<int> takes the static Indexed path, where ensure_unique_static_iter_source always calls store_result_value, while the dynamic path returns before the rebind. That asymmetry is precisely what made array<mixed> lethal and array<int> merely lossy.

Not fixed here

Two pre-existing leaks surfaced during the measurements and are filed separately — neither is on this fix's path, and both were measured identically before and after it. Also: subscript_chain_is_variable_rooted in #648 accepts ExprKind::Variable(_) but not ExprKind::This; harmless today, but worth a look if $this->x[0] ever routes through it.

[mirchaemanuel]

The two pre-existing leaks mentioned above are now filed, both re-measured by me on unpatched main 2829ce0ab before opening:

• By-ref foreach writing strings into a boxed-mixed array leaks one block per element #652 — a by-ref foreach writing strings into a boxed-mixed array leaks one block per written element. Reproduces on a plain local with no property at all, so it is off this PR's path entirely. The isolation is what makes it actionable: empty body is clean, integer writes are clean, and a homogeneous array<string> is clean — it needs both the mixed array and the string writes, and it scales per element (2 blocks for two elements, 3 for three).
• By-ref foreach over a static array property leaks three blocks #653 — a by-ref foreach over a static array property leaks three blocks, with identical numbers before and after this change. The static path is untouched here.

Neither is masked by an assertion in this PR: the string-property test added here asserts behaviour only, and does not claim a clean heap where there is not one.

[greptile-apps]

Greptile Summary

This PR fixes a memory-safety bug (#642) where by-reference foreach over an object property caused a use-after-free: PropGet acquired the container (refcount 2), IterStart's COW split consumed one reference, and the loop-exit release consumed a second — taking the property's own container to refcount 0 while the property slot still pointed at it.

The fix introduces PropGetForWrite, the property-side counterpart of ArrayGetForWrite from #648. It splits the container up front, publishes the unique copy back into the property slot, and returns the result borrowed — eliminating both the extra acquire and the extra release in one op.

• New IR op PropGetForWrite with write-side effects, hard-error backend (no silent plain-read fallback), and a stable-receiver gate that validates every chain step before emitting the op.
• Reference-cell support: when the property slot holds a ref cell (via $r = &$o->x), the split goes through the cell and publishes back at cell[0], so all aliases observe the update.
• 12 regression tests covering typed/untyped array and hash properties, $this, nested plain chains, reference slots, shared-owner proofs, heap-debug leak checks, mid-loop visibility, receiver replacement, and structural assembly assertions on both AArch64 and x86_64.

Confidence Score: 5/5

• Safe to merge. The fix closes a real use-after-free in generated code, is covered by heap-debug-instrumented regression tests on both architectures, and leaves the fast path (by-value foreach, non-applicable receivers) entirely untouched via the conservative gating logic.
• The refcount arithmetic in PropGetForWrite is exact: the COW helper consumes the property's existing reference and the borrowed result carries no new one, so the slot balance is always zero-delta. The frontend gate independently mirrors the backend slot classifier and falls back to an ordinary read on any shape it cannot prove stable, while the backend hard-errors (rather than silently falling back) if the two sides ever disagree. Assembly tests pin the load → split → republish instruction sequence on both supported architectures, and heap-debug tests with a second live owner confirm no over-release or leak across all covered shapes.
• No files require special attention. The two new implementation files are well-bounded, and the coherence gate between property_is_splittable_container_slot (frontend) and property_container_split (backend) is validated by the comprehensive shape-coverage test.

Important Files Changed

Filename	Overview
src/ir/instr.rs	Adds PropGetForWrite Op variant with effects `READS_HEAP
src/ir/validator.rs	Single-line addition registering PropGetForWrite alongside PropGet in the opcode-rules validator. Straightforward and correct.
src/codegen/lower_inst/objects/property_fetch_for_write.rs	New codegen backend for PropGetForWrite. Loads the container pointer from the property slot (indirecting through the ref cell when present), calls the correct COW helper (__rt_array_ensure_unique or __rt_hash_ensure_unique), reloads the receiver (scratch registers are clobbered by the helper), and publishes the unique container back to the same slot. Returns no reference (result is borrowed). Hard-errors on unsupported slot shapes to enforce frontend/backend coherence.
src/ir_lower/expr/property_fetch_for_write.rs	New IR-lowering frontend for the PropGetForWrite path. property_fetch_for_write_applies guards emission with three independent checks: the receiver is a concrete non-nullable object class, the property slot is an array/hash container the backend can split (mirroring the backend classifier), and every step in the receiver chain has stable backing storage. Falls back to ordinary PropGet when any check fails. Correctly handles $this as a stable root and recursively validates nested property chains.
src/ir_lower/stmt/typed_foreach.rs	Extends lower_foreach_source with a PropertyAccess branch for by-ref loops, setting source_is_borrowed_fetch from the returned ownership annotation. Renames the element-source pin function to pin_by_ref_foreach_borrowed_source so it covers both element and property sources. Logic and ordering constraints (pin after IterStart) are unchanged and correctly documented.
tests/codegen/types/iterable/foreach.rs	Adds 12 regression and guard tests covering all documented shapes: typed/untyped array/hash properties, $this, nested plain chains, static (guard), by-value (guard), shared-owner proofs, heap-debug leak checks, mid-loop visibility, receiver replacement, reference-cell slot, hooked/magic-get chains (must decline), and two structural assembly tests validating the exact load → split → republish sequence on both architectures.
docs/internals/the-ir.md	Documents PropGetForWrite in the IR reference table and adds a detailed prose explanation of why both refcount defects are closed by a single op. Accurately describes the stable-receiver constraint and the fail-loud backend policy.

Sequence Diagram

sequenceDiagram
    participant FE as IR Lowering (frontend)
    participant IR as EIR (PropGetForWrite)
    participant CG as Codegen (backend)
    participant RT as Runtime

    FE->>FE: lower_foreach_source detects PropertyAccess + by-ref
    FE->>FE: "property_fetch_for_write_applies:<br/>non-null object? array/hash slot? stable chain?"
    alt applies
        FE->>IR: "emit PropGetForWrite(object, property)<br/>ownership = Borrowed"
        FE->>FE: release owning temporary receiver (if any)
        IR->>CG: lower_prop_get_for_write
        CG->>CG: resolve_property_slot → offset, is_reference, is_packed
        CG->>CG: property_container_split → helper + through_reference_cell
        CG->>RT: load container from slot (via ref cell if needed)
        CG->>RT: __rt_array_ensure_unique / __rt_hash_ensure_unique
        RT-->>CG: unique container (refcount 1, old shared ref consumed)
        CG->>CG: reload receiver (scratch clobbered by helper)
        CG->>RT: store unique container back to property slot
        CG-->>FE: result register (borrowed, no acquire)
    else does not apply
        FE->>IR: "emit PropGet(object, property)<br/>ordinary retaining read"
    end
    FE->>IR: "emit IterStart (splits borrowed source in place — no-op at rc=1)"
    FE->>IR: "pin_by_ref_foreach_borrowed_source (after IterStart)<br/>keeps container alive if body drops receiver"

Loading

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

[nahime0]

Blocking on two ownership/lifetime correctness gaps that are not covered by the current regression suite. Both reproduce on this exact head (7595113f) and diverge from PHP; one also triggers heap-debug refcount corruption and the other leaks. I left minimal repros and corrective directions inline. The changelog claim should be revised after those cases are fixed.

[nahime0]

P1 — the receiver lifetime is not actually guaranteed here. property_chain_is_variable_rooted() only proves the syntactic root is a variable; an intermediate property can still be a get hook (or __get) that returns an owning temporary. For example:

class Inner { public array $x = [1, 2]; }
class Outer { public Inner $inner { get { return new Inner(); } } }
$o = new Outer();
foreach ($o->inner->x as &$v) { $v *= 2; echo $v; }

On this head, releasing object_value here frees the only owner of x; the loop warns that its source is null and does not run, whereas PHP outputs 24.

Please either reject fetch-for-write unless every intermediate access is a stable backing slot (no property hook, magic getter, dynamic step, or temporary), or retain the receiver through loop teardown without adding a reference to the source container. Add a heap-debug regression for this hooked-intermediate chain.

[nahime0]

P1 — falling back for untyped/reference slots is unsafe because the frontend still emits PropGetForWrite for them when the inferred value is Array/AssocArray. Returning None skips the split and write-back, but the EIR result is still marked Borrowed; with a second owner, IterStart consumes the property's own reference and clones without republishing it.

Repros on this exact head:

• untyped property plus $keep = $o->x: output is 1,2|1,2, followed by heap-debug bad refcount; PHP gives 2,4|1,2;
• property promoted to a reference cell plus a second owner: writes are lost and heap-debug reports 4 live blocks / 184 bytes; PHP gives 2,4|2,4|1,2.

Please split/write back fixed untyped slots using the actual container layout and handle reference slots through the ref-cell, or make the frontend selection truly exclude these shapes. Add shared-owner heap-debug regressions for both.

[nahime0]

This entry is not accurate yet: it claims untyped properties are covered and hooked properties are unchanged, but the two blocking repros above contradict those statements. It is also far longer and more implementation-focused than the repository's terse, user-facing changelog entries. Please rewrite it after the supported scope is fixed and verified, and avoid claiming every-target coverage until compiler CI has completed on the exact head.

[mirchaemanuel]

Thanks — both P1s taken, and the changelog point with them.

Status: the branch is rebased onto current main (dd5de0699), still stacked on the updated #648. Only CHANGELOG.md conflicted and cargo check --all-targets is clean on the new base. Not pushed yet — it will land together with the fixes.

• Hooked/magic intermediate steps. Reproducing your Outer::$inner case first, then closing the gap: proving the syntactic root is a variable is indeed not enough to prove the receiver outlives the loop. I will report which of the two directions I take and why.
• Untyped/reference slots. Agreed on the diagnosis, and I think the sharpest way to put it is that the defect is the disagreement itself: the frontend emits the op, the backend silently declines it, and the result stays marked Borrowed. Whichever way it resolves, frontend selection and backend handling will agree, with no shape left where the op is emitted and then skipped. I will state explicitly how I verified that property rather than just asserting it.
• Regressions. Heap-debug shared-owner cases for both untyped and reference-cell slots, plus the hooked-intermediate chain.
• Changelog. Will be rewritten short and user-facing once the supported scope is settled, describing only what is actually covered, and without claiming every-target coverage before CI has shown it.

[mirchaemanuel]

Both P1s and the changelog point are addressed. Rebased onto current main (e3959fe30), still stacked on #648; head is df32c1e3d.

P1 1 — hooked and magic intermediate steps

Confirmed on the previous head: your Outer::$inner case printed nothing while php prints 24.

I took the restrict-the-gate direction rather than holding the receiver across the loop. Chain stability is now checked per step: a variable or $this, optionally followed by declared, non-hooked, object-typed property slots. An undeclared name fails the same check, which is what rejects __get-routed and dynamic steps — both call code that builds a fresh object instead of handing back stored storage.

The reasoning for preferring it: PHP itself mutates a temporary in that shape, so nothing observable is lost by falling back, and the fallback path already gets the receiver's lifetime right. Retaining across every loop exit would be a new lifetime mechanism for no user-visible gain. Regressions cover both the get-hook and the __get chain, with heap-debug.

P1 2 — untyped and reference slots

Both of your repros confirmed, including the bad refcount and the 4 live blocks / 184 bytes.

The defect is the disagreement itself: the frontend emitted the op, the backend silently declined it, and the result stayed marked Borrowed. Untyped slots hold the same plain container pointer as typed ones and are now split identically; a reference slot is split one indirection deeper, reading and republishing through the cell so the alias observes the mutation.

Frontend and backend now classify slots from the same ClassInfo metadata, and the backend has no fallback left: a slot it cannot split is a hard unsupported error. A plain read there would be unsound, so failing loudly is what keeps the two sides from drifting apart silently. There is a test that walks the shapes and asserts the two sides agree, plus shared-owner heap-debug regressions for untyped and reference-cell slots — the reference case compares live bytes against a baseline that only takes the binding, since the binding itself legitimately keeps memory alive.

Output now matches php on all three: 24 for the hooked chain, 2,4|1,2 for untyped plus a second owner, 2,4|2,4|1,2 for the reference cell.

I checked separately that turning the fallback into a hard error does not reject programs that used to compile. Seven shapes outside the new tests: f()->x, a constructor-promoted property and a private property through $this all behave as php does. Four diverge — a dynamic name $o->$n, $arr[0]->x, ?array with a default, and $o->get()->x — but each behaves identically on a binary built without this PR, so they are pre-existing and not regressions of this change. I can file them separately with the minimal reproducers if useful.

One addition beyond the review

Integrating this branch on top of the updated #648 surfaced a gap neither side showed alone. PropGetForWrite hands the loop the property's own container borrowed, so the property slot is its only owner — the same situation ArrayGetForWrite creates for an element, and the same one your blocking comment on #648 was about. The property path was returning its source without requesting the loop's lifetime pin, because it was written against the signature lower_foreach_source had before that pin existed.

Measured rather than assumed: with the flag forced to false, a loop whose body replaces the receiver prints 1, instead of 1,2,done. Fixed in df32c1e3d with a heap-debug regression.

Changelog

Rewritten short and user-facing. It now describes only what is actually covered — typed and untyped array and hash properties, $this->x, chains of plain declared properties, and reference-bound properties — and states plainly that a source reached through a hook, __get, a dynamic name, or a temporary receiver keeps the previous read. No every-target claim before CI has shown it.

Tests

cargo test --test codegen_tests by_ref_foreach → 49 passed, 0 failed on df32c1e3d. CI is the terminal check on this exact head.

[mirchaemanuel]

Thanks for taking the rebase onto main — I had prepared one locally after the oversized-file refactor and, for what it's worth, landed on the same shape you did: a dedicated module next to the array-side gate rather than threading the code back into the split expr/mod.rs. Yours is on the branch, so I left mine alone.

While re-checking this PR's scope against current main (a7a0e090c) I confirmed the gate does the right thing on every shape I could think of, and that the three receiver kinds it deliberately excludes still lose their writes:

• a dynamic property name, foreach ($o->$n as &$v) — no output
• an object stored in an array, foreach ($arr[0]->x as &$v) — writes lost, count() stays 2
• a property reached through a method, foreach ($o->get()->x as &$v) — no output

Those are outside this PR by design, and they are not in #642's reproducer either, so Fixes #642 stays honest. I filed them as #690 so the gap is tracked rather than implicit — same reasoning that split #642 from #580 in the first place: a different receiver kind rather than a variant of the same reproducer.

Verified alongside it that #580's element shape is fixed on current main, and that every non-by-ref operation through those same accessors (read, write, indexed write, by-value foreach) is correct — so the defect is the by-reference loop, not property resolution.