Skip @phpstan-assert, conditional return types and call effects when the nullsafe chain may short-circuit - #6486
phpstan-bot wants to merge 3 commits into
Conversation
VincentLanglet
left a comment
There was a problem hiding this comment.
Add a non regression bug for phpstan/phpstan#12925 too
|
Done — added the regression test for phpstan/phpstan#12925 and pushed it.
$c = rand() ? new C(new Decimal((string)rand())) : null;
assertType('Bug12925\C|null', $c);
echo $c?->p->isZero() ? 'Free' : 'Buying';
assertType('Bug12925\C|null', $c);
Verification, per the stash-the-fix protocol:
Two notes:
I left the two earlier review comments on this PR (whether |
…n the nullsafe chain may short-circuit
* `DefaultNarrowingHelper::callMayHaveBeenSkipped()` decides whether a plain
call whose receiver chain contains a `?->` may not have run at all in the
branch being narrowed. `MethodCallHandler::specifyTypes()` and
`StaticCallHandler::specifyTypes()` gate all callee-derived narrowing
(type-specifying extensions, conditional return types, `@phpstan-assert*`)
on it, so the short-circuit's `null` no longer gets narrowed as if the
method had returned `false`.
* Added `TypeSpecifierContext::falseyButNotFalse()`: the short-circuit yields
`null`, which is falsey but neither `true` nor `false`, so a strict
`=== false` context keeps its full narrowing while the wider falsey branch
and the statement (`null`) context lose it.
* `MethodCallHandler`/`StaticCallHandler` now merge the post-argument scope
with the pre-argument one and make the argument flow a `VariableFlow::choice`
when the chain may short-circuit, so `@param-out`, `@phpstan-self-out`,
expression invalidation and argument side effects no longer apply
unconditionally, and a `never`-returning callee no longer marks the following
statement unreachable.
* Same treatment for the dynamic sub-expression of the sibling fetches:
`PropertyFetchHandler` (`$a?->b->{$name}`), `StaticPropertyFetchHandler`
(`$a?->b::${$name}`) and `ArrayDimFetchHandler` (`$a?->b[$dim]`).
* `TypeCombinator::addNull()` returned `never` for `never` because `null` is a
supertype of `never`; it now returns `null`, which is what made the
short-circuit of a `never`-returning call invisible.
* Probed and found already correct: the `assert-if-true` / truthy branch,
`FuncCall` (a function call is never a link in a nullsafe chain) and
`NewHandler` asserts, and direct `$a?->foo()` narrowing (decomposed into a
conjunction by `NullsafeMethodCallHandler`).
…@phpstan-assert-if-false` The `@phpstan-assert-if-false self<false> $this` of a method called through `$c?->p->isZero()` was applied in the falsey branch of the ternary, which also covers the short-circuited world where `$c` is null - so after the ternary `$c` was narrowed to `C` instead of staying `C|null`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2355d10 to
9548a55
Compare
Summary
@phpstan-assert-if-false Zero $thison a method called through a nullsafe chain(
$lastPeriod?->price->isZero()) was applied in the whole falsey branch of theternary. But the falsey branch also contains the case where
$lastPeriodisnullandisZero()was never called, so PHPStan concluded$lastPeriod?->priceisZero— and therefore$lastPeriodis non-nullable,producing a
nullsafe.neverNullfalse positive on the next line.The fix drops every piece of narrowing and every scope effect a call's own
declaration implies whenever the branch being described still admits the
chain's short-circuit
null.Changes
src/Analyser/ExprHandler/Helper/DefaultNarrowingHelper.php— newcallMayHaveBeenSkipped(): the receiver result carries a?->, the receivertype contains
null, and the context still admitsnull.src/Analyser/TypeSpecifierContext.php— newfalseyButNotFalse(). Theshort-circuit produces
null, which is falsey but neithertruenorfalse,and the existing
false()/falsey()accessors cannot tell a strictfalsecontext from the wider falsey one.
src/Analyser/ExprHandler/MethodCallHandler.phpandsrc/Analyser/ExprHandler/StaticCallHandler.php—specifyTypes()gatestype-specifying extensions, conditional-return-type narrowing and
@phpstan-assert*oncallMayHaveBeenSkipped();processExpr()merges thepost-argument scope with the pre-argument one, turns the argument
VariableFlowinto achoice, and stops the callee'sneverreturn /early-terminating configuration from marking the statement terminating.
src/Analyser/ExprHandler/PropertyFetchHandler.php,src/Analyser/ExprHandler/StaticPropertyFetchHandler.php,src/Analyser/ExprHandler/ArrayDimFetchHandler.php— the same merge + flowchoice for the dynamic sub-expression that the short-circuit skips
(
$a?->b->{$name},$a?->b::${$name},$a?->b[$dim]).src/Type/TypeCombinator.php—addNull(never)now returnsnullinstead ofnever.Analogous cases fixed alongside the report:
@phpstan-assert-if-falseon$this@phpstan-assert-if-falseon a parameter@phpstan-assertat statement level$a?->b::c()($x is int ? true : false)$xnarrowed tomixed~intin the falsey branchmixed@param-out@phpstan-self-out@return nevercallee$ytreated as definedProbed and found already correct, so no change and no test kept: the
assert-if-true/ truthy branch and the strict=== falsecontext (bothgenuinely rule the short-circuit out),
FuncCallandNewHandlerassertions(neither is a link in a nullsafe chain), and direct
$a?->foo()narrowing,which
NullsafeMethodCallHandleralready decomposes into($a !== null) && $a->foo().Root cause
A nullsafe chain short-circuits the entire chain, so a plain
->,::or[]link written on top of a?->may never be evaluated.NullsafeMethodCall/
NullsafePropertyFetchhandle this for the link they own — the former evendecomposes into a conjunction — but the plain handlers stacked above them only
propagated the short-circuit into the expression's type
(
containsNullsafe+TypeCombinator::addNull). Everything else they derivedfrom the callee ran as if the call had definitely happened:
MethodCallHandler::specifyTypes()/StaticCallHandler::specifyTypes()(extensions, conditional return types,asserts),
@param-out,@phpstan-self-out, receiver invalidation, argumentevaluation,
neverreturn types and early-terminating calls.For narrowing the deciding question is whether the branch admits
null:truthy contexts and a strict
falsecontext exclude it (so they keep fullprecision), the wider falsey context and the statement context do not. For the
scope and flow effects the question is unconditional: if the chain may
short-circuit, the world where it did must be merged back in.
TypeCombinator::addNull()was a second instance of the same blind spot: itsguard
$nullType->isSuperTypeOf($type)->no()is false fornever(as it is forevery type,
nullbeing a supertype ofnever), so the short-circuit'snullwas swallowed for
never-returning callees.Test
tests/PHPStan/Analyser/nsrt/bug-15016.php— the playground reproducer plusassertType()coverage of the falsey branch, the strict=== falsebranch,the
assert-if-truebranch, the statement-level@phpstan-assert, parameterasserts through both
->and::, a conditional return type, and anon-nullable receiver (which must keep narrowing). 8 assertions fail without
the fix.
tests/PHPStan/Rules/Properties/NullsafePropertyFetchRuleTest::testBug15016with
data/bug-15016.php— the reportednullsafe.neverNullfalse positive.tests/PHPStan/Analyser/nsrt/nullsafe-short-circuit-effects.php—@param-outthrough->and::,@phpstan-self-out,neverreturn types,each paired with the non-nullable-receiver control case.
tests/PHPStan/Rules/DeadCode/UnreachableStatementRuleTest::testNullsafeShortCircuitTerminating— the
never-returning callee no longer kills the following statement, whilethe non-nullsafe and non-nullable-receiver variants still do.
tests/PHPStan/Rules/Variables/DefinedVariableRuleTest::testNullsafeShortCircuitArgs— argument / dynamic-name / array-dimension side effects are now reported as
possibly undefined for all five link kinds.
tests/PHPStan/Type/TypeCombinatorTest—addNull(never)isnull.Fixes phpstan/phpstan#15016
Fixes phpstan/phpstan#12925