diff --git a/src/Rules/ForLoop/OverwriteVariablesWithForLoopInitRule.php b/src/Rules/ForLoop/OverwriteVariablesWithForLoopInitRule.php index f710474e..1e1842e6 100644 --- a/src/Rules/ForLoop/OverwriteVariablesWithForLoopInitRule.php +++ b/src/Rules/ForLoop/OverwriteVariablesWithForLoopInitRule.php @@ -3,74 +3,45 @@ namespace PHPStan\Rules\ForLoop; use PhpParser\Node; -use PhpParser\Node\Expr; -use PhpParser\Node\Expr\Assign; use PhpParser\Node\Stmt\For_; use PHPStan\Analyser\Scope; -use PHPStan\Rules\IdentifierRuleError; +use PHPStan\Node\VariableWritesNode; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; -use function is_string; use function sprintf; /** - * @implements Rule + * @implements Rule */ class OverwriteVariablesWithForLoopInitRule implements Rule { public function getNodeType(): string { - return For_::class; + return VariableWritesNode::class; } public function processNode(Node $node, Scope $scope): array { + if ($node->isOpaque()) { + return []; + } + $errors = []; - foreach ($node->init as $expr) { - if (!($expr instanceof Assign)) { + foreach ($node->getWrites() as $write) { + // only a for loop that takes over a variable assigned before the loop + // and read after it - reusing a spent loop variable is harmless + $loop = $node->getVariableOverwritingLoop($write); + if (!$loop instanceof For_) { continue; } - foreach ($this->checkValueVar($scope, $expr->var) as $error) { - $errors[] = $error; - } - } - - return $errors; - } - - /** - * @return list - */ - private function checkValueVar(Scope $scope, Expr $expr): array - { - $errors = []; - if ( - $expr instanceof Node\Expr\Variable - && is_string($expr->name) - && $scope->hasVariableType($expr->name)->yes() - ) { - $errors[] = RuleErrorBuilder::message(sprintf('For loop initial assignment overwrites variable $%s.', $expr->name)) + $errors[] = RuleErrorBuilder::message(sprintf('For loop initial assignment overwrites variable $%s.', $write->getVariableName())) ->identifier('for.variableOverwrite') + ->line($loop->getStartLine()) ->build(); } - if ( - $expr instanceof Node\Expr\List_ - || $expr instanceof Node\Expr\Array_ - ) { - foreach ($expr->items as $item) { - if ($item === null) { - continue; - } - - foreach ($this->checkValueVar($scope, $item->value) as $error) { - $errors[] = $error; - } - } - } - return $errors; } diff --git a/src/Rules/ForeachLoop/OverwriteVariablesWithForeachRule.php b/src/Rules/ForeachLoop/OverwriteVariablesWithForeachRule.php index 0cf620c3..6268e25b 100644 --- a/src/Rules/ForeachLoop/OverwriteVariablesWithForeachRule.php +++ b/src/Rules/ForeachLoop/OverwriteVariablesWithForeachRule.php @@ -3,75 +3,49 @@ namespace PHPStan\Rules\ForeachLoop; use PhpParser\Node; -use PhpParser\Node\Expr; use PhpParser\Node\Stmt\Foreach_; use PHPStan\Analyser\Scope; -use PHPStan\Rules\IdentifierRuleError; +use PHPStan\Node\Variable\VariableWrite; +use PHPStan\Node\VariableWritesNode; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; -use function is_string; use function sprintf; /** - * @implements Rule + * @implements Rule */ class OverwriteVariablesWithForeachRule implements Rule { public function getNodeType(): string { - return Foreach_::class; + return VariableWritesNode::class; } public function processNode(Node $node, Scope $scope): array { - $errors = []; - if ( - $node->keyVar instanceof Node\Expr\Variable - && is_string($node->keyVar->name) - && $scope->hasVariableType($node->keyVar->name)->yes() - ) { - $errors[] = RuleErrorBuilder::message(sprintf('Foreach overwrites $%s with its key variable.', $node->keyVar->name)) - ->identifier('foreach.keyOverwrite') - ->build(); - } - - foreach ($this->checkValueVar($scope, $node->valueVar) as $error) { - $errors[] = $error; + if ($node->isOpaque()) { + return []; } - return $errors; - } - - /** - * @return list - */ - private function checkValueVar(Scope $scope, Expr $expr): array - { $errors = []; - if ( - $expr instanceof Node\Expr\Variable - && is_string($expr->name) - && $scope->hasVariableType($expr->name)->yes() - ) { - $errors[] = RuleErrorBuilder::message(sprintf('Foreach overwrites $%s with its value variable.', $expr->name)) - ->identifier('foreach.valueOverwrite') - ->build(); - } - - if ( - $expr instanceof Node\Expr\List_ - || $expr instanceof Node\Expr\Array_ - ) { - foreach ($expr->items as $item) { - if ($item === null) { - continue; - } - - foreach ($this->checkValueVar($scope, $item->value) as $error) { - $errors[] = $error; - } + foreach ($node->getWrites() as $write) { + // only a foreach that takes over a variable assigned before the loop + // and read after it - reusing a spent loop variable is harmless + $loop = $node->getVariableOverwritingLoop($write); + if (!$loop instanceof Foreach_) { + continue; } + + $isKey = $write->getKind() === VariableWrite::KIND_FOREACH_KEY; + $errors[] = RuleErrorBuilder::message(sprintf( + 'Foreach overwrites $%s with its %s variable.', + $write->getVariableName(), + $isKey ? 'key' : 'value', + )) + ->identifier($isKey ? 'foreach.keyOverwrite' : 'foreach.valueOverwrite') + ->line($loop->getStartLine()) + ->build(); } return $errors; diff --git a/tests/Rules/ForLoop/OverwriteVariablesWithForLoopInitRuleTest.php b/tests/Rules/ForLoop/OverwriteVariablesWithForLoopInitRuleTest.php index c15303e7..23311c75 100644 --- a/tests/Rules/ForLoop/OverwriteVariablesWithForLoopInitRuleTest.php +++ b/tests/Rules/ForLoop/OverwriteVariablesWithForLoopInitRuleTest.php @@ -25,39 +25,61 @@ public function testRule(): void ], [ 'For loop initial assignment overwrites variable $i.', - 20, + 21, ], [ 'For loop initial assignment overwrites variable $j.', - 20, + 21, ], [ 'For loop initial assignment overwrites variable $i.', - 24, + 26, ], [ 'For loop initial assignment overwrites variable $i.', - 35, + 38, ], [ 'For loop initial assignment overwrites variable $j.', - 35, + 38, ], [ 'For loop initial assignment overwrites variable $i.', - 39, + 43, ], [ 'For loop initial assignment overwrites variable $j.', - 39, + 43, ], [ 'For loop initial assignment overwrites variable $i.', - 50, + 55, ], [ 'For loop initial assignment overwrites variable $i.', - 54, + 60, + ], + ]); + } + + public function testLoopVariableReuse(): void + { + $this->analyse([__DIR__ . '/data/for-reuse.php'], [ + [ + 'For loop initial assignment overwrites variable $i.', + 61, + ], + [ + 'For loop initial assignment overwrites variable $i.', + 70, + ], + [ + 'For loop initial assignment overwrites variable $i.', + 78, + ], + [ + 'For loop initial assignment overwrites variable $i.', + 87, ], ]); } diff --git a/tests/Rules/ForLoop/data/data.php b/tests/Rules/ForLoop/data/data.php index 211e4014..39d24cfd 100644 --- a/tests/Rules/ForLoop/data/data.php +++ b/tests/Rules/ForLoop/data/data.php @@ -13,6 +13,7 @@ public function simple(int $i): void for($j = 0; $j < 10; ++$j){ } + echo $i, $j; } public function multi(int $i, int $j): void @@ -20,10 +21,12 @@ public function multi(int $i, int $j): void for($i = 0, $j = 0; $i < 10; ++$i){ } + echo $i, $j; for($i = 0, $k = 0; $i < 10; ++$i){ } + echo $i, $k; for($k = 0, $l = 0; $k < 10; ++$k){ @@ -35,10 +38,12 @@ public function list(int $i, int $j, array $b): void for(list($i, $j) = $b; $i < 10; ++$i){ } + echo $i, $j; for(list($i, list($j, $k)) = $b; $i < 10; ++$i){ } + echo $i, $j; for(list($k, list($l, $m)) = $b; $k < 10; ++$k){ @@ -50,13 +55,15 @@ public function array(int $i, array $b): void for([$i, $j] = $b; $i < 10; ++$i){ } + echo $i, $j; for([$i, [$j, $k]] = $b; $i < 10; ++$i){ } + echo $i; for([$k, [$l, $m]] = $b; $k < 10; ++$k){ } } -} \ No newline at end of file +} diff --git a/tests/Rules/ForLoop/data/for-reuse.php b/tests/Rules/ForLoop/data/for-reuse.php new file mode 100644 index 00000000..557d1054 --- /dev/null +++ b/tests/Rules/ForLoop/data/for-reuse.php @@ -0,0 +1,92 @@ +analyse([__DIR__ . '/data/foreach-reuse.php'], [ + [ + 'Foreach overwrites $x with its value variable.', + 86, + ], + [ + 'Foreach overwrites $x with its value variable.', + 99, + ], + [ + 'Foreach overwrites $x with its value variable.', + 110, + ], + ]); + } + + public function testBug9940(): void + { + $this->analyse([__DIR__ . '/data/bug-9940.php'], []); + } + } diff --git a/tests/Rules/ForeachLoop/data/bug-9940.php b/tests/Rules/ForeachLoop/data/bug-9940.php new file mode 100644 index 00000000..6f200a10 --- /dev/null +++ b/tests/Rules/ForeachLoop/data/bug-9940.php @@ -0,0 +1,26 @@ + */ +function doSomething():array { + return []; +} diff --git a/tests/Rules/ForeachLoop/data/foreach-reuse.php b/tests/Rules/ForeachLoop/data/foreach-reuse.php new file mode 100644 index 00000000..147a0bb1 --- /dev/null +++ b/tests/Rules/ForeachLoop/data/foreach-reuse.php @@ -0,0 +1,115 @@ + $x) { + echo $k, $x; + } + foreach ($b as $k => $x) { + echo $k, $x; + } + } + + /** + * @param string[] $catches + * @param string[] $exceptions + */ + public function definednessProvedByFlag(array $catches, array $exceptions): void + { + $hasGeneralCatch = false; + foreach ($catches as $catch) { + if ($catch === 'x') { + $hasGeneralCatch = true; + break; + } + } + if (!$hasGeneralCatch) { + return; + } + + foreach ($exceptions as $exception) { + foreach ($catches as $catch) { + echo $exception, $catch; + } + } + } + + /** @param string[] $a */ + public function notReadAfterLoop(array $a, string $x): void + { + echo $x; + foreach ($a as $x) { + echo $x; + } + } + + /** @param string[] $a */ + public function freshVariableReadAfterLoop(array $a): void + { + foreach ($a as $x) { + } + echo $x; + } + + /** @param string[] $a */ + public function reassignedAfterLoop(array $a, string $x): void + { + echo $x; + foreach ($a as $x) { + } + $x = 'other'; + echo $x; + } + + /** + * @param string[] $outer + * @param string[] $inner + */ + public function outerLoopVariableClobbered(array $outer, array $inner): void + { + foreach ($outer as $x) { + foreach ($inner as $x) { + echo $x; + } + echo $x; + } + } + + /** @param string[] $a */ + public function conditionallyAssignedBeforeLoop(array $a, bool $c): void + { + if ($c) { + $x = 'default'; + } + foreach ($a as $x) { + } + echo $x; + } + + /** @param string[] $a */ + public function readInLaterIterationOfOuterLoop(array $outer, array $a): void + { + $x = 'initial'; + foreach ($outer as $o) { + echo $o, $x; + foreach ($a as $x) { + } + } + } + +} diff --git a/tests/Rules/ForeachLoop/data/foreach.php b/tests/Rules/ForeachLoop/data/foreach.php index da797ddb..e45a4673 100644 --- a/tests/Rules/ForeachLoop/data/foreach.php +++ b/tests/Rules/ForeachLoop/data/foreach.php @@ -14,6 +14,7 @@ public function doFoo(array $a, array $b, string $str) foreach ($a as $str) { } + echo $str; foreach ($a as $val) { foreach ($b as $var) { @@ -26,12 +27,14 @@ public function doBar(array $a, string $b, string $d) { foreach ($a as [$b, $c, [$d, $e]]) { } + echo $b, $d; } public function doBaz(array $a, string $b, string $d) { foreach ($a as list($b, $c, list($d, $e))) { } + echo $b, $d; } public function doLorem(array $a, string $b) { @@ -41,6 +44,7 @@ public function doLorem(array $a, string $b) { foreach ($a as $c => $val) { } + echo $b, $c; } }