From ea0ea4414a4440968841c778ba6f07ede360f29e Mon Sep 17 00:00:00 2001 From: Alessio Giacobbe Date: Wed, 2 Sep 2026 13:41:18 +0200 Subject: [PATCH] fix(codegen): keep Zend operand read order around hoisted side effects (#52) --skip-tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(codegen): keep Zend operand read order around hoisted side effects Lowering a later call argument or concat operand that materializes captured statements (an assignment, a call result) appended them to the enclosing statement, executing the side effect before earlier operands were read: two($j, $j = 5) with $j = 1 produced "5,5" (Zend "1,5") and $m . "," . ($m = 9) produced "9,9" (Zend "1,9"). Call arguments: Zend SENDs strictly left to right, so when a later argument hoists statements, every earlier by-value plain-variable argument is snapshotted into a temporary at its own argument position. By-reference parameters, unpacked arguments, $this and $GLOBALS are left alone. Concat chains: Zend reads a CV operand when its CONCAT opcode executes, so in the left-associated chain the first two items are read together at the first op (after both items' side effects: $s . ($s = 'b') . $s is "bbb") and each later item after the side effects of everything up to itself. The flattened braced-list lowering now snapshots a plain-variable item exactly at that read position, deferring the first item's snapshot until the second item has been lowered. Plain arithmetic is intentionally unchanged: Zend's ADD reads the CV at op time, so $k + ($k = 5) is 10 in both worlds, and the existing codegen already matches. * test(codegen): platform-neutral integer-literal suffixes in eval-order assertions * fix(codegen): detect wrapped side effects when ordering operand reads Zend sends call arguments strictly left to right and reads a concat operand's CV when its opcode executes: an assignment nested in a later argument runs after every earlier by-value argument has been sent, so pairValue($i, (int) ($i = 5)) passes the old value ("1,5"), and the same holds when the assignment is wrapped in !, unary +/-, ~, or @. shouldMaterializeOrderedOperand() only descended through BinaryOp nodes, so an assignment inside any other expression wrapper was not classified as side-effecting: no earlier operand was snapshotted and the assignment could even stay inline in the C++ argument list (php::toInt(i = 5LL)), mutating the variable at an unsequenced point. Deriving the decision from the captured statements lowering produces would miss exactly these inline cases, so the classifier now recurses structurally through every sub-expression: any wrapper of a side-effecting node is itself side-effecting. Closure and arrow function bodies do not run at creation time and stop the walk; throw, yield, yield from, and backtick expressions join the side-effecting leaves. Zend's ADD opcode still reads its CV at op time, so $k + ($k = 5) keeps its existing codegen (10 in both worlds, no snapshot). * fix(codegen): type materialized wrapper operands as dynamic temporaries A wrapper expression around a side effect (-strlen($s), a cast, error suppression) is now materialized for evaluation order, but the temp-type fallback typed it by the detected PHP type. The lowered C++ can still be dynamic — an unqualified namespaced call lowers to php::call(...), a Variant — so a php::Int temporary failed to compile in the self-build. Follow the call/binary-op policy: native scalar types only in native-types mode, otherwise a dynamic temporary. --- phpunit/code/eval-order-side-effects.php | 41 ++++++ .../src/EvalOrderSideEffectsCodegenTest.php | 122 +++++++++++++++++ src/Generator/CallArgumentGenerator.php | 29 +++- src/Parser/BinaryOpTrait.php | 129 ++++++++++++++++-- .../operator/eval-order-side-effects.phpt | 105 ++++++++++++++ 5 files changed, 410 insertions(+), 16 deletions(-) create mode 100644 phpunit/code/eval-order-side-effects.php create mode 100644 phpunit/src/EvalOrderSideEffectsCodegenTest.php create mode 100644 tests/compiler/operator/eval-order-side-effects.phpt diff --git a/phpunit/code/eval-order-side-effects.php b/phpunit/code/eval-order-side-effects.php new file mode 100644 index 00000000..e1bb0452 --- /dev/null +++ b/phpunit/code/eval-order-side-effects.php @@ -0,0 +1,41 @@ +compileFixture(); + $body = $this->extractFunctionBody($code, 'php_callargorder()'); + + self::assertMatchesRegularExpression( + '/(tmp_var_\d+) = j;\s*\n\s*(tmp_var_\d+) = j = 5L{1,2};/', + $body, + 'the old value of $j must be captured before $j = 5 executes', + ); + self::assertDoesNotMatchRegularExpression( + '/php_pair\(php::toIntArgExact\(j,/', + $body, + '$j must not be read directly after the hoisted assignment', + ); + } + + public function testConcatOperandReadIsSnapshottedBeforeLaterAssignment(): void + { + $code = $this->compileFixture(); + $body = $this->extractFunctionBody($code, 'php_concatorder()'); + + self::assertMatchesRegularExpression( + '/(tmp_var_\d+) = m;\s*\n\s*(tmp_var_\d+) = m = 9L{1,2};/', + $body, + 'the old value of $m must be captured before $m = 9 executes', + ); + self::assertDoesNotMatchRegularExpression( + '/php::concat\(\{php::toString\(m\)/', + $body, + '$m must not be read directly after the hoisted assignment', + ); + } + + public function testCastWrappedAssignmentStillSnapshotsEarlierArgument(): void + { + $code = $this->compileFixture(); + $body = $this->extractFunctionBody($code, 'php_castwrappedcallargorder()'); + + self::assertMatchesRegularExpression( + '/(tmp_var_\d+) = i;\s*\n\s*(tmp_var_\d+) = php::toInt\(i = 5L{1,2}\);/', + $body, + 'the old value of $i must be captured before the cast-wrapped $i = 5 executes', + ); + self::assertDoesNotMatchRegularExpression( + '/php_pair\(php::toIntArgExact\(i,/', + $body, + '$i must not be read directly alongside the wrapped assignment', + ); + } + + public function testBooleanNotWrappedAssignmentStillSnapshotsEarlierArgument(): void + { + $code = $this->compileFixture(); + $body = $this->extractFunctionBody($code, 'php_notwrappedcallargorder()'); + + self::assertMatchesRegularExpression( + '/(tmp_var_\d+) = k;\s*\n\s*(tmp_var_\d+) = !\(php::toBool\(k = 0L{1,2}\)\);/', + $body, + 'the old value of $k must be captured before the negated $k = 0 executes', + ); + self::assertDoesNotMatchRegularExpression( + '/php_pairvalue\(k,/', + $body, + '$k must not be read directly alongside the wrapped assignment', + ); + } + + public function testPlainArithmeticKeepsZendCvReadSemantics(): void + { + $code = $this->compileFixture(); + $body = $this->extractFunctionBody($code, 'php_plainarithmeticunchanged()'); + + // Zend reads the CV when the ADD executes, i.e. after the nested + // assignment; the direct read of k matches that and must stay. + self::assertMatchesRegularExpression( + '/(tmp_var_\d+) = k = 5L{1,2};\s*\n[^\n]*\(\(k\) \+ \(\1\)\)/', + $body, + ); + self::assertStringNotContainsString('= k;', $body); + } + + private function extractFunctionBody(string $code, string $marker): string + { + $start = strpos($code, $marker); + self::assertIsInt($start, "missing function: {$marker}"); + $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/eval-order-side-effects.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/Generator/CallArgumentGenerator.php b/src/Generator/CallArgumentGenerator.php index 03c70f44..0f1c836f 100644 --- a/src/Generator/CallArgumentGenerator.php +++ b/src/Generator/CallArgumentGenerator.php @@ -130,10 +130,37 @@ trait CallArgumentGenerator $variadicVar = null; $callableName = $functionDef->displayName ?: $functionDef->getNamespacedName(); + // PHP evaluates arguments left to right. A later argument that hoists + // captured statements while being lowered (an assignment, a call) + // would execute those side effects before an earlier plain-variable + // argument is read: `two($j, $j = 5)` must pass the old value of $j. + // Record the last such argument so every earlier by-value variable + // read can be snapshotted at its own argument position. + $lastHoistingSourceIndex = -1; + foreach ($sourceArgs as $sourceIndex => [, , $arg]) { + if ($arg instanceof Node\Arg && $this->shouldMaterializeOrderedOperand($arg->value)) { + $lastHoistingSourceIndex = $sourceIndex; + } + } + // Evaluate every supplied argument in PHP source order. The resulting // expressions/temporaries may then be rearranged safely for the native // C++ ABI without changing observable call order. - foreach ($sourceArgs as [$argIndex, $variadicName, $arg]) { + foreach ($sourceArgs as $sourceIndex => [$argIndex, $variadicName, $arg]) { + if ($sourceIndex < $lastHoistingSourceIndex + && $arg instanceof Node\Arg + && !$arg->unpack + && $this->isSnapshotableVariableRead($arg->value) + ) { + $paramInfo = $argIndex === $variadicArgIndex + ? $functionDef->argInfoList[$variadicArgIndex] + : $this->getArgInfo($arg, $nativeFunc, $argIndex); + if ($paramInfo !== null && !$paramInfo->byRef) { + $snapshot = $this->parseOrderedOperand($arg->value, false, true); + $arg = clone $arg; + $arg->value = new Expr\Variable($snapshot, $arg->value->getAttributes()); + } + } if ($argIndex !== $variadicArgIndex) { $argInfo = $this->getArgInfo($arg, $nativeFunc, $argIndex); $resolvedArgs[$argIndex] = $this->getTypeConvertedArg( diff --git a/src/Parser/BinaryOpTrait.php b/src/Parser/BinaryOpTrait.php index 5325d2ce..a6cb87ad 100644 --- a/src/Parser/BinaryOpTrait.php +++ b/src/Parser/BinaryOpTrait.php @@ -608,12 +608,13 @@ trait BinaryOpTrait protected function shouldMaterializeOrderedOperand(NodeAbstract $expr): bool { - if ($expr instanceof Expr\BinaryOp) { - return $this->shouldMaterializeOrderedOperand($expr->left) - || $this->shouldMaterializeOrderedOperand($expr->right); + // A closure or arrow function body does not run when the closure is + // created, so nothing inside it can execute at this operand position. + if ($expr instanceof Expr\Closure || $expr instanceof Expr\ArrowFunction) { + return false; } - return $expr instanceof Expr\FuncCall + if ($expr instanceof Expr\FuncCall || $expr instanceof Expr\MethodCall || $expr instanceof Expr\StaticCall || $expr instanceof Expr\New_ @@ -635,7 +636,31 @@ trait BinaryOpTrait || $expr instanceof Expr\NullsafePropertyFetch || $expr instanceof Expr\Clone_ || $expr instanceof Expr\Include_ - || $expr instanceof Expr\Eval_; + || $expr instanceof Expr\Eval_ + || $expr instanceof Expr\Throw_ + || $expr instanceof Expr\Yield_ + || $expr instanceof Expr\YieldFrom + || $expr instanceof Expr\ShellExec + ) { + return true; + } + + // Recurse structurally through every remaining expression wrapper + // (binary ops, casts, unary plus/minus, boolean/bitwise not, error + // suppression, instanceof, isset/empty, interpolation, ...). A nested + // side effect stays a side effect no matter what wraps it, and it is + // not always hoisted: `(int) ($i = 5)` lowers to the inline C++ + // expression `php::toInt(i = 5LL)`, which mutates `i` at an + // unsequenced point unless the operand is materialized in order. + foreach ($expr->getSubNodeNames() as $name) { + $subNode = $expr->{$name}; + foreach (is_array($subNode) ? $subNode : [$subNode] as $child) { + if ($child instanceof Expr && $this->shouldMaterializeOrderedOperand($child)) { + return true; + } + } + } + return false; } protected function parseOrderedBinaryOperand(NodeAbstract $expr): string @@ -751,7 +776,19 @@ trait BinaryOpTrait } $type = $this->detectTypeOfExpr($expr); - return $type; + if ($expr instanceof Expr\Variable + || in_array($type, [Type::BIGINT, Type::DECIMAL, Type::BIGFLOAT], true) + || ($this->nativeTypes && $this->isNativeType($type)) + ) { + return $type; + } + // A wrapper expression (unary minus, a cast, error suppression, ...) + // around a side effect is materialized for evaluation order, but its + // lowered C++ form can still be dynamic — `-strlen($s)` on an + // unqualified namespaced call lowers to `-(php::call(...))`, a + // Variant. Outside native-types mode the temporary must stay dynamic, + // matching the call and binary-op policy above. + return Type::VAR; } protected function appendCapturedStmtLinesToContext(array $stmts): void @@ -874,8 +911,38 @@ trait BinaryOpTrait $useTwoOperandOverload = $prefixExpressions === [] && $this->canUseTwoOperandConcatOverload($items); + // Zend lowers the left-associated chain i0.i1.i2... into one CONCAT + // opcode per node and reads a CV operand when its opcode executes: + // i0 and i1 are both read at the first op (after the side effects of + // both), and every later item ik at the k-th op (after the side + // effects of i0..ik, before those of later items). The flattened + // braced list hoists all captured side effects ahead of the whole + // expression, so a plain-variable item that Zend reads before a later + // item's side effects (`$m . ',' . ($m = 9)` must yield "1,9") is + // snapshotted into a temporary at its Zend read position. + $lastHoistingIndex = -1; + foreach ($items as $index => $item) { + if ($this->shouldMaterializeOrderedOperand($item) + || $this->isNativeObjectClass($this->detectClassOfExpr($item)) + ) { + $lastHoistingIndex = $index; + } + } + + // The first item is read together with the second at the first op, + // i.e. after the second item's side effects. Its snapshot is deferred + // until the second item has been lowered. + $deferFirstItemSnapshot = $lastHoistingIndex >= 2 + && isset($items[1]) + && $this->isSnapshotableVariableRead($items[0]) + && !($this->isScalarString($items[1]) && $items[1]->value === ''); + $argList = $prefixExpressions; - foreach ($items as $item) { + foreach ($items as $index => $item) { + if ($deferFirstItemSnapshot && $index === 0) { + continue; + } + // Keep one operand so concat still performs PHP string coercion. // Prefix expressions are operands too (for example, the left-hand // value of `.=`), so an empty RHS literal can be omitted there. @@ -883,20 +950,34 @@ trait BinaryOpTrait continue; } + $entryPosition = count($argList); $itemClass = $this->detectClassOfExpr($item); if ($this->isNativeObjectClass($itemClass)) { $toString = new Expr\MethodCall($item, new Node\Identifier('toString')); $argList[] = $this->parseOrderedOperand($toString, false); - continue; + } else { + $type = $this->detectTypeOfExpr($item); + // C++17 evaluates the braced-list elements in order. The + // temporary is still required because lowering a later operand + // may append captured beforeStmtLines ahead of the entire + // concat expression; without it, those statements could + // overtake an earlier Call. + $snapshotEarlierRead = $index >= 1 + && $index < $lastHoistingIndex + && $this->isSnapshotableVariableRead($item); + $parsed = $this->parseOrderedOperand($item, false, $snapshotEarlierRead); + $argList[] = $this->prepareConcatOperand($parsed, $type); } - $type = $this->detectTypeOfExpr($item); - // C++17 evaluates the braced-list elements in order. The temporary - // is still required because lowering a later operand may append - // captured beforeStmtLines ahead of the entire concat expression; - // without it, those statements could overtake an earlier Call. - $parsed = $this->parseOrderedOperand($item, false); - $argList[] = $this->prepareConcatOperand($parsed, $type); + if ($deferFirstItemSnapshot && $index === 1) { + // Snapshot the first item now, after the second item's side + // effects, and keep its leading position in the operand list. + $firstType = $this->detectTypeOfExpr($items[0]); + $firstParsed = $this->parseOrderedOperand($items[0], false, true); + array_splice($argList, $entryPosition, 0, [ + $this->prepareConcatOperand($firstParsed, $firstType), + ]); + } } if ($useTwoOperandOverload && count($argList) === 2) { @@ -906,6 +987,24 @@ trait BinaryOpTrait return Symbol::concat() . '({' . implode(', ', $argList) . '})'; } + /** + * Whether an operand is a plain local variable read whose value can be + * snapshotted into a temporary to preserve left-to-right evaluation when + * a later operand hoists side-effecting statements. `$this` cannot be + * reassigned and $GLOBALS has dedicated lowering; both are left alone. + */ + protected function isSnapshotableVariableRead(NodeAbstract $expr): bool + { + if (!$this->isVarExpr($expr) || !is_string($expr->name)) { + return false; + } + if ($expr->name === 'this' || $expr->name === 'GLOBALS') { + return false; + } + $var = (string) $this->parseIdentifier($expr); + return $this->hasVar($var) && !$this->isStdContainer($var); + } + protected function canUseTwoOperandConcatOverload(array $items): bool { if (count($items) !== 2) { diff --git a/tests/compiler/operator/eval-order-side-effects.phpt b/tests/compiler/operator/eval-order-side-effects.phpt new file mode 100644 index 00000000..4c885941 --- /dev/null +++ b/tests/compiler/operator/eval-order-side-effects.phpt @@ -0,0 +1,105 @@ +--TEST-- +Call arguments and concat operands follow Zend's operand read order around side effects +--FILE-- + +--EXPECT-- +string(3) "1,5" +int(5) +string(3) "2,6" +int(6) +string(3) "1,9" +int(9) +string(3) "bbb" +string(4) "xxxy" +string(3) "pbb" +string(4) "pa,b" +string(5) "preaz" +int(10) +string(3) "1,5" +int(5) +string(3) "1,1" +int(0) +string(4) "1,-5" +string(3) "1,5" +string(3) "1,9" +string(3) "1,1"