Gather all conjuncts of a compound condition side into BooleanAnd/BooleanOr conditional holders

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,46 +2078,105 @@ 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<array<string, ConditionalExpressionHolder[]>> $holderLists
+	 * @return array<string, ConditionalExpressionHolder[]>
+	 */
+	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<string, ConditionalExpressionHolder[]>
 	 */
-	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(
 				$expr,
 				$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;
 			}
 

tests/PHPStan/Analyser/nsrt/bug-14780.php

@@ -0,0 +1,69 @@
+<?php declare(strict_types = 1);
+
+namespace Bug14780;
+
+use function PHPStan\Testing\assertType;
+
+function withBothGuards(bool $forAll, ?int $expiration, ?int $auto): void
+{
+	if ($forAll && $expiration !== null) {
+		throw new \LogicException('A');
+	}
+	if (!$forAll && $expiration === null && $auto !== null) {
+		throw new \LogicException('B');
+	}
+
+	// Reachable with $auto !== null:  $forAll=true, $expiration=null, $auto=5
+	//   A: true && false  -> 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);
+}