Skip to content

Commit 57b07b8

Browse files
VincentLangletphpstan-bot
VincentLanglet
authored and
phpstan-bot
committed
Do not merge dead scope into break exit points when loop body always terminates
- When all paths in a loop body terminate (break/return/throw), the scope returned by getScope() is the unreachable "dead" scope after the always-terminating statement. This dead scope was being merged into the final post-loop scope, polluting it with stale types. - Fix unrolled constant-array foreach: skip dead iterEndScope when body always terminated; when no iteration completed normally, build endScope from only break scopes instead of merging into chainScope. - Fix non-unrolled foreach: when body always terminated, start finalScope from null and build it from only continue/break scopes. - Fix while loop: capture isAlwaysTerminating before filterOutLoopExitPoints; when body always terminated, start break scope from null instead of dead finalScope. - Fix do-while loop: same pattern — start break scope from null when body always terminated. - Fix for loop: same pattern — start break scope from null when body always terminated. - Update for-loop-i-type.php test: immediate break in for($i=1;$i<50) now correctly gives int<1,49> instead of int<1,max> since the dead scope's $i>=50 range no longer leaks through.
1 parent 7604335 commit 57b07b8

3 files changed

Lines changed: 245 additions & 19 deletions

File tree

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 58 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1288,15 +1288,17 @@ public function processStmtNode(
12881288
$bodyScope = $bodyScope->mergeWith($this->polluteScopeWithAlwaysIterableForeach ? $scope->filterByTruthyValue($arrayComparisonExpr) : $scope);
12891289
$storage = $originalStorage;
12901290
$bodyScope = $this->enterForeach($bodyScope, $storage, $originalScope, $stmt, $nodeCallback);
1291-
$finalScopeResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage, $nodeCallback, $context)->filterOutLoopExitPoints();
1292-
$finalScope = $finalScopeResult->getScope();
1291+
$rawFinalScopeResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage, $nodeCallback, $context);
1292+
$foreachBodyAlwaysTerminated = $rawFinalScopeResult->isAlwaysTerminating();
1293+
$finalScopeResult = $rawFinalScopeResult->filterOutLoopExitPoints();
1294+
$finalScope = $foreachBodyAlwaysTerminated ? null : $finalScopeResult->getScope();
12931295
$scopesWithIterableValueType = [];
12941296

