Skip to content

Commit b48812e

Browse files
authored
Keep property fetches on the receiver when a static method or static closure invalidates it (#6391)
1 parent 3b094df commit b48812e

12 files changed

Lines changed: 711 additions & 17 deletions

File tree

‎build/PHPStan/Build/TurboAttributeCollector.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,15 @@
1414
use PhpParser\Node\Expr\FuncCall;
1515
use PhpParser\Node\Expr\MethodCall;
1616
use PhpParser\Node\Expr\New_;
17+
use PhpParser\Node\Expr\NullsafePropertyFetch;
1718
use PhpParser\Node\Expr\PropertyFetch;
1819
use PhpParser\Node\Expr\StaticCall;
1920
use PhpParser\Node\Expr\UnaryMinus;
2021
use PhpParser\Node\Expr\Variable;
2122
use PhpParser\Node\Expr\Yield_;
2223
use PhpParser\Node\Expr\YieldFrom;
2324
use PhpParser\Node\FunctionLike;
25+
use PhpParser\Node\Identifier;
2426
use PhpParser\Node\Name;
2527
use PhpParser\Node\Scalar;
2628
use PhpParser\Node\Stmt;
@@ -88,6 +90,8 @@ final class TurboAttributeCollector
8890
'name' => Name::class,
8991
'expr' => Expr::class,
9092
'propertyFetch' => PropertyFetch::class,
93+
'nullsafePropertyFetch' => NullsafePropertyFetch::class,
94+
'identifier' => Identifier::class,
9195
'arrayDimFetch' => ArrayDimFetch::class,
9296
'methodCall' => MethodCall::class,
9397
'functionLike' => FunctionLike::class,

‎src/Analyser/ExprHandler/Helper/FuncCallScopeEffectsHelper.php‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use PHPStan\DependencyInjection\AutowiredService;
1919
use PHPStan\Node\Expr\NativeTypeExpr;
2020
use PHPStan\Node\Expr\PossiblyImpureCallExpr;
21+
use PHPStan\Node\InvalidateExprNode;
2122
use PHPStan\Reflection\Callables\CallableParametersAcceptor;
2223
use PHPStan\Reflection\FunctionReflection;
2324
use PHPStan\Reflection\ParametersAcceptor;
@@ -34,6 +35,7 @@
3435
use PHPStan\Type\IntersectionType;
3536
use PHPStan\Type\MixedType;
3637
use PHPStan\Type\NullType;
38+
use PHPStan\Type\ResourceType;
3739
use PHPStan\Type\StringType;
3840
use PHPStan\Type\Type;
3941
use PHPStan\Type\TypeCombinator;
@@ -101,7 +103,25 @@ public function applyCallScopeEffects(NodeScopeResolver $nodeScopeResolver, Stmt
101103
$parametersAcceptor instanceof ClosureType && count($parametersAcceptor->getImpurePoints()) > 0
102104
&& $scope->isInClass()
103105
) {
104-
$scope = $scope->invalidateExpression(new Variable('this'), true);
106+
$isStaticClosure = $parametersAcceptor->isStaticClosure()->yes();
107+
108+
// A static closure is never bound to $this, so property fetches on it survive.
109+
// But a capture may be the object under another name ('$self = $this;' then
110+
// 'use ($self)'), and PHPStan does not track that aliasing: a write would land
111+
// on '$self->foo' while the caller remembers '$this->foo'. An arrow function
112+
// captures implicitly and records no used variables at all, so any closure that
113+
// writes anywhere gives the carve-out up too.
114+
$keepPropertyFetches = $isStaticClosure
115+
&& $parametersAcceptor->getUsedVariables() === []
116+
&& $parametersAcceptor->getInvalidateExpressions() === [];
117+
118+
if ($isStaticClosure) {
119+
// The object can still be handed to it as an argument. That's the same channel
120+
// processArgs() invalidates for a callee with side effects, which is what keeps
121+
// '$this' invalidated for 'self::mutate($this)'.
122+
$scope = $this->invalidateObjectArgs($nodeScopeResolver, $normalizedExpr, $argsResult, $scope, $storage, $nodeCallback);
123+
}
124+
$scope = $scope->invalidateExpression(new Variable('this'), true, null, $keepPropertyFetches);
105125
}
106126

107127
if (
@@ -363,6 +383,39 @@ public function applyCallScopeEffects(NodeScopeResolver $nodeScopeResolver, Stmt
363383
return $scope;
364384
}
365385

386+
/**
387+
* Invalidates the arguments a callee could write through, mirroring what
388+
* NodeScopeResolver::processArgs() does for a callee with side effects. A
389+
* closure has no FunctionReflection, so processArgs() skips it.
390+
*
391+
* @param callable(Node $node, Scope $scope): void $nodeCallback
392+
*/
393+
private function invalidateObjectArgs(NodeScopeResolver $nodeScopeResolver, FuncCall $normalizedExpr, ArgsResult $argsResult, MutatingScope $scope, ExpressionResultStorage $storage, callable $nodeCallback): MutatingScope
394+
{
395+
foreach ($normalizedExpr->getArgs() as $arg) {
396+
// a default-value argument ArgumentsNormalizer synthesized for an omitted
397+
// optional parameter was never processed, and holds no expression the caller
398+
// could observe afterwards
399+
$argResult = $argsResult->findArgResult($arg->value);
400+
if ($argResult === null) {
401+
continue;
402+
}
403+
404+
$argType = $argResult->getTypeOnScope($scope, false);
405+
if (
406+
$argType->isObject()->no()
407+
&& (new ResourceType())->isSuperTypeOf($argType)->no()
408+
) {
409+
continue;
410+
}
411+
412+
$nodeScopeResolver->callNodeCallback($nodeCallback, new InvalidateExprNode($arg->value), $scope, $storage);
413+
$scope = $scope->invalidateExpression($arg->value, true);
414+
}
415+
416+
return $scope;
417+
}
418+
366419
private function getArrayFunctionAppendingType(FunctionReflection $functionReflection, Scope $scope, FuncCall $expr, ArgsResult $argsResult): Type
367420
{
368421
$arrayArg = $expr->getArgs()[0]->value;

‎src/Analyser/ExprHandler/MethodCallHandler.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
299299

300300
if ($methodReflection->getName() === '__construct' || $methodReflection->hasSideEffects()->yes()) {
301301
$nodeScopeResolver->callNodeCallback($nodeCallback, new InvalidateExprNode($normalizedExpr->var), $scope, $storage);
302-
$scope = $scope->invalidateExpression($normalizedExpr->var, true, $methodReflection->getDeclaringClass());
302+
$scope = $scope->invalidateExpression($normalizedExpr->var, true, $methodReflection->getDeclaringClass(), $methodReflection->isStatic());
303303
} elseif ($this->rememberPossiblyImpureFunctionValues && $methodReflection->hasSideEffects()->maybe() && !$methodReflection->getDeclaringClass()->isBuiltin()) {
304304
// the remembered call value and the @phpstan-self-out type are
305305
// generic-sensitive: resolve them from the type-driven acceptor

‎src/Analyser/ExprHandler/StaticCallHandler.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,8 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
377377
&& $scope->isInClass()
378378
&& $scope->getClassReflection()->is($methodReflection->getDeclaringClass()->getName())
379379
) {
380-
$scope = $scope->invalidateExpression(new Variable('this'), true, $methodReflection->getDeclaringClass());
380+
// a static method never receives $this, so property fetches on it survive
381+
$scope = $scope->invalidateExpression(new Variable('this'), true, $methodReflection->getDeclaringClass(), $methodReflection->isStatic());
381382
} elseif (
382383
$expr->class instanceof Name
383384
&& $methodReflection !== null

‎src/Analyser/MutatingScope.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3803,7 +3803,12 @@ public function assignInitializedProperty(Type $fetchedOnType, string $propertyN
38033803
return $scope;
38043804
}
38053805

3806-
public function invalidateExpression(Expr $expressionToInvalidate, bool $requireMoreCharacters = false, ?ClassReflection $invalidatingClass = null): self
3806+
/**
3807+
* @param bool $keepPropertyFetches Keeps property fetches on the invalidated expression
3808+
* (like '$this->foo') - for callees that never receive
3809+
* the object, like static methods and static closures.
3810+
*/
3811+
public function invalidateExpression(Expr $expressionToInvalidate, bool $requireMoreCharacters = false, ?ClassReflection $invalidatingClass = null, bool $keepPropertyFetches = false): self
38073812
{
38083813
$exprStringToInvalidate = $this->getNodeKey($expressionToInvalidate);
38093814

@@ -3817,6 +3822,7 @@ public function invalidateExpression(Expr $expressionToInvalidate, bool $require
38173822
$this->expressionTypes,
38183823
$this->nativeExpressionTypes,
38193824
$this->conditionalExpressions,
3825+
$keepPropertyFetches,
38203826
);
38213827
if ($result === null) {
38223828
return $this;

‎src/Analyser/ScopeOps.php‎

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use PhpParser\Node\Expr;
77
use PhpParser\Node\Expr\FuncCall;
88
use PhpParser\Node\Expr\MethodCall;
9+
use PhpParser\Node\Expr\NullsafePropertyFetch;
910
use PhpParser\Node\Expr\PropertyFetch;
1011
use PhpParser\Node\Expr\Variable;
1112
use PhpParser\Node\Name;
@@ -640,6 +641,7 @@ public static function invalidateExpressionEntries(
640641
array $expressionTypes,
641642
array $nativeExpressionTypes,
642643
array $conditionalExpressions,
644+
bool $keepPropertyFetches = false,
643645
): ?array
644646
{
645647
$invalidated = false;
@@ -661,7 +663,7 @@ public static function invalidateExpressionEntries(
661663
) {
662664
continue;
663665
}
664-
if (!self::shouldInvalidateExpression($scope, $exprPrinter, $exprStringToInvalidate, $expressionToInvalidate, $exprTypeHolder->getExpr(), $exprString, $requireMoreCharacters, $invalidatingClass)) {
666+
if (!self::shouldInvalidateExpression($scope, $exprPrinter, $exprStringToInvalidate, $expressionToInvalidate, $exprTypeHolder->getExpr(), $exprString, $requireMoreCharacters, $invalidatingClass, $keepPropertyFetches)) {
665667
continue;
666668
}
667669

@@ -682,7 +684,7 @@ public static function invalidateExpressionEntries(
682684
|| self::keyMayHideSubExpressions($conditionalExprString)
683685
) {
684686
$firstHolder = $holders[array_key_first($holders)]->getTypeHolder();
685-
if (self::shouldInvalidateExpression($scope, $exprPrinter, $exprStringToInvalidate, $expressionToInvalidate, $firstHolder->getExpr(), self::nodeKey($firstHolder->getExpr(), $exprPrinter), $requireMoreCharacters, $invalidatingClass)) {
687+
if (self::shouldInvalidateExpression($scope, $exprPrinter, $exprStringToInvalidate, $expressionToInvalidate, $firstHolder->getExpr(), self::nodeKey($firstHolder->getExpr(), $exprPrinter), $requireMoreCharacters, $invalidatingClass, $keepPropertyFetches)) {
686688
$invalidated = true;
687689
continue;
688690
}
@@ -712,7 +714,7 @@ public static function invalidateExpressionEntries(
712714
$shouldKeep = true;
713715
$conditionalTypeHolders = $holder->getConditionExpressionTypeHolders();
714716
foreach ($conditionalTypeHolders as $conditionalTypeHolderExprString => $conditionalTypeHolder) {
715-
if (self::shouldInvalidateExpression($scope, $exprPrinter, $exprStringToInvalidate, $expressionToInvalidate, $conditionalTypeHolder->getExpr(), $conditionalTypeHolderExprString, invalidatingClass: $invalidatingClass)) {
717+
if (self::shouldInvalidateExpression($scope, $exprPrinter, $exprStringToInvalidate, $expressionToInvalidate, $conditionalTypeHolder->getExpr(), $conditionalTypeHolderExprString, invalidatingClass: $invalidatingClass, keepPropertyFetches: $keepPropertyFetches)) {
716718
$invalidated = true;
717719
$shouldKeep = false;
718720
break;
@@ -799,7 +801,7 @@ private static function containsExpressionToInvalidate(Scope $scope, ExprPrinter
799801
/**
800802
* Mirrors the former MutatingScope::shouldInvalidateExpression().
801803
*/
802-
public static function shouldInvalidateExpression(MutatingScope $scope, ExprPrinter $exprPrinter, string $exprStringToInvalidate, Expr $exprToInvalidate, Expr $expr, string $exprString, bool $requireMoreCharacters = false, ?ClassReflection $invalidatingClass = null): bool
804+
public static function shouldInvalidateExpression(MutatingScope $scope, ExprPrinter $exprPrinter, string $exprStringToInvalidate, Expr $exprToInvalidate, Expr $expr, string $exprString, bool $requireMoreCharacters = false, ?ClassReflection $invalidatingClass = null, bool $keepPropertyFetches = false): bool
803805
{
804806
if (
805807
$expr instanceof IntertwinedVariableByReferenceWithExpr
@@ -831,6 +833,10 @@ public static function shouldInvalidateExpression(MutatingScope $scope, ExprPrin
831833
return $exprStringToInvalidate === $exprString;
832834
}
833835

836+
if ($keepPropertyFetches && self::isPropertyFetchChainOn($expr, $exprStringToInvalidate, $exprPrinter)) {
837+
return false;
838+
}
839+
834840
// nodeKey() is the pretty-printed expression, and the standard printer is
835841
// compositional: the key of any sub-expression appears verbatim as a substring of
836842
// the key of the expression containing it. So if the invalidated expression's key
@@ -874,6 +880,42 @@ public static function shouldInvalidateExpression(MutatingScope $scope, ExprPrin
874880
return true;
875881
}
876882

883+
/**
884+
* Whether $expr is a chain of property fetches rooted at the invalidated
885+
* expression, like '$this->foo', '$this->foo?->bar' or '$this->$name' for '$this'.
886+
*
887+
* Such an expression only reads state reachable through the object itself, so a
888+
* callee that never receives the object - a static method, a static closure -
889+
* cannot change it and it survives the invalidation. Anything else rooted at the
890+
* object (a method call, an offset access) can observe static state and keeps
891+
* being invalidated. A property name computed from anything but a plain variable
892+
* could do the same, so it is not accepted either.
893+
*
894+
* Callers must still invalidate the object when they hand it to the callee as an
895+
* argument. Reaching it through static state ('self::$instance = $this;' and then
896+
* a static method writing through 'self::$instance') is not tracked - the same
897+
* limitation every receiver other than '$this' has always had.
898+
*/
899+
private static function isPropertyFetchChainOn(Expr $expr, string $exprStringToInvalidate, ExprPrinter $exprPrinter): bool
900+
{
901+
if (!$expr instanceof PropertyFetch && !$expr instanceof NullsafePropertyFetch) {
902+
return false;
903+
}
904+
905+
while ($expr instanceof PropertyFetch || $expr instanceof NullsafePropertyFetch) {
906+
if (
907+
!$expr->name instanceof Node\Identifier
908+
&& !($expr->name instanceof Variable && is_string($expr->name->name))
909+
) {
910+
return false;
911+
}
912+
913+
$expr = $expr->var;
914+
}
915+
916+
return self::nodeKey($expr, $exprPrinter) === $exprStringToInvalidate;
917+
}
918+
877919
/**
878920
* The conditional-expressions fixed-point matching of
879921
* MutatingScope::applySpecifiedTypes().

‎src/Turbo/TurboExtensionEnabler.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ final class TurboExtensionEnabler
2222
* version is the short SHA of the last commit touching turbo-ext/src/,
2323
* enforced by the phar.yml turbo-version job.
2424
*/
25-
public const EXPECTED_EXTENSION_VERSION = 'd7ef536';
25+
public const EXPECTED_EXTENSION_VERSION = '91db796';
2626

2727
private static bool $typeCombinatorCacheEnabled = false;
2828

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
<?php // lint >= 8.0
2+
3+
declare(strict_types = 1);
4+
5+
namespace Bug13735;
6+
7+
use function PHPStan\Testing\assertType;
8+
9+
class Bug13735Test
10+
{
11+
private ?Foo $foo = null;
12+
13+
public function testFoo(): void
14+
{
15+
$this->foo = new Foo();
16+
assertType('Bug13735\Foo', $this->foo);
17+
self::assertTrue(true);
18+
assertType('Bug13735\Foo', $this->foo);
19+
}
20+
21+
public static function assertTrue(mixed $condition, string $message = ''): void
22+
{
23+
24+
}
25+
}
26+
27+
class Foo {
28+
public ?Foo $inner = null;
29+
30+
public function doSomething(): bool {
31+
return true;
32+
}
33+
}
34+
35+
class Test
36+
{
37+
private string $data;
38+
39+
public function __construct() {
40+
$this->data = 'abc';
41+
assertType("'abc'", $this->data);
42+
self::noop('foo');
43+
assertType("'abc'", $this->data);
44+
}
45+
46+
static final public function noop(string $message): void {
47+
file_put_contents('log file', $message);
48+
}
49+
50+
}
51+
52+
final class FinalTest
53+
{
54+
private string $data;
55+
56+
public function __construct() {
57+
$this->data = 'abc';
58+
assertType("'abc'", $this->data);
59+
self::noop('foo');
60+
assertType("'abc'", $this->data);
61+
}
62+
63+
static public function noop(string $message): void {
64+
file_put_contents('log file', $message);
65+
}
66+
67+
}
68+
69+
final class PrivateTest
70+
{
71+
private string $data;
72+
73+
public function __construct() {
74+
$this->data = 'abc';
75+
assertType("'abc'", $this->data);
76+
self::noop('foo');
77+
assertType("'abc'", $this->data);
78+
}
79+
80+
static private function noop(string $message): void {
81+
file_put_contents('log file', $message);
82+
}
83+
84+
}

0 commit comments

Comments
 (0)