From 018294e3199f355ae6dcd60da404bda13578af30 Mon Sep 17 00:00:00 2001 From: phpstan-bot <79867460+phpstan-bot@users.noreply.github.com> Date: Fri, 17 Jul 2026 05:59:19 +0000 Subject: [PATCH] Skip boolean conditional holders whose target is an operand of the condition-side arm with an independent sibling operand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Pass the condition-side arm expression into `ConditionalExpressionHolderHelper::processBooleanConditionalTypes()` from `BooleanAndHandler` so the helper can tell which expressions the arm's truth depends on. - When building a `BooleanAnd` false-context conditional holder, skip a target that is an operand of the condition-side arm whenever that arm also narrows an independent sibling operand (neither an ancestor nor a descendant of the target). Example: for `$needle !== null && in_array($needle, $haystack)`, the arm's truthy narrowing carries `$haystack` being non-empty, which is necessary but not sufficient for `in_array()` to be true — firing `$needle === null` from `$haystack` being non-empty alone is unsound. - The existing sound narrowings where the sibling operand refers to the same access path (e.g. `isset($data['k'])` narrowing both `$data` and `$data['k']`) are preserved, because such an operand does capture the arm's truth. - Probed the `BooleanOr` true-context mirror: it only under-narrows there (no false positive), so no change was needed in `BooleanOrHandler`. --- .../ExprHandler/BooleanAndHandler.php | 8 +- .../ConditionalExpressionHolderHelper.php | 80 ++++++++++++++++++- tests/PHPStan/Analyser/nsrt/bug-14966.php | 65 +++++++++++++++ 3 files changed, 148 insertions(+), 5 deletions(-) create mode 100644 tests/PHPStan/Analyser/nsrt/bug-14966.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/Helper/ConditionalExpressionHolderHelper.php b/src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php index 3fb390488a5..d7757f650a0 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 00000000000..9b66ff58a2c --- /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); + } +}