Skip to content

fix(foreach): a by-reference loop keeps the receiver it borrows its source from - #1055

Open
Guikingone wants to merge 2 commits into
mainfrom
fix/690-by-ref-foreach-receivers
Open

Guikingone wants to merge 2 commits into
mainfrom
fix/690-by-ref-foreach-receivers

Conversation

@Guikingone

Copy link
Copy Markdown
Collaborator

Fixes #690.

Three receiver kinds lost their writes, and not in the same way:

class C { public array $x = [1, 2]; }

$o = new C(); $n = 'x';
foreach ($o->$n as &$v) { $v *= 2; }        // PHP 2,4  -- elephc: nothing
foreach ($arr[0]->x as &$v) { $v *= 2; }    // PHP 2,4  -- elephc: 1,2
foreach ($o->get()->x as &$v) { $v *= 2; }  // PHP 2,4  -- elephc: nothing

#642 fixed the direct $o->x receiver by reading the property FOR WRITE: split the
container up front, republish it into the slot, hand the loop a BORROWED result so
no second reference exists for IterStart to copy or the exit to over-release.

What the gate was really protecting

That borrow makes the property slot the container's only owner, so the OBJECT has
to outlive the loop. The gate expressed it as a syntactic property -- the receiver
must name stable backing storage, a variable or $this through declared object
slots -- and everything else kept the retaining read.

That is the right REQUIREMENT stated as the wrong RULE. A receiver that names no
storage is a temporary, and the read dropped it the moment it took the borrow: the
loop then wrote into a container whose owner had just been freed, which is why the
runtime-named and call-result forms printed nothing rather than merely losing
writes (their count() returned a heap address). The array-element form kept its
container intact and lost only the writes, because its property read came back
nullable and fell to the retaining path before any of that.

The receiver is held instead

The loop now holds an unstable receiver for as long as it runs and releases it on
the exit block and, through the loop frame, on every break, return and throw
that skips it -- the same treatment the borrowed container already gets. The gate
drops to what it was always for: the receiver must be an object whose slot the
backend can split.

Three consequences, each load-bearing:

  • A RUNTIME-NAMED property takes the path too, once its name has folded to a
    literal. The slot the backend resolves is the one the static spelling reaches,
    so there was never a reason for the two spellings to differ.

  • The read now describes the property's SLOT rather than a read that can answer
    null. property_slot_result_type is property_get_result_type without the
    nullability a receiver that may MISS its container adds.

  • That is sound because a null receiver no longer reads at all. PHP evaluates a
    by-reference source in a WRITE context, where a null receiver is a fatal
    Error, and PropGetForWrite raises exactly that:

    $arr = [new C()];
    foreach ($arr[5]->x as &$v) { }
    // PHP:    Error: Attempt to modify property "x" on null
    // before: three warnings, then carried on
    // after:  Error: Attempt to modify property "x" on null

Verified

All three of the issue's receivers now match host PHP 8.5.10 byte for byte, read
back both through implode() and through a by-value loop -- the first read hid the
element form's failure behind an unrelated implode() defect, the second does not.

Beside them: the writes are visible through the receiver DURING the loop, not only
after it; a by-VALUE loop over the same three receivers still iterates a copy; and
40 iterations of break, return and throw out of such a loop report
leak summary: clean, which is what pins the release on the paths that skip the
exit block.

The seven new tests were confirmed load-bearing by reverting the source changes: six
fail, and the seventh is the by-value guard, which is meant to pass either way.

cargo test --test codegen_tests -- codegen::types codegen::objects codegen::arrays codegen::references codegen::runtime_gc codegen::regressions passes in full (2084),
as do 1915 unit tests and 1542 error tests. The existing #580 and #642 fixtures pass
unchanged, including the ones pinning that the frontend gate and the backend slot
classifier agree.

Still refused, and it is not this

$o->$n whose name has NOT folded to a literal keeps the retaining read: the
backend's slot resolution needs the name. On this repository that shape does not
compile at all today -- unsupported EIR backend feature: dynamic_prop_get runtime miss for result PHP type Array(Mixed) -- so nothing regresses, and reaching it
needs a runtime slot lookup rather than a wider gate.

Docs and example

docs/internals/the-ir.md restates the receiver rule as the lifetime rule it is,
names the three receivers now covered, and records the write-context Error.

docs/php/control-structures.md says a by-reference loop can take any array the
program can name and lists the shapes, including what a null receiver does.

examples/foreach-ref/main.php mutates through an array element, a runtime-named
property and a call result beside the plain local it already had, and shows the
by-value twin leaving the same source alone. Its output is verbatim host PHP
8.5.10.

