From 4d8bf50ddb113a25b9cc00ad0b3c2817c6494cad Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 27 Feb 2026 15:12:33 +0000 Subject: [PATCH] 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 https://github.com/phpstan/phpstan/issues/5919 --- src/Analyser/NodeScopeResolver.php | 65 +++++++++++++------ .../Variables/DefinedVariableRuleTest.php | 10 +++ .../PHPStan/Rules/Variables/data/bug-5919.php | 59 +++++++++++++++++ 3 files changed, 113 insertions(+), 21 deletions(-) create mode 100644 tests/PHPStan/Rules/Variables/data/bug-5919.php diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 86f616953b..90bb01f3ef 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/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); + } } } diff --git a/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php b/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php index 112a57d4cc..3fd88eaa29 100644 --- a/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php +++ b/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'], []); + } + } diff --git a/tests/PHPStan/Rules/Variables/data/bug-5919.php b/tests/PHPStan/Rules/Variables/data/bug-5919.php new file mode 100644 index 0000000000..11c20eb9cf --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-5919.php @@ -0,0 +1,59 @@ +