Report loop variable overwrites only for variables still in use

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<For_>
+ * @implements Rule<VariableWritesNode>
  */
 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<IdentifierRuleError>
-	 */
-	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;
 	}
 

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<Foreach_>
+ * @implements Rule<VariableWritesNode>
  */
 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<IdentifierRuleError>
-	 */
-	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;

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,
 			],
 		]);
 	}

tests/Rules/ForLoop/data/data.php

@@ -13,17 +13,20 @@ public function simple(int $i): void
 		for($j = 0; $j < 10; ++$j){
 
 		}
+		echo $i, $j;
 	}
 
 	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){
 
 		}
 	}
-}
+}