Fixes #690.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

@github-actions github-actions Bot added area:codegen Touches target-aware assembly or backend lowering. area:eir Touches EIR definitions, lowering, validation, or passes. size:m Medium-sized pull request. type:fix Corrects broken or incompatible behavior. labels Sep 16, 2026
@greptile-apps

greptile-apps Bot commented Sep 16, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

No accepted new issue from changes since the previous review blocks merging.

Summary

This PR fixes by-reference foreach mutation through unstable object-property receivers by retaining the receiver for the loop lifetime and balancing its release across normal and non-local exits.

  • Extends fetch-for-write lowering to array-element, call-result, and folded runtime-named property receivers.
  • Adds write-context handling for null receivers.
  • Refines exception cleanup so caught throws preserve enclosing loop pins and unmatched catches release loops they leave.
  • Adds focused PHP-compatibility and heap-clean regression coverage.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
    S[By-reference foreach property source] --> G{Splittable property slot?}
    G -- No --> R[Ordinary retaining property read]
    G -- Yes --> W[Fetch property for write]
    W --> B[Borrow property container]
    B --> T{Receiver has stable backing storage?}
    T -- Yes --> I[Iterate borrowed container]
    T -- No --> P[Pin temporary receiver for loop lifetime]
    P --> I
    I --> E{Loop exit}
    E -- Normal --> N[Release source and receiver pins at exit block]
    E -- Break / return / escaping throw --> C[Release pins through loop cleanup]
Loading

Reviews (3) · Last reviewed commit: "fix(foreach): a throw releases the loops..."

Comment thread src/ir_lower/stmt/control_exit.rs
…ource from

Three receiver kinds lost their writes, and not in the same way:

    class C { public array $x = [1, 2]; }

    $o = new C(); $n = 'x';
    foreach ($o->$n as &$v) { $v *= 2; }        // PHP 2,4  -- elephc: nothing
    foreach ($arr[0]->x as &$v) { $v *= 2; }    // PHP 2,4  -- elephc: 1,2
    foreach ($o->get()->x as &$v) { $v *= 2; }  // PHP 2,4  -- elephc: nothing

#642 fixed the direct `$o->x` receiver by reading the property FOR WRITE: split the
container up front, republish it into the slot, hand the loop a BORROWED result so
no second reference exists for `IterStart` to copy or the exit to over-release.

## What the gate was really protecting

That borrow makes the property slot the container's only owner, so the OBJECT has
to outlive the loop. The gate expressed it as a syntactic property -- the receiver
must name stable backing storage, a variable or `$this` through declared object
slots -- and everything else kept the retaining read.

That is the right REQUIREMENT stated as the wrong RULE. A receiver that names no
storage is a temporary, and the read dropped it the moment it took the borrow: the
loop then wrote into a container whose owner had just been freed, which is why the
runtime-named and call-result forms printed nothing rather than merely losing
writes (their `count()` returned a heap address). The array-element form kept its
container intact and lost only the writes, because its property read came back
nullable and fell to the retaining path before any of that.

## The receiver is held instead

The loop now holds an unstable receiver for as long as it runs and releases it on
the exit block and, through the loop frame, on every `break`, `return` and `throw`
that skips it -- the same treatment the borrowed container already gets. The gate
drops to what it was always for: the receiver must be an object whose slot the
backend can split.

Three consequences, each load-bearing:

  * A RUNTIME-NAMED property takes the path too, once its name has folded to a
    literal. The slot the backend resolves is the one the static spelling reaches,
    so there was never a reason for the two spellings to differ.
  * The read now describes the property's SLOT rather than a read that can answer
    null. `property_slot_result_type` is `property_get_result_type` without the
    nullability a receiver that may MISS its container adds.
  * That is sound because a null receiver no longer reads at all. PHP evaluates a
    by-reference source in a WRITE context, where a null receiver is a fatal
    `Error`, and `PropGetForWrite` raises exactly that:

        $arr = [new C()];
        foreach ($arr[5]->x as &$v) { }
        PHP:    Error: Attempt to modify property "x" on null
        before: three warnings, then carried on
        after:  Error: Attempt to modify property "x" on null

## Verified

All three of the issue's receivers now match host PHP 8.5.10 byte for byte, read
back both through `implode()` and through a by-value loop -- the first read hid the
element form's failure behind an unrelated `implode()` defect, the second does not.

