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/Helper/ConditionalExpressionHolderHelper.php b/src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php index 3fb390488a..d7757f650a 100644 --- a/src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php +++ b/src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php @@ -7,6 +7,7 @@ use PhpParser\Node\Expr\BinaryOp\BooleanOr; use PhpParser\Node\Expr\BinaryOp\LogicalAnd; use PhpParser\Node\Expr\BinaryOp\LogicalOr; +use PhpParser\NodeFinder; use PHPStan\Analyser\ConditionalExpressionHolder; use PHPStan\Analyser\ExpressionTypeHolder; use PHPStan\Analyser\MutatingScope; @@ -15,6 +16,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 +34,7 @@ final class ConditionalExpressionHolderHelper public function __construct( private TypeSpecifier $typeSpecifier, + private ExprPrinter $exprPrinter, ) { } @@ -141,7 +144,7 @@ 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 side asserts that its sub-expression evaluates truthy. // When that sub-expression is itself a compound boolean (e.g. `$a && $b`), @@ -191,6 +194,13 @@ public function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $con $holders = []; $holderTypes = $holdersFromSureTypes ? $holderSpecifiedTypes->getSureTypes() : $holderSpecifiedTypes->getSureNotTypes(); + // Trackable sub-expressions (operands) of the condition-side arm, used + // by the skip below to detect targets whose antecedent would collapse to + // an insufficient independent-sibling narrowing. + $conditionSideExprStrings = $conditionSideExpr !== null + ? $this->collectExpressionStrings($conditionSideExpr) + : []; + // 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 @@ -216,6 +226,17 @@ public function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $con continue; } + // The target is an operand of the condition-side arm (e.g. the needle + // of `in_array($needle, $haystack)`), so that arm's truth depends on + // the target. Building a holder for it drops the target's own + // self-condition and would keep only the arm's side narrowing of an + // independent sibling operand (e.g. `$haystack` being non-empty). That + // sibling is necessary but not sufficient for the arm to be true, so + // firing a consequent on the target from it alone is unsound. Skip it. + if ($this->conditionSideHasIndependentOperand($exprString, $conditionSideExprStrings)) { + continue; + } + $conditions = $conditionExpressionTypes; $droppedSelfCondition = null; foreach ($conditions as $conditionExprString => $condition) { @@ -298,4 +319,61 @@ private function isTrackableExpression(Expr $expr): bool || $expr instanceof Expr\StaticPropertyFetch; } + /** + * Every trackable sub-expression of $expr, keyed by its printed string so + * keys can be matched against SpecifiedTypes keys. + * + * @return array + */ + private function collectExpressionStrings(Expr $expr): array + { + $result = []; + foreach ((new NodeFinder())->findInstanceOf($expr, Expr::class) as $subExpr) { + if (!$this->isTrackableExpression($subExpr)) { + continue; + } + + $result[$this->exprPrinter->printExpr($subExpr)] = $subExpr; + } + + return $result; + } + + /** + * The condition-side arm mentions an operand that is structurally independent + * of $targetExprString (neither an ancestor nor a descendant of it). Such an + * operand's narrowing is only a side effect of the arm being true, never a + * condition sufficient to establish it. + * + * @param array $conditionSideExprStrings + */ + private function conditionSideHasIndependentOperand(string $targetExprString, array $conditionSideExprStrings): bool + { + if (!array_key_exists($targetExprString, $conditionSideExprStrings)) { + return false; + } + + $targetSubExprStrings = $this->collectExpressionStrings($conditionSideExprStrings[$targetExprString]); + + foreach ($conditionSideExprStrings as $operandExprString => $operandExpr) { + if ($operandExprString === $targetExprString) { + continue; + } + + // Descendant of the target (e.g. `$data` inside `$data['x']`) or an + // ancestor of it — either way the operand refers to the same access + // path, so it does capture the arm's truth. Not independent. + if ( + array_key_exists($operandExprString, $targetSubExprStrings) + || array_key_exists($targetExprString, $this->collectExpressionStrings($operandExpr)) + ) { + continue; + } + + return true; + } + + return false; + } + } diff --git a/tests/PHPStan/Analyser/nsrt/bug-14966.php b/tests/PHPStan/Analyser/nsrt/bug-14966.php new file mode 100644 index 0000000000..9b66ff58a2 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-14966.php @@ -0,0 +1,65 @@ + $haystack + */ +function resolve(?string $needle, array $haystack): string +{ + if ($needle !== null && in_array($needle, $haystack)) { + return $needle; + } elseif ($haystack !== []) { + // Reaching here does not imply $needle is null: the `&&` is also false + // when $needle is a non-null string that simply is not in $haystack. + assertType('string|null', $needle); + if ($needle !== null) { + return 'other:' . $needle; + } + return 'null-branch'; + } + + return 'empty'; +} + +function plainArray(?string $needle, array $haystack): void +{ + if ($needle !== null && in_array($needle, $haystack)) { + return; + } + + if ($haystack !== []) { + assertType('string|null', $needle); + } +} + +/** + * @param list $haystack + */ +function intNeedle(?int $needle, array $haystack): void +{ + if ($needle !== null && in_array($needle, $haystack)) { + return; + } + + if ($haystack !== []) { + assertType('int|null', $needle); + } +} + +/** + * @param list $haystack + */ +function strictStaysNarrowed(?string $needle, array $haystack): void +{ + if ($needle !== null && in_array($needle, $haystack, true)) { + return; + } + + if ($haystack !== []) { + assertType('string|null', $needle); + } +}