Keep property fetches on the receiver when a static method or static closure invalidates it

[phpstan-bot]

Summary

Calling a static method from an instance method threw away every narrowed type
rooted at $this, including plain property fetches:

$this->foo = new Foo();
assertType('Foo', $this->foo);
self::assertTrue(true);       // static method
assertType('Foo|null', $this->foo);   // <- forgotten

A static method never receives $this, so it cannot change the object's own
properties. This PR keeps property fetches on the receiver alive across such a
call, while everything that can observe static state (method calls, offset
accesses on the result of a call, static property fetches) keeps being
invalidated - exactly the boundary drawn in the issue discussion.

Changes

• src/Analyser/ScopeOps.php
  • invalidateExpressionEntries() and shouldInvalidateExpression() take a new
    optional bool $keepPropertyFetches and thread it to all three
    shouldInvalidateExpression() call sites (expression table, conditional
    expression targets, conditional expression conditions).
  • New isPropertyFetchChainOn() helper: is the entry a chain of property
    fetches (PropertyFetch/NullsafePropertyFetch, named by an Identifier or a
    plain variable) whose root prints as the invalidated expression's key?
  • New carve-out in shouldInvalidateExpression(), placed right after the cheap
    Variable fast path and before the substring gate and AST walk.
• src/Analyser/MutatingScope.php - invalidateExpression() gains the flag and
  forwards it; src/Analyser/NodeCallbackScope.php forwards it in its override and
  in the recorded scope op.
• src/Analyser/ExprHandler/StaticCallHandler.php - the reported bug: pass
  $methodReflection->isStatic().
• src/Analyser/ExprHandler/MethodCallHandler.php - analogous case: a static
  method invoked with -> ($this->staticMethod(), $obj->staticMethod()) had the
  same bug on its receiver.
• src/Analyser/ExprHandler/Helper/FuncCallScopeEffectsHelper.php - analogous
  case: invoking a static closure invalidated $this, although a static closure
  is never bound to $this. Non-static closures keep invalidating (they can be
  bound to $this and really can write to it).