Beside them: the writes are visible through the receiver DURING the loop, not only
after it; a by-VALUE loop over the same three receivers still iterates a copy; and
40 iterations of `break`, `return` and `throw` out of such a loop report
`leak summary: clean`, which is what pins the release on the paths that skip the
exit block.

The seven new tests were confirmed load-bearing by reverting the source changes: six
fail, and the seventh is the by-value guard, which is meant to pass either way.

`cargo test --test codegen_tests -- codegen::types codegen::objects codegen::arrays
codegen::references codegen::runtime_gc codegen::regressions` passes in full (2084),
as do 1915 unit tests and 1542 error tests. The existing #580 and #642 fixtures pass
unchanged, including the ones pinning that the frontend gate and the backend slot
classifier agree.

## Still refused, and it is not this

`$o->$n` whose name has NOT folded to a literal keeps the retaining read: the
backend's slot resolution needs the name. On this repository that shape does not
compile at all today -- `unsupported EIR backend feature: dynamic_prop_get runtime
miss for result PHP type Array(Mixed)` -- so nothing regresses, and reaching it
needs a runtime slot lookup rather than a wider gate.

## Docs and example

`docs/internals/the-ir.md` restates the receiver rule as the lifetime rule it is,
names the three receivers now covered, and records the write-context `Error`.

`docs/php/control-structures.md` says a by-reference loop can take any array the
program can name and lists the shapes, including what a null receiver does.

`examples/foreach-ref/main.php` mutates through an array element, a runtime-named
property and a call result beside the plain local it already had, and shows the
by-value twin leaving the same source alone. Its output is verbatim host PHP
8.5.10.

Fixes #690

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
… is inside

Review found a double release on my receiver pin, and it is not mine: the same
double release is on `main`, on the two receivers that predate this PR.

    class C { public array $x = [1, 2, 3]; }
    $o = new C();
    foreach ($o->x as &$v) {
        try {
            if ($v === 2) { throw new RuntimeException('mid'); }
        } catch (RuntimeException $e) { echo "caught;"; }
        $v *= 10;
    }
    echo implode(',', $o->x);

    PHP:    caught;10,20,30
    main:   caught;            <- the property's container was freed
    main, with `$a[0]` instead: SIGSEGV

## What the rule was

`terminate_throw` released the cleanups of EVERY active loop before terminating. That
is right for a throw on its way out of the function, and wrong for one a `try` inside
the loop catches: control resumes in the catch, the loop keeps running on storage it
has just released, and normal termination releases it a second time.

It reached every by-reference source, not just the receivers this PR adds: the element
pin from #580 and the property pin from #642 are released by the same line.

## What it is now

A throw releases the loops it actually LEAVES. `LoweringContext` records the
loop-stack depth at each `try` handler push, and `loops_a_throw_would_leave` is the
difference against the innermost active one -- everything opened INSIDE that `try`, and
nothing outside it. With no `try` active the answer is every loop, which is the old
behaviour and the common case.

The `try`'s record is pushed around its BODY only, so the catch dispatch sees the next
handler out. That matters for the other half: a catch that matches nothing rethrows,
and that rethrow does leave the loop. It used to terminate with a bare
`Terminator::Throw` and release nothing at all; it now goes through `terminate_throw`,
where the abandoned `try` is already popped and the count comes out right.

## Verified

Every fixture in this PR still matches host PHP 8.5.10, and two more join them:

  * a throw caught INSIDE the loop, over all four by-reference sources -- the array
    element, the direct property, the plain element, and a call result. Three of those
    are shapes that predate this PR; one of them used to segfault.
  * an unmatched catch that rethrows out of the loop, 30 times, reporting
    `leak summary: clean` -- the opposite case, and the reason the rule cannot simply be
    "a `try` is active".

`cargo test --test codegen_tests -- codegen::types::iterable` passes in full (100),
as do 1915 unit tests and 1542 error tests.

The runtime-named receiver is deliberately absent from the caught-throw fixture: a
by-reference call in it blocks the propagation that folds the property name, and an
unfolded name is the separate gap this PR already documents. It keeps its own fixture.

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
@Guikingone
Guikingone force-pushed the fix/690-by-ref-foreach-receivers branch from fdec74d to 6d1a263 Compare September 18, 2026 13:10
@Guikingone Guikingone self-assigned this Sep 18, 2026
@Guikingone
Guikingone requested a review from nahime0 September 18, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:codegen Touches target-aware assembly or backend lowering. area:eir Touches EIR definitions, lowering, validation, or passes. size:m Medium-sized pull request. type:fix Corrects broken or incompatible behavior.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

By-ref foreach over a property reached through a dynamic name, an array element, or a method loses the writes

1 participant