From f48bd025ad79c7d17cfa93ac16cfcc1fc6ae5cf0 Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Fri, 11 Sep 2026 12:14:15 +0200 Subject: [PATCH] Expose the loop that overwrites a variable still in use phpstan-strict-rules reports a foreach/for that reuses a variable name. Since definedness became precise, it also fires on reusing a spent loop variable. The rule needs to know whether the loop takes over a variable that was assigned before it and is read after it - a liveness question. VariableWritesNode::getVariableOverwritingLoop() answers it for foreach key/value bindings and for-loop initial assignments: the statement's flow is wrapped, and a probe in the live set follows the variable backwards through the loop (the loop's own bindings and updates let it through, any other write kills it) and records the assignment before the loop that the binding replaces. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01LYiPGg9cpsKLyK6X5BrJTT --- src/Analyser/StmtHandler/ForHandler.php | 47 +++- src/Analyser/StmtHandler/ForeachHandler.php | 9 + src/Analyser/VariableControlFlow.php | 8 + src/Analyser/VariableFlow.php | 22 ++ src/Analyser/VariableFlowBuilder.php | 27 +++ src/Analyser/VariableLivenessResolver.php | 104 +++++++- src/Node/VariableWritesNode.php | 19 ++ .../Node/VariableOverwritingLoopRule.php | 45 ++++ .../Node/VariableOverwritingLoopRuleTest.php | 77 ++++++ .../Node/data/variable-overwriting-loop.php | 226 ++++++++++++++++++ 10 files changed, 582 insertions(+), 2 deletions(-) create mode 100644 tests/PHPStan/Node/VariableOverwritingLoopRule.php create mode 100644 tests/PHPStan/Node/VariableOverwritingLoopRuleTest.php create mode 100644 tests/PHPStan/Node/data/variable-overwriting-loop.php diff --git a/src/Analyser/StmtHandler/ForHandler.php b/src/Analyser/StmtHandler/ForHandler.php index f4ddcd1c2d2..7c55a9c0a96 100644 --- a/src/Analyser/StmtHandler/ForHandler.php +++ b/src/Analyser/StmtHandler/ForHandler.php @@ -32,6 +32,7 @@ use function count; use function in_array; use function is_string; +use function spl_object_id; /** * @implements StmtHandler @@ -138,6 +139,15 @@ public function processStmt( $throwPoints = array_merge($throwPoints, $initResult->getThrowPoints()); $impurePoints = array_merge($impurePoints, $initResult->getImpurePoints()); } + $initTargets = []; + foreach ($stmt->init as $initExpr) { + if (!$initExpr instanceof Assign) { + continue; + } + foreach (self::targetVariables($initExpr->var) as $variable) { + $initTargets[spl_object_id($variable)] = true; + } + } $originalStorage = $storage; @@ -304,7 +314,15 @@ public function processStmt( $loop = $isIterableAtLeastOnce->no() ? VariableFlow::sequence($condition, VariableFlow::dead(VariableFlow::sequence($finalScopeResult->getVariableFlow(), $update))) : VariableFlow::loop($condition, $finalScopeResult->getVariableFlow(), $update, $isIterableAtLeastOnce->yes(), !$alwaysIterates->yes()); - $variableFlow = VariableFlow::sequence(...[...$initFlow, $loop]); + $initWrites = VariableFlowBuilder::writes(VariableFlow::sequence(...$initFlow)); + $bindings = []; + foreach ($initWrites as $write) { + if ($write->isOffsetWrite() || !isset($initTargets[$write->getId()])) { + continue; + } + $bindings[] = $write; + } + $variableFlow = VariableFlow::loopStatement($stmt, VariableFlow::sequence(...[...$initFlow, $loop]), $bindings, [...$initWrites, ...VariableFlowBuilder::writes($update)]); return new InternalStatementResult( $finalScope->addTemplateArgumentConstraints($loopScope->getTemplateArgumentConstraints()), hasYield: $finalScopeResult->hasYield() || $hasYield, @@ -316,4 +334,31 @@ public function processStmt( ); } + /** + * The variables an assignment target binds - the variable itself, or the + * variables of a destructuring list's items. + * + * @return list + */ + private static function targetVariables(Expr $target): array + { + if ($target instanceof Variable) { + return [$target]; + } + if (!$target instanceof Expr\List_ && !$target instanceof Expr\Array_) { + return []; + } + $variables = []; + foreach ($target->items as $item) { + if ($item === null) { + continue; + } + foreach (self::targetVariables($item->value) as $variable) { + $variables[] = $variable; + } + } + + return $variables; + } + } diff --git a/src/Analyser/StmtHandler/ForeachHandler.php b/src/Analyser/StmtHandler/ForeachHandler.php index 7ecf69bd2c0..071b11a1aa5 100644 --- a/src/Analyser/StmtHandler/ForeachHandler.php +++ b/src/Analyser/StmtHandler/ForeachHandler.php @@ -479,6 +479,15 @@ static function () use ($condResult, $emptyArrayType): Type { $stmt->byRef && $stmt->valueVar instanceof Variable && is_string($stmt->valueVar->name) ? VariableFlow::escape($stmt->valueVar->name) : null, ); $loopFlow = VariableFlow::loop($traversableThrowPoint !== null ? VariableFlow::throwing($traversableThrowPoint->getType(), true) : null, VariableFlow::sequence($bindingFlow, $finalScopeResult->getVariableFlow()), null, $isIterableAtLeastOnce->yes() && $nodeScopeResolver->shouldPolluteScopeWithAlwaysIterableForeach(), true); + $bindingWrites = VariableFlowBuilder::writes($bindingFlow); + $bindings = []; + foreach ($bindingWrites as $write) { + if ($write->isOffsetWrite()) { + continue; + } + $bindings[] = $write; + } + $loopFlow = VariableFlow::loopStatement($stmt, $loopFlow, $bindings, $bindingWrites); return new InternalStatementResult( $finalScope->addTemplateArgumentConstraints($finalScopeResult->getScope()->getTemplateArgumentConstraints()), hasYield: $finalScopeResult->hasYield() || $condResult->hasYield(), diff --git a/src/Analyser/VariableControlFlow.php b/src/Analyser/VariableControlFlow.php index 3671163020f..0a18bb5fc9a 100644 --- a/src/Analyser/VariableControlFlow.php +++ b/src/Analyser/VariableControlFlow.php @@ -3,6 +3,9 @@ namespace PHPStan\Analyser; use PhpParser\Node\Expr\ArrowFunction; +use PhpParser\Node\Stmt\For_; +use PhpParser\Node\Stmt\Foreach_; +use PHPStan\Node\Variable\VariableWrite; use PHPStan\Type\Type; final class VariableControlFlow extends VariableFlow @@ -13,6 +16,8 @@ final class VariableControlFlow extends VariableFlow * @param list $children * @param list $catches * @param list $cases + * @param list $bindings + * @param list $ownWrites */ public function __construct( string $kind, @@ -27,6 +32,9 @@ public function __construct( public readonly array $cases = [], public readonly bool $canRepeat = true, public readonly bool $canContainAnyThrowable = false, + public readonly Foreach_|For_|null $stmt = null, + public readonly array $bindings = [], + public readonly array $ownWrites = [], ) { parent::__construct($kind); diff --git a/src/Analyser/VariableFlow.php b/src/Analyser/VariableFlow.php index b73bd61fd6a..24b6b451b59 100644 --- a/src/Analyser/VariableFlow.php +++ b/src/Analyser/VariableFlow.php @@ -3,6 +3,8 @@ namespace PHPStan\Analyser; use PhpParser\Node\Expr\ArrowFunction; +use PhpParser\Node\Stmt\For_; +use PhpParser\Node\Stmt\Foreach_; use PHPStan\Node\Variable\VariableWrite; use PHPStan\Type\Type; use function count; @@ -36,6 +38,7 @@ abstract class VariableFlow public const THROW = 'throw'; public const STOP = 'stop'; public const ARROW = 'arrow'; + public const LOOP_STATEMENT = 'loopStatement'; /** @param self::* $kind */ protected function __construct(public readonly string $kind) @@ -160,6 +163,25 @@ public static function loop(?self $condition, ?self $body, ?self $update, bool $ return new VariableControlFlow(self::LOOP, [$condition, $body, $update], atLeastOnce: $atLeastOnce, canExit: $canExit, canRepeat: $canRepeat); } + /** + * A loop statement whose head binds variables - a foreach key/value or + * a for-loop initial assignment. The bindings are checked for reusing a + * variable that is assigned before the statement and read after it; + * the statement's own writes (the bindings and a for-loop update) are + * the only assignments allowed in between. + * + * @param list $bindings + * @param list $ownWrites + */ + public static function loopStatement(Foreach_|For_ $stmt, ?self $flow, array $bindings, array $ownWrites): ?self + { + if ($bindings === []) { + return $flow; + } + + return new VariableControlFlow(self::LOOP_STATEMENT, [$flow], stmt: $stmt, bindings: $bindings, ownWrites: $ownWrites); + } + /** @param list $catches */ public static function tryCatch(?self $body, array $catches, ?self $finally): self { diff --git a/src/Analyser/VariableFlowBuilder.php b/src/Analyser/VariableFlowBuilder.php index c3020377a31..b9cc5f13a45 100644 --- a/src/Analyser/VariableFlowBuilder.php +++ b/src/Analyser/VariableFlowBuilder.php @@ -91,6 +91,33 @@ public static function targetRead(Expr $target, ExpressionResultStorage $storage return self::child($target, $storage); } + /** + * The writes of a flow composed only of accesses and sequences - an + * assignment target or a loop head. + * + * @return list + */ + public static function writes(?VariableFlow $flow): array + { + if ($flow === null) { + return []; + } + if ($flow instanceof VariableAccessFlow) { + return $flow->write !== null ? [$flow->write] : []; + } + if (!$flow instanceof VariableSequenceFlow) { + return []; + } + $writes = []; + foreach ($flow->children as $child) { + foreach (self::writes($child) as $write) { + $writes[] = $write; + } + } + + return $writes; + } + /** @param VariableWrite::KIND_* $kind */ public static function targetWrite(Expr $target, int $kind, MutatingScope $scope, ExpressionResultStorage $storage, ?Type $redundant = null): ?VariableFlow { diff --git a/src/Analyser/VariableLivenessResolver.php b/src/Analyser/VariableLivenessResolver.php index 48e20c70e66..a6e0e85afc2 100644 --- a/src/Analyser/VariableLivenessResolver.php +++ b/src/Analyser/VariableLivenessResolver.php @@ -3,6 +3,8 @@ namespace PHPStan\Analyser; use PhpParser\Node; +use PhpParser\Node\Stmt\For_; +use PhpParser\Node\Stmt\Foreach_; use PHPStan\Node\Variable\VariableWrite; use PHPStan\Node\VariableWritesNode; use PHPStan\ShouldNotHappenException; @@ -18,6 +20,11 @@ use function is_int; use function is_string; use function spl_object_id; +use function sprintf; +use function str_ends_with; +use function str_starts_with; +use function strlen; +use function substr; /** Resolve liveness backwards over immutable body fragments. */ final class VariableLivenessResolver @@ -77,6 +84,15 @@ final class VariableLivenessResolver /** @var array */ private array $allReadKeys = []; + /** @var array */ + private array $loopStatements = []; + + /** @var array> */ + private array $ownWriteIds = []; + + /** @var array */ + private array $variableOverwritingLoops = []; + private bool $opaque = false; private bool $readsAllVariables = false; @@ -125,7 +141,7 @@ public static function resolve(Node\FunctionLike $function, ?VariableFlow $flow) $self->resolveCoverage(); } - return new VariableWritesNode($function, array_values($self->writes), $self->observedIds + $self->readIds, $self->readIds, $self->coveredIds, $self->readNames, $self->redundantTypes, $self->mentionedNames, $self->escapedNames, $self->opaque, $self->allNamesMentioned); + return new VariableWritesNode($function, array_values($self->writes), $self->observedIds + $self->readIds, $self->readIds, $self->coveredIds, $self->readNames, $self->redundantTypes, $self->mentionedNames, $self->escapedNames, $self->variableOverwritingLoops, $self->opaque, $self->allNamesMentioned); } private function collect(?VariableFlow $flow, bool $dead = false): void @@ -177,6 +193,16 @@ private function collect(?VariableFlow $flow, bool $dead = false): void if (!$flow instanceof VariableControlFlow) { return; } + if ($flow->kind === VariableFlow::LOOP_STATEMENT && $flow->stmt !== null) { + $ownIds = []; + foreach ($flow->ownWrites as $write) { + $ownIds[$write->getId()] = true; + } + foreach ($flow->bindings as $binding) { + $this->loopStatements[$binding->getId()] = $flow->stmt; + $this->ownWriteIds[$binding->getId()] = $ownIds; + } + } if ($flow->kind === VariableFlow::RETURN && $flow->name !== null && $this->returnsByReference) { $this->escapedNames[$flow->name] = true; } @@ -210,11 +236,17 @@ private function liveBefore(?VariableFlow $flow, array $next, VariableFlowContex if (in_array($flow->kind, [VariableFlow::READ, VariableFlow::ESCAPE], true)) { // a by-reference capture aliases the variable - the value it // holds at that point is observable through the alias + if ($flow->kind === VariableFlow::ESCAPE && $this->loopStatements !== []) { + $next = $this->passBindingProbes($next, $flow->name, null); + } return $next + ($this->readKeys[spl_object_id($flow)] ?? []); } if ($flow->write === null || $flow->kind === VariableFlow::DEFINE) { return $next; } + if ($this->loopStatements !== []) { + $next = $this->passBindingProbes($next, $flow->name, $flow->write, $flow->kind === VariableFlow::DISCARD); + } $id = $flow->write->getId(); if ($flow->kind !== VariableFlow::DISCARD) { $this->observeWrite($id, $next); @@ -243,6 +275,34 @@ private function liveBefore(?VariableFlow $flow, array $next, VariableFlowContex if (!$flow instanceof VariableControlFlow) { throw new ShouldNotHappenException(); } + if ($flow->kind === VariableFlow::LOOP_STATEMENT) { + // a binding reusing a variable that is read after the loop: the + // probe follows the variable backwards through the statement; + // surviving to its entry, it is armed to catch the assignment + // before the loop whose value the binding replaces + foreach ($flow->bindings as $binding) { + if (!isset($this->nameKeys[$binding->getVariableName()])) { + continue; + } + foreach (array_keys($this->nameKeys[$binding->getVariableName()]) as $key) { + if (!isset($next[$key])) { + continue; + } + $next[self::bindingProbe($binding, false)] = true; + break; + } + } + $names = $this->liveBefore($flow->children[0], $next, $context); + foreach ($flow->bindings as $binding) { + $probe = self::bindingProbe($binding, false); + if (!isset($names[$probe])) { + continue; + } + unset($names[$probe]); + $names[self::bindingProbe($binding, true)] = true; + } + return $names; + } if ($flow->kind === VariableFlow::ARROW && $flow->arrow !== null) { $outputs = $this->liveBefore($flow->children[1], [], new VariableFlowContext([])); $names = $this->liveBefore($flow->children[0], $outputs, new VariableFlowContext($outputs, uncaught: $outputs)); @@ -362,6 +422,48 @@ private function liveBefore(?VariableFlow $flow, array $next, VariableFlowContex return $next; } + /** + * A probe travels in the live set under a key no read can produce. Armed + * once it has survived its loop statement, it records the assignment (or + * by-reference alias) before the loop whose variable the binding takes + * over; the loop's own writes let it through in either state. + */ + private static function bindingProbe(VariableWrite $binding, bool $armed): string + { + return sprintf("\0%s\0%d%s", $binding->getVariableName(), $binding->getId(), $armed ? "\0" : ''); + } + + /** + * @param array $next + * @return array + */ + private function passBindingProbes(array $next, string $name, ?VariableWrite $write, bool $discard = false): array + { + $prefix = sprintf("\0%s\0", $name); + foreach (array_keys($next) as $key) { + if (!str_starts_with($key, $prefix)) { + continue; + } + $id = substr($key, strlen($prefix)); + $armed = str_ends_with($id, "\0"); + $bindingId = (int) ($armed ? substr($id, 0, -1) : $id); + if ($write !== null && isset($this->ownWriteIds[$bindingId][$write->getId()])) { + continue; + } + if ($armed && !$discard) { + $this->variableOverwritingLoops[$bindingId] = $this->loopStatements[$bindingId]; + } + if ($write === null || $write->isOffsetWrite()) { + // an alias or an offset write keeps the variable - the probe + // carries on to the assignment that created it + continue; + } + unset($next[$key]); + } + + return $next; + } + /** @param int|string $offset */ private static function offsetKey($offset): string { diff --git a/src/Node/VariableWritesNode.php b/src/Node/VariableWritesNode.php index f27a72f4af3..ea21dc84dfd 100644 --- a/src/Node/VariableWritesNode.php +++ b/src/Node/VariableWritesNode.php @@ -4,6 +4,8 @@ use Override; use PhpParser\Node; +use PhpParser\Node\Stmt\For_; +use PhpParser\Node\Stmt\Foreach_; use PhpParser\NodeAbstract; use PHPStan\Node\Variable\VariableWrite; use PHPStan\Type\Type; @@ -29,6 +31,7 @@ final class VariableWritesNode extends NodeAbstract implements VirtualNode * @param array $redundantWriteTypes * @param array $referencedVariableNames * @param array $untrackedVariableNames + * @param array $variableOverwritingLoops */ public function __construct( private Node\FunctionLike $functionLike, @@ -40,6 +43,7 @@ public function __construct( private array $redundantWriteTypes, private array $referencedVariableNames, private array $untrackedVariableNames, + private array $variableOverwritingLoops, private bool $opaque, private bool $allVariableNamesReferenced, ) @@ -123,6 +127,21 @@ public function getRedundantType(VariableWrite $write): ?Type return $this->redundantWriteTypes[$write->getId()] ?? null; } + /** + * The loop statement that binds this write in its head - a foreach key + * or value variable, a for-loop initial assignment - when the variable + * was assigned before the loop and is read after it with no assignment + * in between other than the loop's own bindings and updates: the loop + * takes over a variable still in use, rather than a spent loop variable. + * Null for every other write. + * + * @return Foreach_|For_|null + */ + public function getVariableOverwritingLoop(VariableWrite $write): ?Node\Stmt + { + return $this->variableOverwritingLoops[$write->getId()] ?? null; + } + /** * Whether the body mentions the variable at all: a read, a write, a * statement naming it (global, static, a reference alias), or a construct diff --git a/tests/PHPStan/Node/VariableOverwritingLoopRule.php b/tests/PHPStan/Node/VariableOverwritingLoopRule.php new file mode 100644 index 00000000000..48d692b4009 --- /dev/null +++ b/tests/PHPStan/Node/VariableOverwritingLoopRule.php @@ -0,0 +1,45 @@ + + */ +class VariableOverwritingLoopRule implements Rule +{ + + public function getNodeType(): string + { + return VariableWritesNode::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $errors = []; + foreach ($node->getWrites() as $write) { + $loop = $node->getVariableOverwritingLoop($write); + if ($loop === null) { + continue; + } + + $errors[] = RuleErrorBuilder::message(sprintf( + '%s overwrites $%s.', + $loop instanceof Foreach_ ? 'Foreach' : 'For loop', + $write->getVariableName(), + )) + ->identifier('tests.variableOverwritingLoop') + ->line($loop->getStartLine()) + ->build(); + } + + return $errors; + } + +} diff --git a/tests/PHPStan/Node/VariableOverwritingLoopRuleTest.php b/tests/PHPStan/Node/VariableOverwritingLoopRuleTest.php new file mode 100644 index 00000000000..b5103a7f2fc --- /dev/null +++ b/tests/PHPStan/Node/VariableOverwritingLoopRuleTest.php @@ -0,0 +1,77 @@ + + */ +class VariableOverwritingLoopRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new VariableOverwritingLoopRule(); + } + + public function testRule(): void + { + $this->analyse([__DIR__ . '/data/variable-overwriting-loop.php'], [ + [ + 'Foreach overwrites $x.', + 22, + ], + [ + 'Foreach overwrites $x.', + 68, + ], + [ + 'Foreach overwrites $k.', + 78, + ], + [ + 'Foreach overwrites $x.', + 89, + ], + [ + 'Foreach overwrites $x.', + 97, + ], + [ + 'Foreach overwrites $b.', + 115, + ], + [ + 'Foreach overwrites $x.', + 125, + ], + [ + 'Foreach overwrites $arr.', + 138, + ], + [ + 'For loop overwrites $i.', + 145, + ], + [ + 'For loop overwrites $i.', + 157, + ], + [ + 'For loop overwrites $i.', + 182, + ], + [ + 'Foreach overwrites $x.', + 209, + ], + [ + 'Foreach overwrites $x.', + 218, + ], + ]); + } + +} diff --git a/tests/PHPStan/Node/data/variable-overwriting-loop.php b/tests/PHPStan/Node/data/variable-overwriting-loop.php new file mode 100644 index 00000000000..4bc400288ee --- /dev/null +++ b/tests/PHPStan/Node/data/variable-overwriting-loop.php @@ -0,0 +1,226 @@ + $v) { + } + echo $k; + } + + /** @param string[] $a */ + public function conditionalAssignmentBeforeLoop(array $a, bool $c): void + { + if ($c) { + $x = 'default'; + } + foreach ($a as $x) { + } + echo $x; + } + + /** @param string[] $a */ + public function reassignedInBody(array $a, string $x): void + { + foreach ($a as $x) { + $x = trim($x); + } + echo $x; + } + + /** @param string[] $a */ + public function reassignedAfterLoop(array $a, string $x): void + { + foreach ($a as $x) { + } + $x = 'other'; + echo $x; + } + + /** @param array $a */ + public function listTarget(array $a, string $b, string $c): void + { + foreach ($a as [$b, $c]) { + } + echo $b; + } + + /** @param string[] $a */ + public function referenceBeforeLoop(array $a): void + { + $x = 'x'; + $this->byRef($x); + foreach ($a as $x) { + } + echo $x; + } + + public function byRef(string &$s): void + { + } + + /** @param string[] $a */ + public function offsetWriteBeforeLoop(array $a, array $arr): void + { + $arr[] = 'x'; + foreach ($a as $arr) { + } + echo $arr; + } + + public function forLoop(int $i, int $j): void + { + for ($i = 0; $i < 10; $i++) { + } + echo $i; + + for ($j = 0; $j < 10; $j++) { + } + } + + public function sequentialForLoops(): void + { + for ($i = 0; $i < 10; $i++) { + } + for ($i = 0; $i < 5; $i++) { + } + echo $i; + } + + public function forLoopFlag(int $n): void + { + $found = false; + for ($i = 0; $i < $n; $i++) { + if ($i === 3) { + $found = true; + break; + } + } + if (!$found) { + return; + } + for ($i = 0; $i < $n; $i++) { + echo $i; + } + } + + /** @param array{int, int} $b */ + public function forLoopList(int $i, int $j, array $b): void + { + for ([$i, $j] = $b; $i < 10; $i++) { + } + echo $i; + } + + public function forLoopUpdateOnly(int $i): void + { + for (; $i < 10; $i++) { + } + echo $i; + } + + /** @param string[] $a */ + public function unsetBeforeLoop(array $a, string $x): void + { + unset($x); + foreach ($a as $x) { + } + echo $x; + } + + /** @param string[] $a */ + public function readInLaterOuterIteration(array $outer, array $a): void + { + $x = 'initial'; + foreach ($outer as $o) { + echo $x; + foreach ($a as $x) { + } + } + } + + /** @param string[] $a */ + public function closureBoundary(array $a, string $x): void + { + $f = function () use ($a, $x): void { + foreach ($a as $x) { + } + echo $x; + }; + $f(); + echo $x; + } + +}