Fix variable definedness in always-iterating loops with try/catch

- In while(true), do...while(true), and for(;;) loops, when the only exit
  path is through break, the scope after the loop should come exclusively
  from break exit points, not from the unreachable "condition becomes false" path
- Previously, the normal-flow scope (e.g. from a catch block where the variable
  was not assigned) was merged with break exit point scopes, degrading variable
  certainty from Yes to Maybe
- New regression test in tests/PHPStan/Rules/Variables/data/bug-5919.php

Closes phpstan/phpstan#5919

Author: github-actions[bot]

src/Analyser/NodeScopeResolver.php

@@ -1520,8 +1520,15 @@ private function processStmtNode(
 			}
 
 			$breakExitPoints = $finalScopeResult->getExitPointsByType(Break_::class);
-			foreach ($breakExitPoints as $breakExitPoint) {
-				$finalScope = $finalScope->mergeWith($breakExitPoint->getScope());
+			if ($alwaysIterates && count($breakExitPoints) > 0) {
+				$finalScope = $breakExitPoints[0]->getScope();
+				for ($i = 1, $breakExitPointsCount = count($breakExitPoints); $i < $breakExitPointsCount; $i++) {
+					$finalScope = $finalScope->mergeWith($breakExitPoints[$i]->getScope());
+				}
+			} else {
+				foreach ($breakExitPoints as $breakExitPoint) {
+					$finalScope = $finalScope->mergeWith($breakExitPoint->getScope());
+				}
 			}
 
 			$isIterableAtLeastOnce = $beforeCondBooleanType->isTrue()->yes();
@@ -1627,8 +1634,16 @@ private function processStmtNode(
 			} else {
 				$this->processExprNode($stmt, $stmt->cond, $bodyScope, $storage, $nodeCallback, ExpressionContext::createDeep());
 			}
-			foreach ($bodyScopeResult->getExitPointsByType(Break_::class) as $breakExitPoint) {
-				$finalScope = $breakExitPoint->getScope()->mergeWith($finalScope);
+			$breakExitPoints = $bodyScopeResult->getExitPointsByType(Break_::class);
+			if ($alwaysIterates && count($breakExitPoints) > 0) {
+				$finalScope = $breakExitPoints[0]->getScope();
+				for ($i = 1, $breakExitPointsCount = count($breakExitPoints); $i < $breakExitPointsCount; $i++) {
+					$finalScope = $finalScope->mergeWith($breakExitPoints[$i]->getScope());
+				}
+			} else {
+				foreach ($breakExitPoints as $breakExitPoint) {
+					$finalScope = $breakExitPoint->getScope()->mergeWith($finalScope);
+				}
 			}
 
 			return new InternalStatementResult(
@@ -1739,26 +1754,34 @@ private function processStmtNode(
 				$finalScope = $finalScope->filterByFalseyValue($lastCondExpr);
 			}
 
-			foreach ($finalScopeResult->getExitPointsByType(Break_::class) as $breakExitPoint) {
-				$finalScope = $breakExitPoint->getScope()->mergeWith($finalScope);
-			}
-
-			if ($isIterableAtLeastOnce->no() || $finalScopeResult->isAlwaysTerminating()) {
-				if ($this->polluteScopeWithLoopInitialAssignments) {
-					$finalScope = $initScope;
-				} else {
-					$finalScope = $scope;
+			$breakExitPoints = $finalScopeResult->getExitPointsByType(Break_::class);
+			if ($alwaysIterates->yes() && count($breakExitPoints) > 0) {
+				$finalScope = $breakExitPoints[0]->getScope();
+				for ($i = 1, $breakExitPointsCount = count($breakExitPoints); $i < $breakExitPointsCount; $i++) {
+					$finalScope = $finalScope->mergeWith($breakExitPoints[$i]->getScope());
+				}
+			} else {
+				foreach ($breakExitPoints as $breakExitPoint) {
+					$finalScope = $breakExitPoint->getScope()->mergeWith($finalScope);
 				}
 
-			} elseif ($isIterableAtLeastOnce->maybe()) {
-				if ($this->polluteScopeWithLoopInitialAssignments) {
-					$finalScope = $finalScope->mergeWith($initScope);
+				if ($isIterableAtLeastOnce->no() || $finalScopeResult->isAlwaysTerminating()) {
+					if ($this->polluteScopeWithLoopInitialAssignments) {
+						$finalScope = $initScope;
+					} else {
+						$finalScope = $scope;
+					}
+
+				} elseif ($isIterableAtLeastOnce->maybe()) {
+					if ($this->polluteScopeWithLoopInitialAssignments) {
+						$finalScope = $finalScope->mergeWith($initScope);
+					} else {
+						$finalScope = $finalScope->mergeWith($scope);
+					}
 				} else {
-					$finalScope = $finalScope->mergeWith($scope);
-				}
-			} else {
-				if (!$this->polluteScopeWithLoopInitialAssignments) {
-					$finalScope = $finalScope->mergeWith($scope);
+					if (!$this->polluteScopeWithLoopInitialAssignments) {
+						$finalScope = $finalScope->mergeWith($scope);
+					}
 				}
 			}
 

tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php

@@ -1257,4 +1257,14 @@ public function testBug12944(): void
 		$this->analyse([__DIR__ . '/data/bug-12944.php'], []);
 	}
 
+	public function testBug5919(): void
+	{
+		$this->cliArgumentsVariablesRegistered = true;
+		$this->polluteScopeWithLoopInitialAssignments = true;
+		$this->checkMaybeUndefinedVariables = true;
+		$this->polluteScopeWithAlwaysIterableForeach = true;
+
+		$this->analyse([__DIR__ . '/data/bug-5919.php'], []);
+	}
+
 }

tests/PHPStan/Rules/Variables/data/bug-5919.php

@@ -0,0 +1,59 @@
+<?php declare(strict_types = 1);
+
+namespace Bug5919;
+
+/**
+ * @return mixed[]
+ */
+function queryApi(): array
+{
+	return [5];
+}
+
+function testWhileTrueWithTryCatch(): void
+{
+	while (true) {
+		try {
+			$s = queryApi();
+			break;
+		} catch (\Exception $e) {
+			if (rand(0, 1)) {
+				throw $e;
+			}
+		}
+	}
+
+	var_dump(count($s));
+}
+
+function testDoWhileWithTryCatch(): void
+{
+	do {
+		try {
+			$s = queryApi();
+			break;
+		} catch (\Exception $e) {
+			if (rand(0, 1)) {
+				throw $e;
+			}
+		}
+	} while (true);
+
+	var_dump(count($s));
+}
+
+function testForEverWithTryCatch(): void
+{
+	for (;;) {
+		try {
+			$s = queryApi();
+			break;
+		} catch (\Exception $e) {
+			if (rand(0, 1)) {
+				throw $e;
+			}
+		}
+	}
+
+	var_dump(count($s));
+}