fix(codegen): keep Zend operand read order around hoisted side effects

phpunit/code/eval-order-side-effects.php

@@ -0,0 +1,41 @@
+<?php
+
+function pair(int $a, int $b): string
+{
+    return $a . ',' . $b;
+}
+
+function callArgOrder(): string
+{
+    $j = 1;
+    return pair($j, $j = 5);
+}
+
+function concatOrder(): string
+{
+    $m = 1;
+    return $m . ',' . ($m = 9);
+}
+
+function plainArithmeticUnchanged(): int
+{
+    $k = 1;
+    return $k + ($k = 5);
+}
+
+function pairValue(mixed $a, mixed $b): string
+{
+    return $a . ',' . $b;
+}
+
+function castWrappedCallArgOrder(): string
+{
+    $i = 1;
+    return pair($i, (int) ($i = 5));
+}
+
+function notWrappedCallArgOrder(): string
+{
+    $k = 1;
+    return pairValue($k, !($k = 0));
+}

phpunit/src/EvalOrderSideEffectsCodegenTest.php

@@ -0,0 +1,122 @@
+<?php
+
+use TypePhp\CompilerTest;
+
+/**
+ * PHP evaluates call arguments and concat operands left to right. When a
+ * later operand hoists captured statements (an assignment), earlier
+ * plain-variable reads must be snapshotted at their own position, or the
+ * hoisted side effect executes first: pair($j, $j = 5) must return "1,5"
+ * and $m . ',' . ($m = 9) must be "1,9". Plain arithmetic is exempt:
+ * Zend's ADD opcode reads the CV at op time, so $k + ($k = 5) is 10 in
+ * both worlds and must keep its existing codegen.
+ */
+final class EvalOrderSideEffectsCodegenTest extends \BaseTest
+{
+    public function testCallArgumentReadIsSnapshottedBeforeLaterAssignment(): void
+    {
+        $code = $this->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;
+    }
+}

src/Generator/CallArgumentGenerator.php

@@ -130,10 +130,37 @@ protected function parseNativeCallArgs(
         $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(

src/Parser/BinaryOpTrait.php

@@ -544,12 +544,13 @@ protected function handleNestedConstantDivisionByZero(
 
     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_
@@ -571,7 +572,31 @@ protected function shouldMaterializeOrderedOperand(NodeAbstract $expr): bool
             || $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
@@ -687,7 +712,19 @@ protected function getOrderedOperandTmpType(NodeAbstract $expr, string $value):
         }
 
         $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
@@ -810,29 +847,73 @@ protected function parseFlattenedConcat(NodeAbstract $expr, array $prefixExpress
         $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.
             if ($argList !== [] && $this->isScalarString($item) && $item->value === '') {
                 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) {
@@ -842,6 +923,24 @@ protected function parseFlattenedConcat(NodeAbstract $expr, array $prefixExpress
         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) {