Do not propagate MethodCall/StaticCall narrowing from nullsafe chain sub-expressions through conditional expressions#5495
Closed
phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
Conversation
…ain sub-expressions through conditional expressions - PR phpstan#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 phpstan#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
After PR #5447 (fixing phpstan/phpstan#9455), assigning a nullsafe chain result to a variable (
$x = $a?->b()?->c()) caused sub-expression method calls like$a->b()to be narrowed to non-null through conditional expressions. This caused false positives fromNullsafeMethodCallRule("Using nullsafe method call on non-nullable type") on subsequent uses of$a->b()?->....Changes
src/Analyser/ExprHandler/AssignHandler.php:ExprPrinteras a dependencycollectNullsafeProtectedExprStrings()helper that walks the assigned expression's nullsafe chain and collects expression strings ofMethodCall/StaticCallvar nodesprocessSureTypesForConditionalExpressionsAfterAssignandprocessSureNotTypesForConditionalExpressionsAfterAssignnow accept and check a$nullsafeProtectedExprStringsparameter, skipping entries that matchTest files added:
tests/PHPStan/Rules/Methods/data/bug-14493.php— rule test for the reported false positive (both instance method and static method variants)tests/PHPStan/Rules/Properties/data/bug-14493.php— analogous test forNullsafePropertyFetchchainstests/PHPStan/Analyser/nsrt/bug-14493.php— NSRT test confirming$order->getOrderCustomer()retainsOrderCustomerEntity|nulltype after nullsafe chain narrowingRoot cause
PR #5447 added
MethodCallandStaticCallto the expression types allowed in conditional expression propagation during variable assignment. This was correct for the direct case ($hasA = $b->getA() !== null), but it also allowed sub-expression narrowings from nullsafe chain decomposition to leak into the scope. When theTypeSpecifierprocesses a nullsafe chain like$a?->b()?->c(), it internally decomposes it and creates "not-null" specified types for intermediate expressions like$a->b(). These should remain internal to the chain and not become conditional expressions on the assigned variable.The fix identifies which expressions in the specified types are nullsafe-chain-internal by walking the assigned expression tree and collecting the
varnodes ofNullsafeMethodCall/NullsafePropertyFetchnodes. OnlyMethodCallandStaticCallvars are collected —VariableandPropertyFetchvars are left alone because their narrowing was already working correctly before PR #5447.Analogous cases probed
NullsafePropertyFetchchain ($x = $a?->b()?->prop): tested, already fixed by the same change since the helper walks bothNullsafeMethodCallandNullsafePropertyFetchnodesStaticCallin chain ($x = Foo::get()?->method()?->c()): tested in the main regression test dataVariablenarrowing from nullsafe chain ($x = $var?->prop): verified that existing tests (bug-10482, bug-6991) still pass — Variable narrowing is not affected by the fixPropertyFetchnarrowing from nullsafe chain ($x = $obj->prop?->key): verified via bug-6991 test — PropertyFetch narrowing is not affectedisset/??operators: these use different code paths (IssetCheck), not affected by this changeTest
testBug14493inNullsafeMethodCallRuleTest— false positive for both$order->getOrderCustomer()?->getVatIds()and$order::getStaticOrderCustomer()?->getVatIds()after nullsafe chain narrowingtestBug14493inNullsafePropertyFetchRuleTest— analogous case with property fetch chainsbug-14493.php— type assertions confirming the method call type remains nullableFixes phpstan/phpstan#14493