From 57710f6ee5e0e1d940b62b8ae9092e8bedfc4ef3 Mon Sep 17 00:00:00 2001 From: phpstan-bot <79867460+phpstan-bot@users.noreply.github.com> Date: Fri, 3 Jul 2026 09:36:30 +0000 Subject: [PATCH] Skip boolean conditional expression holders when the antecedent side is an unsplittable compound boolean - `ConditionalExpressionHolderHelper::processBooleanConditionalTypes()` now returns no holders when the condition (antecedent) side is a compound boolean whose asserted truth value is a disjunction (`||` asserted true, or `&&` asserted false). Such a side's per-expression narrowing is only a necessary consequence of its truth value, not a sufficient one, so using it as a guard fires the holder unsoundly. - Generalized the existing `isUnsplittableCompoundHolderSide()` into `isUnsplittableCompoundSide()` and applied it to the antecedent side with the opposite polarity (`!$holderSideIsNegated`), mirroring the consequent-side check that already existed. - Threaded the condition-side expression into `processBooleanConditionalTypes()` from both `BooleanAndHandler` and `BooleanOrHandler` call sites. - The `BooleanOr` true-context path receives the same fix for symmetry; a standalone reproducer for that path could not be constructed, but the check lives in the shared helper and is polarity-correct for both operators. --- .../ExprHandler/BooleanAndHandler.php | 8 +- src/Analyser/ExprHandler/BooleanOrHandler.php | 8 +- .../ConditionalExpressionHolderHelper.php | 38 ++++++--- tests/PHPStan/Analyser/nsrt/bug-14908.php | 82 +++++++++++++++++++ ...mpossibleCheckTypeFunctionCallRuleTest.php | 7 ++ ...rictComparisonOfDifferentTypesRuleTest.php | 6 ++ .../Rules/Comparison/data/bug-14908.php | 56 +++++++++++++ 7 files changed, 186 insertions(+), 19 deletions(-) create mode 100644 tests/PHPStan/Analyser/nsrt/bug-14908.php create mode 100644 tests/PHPStan/Rules/Comparison/data/bug-14908.php diff --git a/src/Analyser/ExprHandler/BooleanAndHandler.php b/src/Analyser/ExprHandler/BooleanAndHandler.php index 383b03c7d66..315d64042ee 100644 --- a/src/Analyser/ExprHandler/BooleanAndHandler.php +++ b/src/Analyser/ExprHandler/BooleanAndHandler.php @@ -152,10 +152,10 @@ public function specifyTypes(TypeSpecifier $typeSpecifier, Scope $scope, Expr $e $result = $result->setAlwaysOverwriteTypes(); } return $result->setNewConditionalExpressionHolders($this->conditionalExpressionHolderHelper->mergeConditionalHolders([ - $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $leftCondTypes, $rightHolderTypes, false, true, $rightScope, $expr->right), - $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $rightCondTypes, $leftHolderTypes, false, true, $scope, $expr->left), - $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $leftCondTypes, $rightHolderTypes, true, true, $rightScope, $expr->right), - $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $rightCondTypes, $leftHolderTypes, true, true, $scope, $expr->left), + $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $leftCondTypes, $rightHolderTypes, false, true, $rightScope, $expr->right, $expr->left), + $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $rightCondTypes, $leftHolderTypes, false, true, $scope, $expr->left, $expr->right), + $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $leftCondTypes, $rightHolderTypes, true, true, $rightScope, $expr->right, $expr->left), + $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $rightCondTypes, $leftHolderTypes, true, true, $scope, $expr->left, $expr->right), ]))->setRootExpr($expr); } diff --git a/src/Analyser/ExprHandler/BooleanOrHandler.php b/src/Analyser/ExprHandler/BooleanOrHandler.php index d439a2c8082..49b973b6cc0 100644 --- a/src/Analyser/ExprHandler/BooleanOrHandler.php +++ b/src/Analyser/ExprHandler/BooleanOrHandler.php @@ -126,10 +126,10 @@ public function specifyTypes(TypeSpecifier $typeSpecifier, Scope $scope, Expr $e $result = $result->setAlwaysOverwriteTypes(); } return $result->setNewConditionalExpressionHolders($this->conditionalExpressionHolderHelper->mergeConditionalHolders([ - $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $leftTypes, $rightTypes, false, false, $rightScope, $expr->right), - $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $rightTypes, $leftTypes, false, false, $scope, $expr->left), - $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $leftTypes, $rightTypes, true, false, $rightScope, $expr->right), - $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $rightTypes, $leftTypes, true, false, $scope, $expr->left), + $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $leftTypes, $rightTypes, false, false, $rightScope, $expr->right, $expr->left), + $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $rightTypes, $leftTypes, false, false, $scope, $expr->left, $expr->right), + $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $leftTypes, $rightTypes, true, false, $rightScope, $expr->right, $expr->left), + $this->conditionalExpressionHolderHelper->processBooleanConditionalTypes($scope, $rightTypes, $leftTypes, true, false, $scope, $expr->left, $expr->right), ]))->setRootExpr($expr); } diff --git a/src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php b/src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php index 3fb390488a5..eb58911a648 100644 --- a/src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php +++ b/src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php @@ -141,8 +141,23 @@ public function mergeConditionalHolders(array $holderLists): array /** * @return array */ - public function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $conditionSpecifiedTypes, SpecifiedTypes $holderSpecifiedTypes, bool $holdersFromSureTypes, bool $holderSideIsNegated, Scope $rightScope, ?Expr $holderSideExpr = null): array + public function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $conditionSpecifiedTypes, SpecifiedTypes $holderSpecifiedTypes, bool $holdersFromSureTypes, bool $holderSideIsNegated, Scope $rightScope, ?Expr $holderSideExpr = null, ?Expr $conditionSideExpr = null): array { + // The condition (antecedent) side asserts a truth value for its + // sub-expression, and the holder guard uses that side's per-expression + // narrowing as if it were equivalent to that truth value. When the + // condition side is a compound boolean whose asserted truth value is a + // disjunction (`$a || $b` asserted true, or `$a && $b` asserted false), + // the narrowing is only a necessary consequence of the truth value, not a + // sufficient one — e.g. `($a && $b) || ($c && $d)` being true narrows a + // shared variable, but that narrowing can hold without the disjunction + // being true. Using such an under-approximating narrowing as a guard fires + // the holder unsoundly, so no holder is built. This mirrors the holder-side + // check below with the opposite polarity. + if ($this->isUnsplittableCompoundSide($conditionSideExpr, !$holderSideIsNegated)) { + return []; + } + // 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 @@ -200,7 +215,7 @@ public function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $con // 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)) { + if ($this->isUnsplittableCompoundSide($holderSideExpr, $holderSideIsNegated)) { return []; } @@ -269,22 +284,23 @@ public function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $con } /** - * 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. + * A boolean operand whose asserted truth value is a disjunction cannot be + * decomposed into independent per-expression narrowings. That happens for a + * conjunction (`&&`) asserted false (its negation is a disjunction) and for a + * disjunction (`||`) asserted true. Applies to both the holder (consequent) + * side and the condition (antecedent) side. */ - private function isUnsplittableCompoundHolderSide(?Expr $holderSideExpr, bool $holderSideIsNegated): bool + private function isUnsplittableCompoundSide(?Expr $sideExpr, bool $isNegated): bool { - if ($holderSideExpr === null) { + if ($sideExpr === null) { return false; } - if ($holderSideIsNegated) { - return $holderSideExpr instanceof BooleanAnd || $holderSideExpr instanceof LogicalAnd; + if ($isNegated) { + return $sideExpr instanceof BooleanAnd || $sideExpr instanceof LogicalAnd; } - return $holderSideExpr instanceof BooleanOr || $holderSideExpr instanceof LogicalOr; + return $sideExpr instanceof BooleanOr || $sideExpr instanceof LogicalOr; } private function isTrackableExpression(Expr $expr): bool diff --git a/tests/PHPStan/Analyser/nsrt/bug-14908.php b/tests/PHPStan/Analyser/nsrt/bug-14908.php new file mode 100644 index 00000000000..59bd8a6c40b --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-14908.php @@ -0,0 +1,82 @@ += 8.1 + +declare(strict_types = 1); + +namespace Bug14908; + +use function PHPStan\Testing\assertType; + +enum Grade +{ + case One; + case Two; + case Three; +} + +enum Kind +{ + case K1; + case K2; + case K3; +} + +class Flags +{ + public bool $flagA = false; +} + +function run(Kind $kind, Grade $grade, Flags $flags, bool $extra, bool $cond): void +{ + $forced = false; + if ( + $grade !== Grade::Three + && $cond + && in_array($kind, [Kind::K1, Kind::K2], true) + && $flags->flagA === true + ) { + $forced = true; + } + + // Intermediate `if` narrowing ANOTHER value (`$extra === false`) inside a + // disjunction. The disjunction's truth is not captured by narrowing `$grade` + // alone, so the boolean-decomposition holder `$grade ⇒ $forced` must not be + // created — otherwise the narrowings from the first `if` leak into the block + // below. + if ( + $forced === false + && ( + ($grade === Grade::One && $extra === false) + || ($cond && $grade !== Grade::Three) + ) + ) { + throw new \Exception(); + } + + if ($grade !== Grade::Three) { + assertType('bool', $flags->flagA); + assertType('Bug14908\\Kind', $kind); + assertType('bool', $forced); + } +} + +// The narrowing must still fire when the guard genuinely selects the branch: +// `$forced === true` really does imply the first `if` was taken. +function soundNarrowing(Kind $kind, Grade $grade, Flags $flags, bool $cond): void +{ + $forced = false; + if ( + $grade !== Grade::Three + && $cond + && in_array($kind, [Kind::K1, Kind::K2], true) + && $flags->flagA === true + ) { + $forced = true; + } + + if ($forced === false) { + throw new \Exception(); + } + + assertType('true', $flags->flagA); + assertType('Bug14908\\Kind::K1|Bug14908\\Kind::K2', $kind); +} diff --git a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php index 5f357bd51b4..14951051263 100644 --- a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php @@ -1346,6 +1346,13 @@ public function testBug8980(): void $this->analyse([__DIR__ . '/data/bug-8980.php'], []); } + #[RequiresPhp('>= 8.1.0')] + public function testBug14908(): void + { + $this->treatPhpDocTypesAsCertain = true; + $this->analyse([__DIR__ . '/data/bug-14908.php'], []); + } + public function testBug6211(): void { $this->treatPhpDocTypesAsCertain = true; diff --git a/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php index 436a0125a5d..b7d6814744d 100644 --- a/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php @@ -1249,6 +1249,12 @@ public function testBug14878(): void $this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14878.php'], []); } + #[RequiresPhp('>= 8.1.0')] + public function testBug14908(): void + { + $this->analyse([__DIR__ . '/data/bug-14908.php'], []); + } + public function testBug14847(): void { $this->analyse([__DIR__ . '/data/bug-14847.php'], [ diff --git a/tests/PHPStan/Rules/Comparison/data/bug-14908.php b/tests/PHPStan/Rules/Comparison/data/bug-14908.php new file mode 100644 index 00000000000..4478ab4bcf3 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/bug-14908.php @@ -0,0 +1,56 @@ += 8.1 + +declare(strict_types = 1); + +namespace BugRule14908; + +enum Grade +{ + case One; + case Two; + case Three; +} + +enum Kind +{ + case K1; + case K2; + case K3; +} + +class Flags +{ + public bool $flagA = false; +} + +function run(Kind $kind, Grade $grade, Flags $flags, bool $extra, bool $cond): void +{ + $forced = false; + if ( + $grade !== Grade::Three + && $cond + && in_array($kind, [Kind::K1, Kind::K2], true) + && $flags->flagA === true + ) { + $forced = true; + } + + if ( + $forced === false + && ( + ($grade === Grade::One && $extra === false) + || ($cond && $grade !== Grade::Three) + ) + ) { + throw new \Exception(); + } + + if ($grade !== Grade::Three) { + if ($flags->flagA === false) { + throw new \Exception(); + } + if (in_array($kind, [Kind::K1, Kind::K2], true)) { + echo 'reachable'; + } + } +}