Skip to content

Commit fbdb66c

Browse files
phpstan-botclaude
authored and
ondrejmirtes
committed
Decide a loop pass's dead back edge with a walk converging nested loops when the body is not replayable
- Loop convergence passes walk the body in deep context, where a nested loop walks only its first iteration. Its unconverged types can make a branch look dead (e.g. `array_key_exists($card, $seen)` with `$seen` still `[]`), so the pass's back edge came out null and the convergence stopped after the first pass - the counter stayed `0` and `$i < 10` was reported as always true. - When a pass finds the back edge dead and the body is not replayable (holds a nested loop, per NodeScopeResolver::isReplayableConvergenceBody()), the body is re-walked once with the top-level context (noop callback, duplicated storage) and that walk's back edge decides. - Applied to every convergence loop that breaks on a dead back edge: ForHandler, WhileHandler, DoWhileHandler, ForeachHandler and the unsealed-shape loop of ForeachHandler::tryProcessUnrolledConstantArrayForeach(). - Mirrored in turbo-ext: shared ptlh::convergedPassBackEdgeScope() helper in LoopHandlerCalls.h, used by ForHandler.cpp, WhileHandler.cpp, DoWhileHandler.cpp and ForeachHandler.cpp (both sites). - Tests cover for/while/do-while/foreach/unsealed-constant-array outer loops and foreach/while/for/do-while inner loops. The while, do-while and foreach outer loops were already wrong in 2.2.17, and so was the inner-loop variant. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 4ef9ab7 commit fbdb66c

11 files changed

Lines changed: 262 additions & 0 deletions

File tree