• turbo-ext/src/ScopeOps.cpp, turbo-ext/src/support.{h,cpp},
  build/PHPStan/Build/TurboAttributeCollector.php - the same logic ported into
  the native mirror of ScopeOps (#[ShadowedByTurboExtension]), plus two new
  class-map entries (nullsafePropertyFetch, identifier) the native helper needs.

Analogous cases probed

Fixed (each has a failing-before test):

• nested property fetches: $this->foo->bar
• nullsafe property fetches: $this->foo?->bar
• dynamic property names: $this->{$name}
• array-typed properties and their offsets: $this->arr / $this->arr['x']
• a static method called with -> on $this or on another object
• calling a static closure

Probed and confirmed still correctly invalidated (kept as regression tests):

• $this->getFoo() - the method body can read static state
• self::$staticProp - a static method really can write to it
• a non-static method called as self::nonStatic()
• calling a non-static closure

Probed and deliberately left alone:

• first-class callables of static methods ($fn = self::sideEffect(...); $fn();)
  still invalidate. InitializerExprTypeResolver::createFirstClassCallable() leaves
  isStaticClosure() as maybe for them; making it yes is semantically correct
  but changes describe() output (static Closure(...)), callable variance in
  CallableTypeHelper and the Closure::bind() extensions, which is well beyond
  this fix.

Root cause

The pattern is "invalidate the whole receiver when the callee may be impure".
MutatingScope::invalidateExpression($receiver, requireMoreCharacters: true) drops
every scope entry whose expression contains the receiver, which is right for a
callee that gets the object (a non-static method, parent::__construct(), a
$this-bound closure) but far too broad for a callee that never does. The existing
carve-outs in ScopeOps::shouldInvalidateExpression() (readonly property fetches,
private properties of a different class) already encode the same idea for narrower
reasons; this adds the "the callee cannot reach the object at all" one, applied at
every site that invalidates a receiver for a callee without a $this:
StaticCallHandler, MethodCallHandler and FuncCallScopeEffectsHelper.

The carve-out is intentionally limited to property-fetch chains. Anything else
rooted at the receiver - a method call, an offset access on a call result - can
observe static state, which a static callee can change, so it keeps being
invalidated.

Test

• tests/PHPStan/Analyser/nsrt/bug-13735.php - both playground reproducers from
  the issue verbatim (the PHPUnit-style self::assertTrue() case, and the
  non-final / final / private-static-method constructor cases).
• tests/PHPStan/Analyser/nsrt/bug-13735b.php - the analogous cases and the
  controls listed above.

Both files fail on 2.2.x (11 wrong assertType()s) and pass with the fix.
make tests, make phpstan and make cs-fix are green; the full suite also
passes with phpstan_turbo loaded, and bin/phpstan analyse -c build/phpstan.neon src produces byte-identical output with and without the extension.
turbo-ext/tests/smoke.php, turbo-ext/tests/signature-parity.php and
turbo-ext/bin/side-by-side.php all pass.

Note

turbo-ext/src/ changed, so this needs the follow-up
make bump-turbo commit setting
TurboExtensionEnabler::EXPECTED_EXTENSION_VERSION to this change's short SHA
once it lands on the target branch.

Fixes phpstan/phpstan#13735

[staabm]

@SanderMuller please review

[staabm]

regarding turbo errors on macos, I updated a upstream issue: shivammathur/setup-php#1116

[SanderMuller]

Two false negatives, both reproduced on the branch and both correct on 2.2.x. The rest of the change holds up well, including the native mirror, which I was able to test. I am not the maintainer, so the calls are yours and @ondrejmirtes's.

A static callee can reach the object, in two ways

The premise in the description is "a static method never receives $this, so it cannot change the object's own properties". A static callee gets no $this, but it can still be handed the object, or find it.

1. A static closure that takes the receiver as an argument. I varied the closure along both axes, with a body that actually writes. $this->foo starts as Foo in every row:

closure	body writes	2.2.x	this branch
static fn ()	Holder::$seen	Foo|null	Foo
static fn (int $i), called $fn(1)	Holder::$seen	Foo|null	Foo
static fn (Holder $h), called $fn($this)	$h->foo = null	Foo|null	Foo
fn (Holder $h), called $fn($this)	$h->foo = null	Foo|null	Foo|null
fn ()	$this->foo = null	Foo|null	Foo|null

Rows 1 and 2 are the improvement, and they are clearly right: that closure cannot reach $this. Rows 4 and 5 are correct controls. Row 3 is the hole. The closure is handed $this and writes through it, and the narrowing survives.

The static method path already covers that channel on this branch:

self::mutate($this);        // still invalidates
$this->mutate($this);       // static method via ->, still invalidates
$other->mutate($other);     // still invalidates

So StaticCallHandler and MethodCallHandler handle the argument case and FuncCallScopeEffectsHelper does not.

2. A static method that reaches the object through static state. Also green on 2.2.x, also lost here:

public static ?Holder $instance = null;

public static function mutateStored(): void
{
    if (self::$instance !== null) {
        self::$instance->foo = null;
    }
}

public function reachedViaStaticProperty(): void
{
    self::$instance = $this;
    $this->foo = new Foo();
    self::mutateStored();
    assertType('Foo|null', $this->foo);   // 2.2.x: passes. This branch: 'Foo'.
}

The description says "everything that can observe static state keeps being invalidated". Property fetches on the receiver no longer are, and static state is the channel by which a static method reaches $this. Registries and singletons write self::$instance = $this routinely.

Case 1 looks like a plain gap to me, and a narrow one. Case 2 is the boundary the description draws, so where you put it is your call.

The native mirror agrees, and I checked rather than read it

ScopeOps carries #[ShadowedByTurboExtension], so this is the case where the PHP and C++ implementations can silently disagree. I built turbo-ext locally on PHP 8.5 arm64 and confirmed the extension was really active, PHPStan\Analyser\ScopeOps extending PHPStanTurbo\ScopeOps, rather than merely loaded.

• The PR's two nsrt files plus my three probe files: 6 errors, byte-identical with and without the extension. Both false negatives above reproduce identically under the native path, so the port is faithful, including in its faults.
• Full suite green both ways: 21324 tests / 96430 assertions, without the extension and with it.
• bin/phpstan analyse -c build/phpstan.neon src byte-identical with and without.

The version bump is right too. EXPECTED_EXTENSION_VERSION = 'e5a7514' is the short SHA of e5a751490, the only commit touching turbo-ext/src/, which is the convention the constant's own docblock states. The [!NOTE] in the description asking for a follow-up make bump-turbo is stale, since 65955e019 already did it.

The tests do fail first

Both nsrt files against 2.2.x: 12 wrong assertType()s. The description says 11, which was right before the static:: case was added on review.

Performance

Flat. Cold corpus run, 2 interleaved rounds, on a 4524-file real-world project: CPU 141.9 / 140.6 s on base against 142.2 / 138.6 s here. maxRSS 348 to 397 MB in both arms, and the error output is identical.

That matches what isPropertyFetchChainOn() does. It returns on the first instanceof for anything that is not a property fetch. The pretty-print only runs for a real chain.

CI

Every red is accounted for and none is this change.

The four macOS Compile Turbo Extension jobs are the setup-php problem you just filed upstream. Worth adding one data point: the same .cpp built cleanly here on PHP 8.5 arm64, so nothing is hiding behind that red.

Tests with old PHPUnit (8.1, ubuntu-latest) is three data sets of IntersectionTypeTest::testIsAcceptedBy, Maybe against No. That flake has been failing on unrelated PRs all week. PHPStan (8.1, windows-latest) is red on base head.

For the maintainer

Whether case 1 blocks. I think it should. 2.2.x is correct there, and the fix looks small. Treat the receiver as invalidated when it appears among the call's arguments, the way the static-method path already does.

Whether case 2 is in scope, or a documented limitation of the carve-out.

[phpstan-bot]

Both findings addressed and pushed as two commits.

Case 1 — static closure handed the object: fixed (868f18e1e)

Reproduced exactly as reported: $fn = static function (Holder $h) { $h->foo = null; }; $fn($this); kept $this->foo narrowed.

The reason the static method path is already correct is NodeScopeResolver::processArgs() (src/Analyser/NodeScopeResolver.php:2308): for a callee with hasSideEffects()->yes(), every argument that may be an object or resource gets invalidateExpression($arg->value, true). A closure has no FunctionReflection, so $calleeReflection is null there and the whole branch is skipped. The fix mirrors that in FuncCallScopeEffectsHelper::invalidateObjectArgs(), applied only where the receiver invalidation is being weakened (src/Analyser/ExprHandler/Helper/FuncCallScopeEffectsHelper.php:97), so non-static closures are untouched.

Probing the adjacent argument channels found one more instance of the same bug and two controls, all now in bug-13735b.php:

case	before	after
$fn($this), static closure	Foo ❌	Foo|null
$fn($this), static arrow fn	Foo ❌	Foo|null
$fn($this->foo) writing $f->bar	Bar ❌	Bar|null, and $this->foo still Foo
$fn(1)	Foo	Foo (unchanged — the improvement survives)
self::mutate($this) / self::mutateFoo($this->foo)	correct	correct (controls)

The property-argument row lands on the same semantics the static-method path has: the callee can change what is inside $this->foo, not which Foo $this->foo points at. Verified by stashing only the source change — lines 93, 102 and 115 fail, the two controls pass either way.

Case 2 — reached through static state: documented, not fixed (b51771485)

Your reproducer is real, but it is not a boundary the branch moved. I checked the same shape with a receiver that is not $this:

Holder::$instance = $other;
$other->foo = new Foo();
Holder::mutateStored();
assertType('Foo', $other->foo);   // passes on 2.2.x and on this branch

StaticCallHandler only ever invalidated new Variable('this'), so the static-state channel has always been untracked for every other receiver — $this was the accident, not the rule. Tracking it soundly would mean assuming any static call can reach any object, which is the feature #13735 asks to remove. So I recorded it as a limitation on isPropertyFetchChainOn()'s docblock and added doReachedViaStaticProperty() asserting today's behaviour, so a future change surfaces there. Happy to flip it if @ondrejmirtes wants the carve-out drawn tighter.

Verification

make tests (21273 tests, 96340 assertions, 97 skipped), make phpstan (no errors) and make cs are green. No turbo-ext/src/ file changed — the ScopeOps.php edit is comment-only — so EXPECTED_EXTENSION_VERSION stays at e5a7514 and no make bump-turbo follow-up is needed.

Two notes on the description, which I have not edited: the assertion count is now higher again after these tests, and the [!NOTE] asking for a make bump-turbo follow-up is stale — 65955e019 already did it, as you spotted.

[SanderMuller]

Both findings resolved. Case 1 is fixed and case 2 I withdraw, because your counter-argument is right and I checked it.

Case 1 is fixed, and the improvement survived

Same channel matrix as before, $this->foo starting as Foo in every row:

closure	body writes	2.2.x	02766a1ad	now
static fn ()	Holder::$seen	Foo|null	Foo	Foo
static fn (int $i), $fn(1)	Holder::$seen	Foo|null	Foo	Foo
static fn (Holder $h), $fn($this)	$h->foo = null	Foo|null	Foo	Foo|null
fn (Holder $h), $fn($this)	$h->foo = null	Foo|null	Foo|null	Foo|null
fn ()	$this->foo = null	Foo|null	Foo|null	Foo|null

Row 3 fixed, rows 1 and 2 still carry the win. That is the shape I wanted.

I confirmed your fails-before claim independently. Reverting only FuncCallScopeEffectsHelper.php to 02766a1ad makes lines 93, 102 and 115 of bug-13735b.php fail, and the two controls pass either way.

Case 2: you are right, I withdraw it

I tested the same static-state shape with three different receivers on 2.2.x:

receiver	2.2.x	this branch
$this	Foo|null	Foo
$other, a parameter	Foo	Foo
$local, a local variable	Foo	Foo

So 2.2.x tracked that channel for $this alone, which is the accident you describe, not a rule the branch broke. Making $this behave like every other receiver is the consistent choice, and pinning it in doReachedViaStaticProperty() is the right way to leave it. My earlier framing was accurate about the one shape and wrong about what it meant.

The two cases you found while probing are real additions. The static arrow function is the same hole in different syntax. The property-argument case landing on "the callee can change what is inside $this->foo, not which Foo it points at" matches the static-method path exactly.

Turbo bookkeeping checks out

ScopeOps.php is the only shadowed class here, and the new commit adds five lines of docblock to it. FuncCallScopeEffectsHelper is not shadowed and turbo-ext/src/ references it nowhere, so e5a7514 stays correct and no make bump-turbo is due.

I re-ran the differential anyway with the extension built and active, PHPStan\Analyser\ScopeOps extending PHPStanTurbo\ScopeOps: 7 files, 12 errors, byte-identical with and without.

Gate at b51771485

Suite 21324 tests / 96430 assertions green, both with and without the extension. make phpstan clean. phpcs clean on the changed files.

Performance re-measured at this head, since invalidateObjectArgs() now runs on every impure closure call. Cold, 2 interleaved rounds, 4524-file corpus. CPU 143.3 / 137.7 s on base against 138.3 / 139.8 s here, with overlapping spreads. maxRSS 361 to 403 MB in both, error output identical. No measurable cost.

CI reds are the setup-php macOS problem you filed upstream, the IntersectionTypeTest::testIsAcceptedBy flake, and PHPStan (8.1, windows-latest) which is red on base head.

Nothing left open from my side. The description still needs its assertion count and its stale [!NOTE] fixing before merge.