From 20f0b5a284d783dedeed27a1b175abb9e4acfb00 Mon Sep 17 00:00:00 2001 From: Alessio Giacobbe Date: Wed, 2 Sep 2026 02:29:29 +0200 Subject: [PATCH] fix(codegen): evaluate compound ??= RHS only when the target is not set (#47) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(codegen): evaluate compound ??= RHS only when the target is not set PHP evaluates the right-hand side of ??= lazily: `$a = 1; $a ??= sideEffect() + 1;` never calls sideEffect(). When the RHS was a compound expression the compiler materialized its lowered statements (the call result temporary) into the enclosing statement context, so the generated C++ executed the side-effecting call unconditionally before the isset check. Generalize the conditional-lambda lowering that already protected native-object targets: whenever the RHS captured before/after statements, emit an immediately-invoked lambda whose not-set branch contains those statements, the assignment and the cleanup. The simple inline form (`$b ??= f()`) keeps its existing conditional-expression codegen unchanged. * fix(codegen): stabilize ??= targets and finish the RHS before assigning Zend evaluates a coalesce-assignment target's receiver and array keys exactly once, before the isset check and regardless of its outcome; the string-based lowering mentioned the target on every use (isset, read, write, returned value), so a side-effecting receiver ran twice when the target was set and three times when it was not. Side-effecting target subexpressions are now materialized into temporaries in source order (array containers keep their original variable — writing through a copied temporary would write to the copy — while object receivers are handles) and the rewritten target reuses them everywhere. The captured branch also assigned the target before running the RHS's deferred write-backs, so a postfix increment on the RHS finished after the outer assignment — observable by a set hook on the target. The RHS now completes into a temporary (write-backs included) before the target is written, and the assignment expression itself is returned so the target is not read again afterwards. * fix(codegen): stabilize every ??= target subexpression, bound temporary lifetimes Three target-stabilization gaps in the coalesce-assignment lowering: - A value-producing array container (makeArray()[keyName()] ??= 42) was left unstabilized: the dimension was materialized first, reversing PHP's container-then-key source order, and the container ran once for the isset check and again for the write. Non-variable containers are now materialized in source order; plain-variable containers keep write-through semantics, and a value-producing container is itself the temporary PHP writes into. - Dynamic property names were re-evaluated on every mention: $box->{propertyName()} ran the name expression twice per branch, and StaticPropertyFetch was not handled at all. Dynamic instance names, static class expressions and static property names are now materialized once, in PHP evaluation order (receiver, name, then dimension), recursively through chained targets. - The materialized temporaries were function-scoped, deferring the receiver's destructor to function exit where PHP destroys it at the end of the statement. Temporaries now follow the established lifetime idiom (stabilizeAssignOpPropertyReceiver): zval-owning Variants are .unset() at statement end and Native pointer temporaries reset to nullptr. --- .../coalesce-assign-side-effect-codegen.php | 21 ++++ .../CoalesceAssignSideEffectCodegenTest.php | 64 ++++++++++ src/Parser/AssignOpTrait.php | 118 +++++++++++++++++- .../assign-coalesce-array-key-once.phpt | 22 ++++ ...ign-coalesce-compound-rhs-side-effect.phpt | 32 +++++ .../assign-coalesce-rhs-postfix-order.phpt | 29 +++++ .../assign-coalesce-target-receiver-once.phpt | 24 ++++ .../assign-coalesce-target-stabilization.phpt | 52 ++++++++ 8 files changed, 359 insertions(+), 3 deletions(-) create mode 100644 phpunit/code/coalesce-assign-side-effect-codegen.php create mode 100644 phpunit/src/CoalesceAssignSideEffectCodegenTest.php create mode 100644 tests/compiler/coalesce/assign-coalesce-array-key-once.phpt create mode 100644 tests/compiler/coalesce/assign-coalesce-compound-rhs-side-effect.phpt create mode 100644 tests/compiler/coalesce/assign-coalesce-rhs-postfix-order.phpt create mode 100644 tests/compiler/coalesce/assign-coalesce-target-receiver-once.phpt create mode 100644 tests/compiler/coalesce/assign-coalesce-target-stabilization.phpt diff --git a/phpunit/code/coalesce-assign-side-effect-codegen.php b/phpunit/code/coalesce-assign-side-effect-codegen.php new file mode 100644 index 00000000..be0ec0ce --- /dev/null +++ b/phpunit/code/coalesce-assign-side-effect-codegen.php @@ -0,0 +1,21 @@ +compileFixture(); + + $body = $this->extractFunctionBody($code, 'php::Int php_coalescecompoundrhs()'); + + // The call must appear after the early-return isset guard of the + // conditional lambda, not as a plain statement before it. + $callPos = strpos($body, 'php_sideeffectcall()'); + self::assertIsInt($callPos); + $guardPos = strpos($body, 'if (php::exists(target)) { return target; }'); + self::assertIsInt($guardPos, 'expected the isset guard inside a conditional lambda'); + self::assertGreaterThan($guardPos, $callPos, 'RHS call must be inside the not-set branch'); + } + + public function testSimpleRhsKeepsPlainConditionalExpression(): void + { + $code = $this->compileFixture(); + + $body = $this->extractFunctionBody($code, 'php::Int php_coalescesimplerhs()'); + + self::assertStringContainsString( + '(php::exists(target)?target:(target = php_sideeffectcall()))', + $body, + ); + } + + private function extractFunctionBody(string $code, string $signature): string + { + $start = strpos($code, $signature); + self::assertIsInt($start, "missing function: {$signature}"); + $end = strpos($code, "\n}", $start); + self::assertIsInt($end); + return substr($code, $start, $end - $start); + } + + private function compileFixture(): string + { + global $translator; + + $compiler = CompilerTest::create(TYPEPHP_ROOT_PATH); + $translator = $compiler; + $source = TYPEPHP_ROOT_PATH . '/phpunit/code/coalesce-assign-side-effect-codegen.php'; + $compiler->addFiles([$source]); + $compiler->prepareFile($source); + $generated = $compiler->convertFile($source); + $code = file_get_contents($generated); + + self::assertIsString($code); + return $code; + } +} diff --git a/src/Parser/AssignOpTrait.php b/src/Parser/AssignOpTrait.php index 16d1652d..213f1e9d 100644 --- a/src/Parser/AssignOpTrait.php +++ b/src/Parser/AssignOpTrait.php @@ -1611,6 +1611,15 @@ trait AssignOpTrait $this->assertNativeArrayAccessDirectWrite($expr->var, false); $this->checkLeftValue($expr->var); + // Zend evaluates the target's receiver and array keys exactly once, + // before the isset check and regardless of its outcome. The lowering + // below mentions the target several times (isset, read, write), so + // side-effecting subexpressions of the target are materialized into + // temporaries first and the target is rewritten to reference them. + if (!$this->isVarExpr($expr->var)) { + $expr->var = $this->stabilizeCoalesceTarget($expr->var); + } + $rightClass = $this->detectClassOfExpr($expr->expr); $nativeRight = $this->isNativeObjectClass($rightClass); @@ -1713,13 +1722,116 @@ trait AssignOpTrait $code .= $this->getIndent() . '}()'; return $code; } - $this->appendCapturedStmtLinesToContext($rightBefore); - foreach ($rightAfter as $stmt) { - $this->context->afterStmtLines[] = $stmt; + if ($rightBefore !== [] || $rightAfter !== []) { + // PHP evaluates the RHS of ??= only when the target is not set. + // A compound RHS materializes captured statements (call results, + // operand temporaries); appending them to the enclosing statement + // would run its side effects unconditionally. Wrap the not-set + // branch in an immediately-invoked lambda so they execute only + // when the assignment actually happens. The RHS is completed — + // including its deferred write-backs (a postfix ++ on the RHS + // must finish before the outer assignment, which a set hook on + // the target can observe) — into a temporary before the target + // is written, and the assignment expression itself is returned + // so the target is not read again afterwards. + $rhsTmp = $this->genTmpVarName(); + $code = '[&]() {' . PHP_EOL; + $code .= $this->getIndent() . 'if (' . $isset . ') { return ' . $var . '; }' . PHP_EOL; + $code .= $this->formatCapturedStmtLines($rightBefore); + $code .= $this->getIndent() . 'auto ' . $rhsTmp . ' = ' . $right . ';' . PHP_EOL; + $code .= $this->formatCapturedStmtLines($rightAfter); + $code .= $this->getIndent() . 'return (' . $var . ' = ' . $rhsTmp . ');' . PHP_EOL; + $code .= $this->getIndent() . '}()'; + return $code; } return '(' . $isset . '?' . $var . ':(' . $var . ' = ' . $right . '))'; } + /** + * Rewrite a coalesce-assignment target so that every side-effecting + * subexpression is evaluated exactly once, in PHP source order — + * container/receiver first, then a dynamic property name, then the array + * dimension — before the target is mentioned. Containers of an array + * write keep their original node when they are plain variables (writing + * through a copied temporary would write to the copy), while a + * value-producing container is itself the temporary PHP writes into; + * object receivers are handles, so a temporary preserves identity. + */ + private function stabilizeCoalesceTarget(Expr $target): Expr + { + if ($target instanceof Expr\PropertyFetch || $target instanceof Expr\NullsafePropertyFetch) { + if (!$this->isCoalesceTargetTrivialSubexpr($target->var)) { + $target->var = $this->materializeCoalesceTargetSubexpr($target->var, true); + } + if ($target->name instanceof Expr && !$this->isCoalesceTargetTrivialSubexpr($target->name)) { + $target->name = $this->materializeCoalesceTargetSubexpr($target->name, false); + } + return $target; + } + if ($target instanceof Expr\StaticPropertyFetch) { + if ($target->class instanceof Expr && !$this->isCoalesceTargetTrivialSubexpr($target->class)) { + $target->class = $this->materializeCoalesceTargetSubexpr($target->class, false); + } + if ($target->name instanceof Expr && !$this->isCoalesceTargetTrivialSubexpr($target->name)) { + $target->name = $this->materializeCoalesceTargetSubexpr($target->name, false); + } + return $target; + } + if ($target instanceof Expr\ArrayDimFetch) { + if ($target->var instanceof Expr\PropertyFetch + || $target->var instanceof Expr\NullsafePropertyFetch + || $target->var instanceof Expr\StaticPropertyFetch + || $target->var instanceof Expr\ArrayDimFetch + ) { + $target->var = $this->stabilizeCoalesceTarget($target->var); + } elseif (!$this->isCoalesceTargetTrivialSubexpr($target->var)) { + $target->var = $this->materializeCoalesceTargetSubexpr($target->var, false); + } + if ($target->dim !== null && !$this->isCoalesceTargetTrivialSubexpr($target->dim)) { + $target->dim = $this->materializeCoalesceTargetSubexpr($target->dim, false); + } + return $target; + } + return $target; + } + + private function isCoalesceTargetTrivialSubexpr(Expr $expr): bool + { + return $expr instanceof Expr\Variable + || $expr instanceof Node\Scalar + || $expr instanceof Expr\ConstFetch + || $expr instanceof Expr\ClassConstFetch; + } + + private function materializeCoalesceTargetSubexpr(Expr $sub, bool $isReceiver): Expr\Variable + { + [$code, $before, $after] = $this->parseExprWithCapturedStmts($sub); + $this->appendCapturedStmtLinesToContext($before); + + $class = $isReceiver ? $this->detectClassOfExpr($sub) : ''; + if ($isReceiver && $this->isNativeObjectClass($class)) { + // Boxing a raw Native pointer in a Variant would coerce it to + // bool; keep the typed pointer slot instead. + $tmp = $this->genTmpVarName(); + $this->addLocalVar($tmp, $this->getNativeObjectPointerType($class)); + $this->addNativeObject($tmp, $class); + $cleanup = $tmp . ' = nullptr;'; + } else { + // Keep the value in a Variant: the rewritten target then goes + // through the generic Zend handlers, which also covers receivers + // whose concrete class is known only at runtime. + $tmp = $this->addTmpVar(Type::VAR); + $cleanup = $tmp . '.unset();'; + } + $this->context->beforeStmtLines[] = $tmp . ' = ' . $code . ';'; + $this->appendCapturedStmtLinesToContext($after); + // The temporary must not outlive the statement: PHP destroys the + // target's receiver/container at the end of the statement, and a + // function-scoped Variant would defer destructors to function exit. + $this->context->afterStmtLines[] = $cleanup; + return new Expr\Variable($tmp, $sub->getAttributes()); + } + protected function getNormalAssignType(string $type): string { return $type === Type::REF || $type === Type::VOID ? Type::VAR : $type; diff --git a/tests/compiler/coalesce/assign-coalesce-array-key-once.phpt b/tests/compiler/coalesce/assign-coalesce-array-key-once.phpt new file mode 100644 index 00000000..cbef1c43 --- /dev/null +++ b/tests/compiler/coalesce/assign-coalesce-array-key-once.phpt @@ -0,0 +1,22 @@ +--TEST-- +??= evaluates a side-effecting array key exactly once, on both branches +--FILE-- + +--EXPECT-- +KEY +SIDE +int(42) +KEY +int(42) diff --git a/tests/compiler/coalesce/assign-coalesce-compound-rhs-side-effect.phpt b/tests/compiler/coalesce/assign-coalesce-compound-rhs-side-effect.phpt new file mode 100644 index 00000000..cce33885 --- /dev/null +++ b/tests/compiler/coalesce/assign-coalesce-compound-rhs-side-effect.phpt @@ -0,0 +1,32 @@ +--TEST-- +??= does not evaluate a compound side-effecting RHS when the target is set +--FILE-- + +--EXPECT-- +int(1) +side effect! +int(42) +string(3) "set" diff --git a/tests/compiler/coalesce/assign-coalesce-rhs-postfix-order.phpt b/tests/compiler/coalesce/assign-coalesce-rhs-postfix-order.phpt new file mode 100644 index 00000000..5abec824 --- /dev/null +++ b/tests/compiler/coalesce/assign-coalesce-rhs-postfix-order.phpt @@ -0,0 +1,29 @@ +--TEST-- +??= completes the RHS postfix write-back before the target assignment +--FILE-- +stored; } + set { + var_dump(State::$assigned); + $this->stored = $value; + } + } +} + +function main(): void +{ + $source = new Source(); + State::$assigned ??= $source->value++; + var_dump(State::$assigned); + var_dump($source->value); +} +?> +--EXPECT-- +NULL +int(5) +int(6) diff --git a/tests/compiler/coalesce/assign-coalesce-target-receiver-once.phpt b/tests/compiler/coalesce/assign-coalesce-target-receiver-once.phpt new file mode 100644 index 00000000..22255978 --- /dev/null +++ b/tests/compiler/coalesce/assign-coalesce-target-receiver-once.phpt @@ -0,0 +1,24 @@ +--TEST-- +??= evaluates a side-effecting property receiver exactly once, on both branches +--FILE-- +value ??= sideEffect() + 1; + var_dump($box->value); + receiver($box)->value ??= sideEffect() + 1; + var_dump($box->value); +} +?> +--EXPECT-- +RECV +SIDE +int(42) +RECV +int(42) diff --git a/tests/compiler/coalesce/assign-coalesce-target-stabilization.phpt b/tests/compiler/coalesce/assign-coalesce-target-stabilization.phpt new file mode 100644 index 00000000..65f6d6b5 --- /dev/null +++ b/tests/compiler/coalesce/assign-coalesce-target-stabilization.phpt @@ -0,0 +1,52 @@ +--TEST-- +??= stabilizes value containers, dynamic property names, and temporary lifetimes +--FILE-- +value ??= 42; + echo "AFTER\n"; + // A dynamic instance property name is evaluated once, on both branches. + $box = new Box(); + $box->{propertyName()} ??= rhs(); + var_dump($box->value); + $box->{propertyName()} ??= rhs(); + var_dump($box->value); + // A dynamic static property name is evaluated once. + Box::${slotName()} ??= rhs(); + var_dump(Box::$slot); +} +?> +--EXPECT-- +ARRAY +KEY +int(42) +MAKE +DESTRUCT +AFTER +NAME +RHS +int(42) +NAME +int(42) +SLOT +RHS +int(42) +DESTRUCT