From 329c41cf7bd25839e736e9815fa1aa1e22e6f2ea Mon Sep 17 00:00:00 2001 From: staabm <120441+staabm@users.noreply.github.com> Date: Sun, 19 Apr 2026 06:31:55 +0000 Subject: [PATCH] Do not propagate `MethodCall`/`StaticCall` narrowing from nullsafe chain sub-expressions through conditional expressions - PR #5447 added MethodCall/StaticCall to the allowed expression types in processSureTypesForConditionalExpressionsAfterAssign and processSureNotTypesForConditionalExpressionsAfterAssign. This correctly fixed bug-9455 ($hasA = $b->getA() !== null; if ($hasA) { ... }). - However, it also caused nullsafe chain sub-expressions to leak into the broader scope: $x = $a?->b()?->c() would narrow $a->b() to non-null via conditional expressions, causing "nullsafe on non-nullable type" false positives on subsequent $a->b()?->... calls. - Fix: walk the assigned expression's nullsafe chain and collect exprStrings of MethodCall/StaticCall var nodes. Skip these in conditional expression propagation. Variable and PropertyFetch vars are not affected (they were already in the allowed list before PR #5447 and their narrowing is correct). - Also tested the NullsafePropertyFetch analogous case (already fixed by the same change) and verified bug-9455/bug-5207 still pass. --- src/Analyser/ExprHandler/AssignHandler.php | 58 ++++++++++++----- tests/PHPStan/Analyser/nsrt/bug-14493.php | 63 +++++++++++++++++++ .../Methods/NullsafeMethodCallRuleTest.php | 6 ++ .../PHPStan/Rules/Methods/data/bug-14493.php | 63 +++++++++++++++++++ .../NullsafePropertyFetchRuleTest.php | 6 ++ .../Rules/Properties/data/bug-14493.php | 38 +++++++++++ 6 files changed, 220 insertions(+), 14 deletions(-) create mode 100644 tests/PHPStan/Analyser/nsrt/bug-14493.php create mode 100644 tests/PHPStan/Rules/Methods/data/bug-14493.php create mode 100644 tests/PHPStan/Rules/Properties/data/bug-14493.php diff --git a/src/Analyser/ExprHandler/AssignHandler.php b/src/Analyser/ExprHandler/AssignHandler.php index 3ea0196e4aa..1a3ddcf0b71 100644 --- a/src/Analyser/ExprHandler/AssignHandler.php +++ b/src/Analyser/ExprHandler/AssignHandler.php @@ -42,6 +42,7 @@ use PHPStan\Node\Expr\SetExistingOffsetValueTypeExpr; use PHPStan\Node\Expr\SetOffsetValueTypeExpr; use PHPStan\Node\Expr\TypeExpr; +use PHPStan\Node\Printer\ExprPrinter; use PHPStan\Node\PropertyAssignNode; use PHPStan\Node\VariableAssignNode; use PHPStan\Php\PhpVersion; @@ -84,6 +85,7 @@ final class AssignHandler implements ExprHandler public function __construct( private TypeSpecifier $typeSpecifier, private PhpVersion $phpVersion, + private ExprPrinter $exprPrinter, ) { } @@ -242,6 +244,7 @@ public function processAssignVar( $type = $scopeBeforeAssignEval->getType($assignedExpr); $conditionalExpressions = []; + $nullsafeProtectedExprStrings = $this->collectNullsafeProtectedExprStrings($assignedExpr); if ($assignedExpr instanceof Ternary) { $if = $assignedExpr->if; if ($if === null) { @@ -259,23 +262,23 @@ public function processAssignVar( $truthyType->isSuperTypeOf($falseyType)->no() && $falseyType->isSuperTypeOf($truthyType)->no() ) { - $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType); - $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType); - $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType); - $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType); + $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType, $nullsafeProtectedExprStrings); + $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType, $nullsafeProtectedExprStrings); + $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType, $nullsafeProtectedExprStrings); + $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($condScope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType, $nullsafeProtectedExprStrings); } } $truthyType = TypeCombinator::removeFalsey($type); if ($truthyType !== $type) { $truthySpecifiedTypes = $this->typeSpecifier->specifyTypesInCondition($scope, $assignedExpr, TypeSpecifierContext::createTruthy()); - $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType); - $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType); + $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType, $nullsafeProtectedExprStrings); + $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType, $nullsafeProtectedExprStrings); $falseyType = TypeCombinator::intersect($type, StaticTypeFactory::falsey()); $falseySpecifiedTypes = $this->typeSpecifier->specifyTypesInCondition($scope, $assignedExpr, TypeSpecifierContext::createFalsey()); - $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType); - $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType); + $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType, $nullsafeProtectedExprStrings); + $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType, $nullsafeProtectedExprStrings); } foreach ([null, false, 0, 0.0, '', '0', []] as $falseyScalar) { @@ -304,13 +307,13 @@ public function processAssignVar( $notIdenticalConditionExpr = new Expr\BinaryOp\NotIdentical($assignedExpr, $astNode); $notIdenticalSpecifiedTypes = $this->typeSpecifier->specifyTypesInCondition($scope, $notIdenticalConditionExpr, TypeSpecifierContext::createTrue()); - $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $notIdenticalSpecifiedTypes, $withoutFalseyType); - $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $notIdenticalSpecifiedTypes, $withoutFalseyType); + $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $notIdenticalSpecifiedTypes, $withoutFalseyType, $nullsafeProtectedExprStrings); + $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $notIdenticalSpecifiedTypes, $withoutFalseyType, $nullsafeProtectedExprStrings); $identicalConditionExpr = new Expr\BinaryOp\Identical($assignedExpr, $astNode); $identicalSpecifiedTypes = $this->typeSpecifier->specifyTypesInCondition($scope, $identicalConditionExpr, TypeSpecifierContext::createTrue()); - $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $identicalSpecifiedTypes, $falseyType); - $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $identicalSpecifiedTypes, $falseyType); + $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $identicalSpecifiedTypes, $falseyType, $nullsafeProtectedExprStrings); + $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $identicalSpecifiedTypes, $falseyType, $nullsafeProtectedExprStrings); } $nodeScopeResolver->callNodeCallback($nodeCallback, new VariableAssignNode($var, $assignedExpr), $scopeBeforeAssignEval, $storage); @@ -848,9 +851,10 @@ private function unwrapAssign(Expr $expr): Expr /** * @param array $conditionalExpressions + * @param array $nullsafeProtectedExprStrings * @return array */ - private function processSureTypesForConditionalExpressionsAfterAssign(Scope $scope, string $variableName, array $conditionalExpressions, SpecifiedTypes $specifiedTypes, Type $variableType): array + private function processSureTypesForConditionalExpressionsAfterAssign(Scope $scope, string $variableName, array $conditionalExpressions, SpecifiedTypes $specifiedTypes, Type $variableType, array $nullsafeProtectedExprStrings = []): array { foreach ($specifiedTypes->getSureTypes() as $exprString => [$expr, $exprType]) { if ($expr instanceof Variable) { @@ -871,6 +875,10 @@ private function processSureTypesForConditionalExpressionsAfterAssign(Scope $sco continue; } + if (isset($nullsafeProtectedExprStrings[$exprString])) { + continue; + } + if (!isset($conditionalExpressions[$exprString])) { $conditionalExpressions[$exprString] = []; } @@ -889,9 +897,10 @@ private function processSureTypesForConditionalExpressionsAfterAssign(Scope $sco /** * @param array $conditionalExpressions + * @param array $nullsafeProtectedExprStrings * @return array */ - private function processSureNotTypesForConditionalExpressionsAfterAssign(Scope $scope, string $variableName, array $conditionalExpressions, SpecifiedTypes $specifiedTypes, Type $variableType): array + private function processSureNotTypesForConditionalExpressionsAfterAssign(Scope $scope, string $variableName, array $conditionalExpressions, SpecifiedTypes $specifiedTypes, Type $variableType, array $nullsafeProtectedExprStrings = []): array { foreach ($specifiedTypes->getSureNotTypes() as $exprString => [$expr, $exprType]) { if ($expr instanceof Variable) { @@ -912,6 +921,10 @@ private function processSureNotTypesForConditionalExpressionsAfterAssign(Scope $ continue; } + if (isset($nullsafeProtectedExprStrings[$exprString])) { + continue; + } + if (!isset($conditionalExpressions[$exprString])) { $conditionalExpressions[$exprString] = []; } @@ -928,6 +941,23 @@ private function processSureNotTypesForConditionalExpressionsAfterAssign(Scope $ return $conditionalExpressions; } + /** + * @return array + */ + private function collectNullsafeProtectedExprStrings(Expr $expr): array + { + $result = []; + $current = $expr; + while ($current instanceof Expr\NullsafeMethodCall || $current instanceof Expr\NullsafePropertyFetch) { + $var = $current->var; + if ($var instanceof MethodCall || $var instanceof Expr\StaticCall) { + $result[$this->exprPrinter->printExpr($var)] = true; + } + $current = $var; + } + return $result; + } + /** * @param list $dimFetchStack */ diff --git a/tests/PHPStan/Analyser/nsrt/bug-14493.php b/tests/PHPStan/Analyser/nsrt/bug-14493.php new file mode 100644 index 00000000000..e9cdd17c072 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-14493.php @@ -0,0 +1,63 @@ += 8.0 + +declare(strict_types = 1); + +namespace Bug14493Nsrt; + +use function PHPStan\Testing\assertType; + +class OrderEntity { + static public function getStaticOrderCustomer(): ?OrderCustomerEntity + { + return new OrderCustomerEntity(); + } + + public function getOrderCustomer(): ?OrderCustomerEntity + { + return new OrderCustomerEntity(); + } +} + +class OrderCustomerEntity { + public function getCustomer(): ?CustomerEntity { return null; } + /** @return array|null */ + public function getVatIds(): ?array { return null; } +} + +class CustomerEntity { + final public const ACCOUNT_TYPE_BUSINESS = 'business'; + + public function getAccountType(): string { return ''; } +} + +class TypeTest +{ + public function doFoo(OrderEntity $order): void + { + $customerType = $order->getOrderCustomer()?->getCustomer()?->getAccountType(); + assertType('string|null', $customerType); + + if ($customerType !== CustomerEntity::ACCOUNT_TYPE_BUSINESS) { + return; + } + + assertType("'business'", $customerType); + + // The method still returns nullable - the nullsafe chain narrowing + // should not leak to the broader scope for subsequent calls + assertType('Bug14493Nsrt\OrderCustomerEntity|null', $order->getOrderCustomer()); + } + + public function doBar(OrderEntity $order): void + { + $customerType = $order::getStaticOrderCustomer()?->getCustomer()?->getAccountType(); + assertType('string|null', $customerType); + + if ($customerType !== CustomerEntity::ACCOUNT_TYPE_BUSINESS) { + return; + } + + assertType("'business'", $customerType); + assertType('Bug14493Nsrt\OrderCustomerEntity|null', $order::getStaticOrderCustomer()); + } +} diff --git a/tests/PHPStan/Rules/Methods/NullsafeMethodCallRuleTest.php b/tests/PHPStan/Rules/Methods/NullsafeMethodCallRuleTest.php index 0d1bc3e1b6e..2941692d9ce 100644 --- a/tests/PHPStan/Rules/Methods/NullsafeMethodCallRuleTest.php +++ b/tests/PHPStan/Rules/Methods/NullsafeMethodCallRuleTest.php @@ -80,4 +80,10 @@ public function testBug12222(): void $this->analyse([__DIR__ . '/data/bug-12222.php'], []); } + #[RequiresPhp('>= 8.0.0')] + public function testBug14493(): void + { + $this->analyse([__DIR__ . '/data/bug-14493.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Methods/data/bug-14493.php b/tests/PHPStan/Rules/Methods/data/bug-14493.php new file mode 100644 index 00000000000..6c6051db73a --- /dev/null +++ b/tests/PHPStan/Rules/Methods/data/bug-14493.php @@ -0,0 +1,63 @@ += 8.0 + +declare(strict_types=1); + +namespace Bug14493; + +class OrderEntity { + static public function getStaticOrderCustomer(): ?OrderCustomerEntity + { + return new OrderCustomerEntity(); + } + + public function getOrderCustomer(): ?OrderCustomerEntity + { + return new OrderCustomerEntity(); + } +} + +class OrderCustomerEntity { + public function getCustomer(): ?CustomerEntity { return null; } + /** @return array|null */ + public function getVatIds(): ?array { return null; } +} + +class CustomerEntity { + final public const ACCOUNT_TYPE_BUSINESS = 'business'; + + public function getAccountType(): string { return ''; } +} + + +abstract class AbstractDocumentRenderer +{ + protected function doFoo(OrderEntity $order): bool + { + $customerType = $order->getOrderCustomer()?->getCustomer()?->getAccountType(); + if ($customerType !== CustomerEntity::ACCOUNT_TYPE_BUSINESS) { + return false; + } + + $vatIds = $order->getOrderCustomer()?->getVatIds(); + if (!is_array($vatIds)) { + return false; + } + + return true; + } + + protected function doBar(OrderEntity $order): bool + { + $customerType = $order::getStaticOrderCustomer()?->getCustomer()?->getAccountType(); + if ($customerType !== CustomerEntity::ACCOUNT_TYPE_BUSINESS) { + return false; + } + + $vatIds = $order::getStaticOrderCustomer()?->getVatIds(); + if (!is_array($vatIds)) { + return false; + } + + return true; + } +} diff --git a/tests/PHPStan/Rules/Properties/NullsafePropertyFetchRuleTest.php b/tests/PHPStan/Rules/Properties/NullsafePropertyFetchRuleTest.php index e4c37b08ca0..4dfd535fb16 100644 --- a/tests/PHPStan/Rules/Properties/NullsafePropertyFetchRuleTest.php +++ b/tests/PHPStan/Rules/Properties/NullsafePropertyFetchRuleTest.php @@ -81,4 +81,10 @@ public function testBug6922(): void $this->analyse([__DIR__ . '/data/bug-6922.php'], []); } + #[RequiresPhp('>= 8.0.0')] + public function testBug14493(): void + { + $this->analyse([__DIR__ . '/data/bug-14493.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Properties/data/bug-14493.php b/tests/PHPStan/Rules/Properties/data/bug-14493.php new file mode 100644 index 00000000000..46690278526 --- /dev/null +++ b/tests/PHPStan/Rules/Properties/data/bug-14493.php @@ -0,0 +1,38 @@ += 8.0 + +declare(strict_types=1); + +namespace Bug14493NullsafeProperty; + +class Inner { + public string $value = ''; + public ?string $nullableValue = null; +} + +class Middle { + public ?Inner $inner = null; + public function getInner(): ?Inner { return $this->inner; } +} + +class Outer { + public function getMiddle(): ?Middle { return null; } +} + +class TestPropertyFetch +{ + public function doFoo(Outer $outer): bool + { + $value = $outer->getMiddle()?->getInner()?->value; + if ($value !== 'expected') { + return false; + } + + // After narrowing, $outer->getMiddle() should NOT be flagged + $nullableValue = $outer->getMiddle()?->inner?->nullableValue; + if ($nullableValue === null) { + return false; + } + + return true; + } +}