‎src/Analyser/StmtHandler/DoWhileHandler.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,11 @@ public function processStmt(
9595
$previousPass = $passRecord;
9696
$previousPassStorage = $storage;
9797
$backEdgeScope = $bodyScopeResult->getLoopBackEdgeScope();
98+
if ($backEdgeScope === null && !$bodyIsReplayable) {
99+
// a nested loop walks only its first iteration in a pass - its unconverged
100+
// types can make the back edge look dead, so a walk converging it decides
101+
$backEdgeScope = $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage->duplicate(), new NoopNodeCallback(), $context->withoutTemplateArgumentResolution())->filterOutLoopExitPoints()->getLoopBackEdgeScope();
102+
}
98103
$finalScope = $backEdgeScope === null ? $finalScope : $backEdgeScope->mergeWith($finalScope);
99104
foreach ($bodyScopeResult->getExitPointsByType(Break_::class) as $breakExitPoint) {
100105
$finalScope = $breakExitPoint->getScope()->mergeWith($finalScope);

‎src/Analyser/StmtHandler/ForHandler.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,7 @@ public function processStmt(
202202
// entry state differs from the previous pass's (StatementsHandler::processLoopPassStmtNodes())
203203
$previousPass = null;
204204
$previousPassStorage = null;
205+
$bodyIsReplayable = $nodeScopeResolver->isReplayableConvergenceBody($stmt, $stmt->stmts);
205206
do {
206207
$prevScope = $bodyScope;
207208
$storage = $originalStorage->duplicate();
@@ -231,6 +232,11 @@ public function processStmt(
231232
$previousPassStorage = $storage;
232233
$passFlows[] = $bodyScopeResult->getVariableFlow();
233234
$backEdgeScope = $bodyScopeResult->getLoopBackEdgeScope();
235+
if ($backEdgeScope === null && !$bodyIsReplayable) {
236+
// a nested loop walks only its first iteration in a pass - its unconverged
237+
// types can make the back edge look dead, so a walk converging it decides
238+
$backEdgeScope = $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage->duplicate(), new NoopNodeCallback(), $context->withoutTemplateArgumentResolution())->filterOutLoopExitPoints()->getLoopBackEdgeScope();
239+
}
234240
if ($backEdgeScope === null) {
235241
$bodyScope = $prevScope;
236242
break;

‎src/Analyser/StmtHandler/ForeachHandler.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,11 @@ static function () use ($condResult, $emptyArrayType): Type {
254254
$previousPass = $passRecord;
255255
$previousPassStorage = $storage;
256256
$backEdgeScope = $bodyScopeResult->getLoopBackEdgeScope();
257+
if ($backEdgeScope === null && !$bodyIsReplayable) {
258+
// a nested loop walks only its first iteration in a pass - its unconverged
259+
// types can make the back edge look dead, so a walk converging it decides
260+
$backEdgeScope = $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage->duplicate(), new NoopNodeCallback(), $context->withoutTemplateArgumentResolution())->filterOutLoopExitPoints()->getLoopBackEdgeScope();
261+
}
257262
if ($backEdgeScope === null) {
258263
$bodyScope = $prevScope;
259264
break;
@@ -868,6 +873,11 @@ private function tryProcessUnrolledConstantArrayForeach(
868873
$iterBodyScope = $this->enterForeach($nodeScopeResolver, $iterBodyScope, $iterStorage, $originalScope, $stmt, $iterateeType, $nativeIterateeType, new NoopNodeCallback());
869874
$iterBodyScopeResult = $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $iterBodyScope, $iterStorage, new NoopNodeCallback(), $context->enterDeep()->withoutTemplateArgumentResolution())->filterOutLoopExitPoints();
870875
$backEdgeScope = $iterBodyScopeResult->getLoopBackEdgeScope();
876+
if ($backEdgeScope === null && !$nodeScopeResolver->isReplayableConvergenceBody($stmt, $stmt->stmts)) {
877+
// a nested loop walks only its first iteration in a pass - its unconverged
878+
// types can make the back edge look dead, so a walk converging it decides
879+
$backEdgeScope = $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $iterBodyScope, $iterStorage->duplicate(), new NoopNodeCallback(), $context->withoutTemplateArgumentResolution())->filterOutLoopExitPoints()->getLoopBackEdgeScope();
880+
}
871881
foreach ($iterBodyScopeResult->getExitPointsByType(Break_::class) as $breakExitPoint) {
872882
$endScope = $endScope->mergeWith($breakExitPoint->getScope());
873883
}

‎src/Analyser/StmtHandler/WhileHandler.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,11 @@ public function processStmt(
123123
$previousPass = $passRecord;
124124
$previousPassStorage = $storage;
125125
$backEdgeScope = $bodyScopeResult->getLoopBackEdgeScope();
126+
if ($backEdgeScope === null && !$bodyIsReplayable) {
127+
// a nested loop walks only its first iteration in a pass - its unconverged
128+
// types can make the back edge look dead, so a walk converging it decides
129+
$backEdgeScope = $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage->duplicate(), new NoopNodeCallback(), $context->withoutTemplateArgumentResolution())->filterOutLoopExitPoints()->getLoopBackEdgeScope();
130+
}
126131
} finally {
127132
$scope->popExpressionResultStorage();
128133
}

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

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

‎tests/PHPStan/Rules/Comparison/NumberComparisonOperatorsConstantConditionRuleTest.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,11 @@ public function testBug15243(): void
326326
$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-15243.php'], []);
327327
}
328328

329+
public function testBug15420(): void
330+
{
331+
$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-15420.php'], []);
332+
}
333+
329334
public function testShiftLeftUnboundedRange(): void
330335
{
331336
$this->analyse([__DIR__ . '/../../Analyser/nsrt/shift-left-unbounded-range.php'], []);

‎turbo-ext/src/DoWhileHandler.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,10 @@ class DoWhileHandler
130130
if (UNEXPECTED(bodyScopeResult.isUndef())) return;
131131
zv::Val backEdgeScope = pt_internal_statement_result_loop_back_edge_scope(bodyScopeResult.raw());
132132
if (UNEXPECTED(backEdgeScope.isUndef())) return;
133+
if (backEdgeScope.isNull() && !bodyIsReplayable) {
134+
backEdgeScope = ptlh::convergedPassBackEdgeScope(nodeScopeResolver, stmt, stmtsHold.raw(), bodyScope.raw(), storage.raw(), context);
135+
if (UNEXPECTED(backEdgeScope.isUndef())) return;
136+
}
133137
if (!backEdgeScope.isNull()) {
134138
if (UNEXPECTED(!ptlh::otherMergeWith(backEdgeScope.raw(), finalScope))) return;
135139
}

‎turbo-ext/src/ForHandler.cpp‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,12 @@ class ForHandler
219219
// entry state differs from the previous pass's (StatementsHandler::processLoopPassStmtNodes())
220220
zv::Val previousPass = zv::Val::null();
221221
zv::Val previousPassStorage = zv::Val::null();
222+
bool bodyIsReplayable;
223+
{
224+
zval *bodyStmts = ptsh::readNodeProperty(pt_fh_stmts_site, stmt, PT_LC("stmts"));
225+
if (UNEXPECTED(bodyStmts == NULL)) return zv::Val();
226+
if (UNEXPECTED(!pt_node_scope_resolver_is_replayable_convergence_body(nodeScopeResolver, stmt, bodyStmts, bodyIsReplayable))) return zv::Val();
227+
}
222228
do {
223229
zv::Val prevScope = zv::Val::copyOf(bodyScope.ref());
224230
zv::Val storage = pt_expression_result_storage_duplicate(originalStorage);
@@ -279,6 +285,10 @@ class ForHandler
279285
}
280286
zv::Val backEdgeScope = pt_internal_statement_result_loop_back_edge_scope(bodyScopeResult.raw());
281287
if (UNEXPECTED(backEdgeScope.isUndef())) return;
288+
if (backEdgeScope.isNull() && !bodyIsReplayable) {
289+
backEdgeScope = ptlh::convergedPassBackEdgeScope(nodeScopeResolver, stmt, stmtsHold.raw(), bodyScope.raw(), storage.raw(), context);
290+
if (UNEXPECTED(backEdgeScope.isUndef())) return;
291+
}
282292
if (backEdgeScope.isNull()) {
283293
bodyScope = std::move(prevScope);
284294
brokeOut = true;

‎turbo-ext/src/ForeachHandler.cpp‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -543,6 +543,10 @@ class ForeachHandler
543543
if (UNEXPECTED(bodyScopeResult.isUndef())) return;
544544
zv::Val backEdgeScope = pt_internal_statement_result_loop_back_edge_scope(bodyScopeResult.raw());
545545
if (UNEXPECTED(backEdgeScope.isUndef())) return;
546+
if (backEdgeScope.isNull() && !bodyIsReplayable) {
547+
backEdgeScope = ptlh::convergedPassBackEdgeScope(nodeScopeResolver, stmt, parts.stmts.raw(), bodyScope.raw(), storage.raw(), context);
548+
if (UNEXPECTED(backEdgeScope.isUndef())) return;
549+
}
546550
if (backEdgeScope.isNull()) {
547551
bodyScope = std::move(prevScope);
548552
brokeOut = true;
@@ -1724,6 +1728,14 @@ class ForeachHandler
17241728
if (UNEXPECTED(iterBodyScopeResult.isUndef())) return false;
17251729
zv::Val backEdgeScope = pt_internal_statement_result_loop_back_edge_scope(iterBodyScopeResult.raw());
17261730
if (UNEXPECTED(backEdgeScope.isUndef())) return false;
1731+
if (backEdgeScope.isNull()) {
1732+
bool bodyIsReplayable;
1733+
if (UNEXPECTED(!pt_node_scope_resolver_is_replayable_convergence_body(nodeScopeResolver, stmt, parts.stmts.raw(), bodyIsReplayable))) return false;
1734+
if (!bodyIsReplayable) {
1735+
backEdgeScope = ptlh::convergedPassBackEdgeScope(nodeScopeResolver, stmt, parts.stmts.raw(), iterBodyScope.raw(), iterStorage.raw(), context);
1736+
if (UNEXPECTED(backEdgeScope.isUndef())) return false;
1737+
}
1738+
}
17271739
{
17281740
zv::Val breaks = ptlh::breakExitPoints(iterBodyScopeResult.raw());
17291741
if (UNEXPECTED(breaks.isUndef())) return false;

‎turbo-ext/src/LoopHandlerCalls.h‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,27 @@ inline zv::Val newPassNodeCallback(bool replayable)
7979
return replayable ? newRecordingNodeCallback() : newNoopNodeCallback();
8080
}
8181

82+
/* a nested loop walks only its first iteration in a pass - its unconverged
83+
* types can make the back edge look dead, so a walk converging it decides:
84+
* $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $entryScope,
85+
* $storage->duplicate(), new NoopNodeCallback(), $context->withoutTemplateArgumentResolution())
86+
* ->filterOutLoopExitPoints()->getLoopBackEdgeScope()
87+
* (UNDEF = pending exception) */
88+
inline zv::Val convergedPassBackEdgeScope(zval *nodeScopeResolver, zval *stmt, zval *stmts, zval *entryScope, zval *storage, zval *context)
89+
{
90+
zv::Val storageCopy = pt_expression_result_storage_duplicate(storage);
91+
if (UNEXPECTED(storageCopy.isUndef())) return zv::Val();
92+
zv::Val noop = newNoopNodeCallback();
93+
if (UNEXPECTED(noop.isUndef())) return zv::Val();
94+
zv::Val walkContext = pt_statement_context_without_template_argument_resolution(context);
95+
if (UNEXPECTED(walkContext.isUndef())) return zv::Val();
96+
zv::Val walked = pt_node_scope_resolver_process_stmt_nodes_internal(nodeScopeResolver, stmt, stmts, entryScope, storageCopy.raw(), noop.raw(), walkContext.raw());
97+
if (UNEXPECTED(walked.isUndef())) return zv::Val();
98+
zv::Val result = pt_internal_statement_result_filter_out_loop_exit_points(walked.raw());
99+
if (UNEXPECTED(result.isUndef())) return zv::Val();
100+
return pt_internal_statement_result_loop_back_edge_scope(result.raw());
101+
}
102+
82103
/* $into = array_merge($into, $more): the array itself when $into is empty and
83104
* $more a list without holes (what array_merge() returns then), appended
84105
* otherwise; false = pending exception */

0 commit comments

Comments
 (0)