Preserve maybe-certainty only for Variable when narrowing the base of ?->, ??, isset() and empty() - #6109
Conversation
…of `?->`, `??`, `isset()` and `empty()` - `NonNullabilityHelper::ensureShallowNonNullability()` copied the existing `Maybe` certainty of the narrowed expression onto the specified non-null type. For non-`Variable` expressions a `Maybe` certainty makes the narrowed type invisible to `Scope::getType()` (`ScopeOps::expressionTypeByKey()` only returns types of holders whose certainty is `Yes`), so the narrowing was silently thrown away. - Certainty is now only kept for `Variable` nodes, where `Maybe` still carries the "might not be defined" meaning consumed by `DefinedVariableRule` and where the narrowed type is still returned (variables resolve through `getVariableType()`, which ignores certainty). - Fixes the whole family at once: `NullsafeMethodCallHandler`, `NullsafePropertyFetchHandler`, `CoalesceHandler`, `IssetHandler` and `EmptyHandler` all go through the same helper. - Regression tests cover `?->` on repeated function calls, chained `?->prop?->prop?->foo()`, method-call and static-property receivers, and a `Maybe` certainty arriving from an unrelated scope merge (if/else, ternary, match, switch, try/catch, for/foreach/while/do-while, closure, `??`, multiple nullsafe calls in one argument list).
| function staticProperty(): void | ||
| { | ||
| $a = Acme::$stat?->foo(); | ||
| $b = Acme::$stat?->foo(); | ||
| } |
There was a problem hiding this comment.
static property fetches need to be tested by AccessStaticPropertiesRuleTest and static method calls by CallStaticMethodsRuleTest.
these tests can reference the existing file tests/PHPStan/Rules/Methods/data/bug-15002.php instead of creating another copy
There was a problem hiding this comment.
Pushed as 2d7a223e8.
What changed
AccessStaticPropertiesRuleTest::testBug15002()— analyses the existingtests/PHPStan/Rules/Methods/data/bug-15002.php(via/../Methods/data/...), so theAcme::$stat?->foo()cases are actually checked by the rule that owns static property fetches. No copy of the data file.CallStaticMethodsRuleTest::testBug15002()— same file, so static method calls are checked by their own rule.tests/PHPStan/Rules/Methods/data/bug-15002.php— addedAcme::create(): ?AcmeplusstaticMethodCall(),staticMethodCallChained()andstaticPropertyChained(), soCallStaticMethodsRulehas static calls to check rather than an empty file.
One thing worth flagging: the static-base cases do not reproduce the bug. Re-running with the source fix reverted, the false positives land only on function-call bases (getAcme()?->foo()); lines 37/43/49/55 (Acme::$stat?->…, Acme::create()?->…) are clean both before and after the fix — the count stays at 15 either way. So the two new tests, and the static cases in the data file, are coverage that these rules stay quiet on nullsafe bases; they are not regression tests for #15002 itself. The regression protection still comes solely from CallMethodsRuleTest/AccessPropertiesRuleTest.
Verification: the 15-error pre-fix failure was reconfirmed by checking out fdedb4565^'s NonNullabilityHelper.php. make tests (17632 tests, 97 skipped) and make phpstan are green.
Static property fetches are checked by AccessStaticPropertiesRule and static method calls by CallStaticMethodsRule, neither of which was run over the data file. Both test classes now analyse the existing tests/PHPStan/Rules/Methods/data/bug-15002.php instead of duplicating it, and the data file gained repeated `Acme::create()?->foo()` cases so the static method call rule has something to check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
getAcme()?->foo();twice in a row reportedCannot call method foo() on Acme|null.on the second occurrence, even though each call is guarded by?->. Assigning the call result to a variable first was a workaround.The cause was that the non-null narrowing PHPStan applies to the left side of a nullsafe operator was being stored with
Maybecertainty, which makes it invisible toScope::getType()for anything that is not aVariable. The fix restricts the certainty preservation toVariablenodes, which fixes the reported case plus a whole family of related false positives.Changes
src/Analyser/ExprHandler/Helper/NonNullabilityHelper.php:ensureShallowNonNullability()now only copies an existingMaybecertainty onto the narrowed expression when that expression is aPhpParser\Node\Expr\Variable; every other expression is specified withYes.tests/PHPStan/Rules/Methods/data/bug-15002.php+testBug15002()intests/PHPStan/Rules/Methods/CallMethodsRuleTest.php.tests/PHPStan/Rules/Properties/data/bug-15002.php+testBug15002()intests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php.Analogous cases probed and covered by the same fix (all were failing before, all pass now):
?->method call vs.?->property fetch (NullsafeMethodCallHandler/NullsafePropertyFetchHandler).getAcme()?->prop?->prop?->foo()andgetAcme()?->prop?->prop?->get().$o->get()?->foo(),Acme::$stat?->foo(), array-dim base.Maybecertainty other than the nullsafe scope merge itself — a narrowing of the same expression inside a nestedif, a ternary branch, amatcharm, aswitchcase, atry/catch,for/foreach/while/do-whilebodies, a closure body, and two nullsafe calls in the same argument list.??,isset()andempty()go through the same helper (CoalesceHandler,IssetHandler,EmptyHandler), so they are fixed by construction; probes with repeatedgetP()->prop ?? null,isset(getP()->prop->prop)andempty(...)behave correctly.getAcme()?->proprepeated on its own —NullsafePropertyFetchHandlerdoes not merge with the pre-nullsafe scope, so noMaybeentry was produced there.Root cause
NonNullabilityHelper::ensureShallowNonNullability()narrows the base expression of?->/??/isset()/empty()to its non-null form and records the original state so it can be reverted afterwards. Sincee300c7eaeit also carried over the expression's existing certainty:That was introduced so that a maybe-defined variable used as
$a?->foo()still gets reported byDefinedVariableRule. For variables it is harmless, becauseVariableHandler::resolveType()goes throughMutatingScope::getVariableType(), which returns the holder's type regardless of certainty.For every other expression it is destructive:
ScopeOps::expressionTypeByKey()— the pathMutatingScope::resolveType()uses for non-Variablenodes — only returns a tracked type when the holder's certainty isYes. WithMaybe, theAcme|null → Acmenarrowing is stored but never read back, so the rule seesAcme|nullagain.getAcme()?->foo()produced exactly such aMaybeentry:NullsafeMethodCallHandler::processExpr()merges the post-call scope (which has agetAcme()entry) with the pre-call scope (which has none) to model argument short-circuiting, and a merge of "present" with "absent" yieldsMaybe. The nextgetAcme()?->foo()then hit the broken branch. The sameMaybecan arrive from any scope merge — anif/else, a loop body, a ternary, amatcharm,try/catch— which is why the bug was much wider than the reported snippet.The fix keeps the certainty only where it is both meaningful and lossless (
Variable), and specifiesYeseverywhere else.Test
CallMethodsRuleTest::testBug15002()overtests/PHPStan/Rules/Methods/data/bug-15002.php— the verbatim reproducer from the issue's playground plus 14 analogous cases. Without the fix it reports 15method.nonObjectfalse positives; with the fix it analyses clean.AccessPropertiesRuleTest::testBug15002()overtests/PHPStan/Rules/Properties/data/bug-15002.php— the property-fetch counterparts. Without the fix it reports 2property.nonObjectfalse positives; with the fix it analyses clean.Both were verified to fail on the unpatched source.
make testsandmake phpstanare green. (make name-collisionfails ontests/PHPStan/Build/data/final-class-rule-pipe.phpboth with and without this change — pre-existing, unrelated.)Fixes phpstan/phpstan#15002