12951297
$originalKeyVarExpr = null;
12961298
$continueExitPointHasUnoriginalKeyType = false;
12971299
if ($stmt->keyVar instanceof Variable && is_string($stmt->keyVar->name)) {
12981300
$originalKeyVarExpr = new OriginalForeachKeyExpr($stmt->keyVar->name);
1299-
if ($finalScope->hasExpressionType($originalKeyVarExpr)->yes()) {
1301+
if ($finalScope !== null && $finalScope->hasExpressionType($originalKeyVarExpr)->yes()) {
13001302
$scopesWithIterableValueType[] = $finalScope;
13011303
} else {
13021304
$continueExitPointHasUnoriginalKeyType = true;
@@ -1305,16 +1307,29 @@ public function processStmtNode(
13051307

13061308
foreach ($finalScopeResult->getExitPointsByType(Continue_::class) as $continueExitPoint) {
13071309
$continueScope = $continueExitPoint->getScope();
1308-
$finalScope = $continueScope->mergeWith($finalScope);
1310+
if ($finalScope === null) {
1311+
$finalScope = $continueScope;
1312+
} else {
1313+
$finalScope = $continueScope->mergeWith($finalScope);
1314+
}
13091315
if ($originalKeyVarExpr === null || !$continueScope->hasExpressionType($originalKeyVarExpr)->yes()) {
13101316
$continueExitPointHasUnoriginalKeyType = true;
13111317
continue;
13121318
}
13131319
$scopesWithIterableValueType[] = $continueScope;
13141320
}
13151321
$breakExitPoints = $finalScopeResult->getExitPointsByType(Break_::class);
1316-
foreach ($breakExitPoints as $breakExitPoint) {
1317-
$finalScope = $breakExitPoint->getScope()->mergeWith($finalScope);
1322+
if ($finalScope !== null) {
1323+
foreach ($breakExitPoints as $breakExitPoint) {
1324+
$finalScope = $breakExitPoint->getScope()->mergeWith($finalScope);
1325+
}
1326+
} elseif (count($breakExitPoints) > 0) {
1327+
$finalScope = $breakExitPoints[0]->getScope();
1328+
for ($i = 1, $c = count($breakExitPoints); $i < $c; $i++) {
1329+
$finalScope = $breakExitPoints[$i]->getScope()->mergeWith($finalScope);
1330+
}
1331+
} else {
1332+
$finalScope = $finalScopeResult->getScope();
13181333
}
13191334

13201335
if ($unrolledEndScope !== null) {
@@ -1514,7 +1529,9 @@ public function processStmtNode(
15141529
$bodyScopeMaybeRan = $bodyScope;
15151530
$storage = $originalStorage;
15161531
$bodyScope = $this->processExprNode($stmt, $stmt->cond, $bodyScope, $storage, $nodeCallback, ExpressionContext::createDeep())->getTruthyScope();
1517-
$finalScopeResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage, $nodeCallback, $context)->filterOutLoopExitPoints();
1532+
$rawWhileBodyResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage, $nodeCallback, $context);
1533+
$whileBodyAlwaysTerminated = $rawWhileBodyResult->isAlwaysTerminating();
1534+
$finalScopeResult = $rawWhileBodyResult->filterOutLoopExitPoints();
15181535
$finalScope = $finalScopeResult->getScope()->filterByFalseyValue($stmt->cond);
15191536

15201537
$alwaysIterates = false;
@@ -1532,7 +1549,7 @@ public function processStmtNode(
15321549

15331550
$breakExitPoints = $finalScopeResult->getExitPointsByType(Break_::class);
15341551
if (count($breakExitPoints) > 0) {
1535-
$breakScope = $alwaysIterates ? null : $finalScope;
1552+
$breakScope = $alwaysIterates || $whileBodyAlwaysTerminated ? null : $finalScope;
15361553
foreach ($breakExitPoints as $breakExitPoint) {
15371554
$breakScope = $breakScope === null ? $breakExitPoint->getScope() : $breakScope->mergeWith($breakExitPoint->getScope());
15381555
}
@@ -1610,7 +1627,9 @@ public function processStmtNode(
16101627
}
16111628

16121629
$storage = $originalStorage;
1613-
$bodyScopeResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage, $nodeCallback, $context)->filterOutLoopExitPoints();
1630+
$rawDoWhileBodyResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage, $nodeCallback, $context);
1631+
$doWhileBodyAlwaysTerminated = $rawDoWhileBodyResult->isAlwaysTerminating();
1632+
$bodyScopeResult = $rawDoWhileBodyResult->filterOutLoopExitPoints();
16141633
$bodyScope = $bodyScopeResult->getScope();
16151634
foreach ($bodyScopeResult->getExitPointsByType(Continue_::class) as $continueExitPoint) {
16161635
$bodyScope = $bodyScope->mergeWith($continueExitPoint->getScope());
@@ -1645,7 +1664,7 @@ public function processStmtNode(
16451664

16461665
$breakExitPoints = $bodyScopeResult->getExitPointsByType(Break_::class);
16471666
if (count($breakExitPoints) > 0) {
1648-
$breakScope = $alwaysIterates ? null : $finalScope;
1667+
$breakScope = $alwaysIterates || $doWhileBodyAlwaysTerminated ? null : $finalScope;
16491668
foreach ($breakExitPoints as $breakExitPoint) {
16501669
$breakScope = $breakScope === null ? $breakExitPoint->getScope() : $breakScope->mergeWith($breakExitPoint->getScope());
16511670
}
@@ -1744,7 +1763,9 @@ public function processStmtNode(
17441763
$bodyScope = $this->inferForLoopExpressions($stmt, $lastCondExpr, $bodyScope);
17451764
}
17461765

1747-
$finalScopeResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage, $nodeCallback, $context)->filterOutLoopExitPoints();
1766+
$rawForBodyResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage, $nodeCallback, $context);
1767+
$forBodyAlwaysTerminated = $rawForBodyResult->isAlwaysTerminating();
1768+
$finalScopeResult = $rawForBodyResult->filterOutLoopExitPoints();
17481769
$finalScope = $finalScopeResult->getScope();
17491770
foreach ($finalScopeResult->getExitPointsByType(Continue_::class) as $continueExitPoint) {
17501771
$finalScope = $continueExitPoint->getScope()->mergeWith($finalScope);
@@ -1762,7 +1783,7 @@ public function processStmtNode(
17621783

17631784
$breakExitPoints = $finalScopeResult->getExitPointsByType(Break_::class);
17641785
if (count($breakExitPoints) > 0) {
1765-
$breakScope = $alwaysIterates->yes() ? null : $finalScope;
1786+
$breakScope = $alwaysIterates->yes() || $forBodyAlwaysTerminated ? null : $finalScope;
17661787
foreach ($breakExitPoints as $breakExitPoint) {
17671788
$breakScope = $breakScope === null ? $breakExitPoint->getScope() : $breakScope->mergeWith($breakExitPoint->getScope());
17681789
}
@@ -3921,6 +3942,7 @@ private function tryProcessUnrolledConstantArrayForeach(
39213942
$chainScope = $originalScope;
39223943
$entryScopes = [];
39233944
$breakScopes = [];
3945+
$hasNormalCompletion = false;
39243946
foreach ($keyTypes as $i => $keyType) {
39253947
$valueType = $valueTypes[$i];
39263948
$isOptional = isset($optionalKeys[$i]);
@@ -3965,28 +3987,39 @@ private function tryProcessUnrolledConstantArrayForeach(
39653987
$entryScopes[] = $iterScope;
39663988

39673989
$iterStorage = $originalStorage->duplicate();
3968-
$bodyResult = $this->processStmtNodesInternal(
3990+
$rawBodyResult = $this->processStmtNodesInternal(
39693991
$stmt,
39703992
$stmt->stmts,
39713993
$iterScope,
39723994
$iterStorage,
39733995
new NoopNodeCallback(),
39743996
$context->enterDeep(),
3975-
)->filterOutLoopExitPoints();
3997+
);
3998+
$bodyAlwaysTerminated = $rawBodyResult->isAlwaysTerminating();
3999+
$bodyResult = $rawBodyResult->filterOutLoopExitPoints();
39764000

3977-
$iterEndScope = $bodyResult->getScope();
4001+
$iterEndScope = $bodyAlwaysTerminated ? null : $bodyResult->getScope();
39784002
foreach ($bodyResult->getExitPointsByType(Continue_::class) as $continueExitPoint) {
3979-
$iterEndScope = $iterEndScope->mergeWith($continueExitPoint->getScope());
4003+
if ($iterEndScope === null) {
4004+
$iterEndScope = $continueExitPoint->getScope();
4005+
} else {
4006+
$iterEndScope = $iterEndScope->mergeWith($continueExitPoint->getScope());
4007+
}
39804008
}
39814009
foreach ($bodyResult->getExitPointsByType(Break_::class) as $breakExitPoint) {
39824010
$breakScopes[] = $breakExitPoint->getScope();
39834011
}
39844012

4013+
if ($iterEndScope === null) {
4014+
continue;
4015+
}
4016+
39854017
if ($isOptional) {
39864018
$chainScope = $iterEndScope->mergeWith($chainScope);
39874019
} else {
39884020
$chainScope = $iterEndScope;
39894021
}
4022+
$hasNormalCompletion = true;
39904023
}
39914024

39924025
$bodyScope = $entryScopes[0];
@@ -4002,8 +4035,15 @@ private function tryProcessUnrolledConstantArrayForeach(
40024035
$bodyScope = $bodyScope->mergeWith($chainScope);
40034036
}
40044037

4005-
foreach ($breakScopes as $breakScope) {
4006-
$chainScope = $chainScope->mergeWith($breakScope);
4038+
if ($hasNormalCompletion) {
4039+
foreach ($breakScopes as $breakScope) {
4040+
$chainScope = $chainScope->mergeWith($breakScope);
4041+
}
4042+
} elseif (count($breakScopes) > 0) {
4043+
$chainScope = $breakScopes[0];
4044+
for ($i = 1, $c = count($breakScopes); $i < $c; $i++) {
4045+
$chainScope = $chainScope->mergeWith($breakScopes[$i]);
4046+
}
40074047
}
40084048

40094049
return ['bodyScope' => $bodyScope, 'endScope' => $chainScope];

‎tests/PHPStan/Analyser/nsrt/bug-1946.php‎

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
<?php
2+
3+
declare(strict_types = 1);
4+
5+
namespace Bug1946;
6+
7+
use function PHPStan\Testing\assertType;
8+
9+
function foreachAllBranchesBreakWithAssignment(): void
10+
{
11+
$tag = null;
12+
foreach (["a", "b", "c"] as $tag) {
13+
if ($tag === "a") {
14+
$tag = null;
15+
break;
16+
} else {
17+
$tag = null;
18+
break;
19+
}
20+
}
21+
22+
assertType('null', $tag);
23+
}
24+
25+
function foreachIfElseBreakDifferentTypes(): void
26+
{
27+
$tag = null;
28+
foreach (["a", "b", "c"] as $tag) {
29+
if ($tag === "a") {
30+
$tag = 1;
31+
break;
32+
} else {
33+
$tag = 2;
34+
break;
35+
}
36+
}
37+
38+
assertType('1|2', $tag);
39+
}
40+
41+
function foreachAllBranchesReturn(): void
42+
{
43+
$tag = null;
44+
foreach (["a", "b", "c"] as $tag) {
45+
if ($tag === "a") {
46+
$tag = null;
47+
return;
48+
} else {
49+
$tag = null;
50+
return;
51+
}
52+
}
53+
54+
assertType('null', $tag);
55+
}
56+
57+
function foreachOnlyIfBreaksNoElse(): void
58+
{
59+
$tag = null;
60+
foreach (["a", "b", "c"] as $tag) {
61+
if ($tag === "a") {
62+
$tag = null;
63+
break;
64+
}
65+
}
66+
67+
assertType("'c'|null", $tag);
68+
}
69+
70+
/**
71+
* @param string[] $arr
72+
*/
73+
function foreachNonConstantArrayAllBreak(array $arr): void
74+
{
75+
$tag = null;
76+
foreach ($arr as $tag) {
77+
if ($tag === "a") {
78+
$tag = null;
79+
break;
80+
} else {
81+
$tag = null;
82+
break;
83+
}
84+
}
85+
86+
assertType('null', $tag);
87+
}
88+
89+
function foreachElseIfAllBreak(): void
90+
{
91+
$tag = null;
92+
foreach (["a", "b", "c"] as $tag) {
93+
if ($tag === "a") {
94+
$tag = 1;
95+
break;
96+
} elseif ($tag === "b") {
97+
$tag = 2;
98+
break;
99+
} else {
100+
$tag = 3;
101+
break;
102+
}
103+
}
104+
105+
assertType('1|2|3', $tag);
106+
}
107+
108+
function foreachBreakWithContinue(): void
109+
{
110+
$tag = null;
111+
foreach (["a", "b", "c"] as $tag) {
112+
if ($tag === "a") {
113+
$tag = null;
114+
continue;
115+
} else {
116+
$tag = null;
117+
break;
118+
}
119+
}
120+
121+
assertType('null', $tag);
122+
}
123+
124+
function whileAllBreakMayNotIterate(): void
125+
{
126+
$x = 'hello';
127+
while (rand(0, 1)) {
128+
if (rand(0, 1)) {
129+
$x = 1;
130+
break;
131+
} else {
132+
$x = 2;
133+
break;
134+
}
135+
}
136+
137+
assertType("1|2|'hello'", $x);
138+
}
139+
140+
function whileTrueAllBreak(): void
141+
{
142+
$x = 'hello';
143+
while (true) {
144+
if (rand(0, 1)) {
145+
$x = 1;
146+
break;
147+
} else {
148+
$x = 2;
149+
break;
150+
}
151+
}
152+
153+
assertType('1|2', $x);
154+
}
155+
156+
function doWhileAllBreak(): void
157+
{
158+
$x = 'hello';
159+
do {
160+
if (rand(0, 1)) {
161+
$x = 1;
162+
break;
163+
} else {
164+
$x = 2;
165+
break;
166+
}
167+
} while (rand(0, 1));
168+
169+
assertType('1|2', $x);
170+
}
171+
172+
function forAllBreakAlwaysIterates(): void
173+
{
174+
$x = 'hello';
175+
for ($i = 0; $i < 10; $i++) {
176+
if (rand(0, 1)) {
177+
$x = 1;
178+
break;
179+
} else {
180+
$x = 2;
181+
break;
182+
}
183+
}
184+
185+
assertType('1|2', $x);
186+
}

‎tests/PHPStan/Analyser/nsrt/for-loop-i-type.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public function doLOrem() {
5959
break;
6060
}
6161

62-
assertType('int<1, max>', $i);
62+
assertType('int<1, 49>', $i);
6363
}
6464

6565
}

0 commit comments

Comments
 (0)