diff --git a/src/Analyser/StmtHandler/WhileHandler.php b/src/Analyser/StmtHandler/WhileHandler.php index aa198450901..ca0e890595a 100644 --- a/src/Analyser/StmtHandler/WhileHandler.php +++ b/src/Analyser/StmtHandler/WhileHandler.php @@ -15,7 +15,6 @@ use PHPStan\Analyser\RecordingNodeCallback; use PHPStan\Analyser\StatementContext; use PHPStan\Analyser\StmtHandler; -use PHPStan\Analyser\TypeSpecifierContext; use PHPStan\Analyser\VariableFlow; use PHPStan\Dependency\Dependencies; use PHPStan\DependencyInjection\AutowiredParameter; @@ -196,12 +195,18 @@ public function processStmt( $backEdgeScope = $finalScopeResult->getLoopBackEdgeScope(); $backEdgeDead = $backEdgeScope === null; $finalScope = $backEdgeScope ?? $finalScopeResult->getScope(); - // the loop condition narrows the post-loop scope to its falsey branch; - // $finalScope (after the body ran) is a different scope than the condition's - // own, so reprocess the condition there rather than re-running its result. - // The duplicate lets subresults whose state did not change in the body - // answer from the final pass instead of being re-priced. - $finalScope = $finalScope->applySpecifiedTypes($nodeScopeResolver->processExprOnDemand($stmt->cond, $finalScope, $storage->duplicate())->getSpecifiedTypesForScope($finalScope, TypeSpecifierContext::createFalsey())); + // the loop exits when the condition, evaluated again after the body ran, + // is falsey - walk it on $finalScope and take its falsey scope, so the + // assignments it makes (`while ($row = fetch())`) replace the body's + // values instead of the body's stale values being narrowed by them. + // The duplicate keeps this walk's results out of the final pass's storage. + $exitCondStorage = $storage->duplicate(); + $scope->pushExpressionResultStorage($exitCondStorage); + try { + $finalScope = $nodeScopeResolver->processExprNode($stmt, $stmt->cond, $finalScope, $exitCondStorage, new NoopNodeCallback(), ExpressionContext::createDeep(resolveTemplateArguments: false))->getFalseyScope(); + } finally { + $scope->popExpressionResultStorage(); + } $alwaysIterates = false; $neverIterates = false; @@ -236,7 +241,10 @@ public function processStmt( if (!$this->polluteScopeWithLoopInitialAssignments) { $condScope = $condScope->mergeWith($scope); } - $finalScope = $finalScope === null ? $condScope : $finalScope->mergeWith($condScope); + // the loop either ran or exited on its first condition, so these are + // alternative paths like an if's branches: keep a conditional that only + // one path records when the other path makes it vacuously true + $finalScope = $finalScope === null ? $condScope : $finalScope->mergeWith($condScope, true); } $finalScope ??= $finalScopeResult->getScope(); diff --git a/tests/PHPStan/Analyser/Bug15722WithoutLoopPollutionTest.php b/tests/PHPStan/Analyser/Bug15722WithoutLoopPollutionTest.php new file mode 100644 index 00000000000..a90a52dd6a8 --- /dev/null +++ b/tests/PHPStan/Analyser/Bug15722WithoutLoopPollutionTest.php @@ -0,0 +1,36 @@ +assertFileAsserts($assertType, $file, ...$args); + } + + public static function getAdditionalConfigFiles(): array + { + return [ + __DIR__ . '/../Rules/without-loop-pollution.neon', + ]; + } + +} diff --git a/tests/PHPStan/Analyser/data/bug-15722-without-loop-pollution.php b/tests/PHPStan/Analyser/data/bug-15722-without-loop-pollution.php new file mode 100644 index 00000000000..3ea1e24c5bf --- /dev/null +++ b/tests/PHPStan/Analyser/data/bug-15722-without-loop-pollution.php @@ -0,0 +1,55 @@ +stop()) && ($rows = $s->next())) { + } + + if ($rows !== 0) { + assertType('true', $stop); + } + + if ($rows === 0) { + assertType('bool', $stop); + } else { + assertType('true', $stop); + } +} + +function noCorrelation(): void +{ + $n = 0; + $flag = false; + while (rand(0, 1)) { + $n = 1; + $flag = (bool) rand(0, 1); + } + if ($n === 1) { + assertType('bool', $flag); + } +} + +function guardOverlaps(): void +{ + $n = rand(0, 2); + $flag = false; + while (rand(0, 1)) { + $n = 1; + $flag = true; + } + if ($n === 1) { + // the loop may not have run, with $n = 1 and $flag still false + assertType('bool', $flag); + } +} diff --git a/tests/PHPStan/Analyser/nsrt/bug-15726.php b/tests/PHPStan/Analyser/nsrt/bug-15726.php new file mode 100644 index 00000000000..d1c6d8f2b44 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-15726.php @@ -0,0 +1,64 @@ + 0) { + assertType('int<0, 9>', $i); + } + assertType('-1', $i); +} + +// https://github.com/phpstan/phpstan/issues/15722 Problem B + +interface Source +{ + public function stop(): bool; + public function next(): int; +} + +function loop(Source $s): void +{ + $rows = 0; + while (!($stop = $s->stop()) && ($rows = $s->next())) { + } + + if ($rows !== 0) { + assertType('true', $stop); + } + + if ($rows === 0) { + assertType('bool', $stop); + } else { + assertType('true', $stop); + } +} diff --git a/tests/PHPStan/Analyser/nsrt/while-loop-variables.php b/tests/PHPStan/Analyser/nsrt/while-loop-variables.php index 3ae86530ff6..9e586864bf1 100644 --- a/tests/PHPStan/Analyser/nsrt/while-loop-variables.php +++ b/tests/PHPStan/Analyser/nsrt/while-loop-variables.php @@ -49,7 +49,7 @@ function () { assertType('int<1, 10>', $i); } - assertType('int<0, 10>', $i); + assertType('int<0, 11>', $i); // the condition increments $i to 11 when $i++ < 10 fails assertType('LoopVariables\Bar|LoopVariables\Foo|LoopVariables\Lorem|null', $foo); diff --git a/turbo-ext/src/WhileHandler.cpp b/turbo-ext/src/WhileHandler.cpp index d87791b76ca..4b9354984ee 100644 --- a/turbo-ext/src/WhileHandler.cpp +++ b/turbo-ext/src/WhileHandler.cpp @@ -294,27 +294,31 @@ class WhileHandler } else { finalScope = std::move(backEdgeScope); } - // the loop condition narrows the post-loop scope to its falsey branch; - // $finalScope (after the body ran) is a different scope than the condition's - // own, so reprocess the condition there rather than re-running its result. - // The duplicate lets subresults whose state did not change in the body - // answer from the final pass instead of being re-priced. + // the loop exits when the condition, evaluated again after the body ran, + // is falsey - walk it on $finalScope and take its falsey scope, so the + // assignments it makes (`while ($row = fetch())`) replace the body's + // values instead of the body's stale values being narrowed by them. + // The duplicate keeps this walk's results out of the final pass's storage. { - zval *cond = ptsh::readNodeProperty(pt_wh_cond_site, stmt, PT_LC("cond")); - if (UNEXPECTED(cond == NULL)) return zv::Val(); - zv::Val condHold = zv::Val::copyOf(zv::Ref(cond)); - zv::Val duplicate = pt_expression_result_storage_duplicate(finalStorage); - if (UNEXPECTED(duplicate.isUndef())) return zv::Val(); - zv::Val onDemand = pt_node_scope_resolver_process_expr_on_demand(nodeScopeResolver, condHold.raw(), finalScope.raw(), duplicate.raw()); - if (UNEXPECTED(onDemand.isUndef())) return zv::Val(); - zend_object *falsey = pt_type_specifier_context_create_falsey(); - if (UNEXPECTED(falsey == NULL)) return zv::Val(); - zval falseyContext; - ZVAL_OBJ(&falseyContext, falsey); - zv::Val specifiedTypes = pt_expression_result_get_specified_types_for_scope(onDemand.raw(), finalScope.raw(), &falseyContext); - if (UNEXPECTED(specifiedTypes.isUndef())) return zv::Val(); - finalScope = pt_mutating_scope_apply_specified_types(Z_OBJ_P(finalScope.raw()), specifiedTypes.raw()); - if (UNEXPECTED(finalScope.isUndef())) return zv::Val(); + zv::Val exitCondStorage = pt_expression_result_storage_duplicate(finalStorage); + if (UNEXPECTED(exitCondStorage.isUndef())) return zv::Val(); + if (UNEXPECTED(!pt_mutating_scope_push_expression_result_storage(Z_OBJ_P(scope.raw()), exitCondStorage.raw()))) return zv::Val(); + zv::Val falseyScope; + [&]() { + zval *cond = ptsh::readNodeProperty(pt_wh_cond_site, stmt, PT_LC("cond")); + if (UNEXPECTED(cond == NULL)) return; + zv::Val condHold = zv::Val::copyOf(zv::Ref(cond)); + zv::Val noop = ptlh::newNoopNodeCallback(); + if (UNEXPECTED(noop.isUndef())) return; + zv::Val expressionContext = pt_expression_context_create_deep(false); + if (UNEXPECTED(expressionContext.isUndef())) return; + zv::Val exitCondResult = pt_node_scope_resolver_process_expr_node(nodeScopeResolver, stmt, condHold.raw(), finalScope.raw(), exitCondStorage.raw(), noop.raw(), expressionContext.raw()); + if (UNEXPECTED(exitCondResult.isUndef())) return; + falseyScope = pt_expression_result_get_falsey_scope(exitCondResult.raw()); + }(); + pt_finally([&]() { (void) pt_mutating_scope_pop_expression_result_storage(Z_OBJ_P(scope.raw())); }); + if (UNEXPECTED(EG(exception))) return zv::Val(); + finalScope = std::move(falseyScope); } bool alwaysIterates = false; @@ -376,7 +380,10 @@ class WhileHandler if (finalScope.isNull()) { finalScope = zv::Val::copyOf(condScope.ref()); } else { - finalScope = pt_mutating_scope_merge_with(Z_OBJ_P(finalScope.raw()), condScope.raw()); + // the loop either ran or exited on its first condition, so these are + // alternative paths like an if's branches: keep a conditional that only + // one path records when the other path makes it vacuously true + finalScope = pt_mutating_scope_merge_with(Z_OBJ_P(finalScope.raw()), condScope.raw(), true); if (UNEXPECTED(finalScope.isUndef())) return zv::Val(); } }