diff --git a/src/Analyser/ExprHandler/BooleanAndHandler.php b/src/Analyser/ExprHandler/BooleanAndHandler.php index 383b03c7d66..0c59ec4fac0 100644 --- a/src/Analyser/ExprHandler/BooleanAndHandler.php +++ b/src/Analyser/ExprHandler/BooleanAndHandler.php @@ -277,8 +277,8 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex isAlwaysTerminating: $leftResult->isAlwaysTerminating(), throwPoints: array_merge($leftResult->getThrowPoints(), $rightResult->getThrowPoints()), impurePoints: array_merge($leftResult->getImpurePoints(), $rightResult->getImpurePoints()), - truthyScopeCallback: static fn (): MutatingScope => $rightResult->getScope()->filterByTruthyValue($expr->right), - falseyScopeCallback: static fn (): MutatingScope => $leftMergedWithRightScope->filterByFalseyValue($expr), + truthyScopeCallback: fn (): MutatingScope => $this->conditionalExpressionHolderHelper->specifyWholeExpressionType($rightResult->getScope()->filterByTruthyValue($expr->right), $expr, true), + falseyScopeCallback: fn (): MutatingScope => $this->conditionalExpressionHolderHelper->specifyWholeExpressionType($leftMergedWithRightScope->filterByFalseyValue($expr), $expr, false), ); } diff --git a/src/Analyser/ExprHandler/BooleanOrHandler.php b/src/Analyser/ExprHandler/BooleanOrHandler.php index d439a2c8082..657d884b932 100644 --- a/src/Analyser/ExprHandler/BooleanOrHandler.php +++ b/src/Analyser/ExprHandler/BooleanOrHandler.php @@ -309,8 +309,8 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex isAlwaysTerminating: $leftResult->isAlwaysTerminating(), throwPoints: array_merge($leftResult->getThrowPoints(), $rightResult->getThrowPoints()), impurePoints: array_merge($leftResult->getImpurePoints(), $rightResult->getImpurePoints()), - truthyScopeCallback: static fn (): MutatingScope => $leftMergedWithRightScope->filterByTruthyValue($expr), - falseyScopeCallback: static fn (): MutatingScope => $rightResult->getScope()->filterByFalseyValue($expr->right), + truthyScopeCallback: fn (): MutatingScope => $this->conditionalExpressionHolderHelper->specifyWholeExpressionType($leftMergedWithRightScope->filterByTruthyValue($expr), $expr, true), + falseyScopeCallback: fn (): MutatingScope => $this->conditionalExpressionHolderHelper->specifyWholeExpressionType($rightResult->getScope()->filterByFalseyValue($expr->right), $expr, false), ); } diff --git a/src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php b/src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php index e8c1008c183..8d5d7c00e08 100644 --- a/src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php +++ b/src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php @@ -15,6 +15,7 @@ use PHPStan\Analyser\TypeSpecifier; use PHPStan\Analyser\TypeSpecifierContext; use PHPStan\DependencyInjection\AutowiredService; +use PHPStan\Type\Constant\ConstantBooleanType; use PHPStan\Type\NeverType; use PHPStan\Type\TypeCombinator; use function array_keys; @@ -36,6 +37,34 @@ public function __construct( { } + /** + * Pins the boolean value of a whole `&&`/`||` condition (`true` in the + * truthy branch, `false` in the falsey branch) into a scope and runs the + * conditional-expression resolution against it, without re-narrowing the + * operands. + * + * The single-variable case (`if ($cond) { $x = 1; } if ($cond) { echo $x; }`) + * already works because the narrowed variable is stored in the scope and + * becomes a guard when the branches merge. A compound condition narrows no + * single variable to a definite value, so nothing is left to guard on. + * Pinning the condition expression's boolean value gives the merge a guard + * keyed on the whole condition, and re-running the resolution lets a later + * identical `if` re-derive the same narrowing — all while preserving any + * side effects (e.g. assignments) already baked into the operand-narrowed + * scope. + */ + public function specifyWholeExpressionType(MutatingScope $scope, Expr $expr, bool $value): MutatingScope + { + $specifiedTypes = $this->typeSpecifier->create( + $expr, + new ConstantBooleanType($value), + TypeSpecifierContext::createTrue(), + $scope, + )->setRootExpr($expr); + + return $scope->filterBySpecifiedTypes($specifiedTypes); + } + public function augmentDisjunctionTypes( MutatingScope $scope, MutatingScope $rightScope, diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index e02c18b0da3..c0164532acd 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -3325,6 +3325,33 @@ public function filterByFalseyValue(Expr $expr): self return $scope; } + /** + * A multi-guard conditional holder may take some of its guards from the + * current scope's already-narrowed expressions (e.g. the `!prevCond` part of + * an `if`/`elseif` chain that is definite in scope but not re-specified by the + * condition currently being applied), as long as at least one of its guards is + * actively specified — so the holder is relevant to that condition rather than + * firing on ambient scope state. Single-guard holders never seed, preserving + * their existing behaviour. + * + * @param array $conditionHolders + * @param array $specifiedExpressions + */ + private function multiGuardCanSeedFromScope(array $conditionHolders, array $specifiedExpressions): bool + { + if (count($conditionHolders) < 2) { + return false; + } + + foreach (array_keys($conditionHolders) as $holderExprString) { + if (array_key_exists($holderExprString, $specifiedExpressions)) { + return true; + } + } + + return false; + } + /** * @return static */ @@ -3415,13 +3442,25 @@ public function filterBySpecifiedTypes(SpecifiedTypes $specifiedTypes): self ) { continue; } - foreach ($conditionalExpression->getConditionExpressionTypeHolders() as $holderExprString => $conditionalTypeHolder) { + $conditionHolders = $conditionalExpression->getConditionExpressionTypeHolders(); + $allowSeed = $this->multiGuardCanSeedFromScope($conditionHolders, $specifiedExpressions); + foreach ($conditionHolders as $holderExprString => $conditionalTypeHolder) { if ( - !array_key_exists($holderExprString, $specifiedExpressions) - || !$conditionalTypeHolder->equals($specifiedExpressions[$holderExprString]) + array_key_exists($holderExprString, $specifiedExpressions) + && $conditionalTypeHolder->equals($specifiedExpressions[$holderExprString]) ) { - continue 2; + continue; + } + if ( + $allowSeed + && array_key_exists($holderExprString, $scope->expressionTypes) + && $scope->expressionTypes[$holderExprString]->getCertainty()->yes() + && $conditionalTypeHolder->getType()->equals($scope->expressionTypes[$holderExprString]->getType()) + ) { + continue; } + + continue 2; } $conditions[$conditionalExprString][] = $conditionalExpression; @@ -3437,14 +3476,26 @@ public function filterBySpecifiedTypes(SpecifiedTypes $specifiedTypes): self if ($conditionalExpression->getTypeHolder()->getCertainty()->no()) { continue; } - foreach ($conditionalExpression->getConditionExpressionTypeHolders() as $holderExprString => $conditionalTypeHolder) { + $conditionHolders = $conditionalExpression->getConditionExpressionTypeHolders(); + $allowSeed = $this->multiGuardCanSeedFromScope($conditionHolders, $specifiedExpressions); + foreach ($conditionHolders as $holderExprString => $conditionalTypeHolder) { + if ( + array_key_exists($holderExprString, $specifiedExpressions) + && $conditionalTypeHolder->getCertainty()->equals($specifiedExpressions[$holderExprString]->getCertainty()) + && $conditionalTypeHolder->getType()->isSuperTypeOf($specifiedExpressions[$holderExprString]->getType())->yes() + ) { + continue; + } if ( - !array_key_exists($holderExprString, $specifiedExpressions) - || !$conditionalTypeHolder->getCertainty()->equals($specifiedExpressions[$holderExprString]->getCertainty()) - || !$conditionalTypeHolder->getType()->isSuperTypeOf($specifiedExpressions[$holderExprString]->getType())->yes() + $allowSeed + && array_key_exists($holderExprString, $scope->expressionTypes) + && $conditionalTypeHolder->getCertainty()->equals($scope->expressionTypes[$holderExprString]->getCertainty()) + && $conditionalTypeHolder->getType()->isSuperTypeOf($scope->expressionTypes[$holderExprString]->getType())->yes() ) { - continue 2; + continue; } + + continue 2; } $conditions[$conditionalExprString][] = $conditionalExpression; @@ -3843,7 +3894,43 @@ private function createConditionalExpressions( $typeGuards[$exprString] = $holder; } - if (count($typeGuards) === 0) { + // Path-condition guards distinguish this branch from the other one along + // an `if`/`elseif`/`else` chain. Unlike $typeGuards they keep the branch's + // own condition (e.g. the `elseif` operand), which $guardsToExclude drops + // because the merge absorbs its narrowed value. A branch reached after one + // or more preceding branches were skipped needs *all* of these together + // (`!prevCond && thisCond`) as a single guard; any one of them alone would + // also match a sibling branch and is dropped when that sibling merges in. + // Only pre-existing variables narrowed by the condition qualify — freshly + // assigned variables (absent from the other branch) are not conditions. + $conditionGuards = []; + foreach ($newVariableTypes as $exprString => $holder) { + if ($holder->getExpr() instanceof VirtualNode) { + continue; + } + if (!$holder->getCertainty()->yes()) { + continue; + } + if (!array_key_exists($exprString, $mergedExpressionTypes)) { + continue; + } + if ( + !array_key_exists($exprString, $theirExpressionTypes) + || !$theirExpressionTypes[$exprString]->getCertainty()->yes() + ) { + continue; + } + if ($mergedExpressionTypes[$exprString]->equalTypes($holder)) { + continue; + } + + $conditionGuards[$exprString] = $holder; + } + + // $typeGuards drives the single-guard holders; $conditionGuards drives the + // multi-guard (path-condition) holders. Either may be empty on its own, but + // when both are there is nothing to record. + if (count($typeGuards) === 0 && count($conditionGuards) < 2) { return $conditionalExpressions; } @@ -3858,6 +3945,27 @@ private function createConditionalExpressions( continue; } + // Carry a value across the chain only for a plain variable whose + // *definedness* the branch establishes (defined here, not in the other + // branch). A variable already defined in both branches is a condition + // variable, not a branch product, so keying its narrowed value on the + // sibling conditions (`if rel=false then document=false`) would be + // unsound. Restricting to variables also keeps these path-condition + // holders out of the way of the narrowing recorded for compound + // boolean operands (e.g. `$x = $a->foo() !== null && ...; if ($x)`). + $targetIsConditionVariable = array_key_exists($exprString, $theirExpressionTypes) + && $theirExpressionTypes[$exprString]->getCertainty()->yes() + && $holder->getCertainty()->yes(); + + if ($holder->getExpr() instanceof Variable && !$targetIsConditionVariable) { + $pathGuards = $conditionGuards; + unset($pathGuards[$exprString]); + if (count($pathGuards) >= 2) { + $conditionalExpression = new ConditionalExpressionHolder($pathGuards, $holder); + $conditionalExpressions[$exprString][$conditionalExpression->getKey()] = $conditionalExpression; + } + } + $variableTypeGuards = $typeGuards; unset($variableTypeGuards[$exprString]); diff --git a/tests/PHPStan/Analyser/nsrt/bug-14871.php b/tests/PHPStan/Analyser/nsrt/bug-14871.php new file mode 100644 index 00000000000..1e575bf8b80 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-14871.php @@ -0,0 +1,147 @@ + 0 || $b > 0) { + $x = 1; + } + + if ($a > 0 || $b > 0) { + assertVariableCertainty(TrinaryLogic::createYes(), $x); + } +} + +function typeNarrowingCarried(?int $a, bool $b): void +{ + if ($a !== null || $b) { + $x = 'set'; + } + + if ($a !== null || $b) { + assertType("'set'", $x); + } +} + +// https://phpstan.org/r/aab74e73-2bfe-432b-8bcd-f9b939d2eaab +// An `if`/`elseif`/`else` chain repeated with identical compound conditions must +// carry definedness across every branch, not just the first. +function compoundElseIfChain(bool $rel, bool $document, bool $overwrite): void +{ + if ($rel || $overwrite) { + $vvv = 1; + } elseif ($document) { + $aaa = 2; + } else { + $eee = 3; + } + + if ($rel || $overwrite) { + assertVariableCertainty(TrinaryLogic::createYes(), $vvv); + } elseif ($document) { + assertVariableCertainty(TrinaryLogic::createYes(), $aaa); + } else { + assertVariableCertainty(TrinaryLogic::createYes(), $eee); + } +} + +// The same chain with single-variable conditions (a pre-existing limitation for +// the non-first branches) must work too. +function singleVarElseIfChain(bool $a, bool $b): void +{ + if ($a) { + $x = 1; + } elseif ($b) { + $y = 2; + } else { + $z = 3; + } + + if ($a) { + assertVariableCertainty(TrinaryLogic::createYes(), $x); + } elseif ($b) { + assertVariableCertainty(TrinaryLogic::createYes(), $y); + } else { + assertVariableCertainty(TrinaryLogic::createYes(), $z); + } +} + +// A condition with a side effect in one operand must still narrow correctly +// after the `if` (the whole-condition pinning must not re-run the assignment). +function assignmentInOperand(string $foo): int +{ + if (!ctype_digit($foo) || ($foo = intval($foo)) < 1) { + return -1; + } + + assertType('int<1, max>', $foo); + + return $foo; +} + +function unsetBetweenIfs(?int $a, bool $b): void +{ + if ($a !== null || $b) { + $x = 'set'; + } + + if (rand(0,1)) { + unset($x); + } + + if ($a !== null || $b) { + assertType("'set'", $x); + assertVariableCertainty(TrinaryLogic::createMaybe(), $x); + } +}