From fbdb66cf1153771cbccf10a378d8cefd916e17b6 Mon Sep 17 00:00:00 2001 From: phpstan-bot <79867460+phpstan-bot@users.noreply.github.com> Date: Sat, 24 Oct 2026 15:41:12 +0000 Subject: [PATCH] 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 --- src/Analyser/StmtHandler/DoWhileHandler.php | 5 + src/Analyser/StmtHandler/ForHandler.php | 6 + src/Analyser/StmtHandler/ForeachHandler.php | 10 + src/Analyser/StmtHandler/WhileHandler.php | 5 + tests/PHPStan/Analyser/nsrt/bug-15420.php | 180 ++++++++++++++++++ ...isonOperatorsConstantConditionRuleTest.php | 5 + turbo-ext/src/DoWhileHandler.cpp | 4 + turbo-ext/src/ForHandler.cpp | 10 + turbo-ext/src/ForeachHandler.cpp | 12 ++ turbo-ext/src/LoopHandlerCalls.h | 21 ++ turbo-ext/src/WhileHandler.cpp | 4 + 11 files changed, 262 insertions(+) create mode 100644 tests/PHPStan/Analyser/nsrt/bug-15420.php diff --git a/src/Analyser/StmtHandler/DoWhileHandler.php b/src/Analyser/StmtHandler/DoWhileHandler.php index f05e8967912..b1933d6998b 100644 --- a/src/Analyser/StmtHandler/DoWhileHandler.php +++ b/src/Analyser/StmtHandler/DoWhileHandler.php @@ -95,6 +95,11 @@ public function processStmt( $previousPass = $passRecord; $previousPassStorage = $storage; $backEdgeScope = $bodyScopeResult->getLoopBackEdgeScope(); + if ($backEdgeScope === null && !$bodyIsReplayable) { + // a nested loop walks only its first iteration in a pass - its unconverged + // types can make the back edge look dead, so a walk converging it decides + $backEdgeScope = $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage->duplicate(), new NoopNodeCallback(), $context->withoutTemplateArgumentResolution())->filterOutLoopExitPoints()->getLoopBackEdgeScope(); + } $finalScope = $backEdgeScope === null ? $finalScope : $backEdgeScope->mergeWith($finalScope); foreach ($bodyScopeResult->getExitPointsByType(Break_::class) as $breakExitPoint) { $finalScope = $breakExitPoint->getScope()->mergeWith($finalScope); diff --git a/src/Analyser/StmtHandler/ForHandler.php b/src/Analyser/StmtHandler/ForHandler.php index 7a95e2c5710..3879f51adb3 100644 --- a/src/Analyser/StmtHandler/ForHandler.php +++ b/src/Analyser/StmtHandler/ForHandler.php @@ -202,6 +202,7 @@ public function processStmt( // entry state differs from the previous pass's (StatementsHandler::processLoopPassStmtNodes()) $previousPass = null; $previousPassStorage = null; + $bodyIsReplayable = $nodeScopeResolver->isReplayableConvergenceBody($stmt, $stmt->stmts); do { $prevScope = $bodyScope; $storage = $originalStorage->duplicate(); @@ -231,6 +232,11 @@ public function processStmt( $previousPassStorage = $storage; $passFlows[] = $bodyScopeResult->getVariableFlow(); $backEdgeScope = $bodyScopeResult->getLoopBackEdgeScope(); + if ($backEdgeScope === null && !$bodyIsReplayable) { + // a nested loop walks only its first iteration in a pass - its unconverged + // types can make the back edge look dead, so a walk converging it decides + $backEdgeScope = $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage->duplicate(), new NoopNodeCallback(), $context->withoutTemplateArgumentResolution())->filterOutLoopExitPoints()->getLoopBackEdgeScope(); + } if ($backEdgeScope === null) { $bodyScope = $prevScope; break; diff --git a/src/Analyser/StmtHandler/ForeachHandler.php b/src/Analyser/StmtHandler/ForeachHandler.php index 2ba5323bf7b..260541c2afb 100644 --- a/src/Analyser/StmtHandler/ForeachHandler.php +++ b/src/Analyser/StmtHandler/ForeachHandler.php @@ -254,6 +254,11 @@ static function () use ($condResult, $emptyArrayType): Type { $previousPass = $passRecord; $previousPassStorage = $storage; $backEdgeScope = $bodyScopeResult->getLoopBackEdgeScope(); + if ($backEdgeScope === null && !$bodyIsReplayable) { + // a nested loop walks only its first iteration in a pass - its unconverged + // types can make the back edge look dead, so a walk converging it decides + $backEdgeScope = $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage->duplicate(), new NoopNodeCallback(), $context->withoutTemplateArgumentResolution())->filterOutLoopExitPoints()->getLoopBackEdgeScope(); + } if ($backEdgeScope === null) { $bodyScope = $prevScope; break; @@ -868,6 +873,11 @@ private function tryProcessUnrolledConstantArrayForeach( $iterBodyScope = $this->enterForeach($nodeScopeResolver, $iterBodyScope, $iterStorage, $originalScope, $stmt, $iterateeType, $nativeIterateeType, new NoopNodeCallback()); $iterBodyScopeResult = $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $iterBodyScope, $iterStorage, new NoopNodeCallback(), $context->enterDeep()->withoutTemplateArgumentResolution())->filterOutLoopExitPoints(); $backEdgeScope = $iterBodyScopeResult->getLoopBackEdgeScope(); + if ($backEdgeScope === null && !$nodeScopeResolver->isReplayableConvergenceBody($stmt, $stmt->stmts)) { + // a nested loop walks only its first iteration in a pass - its unconverged + // types can make the back edge look dead, so a walk converging it decides + $backEdgeScope = $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $iterBodyScope, $iterStorage->duplicate(), new NoopNodeCallback(), $context->withoutTemplateArgumentResolution())->filterOutLoopExitPoints()->getLoopBackEdgeScope(); + } foreach ($iterBodyScopeResult->getExitPointsByType(Break_::class) as $breakExitPoint) { $endScope = $endScope->mergeWith($breakExitPoint->getScope()); } diff --git a/src/Analyser/StmtHandler/WhileHandler.php b/src/Analyser/StmtHandler/WhileHandler.php index aa198450901..7f19d7c95f5 100644 --- a/src/Analyser/StmtHandler/WhileHandler.php +++ b/src/Analyser/StmtHandler/WhileHandler.php @@ -123,6 +123,11 @@ public function processStmt( $previousPass = $passRecord; $previousPassStorage = $storage; $backEdgeScope = $bodyScopeResult->getLoopBackEdgeScope(); + if ($backEdgeScope === null && !$bodyIsReplayable) { + // a nested loop walks only its first iteration in a pass - its unconverged + // types can make the back edge look dead, so a walk converging it decides + $backEdgeScope = $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage->duplicate(), new NoopNodeCallback(), $context->withoutTemplateArgumentResolution())->filterOutLoopExitPoints()->getLoopBackEdgeScope(); + } } finally { $scope->popExpressionResultStorage(); } diff --git a/tests/PHPStan/Analyser/nsrt/bug-15420.php b/tests/PHPStan/Analyser/nsrt/bug-15420.php new file mode 100644 index 00000000000..3a02536cf74 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-15420.php @@ -0,0 +1,180 @@ +', $i); + if ($later !== []) { + $cards = $later; + } else { + break; + } + } + } +} + +/** @param string[] $cards */ +function whileLoop(array $cards): void +{ + $i = 0; + while ($i < 10) { + $seen = []; + $later = []; + foreach ($cards as $card) { + if (array_key_exists($card, $seen)) { + $later[] = $card; + } + $seen[$card] = true; + } + if ($later === []) { + break; + } + assertType('int<0, 9>', $i); + $i++; + } +} + +/** @param string[] $cards */ +function doWhileLoop(array $cards): void +{ + $i = 0; + do { + $seen = []; + $later = []; + foreach ($cards as $card) { + if (array_key_exists($card, $seen)) { + $later[] = $card; + } + $seen[$card] = true; + } + if ($later === []) { + break; + } + assertType('int<0, max>', $i); + $i++; + } while ($i < 10); +} + +/** + * @param string[] $cards + * @param int[] $rounds + */ +function foreachLoop(array $cards, array $rounds): void +{ + $i = 0; + foreach ($rounds as $round) { + $seen = []; + $later = []; + foreach ($cards as $card) { + if (array_key_exists($card, $seen)) { + $later[] = $card; + } + $seen[$card] = true; + } + if ($later === []) { + break; + } + assertType('int<0, max>', $i); + $i++; + } +} + +/** + * @param string[] $cards + * @param array{a: int, b: int, ...} $rounds + */ +function unsealedConstantArrayForeachLoop(array $cards, array $rounds): void +{ + $i = 0; + foreach ($rounds as $round) { + $seen = []; + $later = []; + foreach ($cards as $card) { + if (array_key_exists($card, $seen)) { + $later[] = $card; + } + $seen[$card] = true; + } + if ($later === []) { + break; + } + assertType('int<0, max>', $i); + $i++; + } +} + +/** @param string[] $cards */ +function innerWhile(array $cards): void +{ + for ($i = 0; $i < 10; $i++) { + $seen = false; + $later = false; + $j = 0; + while ($j < count($cards)) { + if ($seen) { + $later = true; + } + $seen = true; + $j++; + } + if (!$later) { + break; + } + assertType('int<0, 9>', $i); + } +} + +/** @param string[] $cards */ +function innerFor(array $cards): void +{ + for ($i = 0; $i < 10; $i++) { + $seen = false; + $later = false; + for ($j = 0; $j < count($cards); $j++) { + if ($seen) { + $later = true; + } + $seen = true; + } + if (!$later) { + break; + } + assertType('int<0, 9>', $i); + } +} + +/** @param string[] $cards */ +function innerDoWhile(array $cards): void +{ + for ($i = 0; $i < 10; $i++) { + $seen = false; + $later = false; + $j = 0; + do { + if ($seen) { + $later = true; + } + $seen = true; + $j++; + } while ($j < count($cards)); + if (!$later) { + break; + } + assertType('int<0, 9>', $i); + } +} diff --git a/tests/PHPStan/Rules/Comparison/NumberComparisonOperatorsConstantConditionRuleTest.php b/tests/PHPStan/Rules/Comparison/NumberComparisonOperatorsConstantConditionRuleTest.php index 4e1074b480a..1e46c065b95 100644 --- a/tests/PHPStan/Rules/Comparison/NumberComparisonOperatorsConstantConditionRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/NumberComparisonOperatorsConstantConditionRuleTest.php @@ -326,6 +326,11 @@ public function testBug15243(): void $this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-15243.php'], []); } + public function testBug15420(): void + { + $this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-15420.php'], []); + } + public function testShiftLeftUnboundedRange(): void { $this->analyse([__DIR__ . '/../../Analyser/nsrt/shift-left-unbounded-range.php'], []); diff --git a/turbo-ext/src/DoWhileHandler.cpp b/turbo-ext/src/DoWhileHandler.cpp index 8bd3377df21..931908c68a3 100644 --- a/turbo-ext/src/DoWhileHandler.cpp +++ b/turbo-ext/src/DoWhileHandler.cpp @@ -130,6 +130,10 @@ class DoWhileHandler if (UNEXPECTED(bodyScopeResult.isUndef())) return; zv::Val backEdgeScope = pt_internal_statement_result_loop_back_edge_scope(bodyScopeResult.raw()); if (UNEXPECTED(backEdgeScope.isUndef())) return; + if (backEdgeScope.isNull() && !bodyIsReplayable) { + backEdgeScope = ptlh::convergedPassBackEdgeScope(nodeScopeResolver, stmt, stmtsHold.raw(), bodyScope.raw(), storage.raw(), context); + if (UNEXPECTED(backEdgeScope.isUndef())) return; + } if (!backEdgeScope.isNull()) { if (UNEXPECTED(!ptlh::otherMergeWith(backEdgeScope.raw(), finalScope))) return; } diff --git a/turbo-ext/src/ForHandler.cpp b/turbo-ext/src/ForHandler.cpp index f187def3240..02b0cec7eab 100644 --- a/turbo-ext/src/ForHandler.cpp +++ b/turbo-ext/src/ForHandler.cpp @@ -219,6 +219,12 @@ class ForHandler // entry state differs from the previous pass's (StatementsHandler::processLoopPassStmtNodes()) zv::Val previousPass = zv::Val::null(); zv::Val previousPassStorage = zv::Val::null(); + bool bodyIsReplayable; + { + zval *bodyStmts = ptsh::readNodeProperty(pt_fh_stmts_site, stmt, PT_LC("stmts")); + if (UNEXPECTED(bodyStmts == NULL)) return zv::Val(); + if (UNEXPECTED(!pt_node_scope_resolver_is_replayable_convergence_body(nodeScopeResolver, stmt, bodyStmts, bodyIsReplayable))) return zv::Val(); + } do { zv::Val prevScope = zv::Val::copyOf(bodyScope.ref()); zv::Val storage = pt_expression_result_storage_duplicate(originalStorage); @@ -279,6 +285,10 @@ class ForHandler } zv::Val backEdgeScope = pt_internal_statement_result_loop_back_edge_scope(bodyScopeResult.raw()); if (UNEXPECTED(backEdgeScope.isUndef())) return; + if (backEdgeScope.isNull() && !bodyIsReplayable) { + backEdgeScope = ptlh::convergedPassBackEdgeScope(nodeScopeResolver, stmt, stmtsHold.raw(), bodyScope.raw(), storage.raw(), context); + if (UNEXPECTED(backEdgeScope.isUndef())) return; + } if (backEdgeScope.isNull()) { bodyScope = std::move(prevScope); brokeOut = true; diff --git a/turbo-ext/src/ForeachHandler.cpp b/turbo-ext/src/ForeachHandler.cpp index 93d6fd6990c..e7e92bffb71 100644 --- a/turbo-ext/src/ForeachHandler.cpp +++ b/turbo-ext/src/ForeachHandler.cpp @@ -543,6 +543,10 @@ class ForeachHandler if (UNEXPECTED(bodyScopeResult.isUndef())) return; zv::Val backEdgeScope = pt_internal_statement_result_loop_back_edge_scope(bodyScopeResult.raw()); if (UNEXPECTED(backEdgeScope.isUndef())) return; + if (backEdgeScope.isNull() && !bodyIsReplayable) { + backEdgeScope = ptlh::convergedPassBackEdgeScope(nodeScopeResolver, stmt, parts.stmts.raw(), bodyScope.raw(), storage.raw(), context); + if (UNEXPECTED(backEdgeScope.isUndef())) return; + } if (backEdgeScope.isNull()) { bodyScope = std::move(prevScope); brokeOut = true; @@ -1724,6 +1728,14 @@ class ForeachHandler if (UNEXPECTED(iterBodyScopeResult.isUndef())) return false; zv::Val backEdgeScope = pt_internal_statement_result_loop_back_edge_scope(iterBodyScopeResult.raw()); if (UNEXPECTED(backEdgeScope.isUndef())) return false; + if (backEdgeScope.isNull()) { + bool bodyIsReplayable; + if (UNEXPECTED(!pt_node_scope_resolver_is_replayable_convergence_body(nodeScopeResolver, stmt, parts.stmts.raw(), bodyIsReplayable))) return false; + if (!bodyIsReplayable) { + backEdgeScope = ptlh::convergedPassBackEdgeScope(nodeScopeResolver, stmt, parts.stmts.raw(), iterBodyScope.raw(), iterStorage.raw(), context); + if (UNEXPECTED(backEdgeScope.isUndef())) return false; + } + } { zv::Val breaks = ptlh::breakExitPoints(iterBodyScopeResult.raw()); if (UNEXPECTED(breaks.isUndef())) return false; diff --git a/turbo-ext/src/LoopHandlerCalls.h b/turbo-ext/src/LoopHandlerCalls.h index ba2067e2346..f7fb5c8d3c5 100644 --- a/turbo-ext/src/LoopHandlerCalls.h +++ b/turbo-ext/src/LoopHandlerCalls.h @@ -79,6 +79,27 @@ inline zv::Val newPassNodeCallback(bool replayable) return replayable ? newRecordingNodeCallback() : newNoopNodeCallback(); } +/* a nested loop walks only its first iteration in a pass - its unconverged + * types can make the back edge look dead, so a walk converging it decides: + * $nodeScopeResolver->processStmtNodesInternal($stmt, $stmt->stmts, $entryScope, + * $storage->duplicate(), new NoopNodeCallback(), $context->withoutTemplateArgumentResolution()) + * ->filterOutLoopExitPoints()->getLoopBackEdgeScope() + * (UNDEF = pending exception) */ +inline zv::Val convergedPassBackEdgeScope(zval *nodeScopeResolver, zval *stmt, zval *stmts, zval *entryScope, zval *storage, zval *context) +{ + zv::Val storageCopy = pt_expression_result_storage_duplicate(storage); + if (UNEXPECTED(storageCopy.isUndef())) return zv::Val(); + zv::Val noop = newNoopNodeCallback(); + if (UNEXPECTED(noop.isUndef())) return zv::Val(); + zv::Val walkContext = pt_statement_context_without_template_argument_resolution(context); + if (UNEXPECTED(walkContext.isUndef())) return zv::Val(); + zv::Val walked = pt_node_scope_resolver_process_stmt_nodes_internal(nodeScopeResolver, stmt, stmts, entryScope, storageCopy.raw(), noop.raw(), walkContext.raw()); + if (UNEXPECTED(walked.isUndef())) return zv::Val(); + zv::Val result = pt_internal_statement_result_filter_out_loop_exit_points(walked.raw()); + if (UNEXPECTED(result.isUndef())) return zv::Val(); + return pt_internal_statement_result_loop_back_edge_scope(result.raw()); +} + /* $into = array_merge($into, $more): the array itself when $into is empty and * $more a list without holes (what array_merge() returns then), appended * otherwise; false = pending exception */ diff --git a/turbo-ext/src/WhileHandler.cpp b/turbo-ext/src/WhileHandler.cpp index d87791b76ca..ec6e6bf1ae9 100644 --- a/turbo-ext/src/WhileHandler.cpp +++ b/turbo-ext/src/WhileHandler.cpp @@ -193,6 +193,10 @@ class WhileHandler bodyScopeResult = pt_internal_statement_result_filter_out_loop_exit_points(walked.raw()); if (UNEXPECTED(bodyScopeResult.isUndef())) return; backEdgeScope = pt_internal_statement_result_loop_back_edge_scope(bodyScopeResult.raw()); + if (UNEXPECTED(backEdgeScope.isUndef())) return; + if (backEdgeScope.isNull() && !bodyIsReplayable) { + backEdgeScope = ptlh::convergedPassBackEdgeScope(nodeScopeResolver, stmt, stmtsHold.raw(), bodyScope.raw(), storage.raw(), context); + } }(); pt_finally([&]() { (void) pt_mutating_scope_pop_expression_result_storage(Z_OBJ_P(scope.raw())); }); if (UNEXPECTED(EG(exception))) return zv::Val();