From 6b6ce4fc825c7a8b20482dbdec27aedf5a305e81 Mon Sep 17 00:00:00 2001 From: Giandonn Date: Sun, 30 Aug 2026 10:47:25 -0300 Subject: [PATCH] fix(optimizer): restrict the count() literal fold to provably inert items The first whitelist was too broad. ConstFetch, ClassConstFetch and the base Node\Scalar type all admit expressions PHP must still evaluate, so count([UNDEFINED_COUNT_LITERAL]), count([KnownClass::MISSING]) and count(["{$object->property}"]) folded to 1, dropping two Errors and a __get() call. The defined-variable check was not a purity proof either: hasVar() only reports a compiler slot, not that the variable is still initialized on every path after unset(). Narrow the fold to items whose evaluation cannot be observed: - literal Int_, Float_ and String_ (an interpolated string is a distinct InterpolatedString node, so String_ already excludes it); - the language constants true, false and null only; - unary plus/minus over a literal int or float; - recursively safe nested arrays. Variables, general constant and class constant fetches, interpolated strings and every other expression stay on the runtime path, and by-reference items are now rejected explicitly alongside keys and unpacking. Cover the three reported cases plus a by-reference item, a plain variable read and a defined class constant in both the fold-decision test and the PHPT. --- phpunit/code/count-literal-fold-safe.php | 5 +- phpunit/code/count-literal-fold-unsafe.php | 23 +++++++++ phpunit/src/CountLiteralFoldTest.php | 8 +-- src/Optimizer/FuncCallOptimizer.php | 33 ++++++++---- tests/compiler/array/count-literal-fold.phpt | 54 ++++++++++++++++++-- 5 files changed, 103 insertions(+), 20 deletions(-) diff --git a/phpunit/code/count-literal-fold-safe.php b/phpunit/code/count-literal-fold-safe.php index 8c777ccf..fdcfbf0e 100644 --- a/phpunit/code/count-literal-fold-safe.php +++ b/phpunit/code/count-literal-fold-safe.php @@ -8,10 +8,9 @@ function main(): void { - $a = 1; - echo count([1, 2, 3]), "\n"; echo count([[1, 2], [3]]), "\n"; - echo count([$a, -2, true, null]), "\n"; + echo count([1.5, 'text', true, false, null]), "\n"; + echo count([-2, +3, -1.5]), "\n"; echo count([]), "\n"; } diff --git a/phpunit/code/count-literal-fold-unsafe.php b/phpunit/code/count-literal-fold-unsafe.php index f9d51fcd..d78a4906 100644 --- a/phpunit/code/count-literal-fold-unsafe.php +++ b/phpunit/code/count-literal-fold-unsafe.php @@ -6,6 +6,20 @@ * @contact service@swoole.com */ +class KnownClass +{ + public const KNOWN = 1; +} + +class MagicHolder +{ + public function __get(string $name): int + { + echo "get-{$name}\n"; + return 1; + } +} + function bump(): int { echo "bump\n"; @@ -16,9 +30,18 @@ function main(): void { $rest = [1, 2, 3, 4, 5]; $i = 0; + $plain = 1; + $ref = 1; + $object = new MagicHolder(); echo count([bump(), bump()]), "\n"; echo count(['a' => 1, 'a' => 2]), "\n"; echo count([...$rest, 9]), "\n"; echo count([$i++, $i++]), "\n"; + echo count([$plain]), "\n"; + echo count([&$ref]), "\n"; + echo count([UNDEFINED_COUNT_LITERAL]), "\n"; + echo count([KnownClass::MISSING]), "\n"; + echo count(["{$object->property}"]), "\n"; + echo count([KnownClass::KNOWN]), "\n"; } diff --git a/phpunit/src/CountLiteralFoldTest.php b/phpunit/src/CountLiteralFoldTest.php index a68d30af..062a0ea9 100644 --- a/phpunit/src/CountLiteralFoldTest.php +++ b/phpunit/src/CountLiteralFoldTest.php @@ -21,9 +21,11 @@ class CountLiteralFoldTest extends TestCase { $cpp = $this->compileToCpp('count-literal-fold-unsafe.php'); - // Element side effects, a repeated key and a spread each make the - // number of AST items differ from the runtime element count. - self::assertSame(4, substr_count($cpp, 'php::fn::count(')); + // Every call in the fixture must stay on the runtime path: element + // side effects, a repeated key, a spread, a by-reference item, a + // plain variable read, a constant or class constant fetch that may + // be undefined, and an interpolated string that may call __get(). + self::assertSame(10, substr_count($cpp, 'php::fn::count(')); self::assertStringContainsString('php_bump()', $cpp); self::assertStringContainsString('i++', $cpp); } diff --git a/src/Optimizer/FuncCallOptimizer.php b/src/Optimizer/FuncCallOptimizer.php index 3c15ca65..019a4bf5 100644 --- a/src/Optimizer/FuncCallOptimizer.php +++ b/src/Optimizer/FuncCallOptimizer.php @@ -674,9 +674,10 @@ trait FuncCallOptimizer { foreach ($array->items as $item) { // [...$other] contributes an element count only known at runtime, - // and a key may collapse onto an earlier one: ['a' => 1, 'a' => 2] - // counts as one element, not two. - if ($item->unpack || $item->key !== null) { + // a key may collapse onto an earlier one (['a' => 1, 'a' => 2] + // counts as one element, not two), and a by-reference item binds + // its source variable instead of reading it. + if ($item->unpack || $item->key !== null || $item->byRef) { return false; } if (!$this->isCountFoldableItem($item->value)) { @@ -686,24 +687,34 @@ trait FuncCallOptimizer return true; } + /** + * Only expressions whose evaluation is provably free of observable effects + * may be discarded. Variables, general constant and class constant + * fetches, interpolated strings and every other expression stay on the + * runtime path: they can be undefined, autoload, throw or call __get(). + */ protected function isCountFoldableItem(Node\Expr $value): bool { - // ConstFetch also covers true, false and null. - if ($this->isScalar($value) - || $value instanceof Node\Expr\ConstFetch - || $value instanceof Node\Expr\ClassConstFetch + // Node\Scalar\String_ is the literal string only; an interpolated + // string is a distinct Node\Scalar\InterpolatedString node. + if ($value instanceof Node\Scalar\Int_ + || $value instanceof Node\Scalar\Float_ + || $value instanceof Node\Scalar\String_ ) { return true; } + // The language constants only. Any other name may be undefined and + // must still raise the same Error PHP raises. + if ($value instanceof Node\Expr\ConstFetch) { + return in_array(strtolower($value->name->toString()), ['true', 'false', 'null'], true); + } if ($value instanceof Node\Expr\UnaryMinus || $value instanceof Node\Expr\UnaryPlus) { - return $this->isCountFoldableItem($value->expr); + return $value->expr instanceof Node\Scalar\Int_ || $value->expr instanceof Node\Scalar\Float_; } if ($value instanceof Node\Expr\Array_) { return $this->isCountFoldableArray($value); } - // A defined variable is a plain read; an undefined one must reach the - // dynamic path so it still reports the same diagnostic as PHP. - return $this->isVarExpr($value) && is_string($value->name) && $this->hasVar($value->name); + return false; } protected function doFoldKnownClass(Node\Expr\FuncCall $expr): string|false diff --git a/tests/compiler/array/count-literal-fold.phpt b/tests/compiler/array/count-literal-fold.phpt index ade723fc..d8552362 100644 --- a/tests/compiler/array/count-literal-fold.phpt +++ b/tests/compiler/array/count-literal-fold.phpt @@ -2,6 +2,20 @@ count() on an array literal keeps spreads, duplicate keys and element side effects --FILE-- getMessage(), "\n"; + } + + // A missing class constant on a known class must also still throw. + try { + var_dump(count([KnownClass::MISSING])); + echo "class-constant-error-not-thrown\n"; + } catch (Error $e) { + echo "caught=", $e->getMessage(), "\n"; + } + + // An interpolated string may invoke __get(), which must still happen. + $object = new MagicHolder(); + var_dump(count(["{$object->property}"])); + + // A by-reference item binds the source variable instead of reading it. + $ref = 1; + var_dump(count([&$ref])); + + // A defined class constant is still evaluated, not discarded. + var_dump(count([KnownClass::KNOWN])); + // Plain literals stay eligible for the compile-time fold. - $a = 1; var_dump(count([1, 2, 3])); var_dump(count([[1, 2], [3]])); - var_dump(count([$a, -2, true, null])); + var_dump(count([1.5, 'text', true, false, null])); + var_dump(count([-2, +3, -1.5])); var_dump(count([])); } ?> @@ -41,7 +82,14 @@ int(1) int(6) int(2) int(2) +caught=Undefined constant "UNDEFINED_COUNT_LITERAL" +caught=Undefined constant KnownClass::MISSING +get-property +int(1) +int(1) +int(1) int(3) int(2) -int(4) +int(5) +int(3) int(0)