Skip @phpstan-assert, conditional return types and call effects when the nullsafe chain may short-circuit

[phpstan-bot]

Summary

@phpstan-assert-if-false Zero $this on a method called through a nullsafe chain
($lastPeriod?->price->isZero()) was applied in the whole falsey branch of the
ternary. But the falsey branch also contains the case where $lastPeriod is
null and isZero() was never called, so PHPStan concluded
$lastPeriod?->price is Zero — and therefore $lastPeriod is non-nullable,
producing a nullsafe.neverNull false 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 — new
  callMayHaveBeenSkipped(): the receiver result carries a ?->, the receiver
  type contains null, and the context still admits null.
• src/Analyser/TypeSpecifierContext.php — new falseyButNotFalse(). The
  short-circuit produces null, which is falsey but neither true nor false,
  and the existing false() / falsey() accessors cannot tell a strict false
  context from the wider falsey one.
• src/Analyser/ExprHandler/MethodCallHandler.php and
  src/Analyser/ExprHandler/StaticCallHandler.php — specifyTypes() gates
  type-specifying extensions, conditional-return-type narrowing and
  @phpstan-assert* on callMayHaveBeenSkipped(); processExpr() merges the
  post-argument scope with the pre-argument one, turns the argument
  VariableFlow into a choice, and stops the callee's never return /
  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 + flow
  choice 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 returns null instead of
  never.

Analogous cases fixed alongside the report:

case	before	after
@phpstan-assert-if-false on $this	receiver narrowed non-null	untouched
@phpstan-assert-if-false on a parameter	parameter narrowed	untouched
plain @phpstan-assert at statement level	subject narrowed	untouched
same three through $a?->b::c()	narrowed	untouched
conditional return type ($x is int ? true : false)	$x narrowed to mixed~int in the falsey branch	mixed
@param-out	applied unconditionally	merged with the skipped world
@phpstan-self-out	applied unconditionally	merged with the skipped world
@return never callee	next statement reported unreachable	reachable
side effects in arguments	$y treated as defined	"might not be defined"
side effects in a dynamic property name / static property name / array dimension	same	same

Probed and found already correct, so no change and no test kept: the
assert-if-true / truthy branch and the strict === false context (both
genuinely rule the short-circuit out), FuncCall and NewHandler assertions
(neither is a link in a nullsafe chain), and direct $a?->foo() narrowing,
which NullsafeMethodCallHandler already 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
/ NullsafePropertyFetch handle this for the link they own — the former even
decomposes 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 derived
from the callee ran as if the call had definitely happened:

• narrowing: MethodCallHandler::specifyTypes() /
  StaticCallHandler::specifyTypes() (extensions, conditional return types,
  asserts),
• scope: @param-out, @phpstan-self-out, receiver invalidation, argument
  evaluation,
• control flow: never return types and early-terminating calls.

For narrowing the deciding question is whether the branch admits null:
truthy contexts and a strict false context exclude it (so they keep full
precision), 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: its
guard $nullType->isSuperTypeOf($type)->no() is false for never (as it is for
every type, null being a supertype of never), so the short-circuit's null
was swallowed for never-returning callees.

Test

• tests/PHPStan/Analyser/nsrt/bug-15016.php — the playground reproducer plus
  assertType() coverage of the falsey branch, the strict === false branch,
  the assert-if-true branch, the statement-level @phpstan-assert, parameter
  asserts through both -> and ::, a conditional return type, and a
  non-nullable receiver (which must keep narrowing). 8 assertions fail without
  the fix.
• tests/PHPStan/Rules/Properties/NullsafePropertyFetchRuleTest::testBug15016
  with data/bug-15016.php — the reported nullsafe.neverNull false positive.
• tests/PHPStan/Analyser/nsrt/nullsafe-short-circuit-effects.php —
  @param-out through -> and ::, @phpstan-self-out, never return 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, while
  the 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) is null.

Fixes phpstan/phpstan#15016
Fixes phpstan/phpstan#12925

[VincentLanglet]

Add a non regression bug for phpstan/phpstan#12925 too

[phpstan-bot]

Done — added the regression test for phpstan/phpstan#12925 and pushed it.

tests/PHPStan/Analyser/nsrt/bug-12925.php (commit 2df59b08d) — the OP's playground snippet (118bf08a…) copied close to verbatim, with \PHPStan\dumpType() swapped for assertType():

$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);

isZero() carries both @phpstan-assert-if-true self<true> $this and @phpstan-assert-if-false self<false> $this; before the fix the if-false assert was applied in the ternary's falsey branch, which also contains the short-circuited world where $c is null, so both branches narrowed $c to non-null and the post-ternary type collapsed to C.

Verification, per the stash-the-fix protocol:

• With the fix reverted (git show 538518c90 -- src | git apply -R): line 25 fails with Expected: Bug12925\C|null / Actual: Bug12925\C — the exact symptom from the issue. Line 23 passes, confirming the failure is the narrowing, not the setup.
• With the fix restored: passes.
• make tests — OK, 21958 tests, 97570 assertions, 96 skipped (pre-existing skips).
• make phpstan — no errors.

