diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index a30274f6ec..70301970f7 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -757,16 +757,12 @@ public function specifyTypesInCondition( if ($types->shouldOverwrite()) { $result = $result->setAlwaysOverwriteTypes(); } - return $result->setNewConditionalExpressionHolders(array_merge( - $this->processBooleanConditionalTypes($scope, $leftTypesForHolders, false, $rightTypesForHolders, false, $rightScope), - $this->processBooleanConditionalTypes($scope, $rightTypesForHolders, false, $leftTypesForHolders, false, $scope), - $this->processBooleanConditionalTypes($scope, $leftTypesForHolders, true, $rightTypesForHolders, true, $rightScope), - $this->processBooleanConditionalTypes($scope, $rightTypesForHolders, true, $leftTypesForHolders, true, $scope), - $this->processBooleanConditionalTypes($scope, $leftTypesForHolders, false, $rightTypesForHolders, true, $rightScope), - $this->processBooleanConditionalTypes($scope, $rightTypesForHolders, false, $leftTypesForHolders, true, $scope), - $this->processBooleanConditionalTypes($scope, $leftTypesForHolders, true, $rightTypesForHolders, false, $rightScope), - $this->processBooleanConditionalTypes($scope, $rightTypesForHolders, true, $leftTypesForHolders, false, $scope), - ))->setRootExpr($expr); + return $result->setNewConditionalExpressionHolders($this->mergeConditionalHolders([ + $this->processBooleanConditionalTypes($scope, $leftTypesForHolders, $rightTypesForHolders, false, true, $rightScope), + $this->processBooleanConditionalTypes($scope, $rightTypesForHolders, $leftTypesForHolders, false, true, $scope), + $this->processBooleanConditionalTypes($scope, $leftTypesForHolders, $rightTypesForHolders, true, true, $rightScope), + $this->processBooleanConditionalTypes($scope, $rightTypesForHolders, $leftTypesForHolders, true, true, $scope), + ]))->setRootExpr($expr); } return $types; @@ -814,16 +810,12 @@ public function specifyTypesInCondition( if ($types->shouldOverwrite()) { $result = $result->setAlwaysOverwriteTypes(); } - return $result->setNewConditionalExpressionHolders(array_merge( - $this->processBooleanConditionalTypes($scope, $leftTypes, false, $rightTypes, false, $rightScope), - $this->processBooleanConditionalTypes($scope, $rightTypes, false, $leftTypes, false, $scope), - $this->processBooleanConditionalTypes($scope, $leftTypes, true, $rightTypes, true, $rightScope), - $this->processBooleanConditionalTypes($scope, $rightTypes, true, $leftTypes, true, $scope), - $this->processBooleanConditionalTypes($scope, $leftTypes, false, $rightTypes, true, $rightScope), - $this->processBooleanConditionalTypes($scope, $rightTypes, false, $leftTypes, true, $scope), - $this->processBooleanConditionalTypes($scope, $leftTypes, true, $rightTypes, false, $rightScope), - $this->processBooleanConditionalTypes($scope, $rightTypes, true, $leftTypes, false, $scope), - ))->setRootExpr($expr); + return $result->setNewConditionalExpressionHolders($this->mergeConditionalHolders([ + $this->processBooleanConditionalTypes($scope, $leftTypes, $rightTypes, false, false, $rightScope), + $this->processBooleanConditionalTypes($scope, $rightTypes, $leftTypes, false, false, $scope), + $this->processBooleanConditionalTypes($scope, $leftTypes, $rightTypes, true, false, $rightScope), + $this->processBooleanConditionalTypes($scope, $rightTypes, $leftTypes, true, false, $scope), + ]))->setRootExpr($expr); } return $types; @@ -2086,26 +2078,53 @@ private function augmentDisjunctionTypes( return $types; } + /** + * Combines several `processBooleanConditionalTypes()` results into one map. + * + * A plain `array_merge()` would be keyed by the target expression string and + * therefore let a later result overwrite an earlier one targeting the same + * expression, silently dropping a holder. Holders for the same expression are + * unioned by their key instead so all of them survive. + * + * @param list> $holderLists + * @return array + */ + private function mergeConditionalHolders(array $holderLists): array + { + $result = []; + foreach ($holderLists as $holders) { + foreach ($holders as $exprString => $exprHolders) { + foreach ($exprHolders as $key => $holder) { + $result[$exprString][$key] = $holder; + } + } + } + + return $result; + } + /** * @return array */ - private function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $conditionSpecifiedTypes, bool $conditionsFromSureTypes, SpecifiedTypes $holderSpecifiedTypes, bool $holdersFromSureTypes, Scope $rightScope): array + private function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $conditionSpecifiedTypes, SpecifiedTypes $holderSpecifiedTypes, bool $holdersFromSureTypes, bool $holderSideIsNegated, Scope $rightScope): array { + // The condition side asserts that its sub-expression evaluates truthy. + // When that sub-expression is itself a compound boolean (e.g. `$a && $b`), + // the narrowings making it true are spread across both the sure and + // sureNot lists of its specification. All of them are conjuncts of the + // single "this side is true" condition, so they must be gathered together + // into one condition set. Picking only one list would drop a conjunct and + // let the resulting holder fire too eagerly. $conditionExpressionTypes = []; - $conditionTypes = $conditionsFromSureTypes ? $conditionSpecifiedTypes->getSureTypes() : $conditionSpecifiedTypes->getSureNotTypes(); - foreach ($conditionTypes as $exprString => [$expr, $type]) { + foreach ($conditionSpecifiedTypes->getSureTypes() as $exprString => [$expr, $type]) { if (!$this->isTrackableExpression($expr)) { continue; } - if ($conditionsFromSureTypes) { - $scopeType = $scope->getType($expr); - $conditionType = TypeCombinator::remove($scopeType, $type); - if ($scopeType->equals($conditionType)) { - continue; - } - } else { - $conditionType = TypeCombinator::intersect($scope->getType($expr), $type); + $scopeType = $scope->getType($expr); + $conditionType = TypeCombinator::remove($scopeType, $type); + if ($scopeType->equals($conditionType)) { + continue; } $conditionExpressionTypes[$exprString] = ExpressionTypeHolder::createYes( @@ -2113,19 +2132,51 @@ private function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $co $conditionType, ); } + foreach ($conditionSpecifiedTypes->getSureNotTypes() as $exprString => [$expr, $type]) { + if (!$this->isTrackableExpression($expr)) { + continue; + } + + $conditionExpressionTypes[$exprString] = ExpressionTypeHolder::createYes( + $expr, + TypeCombinator::intersect($scope->getType($expr), $type), + ); + } if (count($conditionExpressionTypes) > 0) { $holders = []; $holderTypes = $holdersFromSureTypes ? $holderSpecifiedTypes->getSureTypes() : $holderSpecifiedTypes->getSureNotTypes(); + + // In the `BooleanAnd` false context a true condition implies the other + // side is false. When that other side is a plain conjunction + // (`$a === null && $b === null`), its false narrowing is empty, so the + // holder narrowings are re-derived by swapping the truthy sure/sureNot + // lists — landing in the sureNot list here. The implied negation is then a + // disjunction (`$a !== null || $b !== null`) that cannot be expressed as + // independent per-expression narrowings: splitting it into one holder per + // expression would let each fire on its own and over-narrow, so no holder + // is produced. A genuine negated conjunction (`!($a instanceof X && ...)`) + // instead asserts every conjunct true through the sure list, which is sound + // to split; likewise the `BooleanOr` true context. Only the swapped + // sureNot multi-target case is suppressed. + if ($holderSideIsNegated && !$holdersFromSureTypes) { + $trackableHolderCount = 0; + foreach ($holderTypes as [$holderExpr]) { + if (!$this->isTrackableExpression($holderExpr)) { + continue; + } + $trackableHolderCount++; + } + if ($trackableHolderCount > 1) { + return []; + } + } + foreach ($holderTypes as $exprString => [$expr, $type]) { if (!$this->isTrackableExpression($expr)) { continue; } - if (!isset($holders[$exprString])) { - $holders[$exprString] = []; - } - $conditions = $conditionExpressionTypes; foreach (array_keys($conditions) as $conditionExprString) { if ($conditionExprString !== $exprString) { @@ -2156,6 +2207,7 @@ private function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $co $conditions, ExpressionTypeHolder::createYes($expr, $holderType), ); + $holders[$exprString] ??= []; $holders[$exprString][$holder->getKey()] = $holder; } diff --git a/tests/PHPStan/Analyser/nsrt/bug-14780.php b/tests/PHPStan/Analyser/nsrt/bug-14780.php new file mode 100644 index 0000000000..58750d6b41 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-14780.php @@ -0,0 +1,69 @@ + no throw + // B: false && ... -> no throw + if ($forAll) { + assertType('int|null', $auto); + } +} + +function singleGuard(bool $forAll, ?int $expiration, ?int $auto): void +{ + if (!$forAll && $expiration === null && $auto !== null) { + throw new \LogicException('B'); + } + + // $expiration is unknown here, so $auto must keep both possibilities. + if (!$forAll) { + assertType('int|null', $auto); + } +} + +function fourConditions(bool $a, bool $b, ?int $expiration, ?int $auto): void +{ + if ($a && $expiration !== null) { + throw new \LogicException('A'); + } + if (!$a && $b && $expiration === null && $auto !== null) { + throw new \LogicException('B'); + } + + // Entering `if ($a)` chains `$a => $expiration === null` (guard A) into the + // guard B holder, which must keep all three of its conjuncts. + if ($a) { + assertType('int|null', $auto); + } +} + +function compoundHolderSide(?int $a, ?object $b, bool $mock): void +{ + // The guarded side `$a === null && $b === null` is itself a conjunction. + // Its negation `$a !== null || $b !== null` is a disjunction and must not be + // split into independent `$mock => $a !== null` / `$mock => $b !== null` + // holders that would each over-narrow. + if ($a === null && $b === null && !$mock) { + throw new \LogicException(); + } + + if ($mock) { + return; + } + + // only (a !== null || b !== null) is known here + assertType('int|null', $a); + assertType('object|null', $b); +}