From e2d375a34d4aa50f9c9976d4f7472618ce7fc942 Mon Sep 17 00:00:00 2001 From: phpstan-bot <79867460+phpstan-bot@users.noreply.github.com> Date: Sat, 6 Jun 2026 17:47:38 +0000 Subject: [PATCH] Do not split a compound-boolean holder side whose asserted truth value is a disjunction - `processBooleanConditionalTypes()` now decides whether a holder side may be decomposed into per-expression holders from the holder-side expression's logical structure instead of counting trackable targets in the sureNot list. - A new `isUnsplittableCompoundHolderSide()` suppresses holder creation when the side is a conjunction (`&&`/`and`) asserted false in the `BooleanAnd` false context, or a disjunction (`||`/`or`) asserted true in the `BooleanOr` true context. In both cases the asserted fact is itself a disjunction with no sound per-expression narrowing. - The holder-side expression (`$expr->left` / `$expr->right`) is now threaded into `processBooleanConditionalTypes()` for both the `BooleanAnd` and `BooleanOr` holder paths. - Fixes the reported false positive: after `if ($a && $b && $c) { return; }`, `$c` being true no longer narrowed `$a`/`$b` to `false`, so `$c && $b` was wrongly reported as `Right side of && is always false`. The previous count-based guard only covered the sureNot list, but a plain-boolean conjunction's false narrowing lands (over-narrowed) in the sure list. - Side effect / improvement: a disjunction operand asserted false (e.g. `!(($p || $q) && $r)` with `$r` true) is now correctly narrowed, since the negation of a disjunction is a conjunction and is safe to split. --- src/Analyser/TypeSpecifier.php | 71 +++++++++++++---------- tests/PHPStan/Analyser/nsrt/bug-14787.php | 56 ++++++++++++++++++ 2 files changed, 95 insertions(+), 32 deletions(-) create mode 100644 tests/PHPStan/Analyser/nsrt/bug-14787.php diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 70301970f7..0d1138fdab 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -758,10 +758,10 @@ public function specifyTypesInCondition( $result = $result->setAlwaysOverwriteTypes(); } 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), + $this->processBooleanConditionalTypes($scope, $leftTypesForHolders, $rightTypesForHolders, false, true, $rightScope, $expr->right), + $this->processBooleanConditionalTypes($scope, $rightTypesForHolders, $leftTypesForHolders, false, true, $scope, $expr->left), + $this->processBooleanConditionalTypes($scope, $leftTypesForHolders, $rightTypesForHolders, true, true, $rightScope, $expr->right), + $this->processBooleanConditionalTypes($scope, $rightTypesForHolders, $leftTypesForHolders, true, true, $scope, $expr->left), ]))->setRootExpr($expr); } @@ -811,10 +811,10 @@ public function specifyTypesInCondition( $result = $result->setAlwaysOverwriteTypes(); } 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), + $this->processBooleanConditionalTypes($scope, $leftTypes, $rightTypes, false, false, $rightScope, $expr->right), + $this->processBooleanConditionalTypes($scope, $rightTypes, $leftTypes, false, false, $scope, $expr->left), + $this->processBooleanConditionalTypes($scope, $leftTypes, $rightTypes, true, false, $rightScope, $expr->right), + $this->processBooleanConditionalTypes($scope, $rightTypes, $leftTypes, true, false, $scope, $expr->left), ]))->setRootExpr($expr); } @@ -2106,7 +2106,7 @@ private function mergeConditionalHolders(array $holderLists): array /** * @return array */ - private function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $conditionSpecifiedTypes, SpecifiedTypes $holderSpecifiedTypes, bool $holdersFromSureTypes, bool $holderSideIsNegated, Scope $rightScope): array + private function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $conditionSpecifiedTypes, SpecifiedTypes $holderSpecifiedTypes, bool $holdersFromSureTypes, bool $holderSideIsNegated, Scope $rightScope, ?Expr $holderSideExpr = null): array { // The condition side asserts that its sub-expression evaluates truthy. // When that sub-expression is itself a compound boolean (e.g. `$a && $b`), @@ -2147,29 +2147,17 @@ private function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $co $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 []; - } + // A holder side that is itself a compound boolean cannot always be split + // into independent per-expression holders. In the `BooleanAnd` false + // context the holder asserts its side is false: when that side is a + // conjunction (`$a && $b`), its negation is the disjunction `!$a || !$b`, + // which has no per-expression narrowing — narrowing each conjunct + // independently would drop a reachable value (e.g. `$a = false, $b = true`). + // Symmetrically, in the `BooleanOr` true context the holder asserts its + // side is true, and a disjunction side (`$a || $b`) is itself a disjunction. + // Such a side is left whole rather than split into over-narrowing holders. + if ($this->isUnsplittableCompoundHolderSide($holderSideExpr, $holderSideIsNegated)) { + return []; } foreach ($holderTypes as $exprString => [$expr, $type]) { @@ -2217,6 +2205,25 @@ private function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $co return []; } + /** + * A holder side whose truth value is asserted as a disjunction cannot be + * decomposed into independent per-expression holders. That happens for a + * conjunction (`&&`) asserted false (negated context) and for a disjunction + * (`||`) asserted true. + */ + private function isUnsplittableCompoundHolderSide(?Expr $holderSideExpr, bool $holderSideIsNegated): bool + { + if ($holderSideExpr === null) { + return false; + } + + if ($holderSideIsNegated) { + return $holderSideExpr instanceof BooleanAnd || $holderSideExpr instanceof LogicalAnd; + } + + return $holderSideExpr instanceof BooleanOr || $holderSideExpr instanceof LogicalOr; + } + private function isTrackableExpression(Expr $expr): bool { if ($expr instanceof Expr\Variable) { diff --git a/tests/PHPStan/Analyser/nsrt/bug-14787.php b/tests/PHPStan/Analyser/nsrt/bug-14787.php new file mode 100644 index 0000000000..620843a71a --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-14787.php @@ -0,0 +1,56 @@ +