Two notes:

• The file lives in nsrt/ because the bug is a wrong inferred type; no rule reports an error on this snippet, so there's no rule-test counterpart to add (the nullsafe.neverNull false positive from the sibling issue #15016 already has one in NullsafePropertyFetchRuleTest).
• nsrt/ is excluded from phpcs (phpcs.xml:127), so the reproducer's original formatting — single-line constructor, promoted property — is kept verbatim rather than reformatted.

I left the two earlier review comments on this PR (whether falseyButNotFalse() can be expressed with falsey()/false(), and shortening the callMayHaveBeenSkipped() docblock) untouched, since this round only asked for the #12925 test — say the word if you want those addressed in this push too.

[staabm]

I think this makes sense. as long as CI is red I find it hard to reason about such changes tbh.

we should at least locally verify that this change does not regress performance (run phpbench locally)

[VincentLanglet]

Benchmark/Tests Ci seems to tell that there is a major performance impact because of this PR. PHPUnit tests are also slower (from 1m30 to 1m45).

Analyse why

[phpstan-bot]

Pushed as e9d7462a0 and b60e5a23c.

Analysis: the reported regression does not reproduce

I measured both claims against base a85727903.

Benchmark — all 81 tests/bench/data files, interleaved A/B, min-of-2:

base = 112.84s    head = 113.21s    +0.37s (+0.33%)

nullsafe-chain-walk.php — the one bench file built entirely from ?-> chains, where every new branch in this PR fires on all ~250 links — is +0.0% (2.06 → 2.06, min-of-3). If this PR had a per-node cost, that file is where it would show.

make tests — interleaved, alternating, warm-up discarded:

rep	base	head
1	66.1s	60.0s
2	65.2s	65.5s
min	65.2s	60.0s

Run-to-run spread on a single revision is ~6s, which swamps the 15s delta. Head is the faster side here.

Why CI said otherwise. My own first attempt reproduced the regression — +2.9% overall — by running the two revisions as two sequential batches. The signature was a near-constant +0.10s on every sub-1.2s file and +0.00s elsewhere: batch drift, not per-node cost. It vanished entirely under interleaving. bench.yml has the same structure — the baseline is produced by a separate job (--dump-file=baseline-<php>.xml) and compared against a PR run on a different runner, so a flat ~100ms offset reads as +8–14% on the many small bench files. The outliers in my own sequential run (bug-5390.php +13.7%, bug-12800.php +11.6%) were all exactly +0.10s absolute.

I'd treat the CI numbers as inconclusive unless a baseline re-run on the unchanged base reproduces them.

Two real inefficiencies found and fixed

e9d7462a0 — PropertyFetchHandler and StaticPropertyFetchHandler computed $mayShortCircuit at the top of composeResult(), so every property fetch under a ?-> resolved its receiver type via ExpressionResult::getType(). The flag is only read on the dynamic-name path ($a?->b->{$name}, $a?->b::$$name); an Identifier name never needs it. Both computations moved into the $nameResult !== null branch.

b60e5a23c — addNull() was the PR's only change to a globally hot function, and its guard walked the type twice:

if ($nullType->isSuperTypeOf($type)->no() || $type->isNull()->no()) {

The second call alone suffices — it returns no for never (the case the first gets wrong), and agrees with the old guard on mixed, mixed~null, mixed~int, T of mixed, benevolent unions and intersections, all of which I checked by hand. It's also cheaper where it counts: UnionType::isNull() memoizes into $this->isNull, while the UnionType::isSubTypeOf() behind isSuperTypeOf() recomputes finiteTypeSetContainedIn() + extremeIdentity() every call.

Verification: make tests OK — 21980 tests, 97591 assertions, 96 skipped (pre-existing); make phpstan No errors; make cs clean.

Two caveats I should flag rather than bury. First, an earlier three-way decomposition I ran was invalid and I discarded it: I gave the base worktree a symlinked vendor/, and composer's $baseDir resolved to the HEAD repo, so all three runs silently loaded HEAD's src. The numbers above come from swapping src/+tests/ in place instead. Second, in those in-place runs git checkout <base> -- tests/ does not delete files absent from the base tree, so the base side still picked up the three new nsrt/ data files via NodeScopeResolverTest's directory scan (hence its 3 failures, 21973 vs 21980 tests). That makes the two sides near-identical workloads, which is what a timing comparison wants — but it means this run cannot separately price the cost of the new test files, so I have not verified that hypothesis for the 1m30→1m45 figure.

Unrelated to performance, one consistency gap I noticed while reading and did not touch, to keep this push scoped: MethodCallHandler's dynamic-method-name branch ($a?->b->{$name}()) doesn't get the scope merge and VariableFlow::choice that the dynamic property name, static property name and array dimension all received. Happy to add it with a test if you want it in this PR.

[VincentLanglet]

I think this makes sense. as long as CI is red I find it hard to reason about such changes tbh.

we should at least locally verify that this change does not regress performance (run phpbench locally)

I reduce the Ci failure and improved the benchmark diff. I think it's ok now.

[AJenbo]

Thanks :)