Skip to content

fix(codegen): rebuild closure materializations when regenerating a closure - #152

Merged
matyhtf merged 1 commit into
swoole:masterfrom
yuan-dian:fix/coalesce-repeated-closure-codegen
Oct 8, 2026
Merged

matyhtf merged 1 commit into
swoole:masterfrom
yuan-dian:fix/coalesce-repeated-closure-codegen

Conversation

@yuan-dian

Copy link
Copy Markdown
Contributor

What

Fixes Bug A: ?? inside a closure was compiled to a constant guard — the generated C++ never read the captured value, so branches silently inverted at runtime.

When an outer expression (e.g. preg_replace_callback(...) ?? $text) re-parses an operand containing a closure, the closure body is lowered multiple times. Each lowering marks inner ?? nodes with a replace attribute so repeated parses in the same emission context reuse the same temp instead of re-emitting the materialization. But generations 2+ of a closure are fresh C++ lambda contexts: they reused generation 1's temp name without ever emitting its assignment. The live copy (wired at the call site) then compared an uninitialized temp (null) against '', the guard inverted, and $m[1] was never read.

Fix: clear replace attributes from the closure subtree at doGenClosure entry, so every generation re-lowers its own materializations. Same-context dedup outside closures is preserved (blindly clearing everywhere would re-execute side-effecting left operands like f() ?? x).

Repro

$a = preg_replace_callback('/(X)|(q)(.+?)\2/', function ($m) {
    if (($m[1] ?? '') !== '') {          // compiled to a constant guard
        return '[ONE#' . $m[1] . ']';
    }
    return '<THREE#' . $m[3] . '>';
}, $text) ?? $text;                      // outer ?? re-parses the closure 3x
$a (??) $b (isset() control)
PHP semantics string(23) "[ONE#X] <THREE#-mid-> Z" same
AOT before fix string(16) "[ONE#X] [ONE#] Z" ❌ correct
AOT after fix string(23) "[ONE#X] <THREE#-mid-> Z" ✅ correct

Pre-fix codegen: closure emitted 3×; only generation 1 contained
tmp = php::exists(m, {1}, ...) ? ... : '', while the live generation 3
contained tmp_var_0 = tmp_var_1; with tmp_var_1 never assigned.
isset() is immune because it inlines php::exists(...) in every generation.

Trigger conditions (narrowed by bisection)

  1. Required: a materializing expression inside the closure that sets replace (??; isset() sets nothing)
  2. Required: an outer expression that re-parses an operand containing the closure (outer ?? $text)

Not required: backreference \2 holes (PCRE pads unparticipated groups with ''), $this/self:: inside the closure, multiple returns.

Changes

  • src/Generator/ClosureGenerator.php — doGenClosure now calls a new
    clearReplaceAttributes() helper (NodeFinder walk over the closure subtree)
    before generating the body; docblock documents the cross-generation
    invalidation mechanism and why same-context dedup must be preserved.
  • tests/compiler/coalesce/closure-repeated-parse.phpt — runtime regression
    (outer ?? + closure + inner ?? guard; expects [X] <q-mid-> Z).
  • phpunit/src/ClosureRepeatedCoalesceCodegenTest.php +
    phpunit/code/closure-repeated-coalesce-codegen.php — codegen regression:
    every emitted closure generation must carry its own php::exists materialization.

Verification

  • Red/green: reverting ClosureGenerator.php fails both new tests
    (Failed asserting that 1 is identical to 3; PHPT output [X] [] Z),
    with the fix both pass.
  • Repro binary output byte-identical to PHP interpreter output (above table).
  • PHPT full suite (tests/compiler, 1275 tests): 15 failed vs baseline 16 —
    failure-set diff is exactly the new test (red → green); zero regressions.
  • PHPUnit full suite (2401 tests): 6 errors + 8 failures, all pre-existing
    (SapiApplicationLinkerTest, CompilationStatisticsTest, BashCompletionTest,
    CompilerBaseApiTest) — identical to baseline.
  • Targeted: --filter 'Coalesce|Closure' 52/52; PHPT coalesce+closure+
    native-class+object_property 44/44.
  • PHPStan level 5 clean on the changed file.

Notes

  • The redundant triple generation itself is left as-is (pre-existing); two
    attempted optimizations that skipped the warm-up parses were reverted —
    those parses are load-bearing for nativePropertyAccess annotation
    (tests/compiler/native-class/array-access.phpt regresses). Deduping
    generations is a possible follow-up, tracked as fix direction 2 in the
    original issue.
  • Original report symptom "multi-return closure references an undeclared
    variable" (Bug B) is consistent with this root cause but was not
    independently reproduced; documented in the issue, request for .cc
    attachments from anyone who hits it.
  • Known pre-existing environment issue (unrelated, worth a separate ticket):
    the prebuilt ./tpc binary is out of sync with src/ — full builds fail
    with undefined reference to typephp_opcode_table_install; drive the
    compiler via php bin/tpc.php.

…osure

An expression may be lowered several times: an outer ?? re-parses its
left operand, parseChainedExpr() warms the operand up, and the chain
walk lowers it again. Every lowering regenerates each closure the
expression contains, and every generated php::ClosureFn is delivered to
the enclosing function, so the discarded copies end up in the output
too.

parseValueSelection() marks a materialized node with the "replace"
attribute so a later parse inside the same context reuses the existing
temporary instead of emitting the statement twice; within one lambda
body that deduplication is correct. Inside a regenerated closure,
however, the temporary recorded by the previous generation lives in the
previous, discarded lambda body. Honouring the stale attribute made the
live copy assign that never-initialized temporary, so
($m[1] ?? '') !== '' compared null against '' and the guard was always
true: the wrong branch ran and captured match groups silently vanished
from the output. The isset() spelling kept working because it inlines
php::exists() without a replace attribute.

Drop the replace shortcuts from the closure subtree on every
generation so each copy rebuilds its own materialization.
@matyhtf
matyhtf merged commit 6037e99 into swoole:master Oct 8, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants