Recover never-collapsed variables from the loop-condition falsey scope for side-effecting while/for conditions

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<array{Expr, bool}> $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
 	 */

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<array{Expr, bool}>
+	 */
+	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++) {...}

tests/PHPStan/Analyser/nsrt/bug-10109b.php

@@ -0,0 +1,91 @@
+<?php
+
+namespace Bug10109b;
+
+use function PHPStan\Testing\assertType;
+
+function simple(): void
+{
+	$x = 5;
+	while (--$x > 0) {
+	}
+
+	assertType('int<min, 0>', $x);
+}
+
+function withBody(): void
+{
+	$x = 5;
+	while (--$x > 0) {
+		echo $x;
+	}
+
+	assertType('int<min, 0>', $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<min, 0>', $x);
+}
+
+function postDecrement(): void
+{
+	$x = 5;
+	while ($x-- > 0) {
+	}
+
+	assertType('int<min, -1>', $x);
+}
+
+function forLoop(): void
+{
+	for ($x = 5; --$x > 0;) {
+	}
+
+	assertType('int<min, 0>', $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<min, 5>', $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);
+}

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'], []);
+	}
+
 }

tests/PHPStan/Rules/Comparison/data/bug-10109.php

@@ -0,0 +1,21 @@
+<?php declare(strict_types = 1);
+
+namespace Bug10109;
+
+// simpler
+$x = 5;
+while (--$x > 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";
+}