fix(codegen): keep Zend operand read order around hoisted side effects - #52
fix(codegen): keep Zend operand read order around hoisted side effects#52AlessioGiacobbe wants to merge 2 commits into
Conversation
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.
matyhtf
left a comment
matyhtf
left a comment
There was a problem hiding this comment.
The current operand classifier misses side effects wrapped in non-binary expressions, so the argument-order bug is still observable.
For example:
function pairValue(mixed $a, mixed $b): string
{
return $a . ',' . $b;
}
$i = 1;
var_dump(pairValue($i, (int) ($i = 5)));
$k = 1;
var_dump(pairValue($k, !($k = 0)));Zend PHP outputs:
string(3) "1,5"
string(3) "1,1"
This branch outputs:
string(3) "5,5"
string(3) "0,1"
shouldMaterializeOrderedOperand() only descends through Expr\BinaryOp. An assignment wrapped by Expr\Cast or BooleanNot is therefore classified as non-hoisting, even though lowering it appends captured statements. The earlier by-value argument is not snapshotted and the nested assignment overtakes its read.
Please make the side-effect/hoisting detection structurally recursive through expression wrappers such as casts, unary expressions, boolean-not, bitwise-not, and error suppression, or derive the decision from the captured statements produced by lowering. A generic AST walk must stop at nested Closure/ArrowFunction bodies because their bodies are not evaluated at creation time.
Add PHPT coverage for at least the cast and boolean-not examples above. The existing tests pass but do not cover wrapped side effects.
2 participants
When a later call argument or concat operand hoisted a side effect, it executed before earlier operands were read:
$j = 1; two($j, $j = 5)printed"5,5"where Zend SENDs left-to-right and prints"1,5";$m . "," . ($m = 9)printed"9,9"instead of"1,9".Earlier plain-variable operands are now snapshotted at their exact Zend read positions when a later operand materializes statements. Concat read positions were probed against Zend empirically (the VM reads the first two chain items together after both items' side effects), and plain arithmetic (
$k + ($k = 5)) is deliberately unchanged — Zend itself reads the CV at the op, so the existing codegen already matches; a no-regression test pins that.Verified against Zend 8.4.13; codegen tests + phpt included.
Part of the split of #39.