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; + } + +}