diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index e02c18b0da..3e1a44314f 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -3325,6 +3325,72 @@ public function filterByFalseyValue(Expr $expr): self return $scope; } + /** + * Recovers the loop variables mutated by a side-effecting condition (e.g. the `--$x` of a + * `while (--$x > 0)` loop). filterByFalseyValue() only narrows types, it does not re-apply + * the condition's side effects, so the after-loop type of such a variable can be left wrong + * (collapsed to never, or stuck at its in-body value that excludes a reachable exit value). + * Their corrected type is taken from the loop-condition falsey scope, where the side effects + * were applied exactly once, by unioning it into the current type. + * + * @param list $exprs + */ + public function restoreLoopConditionSideEffects(self $other, array $exprs): self + { + $expressionTypes = $this->expressionTypes; + $nativeExpressionTypes = $this->nativeExpressionTypes; + $changed = false; + foreach ($exprs as [$expr, $onlyWhenNever]) { + $exprString = $this->getNodeKey($expr); + if (!array_key_exists($exprString, $other->expressionTypes)) { + continue; + } + $otherHolder = $other->expressionTypes[$exprString]; + if ($otherHolder->getType() instanceof NeverType) { + continue; + } + if (array_key_exists($exprString, $expressionTypes)) { + if ($onlyWhenNever && !$expressionTypes[$exprString]->getType() instanceof NeverType) { + continue; + } + $expressionTypes[$exprString] = $expressionTypes[$exprString]->and($otherHolder); + } else { + $expressionTypes[$exprString] = $otherHolder; + } + if (array_key_exists($exprString, $other->nativeExpressionTypes)) { + if (array_key_exists($exprString, $nativeExpressionTypes)) { + $nativeExpressionTypes[$exprString] = $nativeExpressionTypes[$exprString]->and($other->nativeExpressionTypes[$exprString]); + } else { + $nativeExpressionTypes[$exprString] = $other->nativeExpressionTypes[$exprString]; + } + } + $changed = true; + } + + if (!$changed) { + return $this; + } + + return $this->scopeFactory->create( + $this->context, + $this->isDeclareStrictTypes(), + $this->getFunction(), + $this->getNamespace(), + $expressionTypes, + $nativeExpressionTypes, + $this->conditionalExpressions, + $this->inClosureBindScopeClasses, + $this->anonymousFunctionReflection, + $this->inFirstLevelStatement, + $this->currentlyAssignedExpressions, + $this->currentlyAllowedUndefinedExpressions, + [], + $this->afterExtractCall, + $this->parentScope, + $this->nativeTypesPromoted, + ); + } + /** * @return static */ diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 3f63434d03..7b2875e85c 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -1716,9 +1716,19 @@ public function processStmtNode( $bodyScope = $bodyScope->mergeWith($scope); $bodyScopeMaybeRan = $bodyScope; $storage = $originalStorage; - $bodyScope = $this->processExprNode($stmt, $stmt->cond, $bodyScope, $storage, $nodeCallback, ExpressionContext::createDeep())->getTruthyScope(); + $condExprResult = $this->processExprNode($stmt, $stmt->cond, $bodyScope, $storage, $nodeCallback, ExpressionContext::createDeep()); + $bodyScope = $condExprResult->getTruthyScope(); $finalScopeResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage, $nodeCallback, $context)->filterOutLoopExitPoints(); $finalScope = $finalScopeResult->getScope()->filterByFalseyValue($stmt->cond); + $condSideEffectExprs = $this->getExprSideEffectTargets($stmt->cond); + if (count($condSideEffectExprs) > 0) { + // A condition with side effects (e.g. `while (--$x > 0)`) mutates its variables as + // part of being evaluated. filterByFalseyValue() only narrows, it does not re-apply + // those side effects, so the after-loop type of such a variable can be left wrong. + // Recover the mutated variables from the loop-condition falsey scope, which applied + // the side effects exactly once. + $finalScope = $finalScope->restoreLoopConditionSideEffects($condExprResult->getFalseyScope(), $condSideEffectExprs); + } $alwaysIterates = false; $neverIterates = false; @@ -1941,9 +1951,11 @@ public function processStmtNode( $bodyScope = $bodyScope->mergeWith($initScope); $alwaysIterates = TrinaryLogic::createFromBoolean($context->isTopLevel()); + $lastCondExprResult = null; if ($lastCondExpr !== null) { $alwaysIterates = $alwaysIterates->and($bodyScope->getType($lastCondExpr)->toBoolean()->isTrue()); - $bodyScope = $this->processExprNode($stmt, $lastCondExpr, $bodyScope, $storage, $nodeCallback, ExpressionContext::createDeep())->getTruthyScope(); + $lastCondExprResult = $this->processExprNode($stmt, $lastCondExpr, $bodyScope, $storage, $nodeCallback, ExpressionContext::createDeep()); + $bodyScope = $lastCondExprResult->getTruthyScope(); $bodyScope = $this->inferForLoopExpressions($stmt, $lastCondExpr, $bodyScope); } @@ -1961,6 +1973,10 @@ public function processStmtNode( if ($lastCondExpr !== null) { $finalScope = $finalScope->filterByFalseyValue($lastCondExpr); + $condSideEffectExprs = $this->getExprSideEffectTargets($lastCondExpr); + if (count($condSideEffectExprs) > 0) { + $finalScope = $finalScope->restoreLoopConditionSideEffects($lastCondExprResult->getFalseyScope(), $condSideEffectExprs); + } } $breakExitPoints = $finalScopeResult->getExitPointsByType(Break_::class); @@ -5119,6 +5135,48 @@ private function getNextUnreachableStatements(array $nodes, bool $earlyBinding): return $stmts; } + /** + * Returns the assignment targets of the side effects (increments, decrements, assignments) + * contained anywhere in the given expression, e.g. `[$x]` for `--$x > 0`. + * + * The boolean flags whether the variable's after-loop type may only be grafted from the + * loop-condition falsey scope when filterByFalseyValue() collapsed it to never. It is false + * for pre-increments/decrements and assignments — there the value compared by the condition + * equals the variable's resulting value, so the falsey scope is the correct after-loop type + * and is always grafted. It is true for post-increments/decrements, where the compared value + * is the pre-mutation one, so grafting only rescues a never-collapse and otherwise keeps the + * more precise in-loop type (e.g. the bound on a `$i++ < 10` counter). + * + * @return list + */ + private function getExprSideEffectTargets(Expr $expr): array + { + $sideEffects = (new NodeFinder())->find([$expr], static fn (Node $node): bool => $node instanceof Expr\PreInc + || $node instanceof Expr\PreDec + || $node instanceof Expr\PostInc + || $node instanceof Expr\PostDec + || $node instanceof Expr\Assign + || $node instanceof Expr\AssignOp + || $node instanceof Expr\AssignRef); + + $targets = []; + foreach ($sideEffects as $sideEffect) { + if ($sideEffect instanceof Expr\PostInc || $sideEffect instanceof Expr\PostDec) { + $targets[] = [$sideEffect->var, true]; + } elseif ( + $sideEffect instanceof Expr\PreInc + || $sideEffect instanceof Expr\PreDec + || $sideEffect instanceof Expr\Assign + || $sideEffect instanceof Expr\AssignOp + || $sideEffect instanceof Expr\AssignRef + ) { + $targets[] = [$sideEffect->var, false]; + } + } + + return $targets; + } + private function inferForLoopExpressions(For_ $stmt, Expr $lastCondExpr, MutatingScope $bodyScope): MutatingScope { // infer $items[$i] type from for ($i = 0; $i < count($items); $i++) {...} diff --git a/tests/PHPStan/Analyser/nsrt/bug-10109b.php b/tests/PHPStan/Analyser/nsrt/bug-10109b.php new file mode 100644 index 0000000000..8d262eb23e --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-10109b.php @@ -0,0 +1,91 @@ + 0) { + } + + assertType('int', $x); +} + +function withBody(): void +{ + $x = 5; + while (--$x > 0) { + echo $x; + } + + assertType('int', $x); +} + +function preIncrement(): void +{ + $x = 5; + while (++$x < 10) { + } + + assertType('int<10, max>', $x); +} + +function assignInCondition(): void +{ + $x = 5; + while (($x = $x - 1) > 0) { + } + + assertType('int', $x); +} + +function postDecrement(): void +{ + $x = 5; + while ($x-- > 0) { + } + + assertType('int', $x); +} + +function forLoop(): void +{ + for ($x = 5; --$x > 0;) { + } + + assertType('int', $x); +} + +function shortCircuitedSideEffect(): void +{ + // The side effect is short-circuited, so the loop can also exit with $x unchanged. The + // after-loop type must still include the values produced by the decrement (notably 0). + $x = 5; + while (mt_rand(0, 10) < 10 && --$x > 0) { + } + + assertType('int', $x); +} + +function shortCircuitedCounter(): void +{ + // A post-increment counter keeps its precise in-loop bound instead of being widened by the + // loop-condition falsey scope. + $i = 0; + while (mt_rand(0, 10) < 10 && $i++ < 10) { + } + + assertType('int<0, 10>', $i); +} + +function noSideEffectInCondition(): void +{ + $x = 5; + while ($x > 0) { + $x = $x - 1; + } + + assertType('0', $x); +} diff --git a/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php index 792d737e48..34d92c510c 100644 --- a/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php @@ -1258,4 +1258,9 @@ public function testBug14847(): void ]); } + public function testBug10109(): void + { + $this->analyse([__DIR__ . '/data/bug-10109.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Comparison/data/bug-10109.php b/tests/PHPStan/Rules/Comparison/data/bug-10109.php new file mode 100644 index 0000000000..cf78497446 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/bug-10109.php @@ -0,0 +1,21 @@ + 0) { + echo "$x\n"; +} +if ($x === 0) { + echo "zero\n"; +} + +// closer to real codebase +$x = 5; +while (mt_rand(0, 10) < 10 && --$x > 0) { + echo "$x\n"; +} +if ($x === 0) { + echo "zero\n"; +}