fix(codegen): keep Zend operand read order around hoisted side effects (#52) --skip-tests

* 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

@@ -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 @@ 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(

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) {

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--
+<?php
+declare(strict_types=1);
+
+function pair(int $a, int $b): string
+{
+    return $a . ',' . $b;
+}
+
+function pairValue(mixed $a, mixed $b): string
+{
+    return $a . ',' . $b;
+}
+
+function main(): void
+{
+    // Call arguments are sent strictly left to right.
+    $j = 1;
+    var_dump(pair($j, $j = 5));
+    var_dump($j);
+
+    $n = 2;
+    var_dump(pair($n, $n *= 3));
+    var_dump($n);
+
+    // Concat chains: a variable is read when its concat op executes, so it
+    // sees the side effects of everything up to and including the operand it
+    // is combined with, but nothing later.
+    $m = 1;
+    var_dump($m . ',' . ($m = 9));
+    var_dump($m);
+
+    // The first two operands are read together at the first op, after the
+    // second operand's assignment.
+    $s = 'a';
+    var_dump($s . ($s = 'b') . $s);
+
+    $a = 'a';
+    var_dump($a . ($a = 'x') . $a . ($a = 'y'));
+
+    $t = 'p';
+    $u = 'a';
+    $t .= $u . ($u = 'b');
+    var_dump($t);
+
+    $t2 = 'p';
+    $u2 = 'a';
+    $t2 .= $u2 . ',' . ($u2 = 'b');
+    var_dump($t2);
+
+    $w = 'a';
+    var_dump('pre' . $w . ($w = 'z'));
+
+    // Zend reads the CV when the ADD executes, after the nested assignment.
+    $k = 1;
+    var_dump($k + ($k = 5));
+
+    // A side-effecting argument stays side-effecting inside an expression
+    // wrapper (a cast, boolean not, unary minus, error suppression): the
+    // earlier by-value argument still reads the value before the assignment.
+    $c = 1;
+    var_dump(pair($c, (int) ($c = 5)));
+    var_dump($c);
+
+    $b = 1;
+    var_dump(pairValue($b, !($b = 0)));
+    var_dump($b);
+
+    $d = 1;
+    var_dump(pair($d, -($d = 5)));
+
+    $e = 1;
+    var_dump(pairValue($e, @($e = 5)));
+
+    // Wrapped side effects inside a concat chain follow the same read order.
+    $g = 1;
+    var_dump($g . ',' . (int) ($g = 9));
+
+    $h = 1;
+    var_dump($h . ',' . !($h = 0));
+}
+?>
+--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"