diff --git a/src/Analyser/ExprHandler/BooleanAndHandler.php b/src/Analyser/ExprHandler/BooleanAndHandler.php index 383b03c7d6..315d64042e 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 d439a2c808..49b973b6cc 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 3fb390488a..49c9c1bdb9 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\Node\Printer\ExprPrinter; use PHPStan\Type\NeverType; use PHPStan\Type\TypeCombinator; use function array_key_exists; @@ -32,6 +33,7 @@ final class ConditionalExpressionHolderHelper public function __construct( private TypeSpecifier $typeSpecifier, + private ExprPrinter $exprPrinter, ) { } @@ -141,8 +143,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 +217,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 []; } @@ -230,6 +247,21 @@ public function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $con continue; } + // The guard (the remaining conditions) must actually identify the + // asserted truth value of the condition side. For a relational predicate + // like `in_array($needle, $haystack)`, the truthy narrowing of an argument + // (`$haystack` becoming non-empty) is only "`$haystack` is truthy" — a + // necessary but not sufficient consequence — so re-asserting it (e.g. + // `$haystack !== []`) would fire this holder even though the predicate is + // false. Skip such an under-approximating guard. + if ( + $conditionSideExpr !== null + && $scope instanceof MutatingScope + && !$this->guardIdentifiesConditionSide($scope, $conditions, $conditionSideExpr, $expr, $holderSideIsNegated) + ) { + continue; + } + $targetScope = $expr instanceof Expr\Variable ? $scope : $rightScope; $targetType = $targetScope->getType($expr); $holderType = $holdersFromSureTypes @@ -269,22 +301,78 @@ 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; + } + + /** + * Decides whether the guard (the conditions of a single holder, after the + * self-condition has been dropped) identifies the asserted truth value of the + * condition side ($conditionAssertedTrue = true when the holder assumes the + * condition side is true, false when it assumes false). The guard is identifying + * when any of the following holds: + * + * 1. a condition narrows its expression beyond plain truthiness/falsiness — such + * a narrowing pins down a real value rather than "the expression is truthy"; + * 2. the consequent is an offset of a guarded container (the + * isset()/array_key_exists() shape `$data = ... => $data[$key] = ...`), whose + * truth PHPStan cannot always re-derive from the offset value type alone + * (e.g. dynamic keys); + * 3. re-applying the guard forces the condition side to its asserted truth value. + * + * A guard that is only "this expression is truthy/falsy" (e.g. `in_array()` + * making its haystack non-empty) satisfies none of these and identifies nothing. + * + * @param array $conditions + */ + private function guardIdentifiesConditionSide(MutatingScope $scope, array $conditions, Expr $conditionSideExpr, Expr $targetExpr, bool $conditionAssertedTrue): bool + { + foreach ($conditions as $condition) { + $accessoryScope = $conditionAssertedTrue + ? $scope->filterByTruthyValue($condition->getExpr()) + : $scope->filterByFalseyValue($condition->getExpr()); + if (!$accessoryScope->getType($condition->getExpr())->equals($condition->getType())) { + return true; + } + } + + if ( + $targetExpr instanceof Expr\ArrayDimFetch + && array_key_exists($this->exprPrinter->printExpr($targetExpr->var), $conditions) + ) { + return true; + } + + $sureTypes = []; + foreach ($conditions as $exprString => $condition) { + $sureTypes[$exprString] = [$condition->getExpr(), $condition->getType()]; + } + + // filterBySpecifiedTypes applies the narrowings container-before-offset and + // intersects them, so an offset guard (`$data['k'] = mixed~null`) is not + // clobbered by its container guard (`$data = ...hasOffset('k')`). Applying + // them naively one by one would drop the offset narrowing. + $guardScope = $scope->filterBySpecifiedTypes(new SpecifiedTypes($sureTypes, [])); + $conditionSideType = $guardScope->getType($conditionSideExpr)->toBoolean(); + + return $conditionAssertedTrue + ? $conditionSideType->isTrue()->yes() + : $conditionSideType->isFalse()->yes(); } 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 0000000000..59bd8a6c40 --- /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/Analyser/nsrt/bug-14966.php b/tests/PHPStan/Analyser/nsrt/bug-14966.php new file mode 100644 index 0000000000..f73d6159f5 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-14966.php @@ -0,0 +1,40 @@ + $haystack + */ +function resolve(?string $needle, array $haystack): string +{ + if ($needle !== null && in_array($needle, $haystack)) { + return $needle; + } elseif ($haystack !== []) { + // The `&&` can also be false when `$needle` is a non-null string that is + // simply not in `$haystack`, so re-narrowing `$haystack` to non-empty here + // must not leak `$needle = null` out of the first `if`. + assertType('string|null', $needle); + if ($needle !== null) { + return 'other:' . $needle; + } + return 'null-branch'; + } + + return 'empty'; +} + +/** + * The narrowing must still fire when the guard genuinely selects the branch: + * re-asserting the same `$needle !== null` really does imply `in_array()` decided + * the first `if`, so the sound projection is preserved. + * + * @param list $haystack + */ +function soundNarrowing(?string $needle, array $haystack): void +{ + if ($needle !== null && in_array($needle, $haystack, true)) { + assertType('string', $needle); + } +} diff --git a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php index 5f357bd51b..1495105126 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 436a0125a5..6f6246cf48 100644 --- a/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php @@ -1249,6 +1249,17 @@ 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 testBug14966(): void + { + $this->analyse([__DIR__ . '/data/bug-14966.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 0000000000..4478ab4bcf --- /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'; + } + } +} diff --git a/tests/PHPStan/Rules/Comparison/data/bug-14966.php b/tests/PHPStan/Rules/Comparison/data/bug-14966.php new file mode 100644 index 0000000000..524fc75d3a --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/bug-14966.php @@ -0,0 +1,21 @@ + $haystack + */ +function resolve(?string $needle, array $haystack): string +{ + if ($needle !== null && in_array($needle, $haystack)) { + return $needle; + } elseif ($haystack !== []) { + // $needle is still `string|null` here, but PHPStan narrowed it to `null`. + if ($needle !== null) { // <-- reported "always false" + return 'other:' . $needle; + } + return 'null-branch'; + } + + return 'empty'; +}