diff --git a/build/PHPStan/Build/TurboAttributeCollector.php b/build/PHPStan/Build/TurboAttributeCollector.php index 9452a0b604b..98076629247 100644 --- a/build/PHPStan/Build/TurboAttributeCollector.php +++ b/build/PHPStan/Build/TurboAttributeCollector.php @@ -14,6 +14,7 @@ use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\New_; +use PhpParser\Node\Expr\NullsafePropertyFetch; use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Expr\StaticCall; use PhpParser\Node\Expr\UnaryMinus; @@ -21,6 +22,7 @@ use PhpParser\Node\Expr\Yield_; use PhpParser\Node\Expr\YieldFrom; use PhpParser\Node\FunctionLike; +use PhpParser\Node\Identifier; use PhpParser\Node\Name; use PhpParser\Node\Scalar; use PhpParser\Node\Stmt; @@ -88,6 +90,8 @@ final class TurboAttributeCollector 'name' => Name::class, 'expr' => Expr::class, 'propertyFetch' => PropertyFetch::class, + 'nullsafePropertyFetch' => NullsafePropertyFetch::class, + 'identifier' => Identifier::class, 'arrayDimFetch' => ArrayDimFetch::class, 'methodCall' => MethodCall::class, 'functionLike' => FunctionLike::class, diff --git a/src/Analyser/ExprHandler/Helper/FuncCallScopeEffectsHelper.php b/src/Analyser/ExprHandler/Helper/FuncCallScopeEffectsHelper.php index 07d5dd85d48..255400602cd 100644 --- a/src/Analyser/ExprHandler/Helper/FuncCallScopeEffectsHelper.php +++ b/src/Analyser/ExprHandler/Helper/FuncCallScopeEffectsHelper.php @@ -18,6 +18,7 @@ use PHPStan\DependencyInjection\AutowiredService; use PHPStan\Node\Expr\NativeTypeExpr; use PHPStan\Node\Expr\PossiblyImpureCallExpr; +use PHPStan\Node\InvalidateExprNode; use PHPStan\Reflection\Callables\CallableParametersAcceptor; use PHPStan\Reflection\FunctionReflection; use PHPStan\Reflection\ParametersAcceptor; @@ -34,6 +35,7 @@ use PHPStan\Type\IntersectionType; use PHPStan\Type\MixedType; use PHPStan\Type\NullType; +use PHPStan\Type\ResourceType; use PHPStan\Type\StringType; use PHPStan\Type\Type; use PHPStan\Type\TypeCombinator; @@ -101,7 +103,25 @@ public function applyCallScopeEffects(NodeScopeResolver $nodeScopeResolver, Stmt $parametersAcceptor instanceof ClosureType && count($parametersAcceptor->getImpurePoints()) > 0 && $scope->isInClass() ) { - $scope = $scope->invalidateExpression(new Variable('this'), true); + $isStaticClosure = $parametersAcceptor->isStaticClosure()->yes(); + + // A static closure is never bound to $this, so property fetches on it survive. + // But a capture may be the object under another name ('$self = $this;' then + // 'use ($self)'), and PHPStan does not track that aliasing: a write would land + // on '$self->foo' while the caller remembers '$this->foo'. An arrow function + // captures implicitly and records no used variables at all, so any closure that + // writes anywhere gives the carve-out up too. + $keepPropertyFetches = $isStaticClosure + && $parametersAcceptor->getUsedVariables() === [] + && $parametersAcceptor->getInvalidateExpressions() === []; + + if ($isStaticClosure) { + // The object can still be handed to it as an argument. That's the same channel + // processArgs() invalidates for a callee with side effects, which is what keeps + // '$this' invalidated for 'self::mutate($this)'. + $scope = $this->invalidateObjectArgs($nodeScopeResolver, $normalizedExpr, $argsResult, $scope, $storage, $nodeCallback); + } + $scope = $scope->invalidateExpression(new Variable('this'), true, null, $keepPropertyFetches); } if ( @@ -363,6 +383,39 @@ public function applyCallScopeEffects(NodeScopeResolver $nodeScopeResolver, Stmt return $scope; } + /** + * Invalidates the arguments a callee could write through, mirroring what + * NodeScopeResolver::processArgs() does for a callee with side effects. A + * closure has no FunctionReflection, so processArgs() skips it. + * + * @param callable(Node $node, Scope $scope): void $nodeCallback + */ + private function invalidateObjectArgs(NodeScopeResolver $nodeScopeResolver, FuncCall $normalizedExpr, ArgsResult $argsResult, MutatingScope $scope, ExpressionResultStorage $storage, callable $nodeCallback): MutatingScope + { + foreach ($normalizedExpr->getArgs() as $arg) { + // a default-value argument ArgumentsNormalizer synthesized for an omitted + // optional parameter was never processed, and holds no expression the caller + // could observe afterwards + $argResult = $argsResult->findArgResult($arg->value); + if ($argResult === null) { + continue; + } + + $argType = $argResult->getTypeOnScope($scope, false); + if ( + $argType->isObject()->no() + && (new ResourceType())->isSuperTypeOf($argType)->no() + ) { + continue; + } + + $nodeScopeResolver->callNodeCallback($nodeCallback, new InvalidateExprNode($arg->value), $scope, $storage); + $scope = $scope->invalidateExpression($arg->value, true); + } + + return $scope; + } + private function getArrayFunctionAppendingType(FunctionReflection $functionReflection, Scope $scope, FuncCall $expr, ArgsResult $argsResult): Type { $arrayArg = $expr->getArgs()[0]->value; diff --git a/src/Analyser/ExprHandler/MethodCallHandler.php b/src/Analyser/ExprHandler/MethodCallHandler.php index 40151a37907..d7f9df021d2 100644 --- a/src/Analyser/ExprHandler/MethodCallHandler.php +++ b/src/Analyser/ExprHandler/MethodCallHandler.php @@ -299,7 +299,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex if ($methodReflection->getName() === '__construct' || $methodReflection->hasSideEffects()->yes()) { $nodeScopeResolver->callNodeCallback($nodeCallback, new InvalidateExprNode($normalizedExpr->var), $scope, $storage); - $scope = $scope->invalidateExpression($normalizedExpr->var, true, $methodReflection->getDeclaringClass()); + $scope = $scope->invalidateExpression($normalizedExpr->var, true, $methodReflection->getDeclaringClass(), $methodReflection->isStatic()); } elseif ($this->rememberPossiblyImpureFunctionValues && $methodReflection->hasSideEffects()->maybe() && !$methodReflection->getDeclaringClass()->isBuiltin()) { // the remembered call value and the @phpstan-self-out type are // generic-sensitive: resolve them from the type-driven acceptor diff --git a/src/Analyser/ExprHandler/StaticCallHandler.php b/src/Analyser/ExprHandler/StaticCallHandler.php index fa45770a113..3699a8fc80e 100644 --- a/src/Analyser/ExprHandler/StaticCallHandler.php +++ b/src/Analyser/ExprHandler/StaticCallHandler.php @@ -377,7 +377,8 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex && $scope->isInClass() && $scope->getClassReflection()->is($methodReflection->getDeclaringClass()->getName()) ) { - $scope = $scope->invalidateExpression(new Variable('this'), true, $methodReflection->getDeclaringClass()); + // a static method never receives $this, so property fetches on it survive + $scope = $scope->invalidateExpression(new Variable('this'), true, $methodReflection->getDeclaringClass(), $methodReflection->isStatic()); } elseif ( $expr->class instanceof Name && $methodReflection !== null diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index 0baec480aa2..dbf2490228c 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -3803,7 +3803,12 @@ public function assignInitializedProperty(Type $fetchedOnType, string $propertyN return $scope; } - public function invalidateExpression(Expr $expressionToInvalidate, bool $requireMoreCharacters = false, ?ClassReflection $invalidatingClass = null): self + /** + * @param bool $keepPropertyFetches Keeps property fetches on the invalidated expression + * (like '$this->foo') - for callees that never receive + * the object, like static methods and static closures. + */ + public function invalidateExpression(Expr $expressionToInvalidate, bool $requireMoreCharacters = false, ?ClassReflection $invalidatingClass = null, bool $keepPropertyFetches = false): self { $exprStringToInvalidate = $this->getNodeKey($expressionToInvalidate); @@ -3817,6 +3822,7 @@ public function invalidateExpression(Expr $expressionToInvalidate, bool $require $this->expressionTypes, $this->nativeExpressionTypes, $this->conditionalExpressions, + $keepPropertyFetches, ); if ($result === null) { return $this; diff --git a/src/Analyser/ScopeOps.php b/src/Analyser/ScopeOps.php index 44e6496fdc3..965fe08105c 100644 --- a/src/Analyser/ScopeOps.php +++ b/src/Analyser/ScopeOps.php @@ -6,6 +6,7 @@ use PhpParser\Node\Expr; use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\MethodCall; +use PhpParser\Node\Expr\NullsafePropertyFetch; use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Expr\Variable; use PhpParser\Node\Name; @@ -640,6 +641,7 @@ public static function invalidateExpressionEntries( array $expressionTypes, array $nativeExpressionTypes, array $conditionalExpressions, + bool $keepPropertyFetches = false, ): ?array { $invalidated = false; @@ -661,7 +663,7 @@ public static function invalidateExpressionEntries( ) { continue; } - if (!self::shouldInvalidateExpression($scope, $exprPrinter, $exprStringToInvalidate, $expressionToInvalidate, $exprTypeHolder->getExpr(), $exprString, $requireMoreCharacters, $invalidatingClass)) { + if (!self::shouldInvalidateExpression($scope, $exprPrinter, $exprStringToInvalidate, $expressionToInvalidate, $exprTypeHolder->getExpr(), $exprString, $requireMoreCharacters, $invalidatingClass, $keepPropertyFetches)) { continue; } @@ -682,7 +684,7 @@ public static function invalidateExpressionEntries( || self::keyMayHideSubExpressions($conditionalExprString) ) { $firstHolder = $holders[array_key_first($holders)]->getTypeHolder(); - if (self::shouldInvalidateExpression($scope, $exprPrinter, $exprStringToInvalidate, $expressionToInvalidate, $firstHolder->getExpr(), self::nodeKey($firstHolder->getExpr(), $exprPrinter), $requireMoreCharacters, $invalidatingClass)) { + if (self::shouldInvalidateExpression($scope, $exprPrinter, $exprStringToInvalidate, $expressionToInvalidate, $firstHolder->getExpr(), self::nodeKey($firstHolder->getExpr(), $exprPrinter), $requireMoreCharacters, $invalidatingClass, $keepPropertyFetches)) { $invalidated = true; continue; } @@ -712,7 +714,7 @@ public static function invalidateExpressionEntries( $shouldKeep = true; $conditionalTypeHolders = $holder->getConditionExpressionTypeHolders(); foreach ($conditionalTypeHolders as $conditionalTypeHolderExprString => $conditionalTypeHolder) { - if (self::shouldInvalidateExpression($scope, $exprPrinter, $exprStringToInvalidate, $expressionToInvalidate, $conditionalTypeHolder->getExpr(), $conditionalTypeHolderExprString, invalidatingClass: $invalidatingClass)) { + if (self::shouldInvalidateExpression($scope, $exprPrinter, $exprStringToInvalidate, $expressionToInvalidate, $conditionalTypeHolder->getExpr(), $conditionalTypeHolderExprString, invalidatingClass: $invalidatingClass, keepPropertyFetches: $keepPropertyFetches)) { $invalidated = true; $shouldKeep = false; break; @@ -799,7 +801,7 @@ private static function containsExpressionToInvalidate(Scope $scope, ExprPrinter /** * Mirrors the former MutatingScope::shouldInvalidateExpression(). */ - public static function shouldInvalidateExpression(MutatingScope $scope, ExprPrinter $exprPrinter, string $exprStringToInvalidate, Expr $exprToInvalidate, Expr $expr, string $exprString, bool $requireMoreCharacters = false, ?ClassReflection $invalidatingClass = null): bool + 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 { if ( $expr instanceof IntertwinedVariableByReferenceWithExpr @@ -831,6 +833,10 @@ public static function shouldInvalidateExpression(MutatingScope $scope, ExprPrin return $exprStringToInvalidate === $exprString; } + if ($keepPropertyFetches && self::isPropertyFetchChainOn($expr, $exprStringToInvalidate, $exprPrinter)) { + return false; + } + // nodeKey() is the pretty-printed expression, and the standard printer is // compositional: the key of any sub-expression appears verbatim as a substring of // 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 return true; } + /** + * Whether $expr is a chain of property fetches rooted at the invalidated + * expression, like '$this->foo', '$this->foo?->bar' or '$this->$name' for '$this'. + * + * Such an expression only reads state reachable through the object itself, so a + * callee that never receives the object - a static method, a static closure - + * cannot change it and it survives the invalidation. Anything else rooted at the + * object (a method call, an offset access) can observe static state and keeps + * being invalidated. A property name computed from anything but a plain variable + * could do the same, so it is not accepted either. + * + * Callers must still invalidate the object when they hand it to the callee as an + * argument. Reaching it through static state ('self::$instance = $this;' and then + * a static method writing through 'self::$instance') is not tracked - the same + * limitation every receiver other than '$this' has always had. + */ + private static function isPropertyFetchChainOn(Expr $expr, string $exprStringToInvalidate, ExprPrinter $exprPrinter): bool + { + if (!$expr instanceof PropertyFetch && !$expr instanceof NullsafePropertyFetch) { + return false; + } + + while ($expr instanceof PropertyFetch || $expr instanceof NullsafePropertyFetch) { + if ( + !$expr->name instanceof Node\Identifier + && !($expr->name instanceof Variable && is_string($expr->name->name)) + ) { + return false; + } + + $expr = $expr->var; + } + + return self::nodeKey($expr, $exprPrinter) === $exprStringToInvalidate; + } + /** * The conditional-expressions fixed-point matching of * MutatingScope::applySpecifiedTypes(). diff --git a/src/Turbo/TurboExtensionEnabler.php b/src/Turbo/TurboExtensionEnabler.php index bb6b27fdd61..034f390d50c 100644 --- a/src/Turbo/TurboExtensionEnabler.php +++ b/src/Turbo/TurboExtensionEnabler.php @@ -22,7 +22,7 @@ final class TurboExtensionEnabler * version is the short SHA of the last commit touching turbo-ext/src/, * enforced by the phar.yml turbo-version job. */ - public const EXPECTED_EXTENSION_VERSION = 'd7ef536'; + public const EXPECTED_EXTENSION_VERSION = '91db796'; private static bool $typeCombinatorCacheEnabled = false; diff --git a/tests/PHPStan/Analyser/nsrt/bug-13735.php b/tests/PHPStan/Analyser/nsrt/bug-13735.php new file mode 100644 index 00000000000..7a601cd0251 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-13735.php @@ -0,0 +1,84 @@ += 8.0 + +declare(strict_types = 1); + +namespace Bug13735; + +use function PHPStan\Testing\assertType; + +class Bug13735Test +{ + private ?Foo $foo = null; + + public function testFoo(): void + { + $this->foo = new Foo(); + assertType('Bug13735\Foo', $this->foo); + self::assertTrue(true); + assertType('Bug13735\Foo', $this->foo); + } + + public static function assertTrue(mixed $condition, string $message = ''): void + { + + } +} + +class Foo { + public ?Foo $inner = null; + + public function doSomething(): bool { + return true; + } +} + +class Test +{ + private string $data; + + public function __construct() { + $this->data = 'abc'; + assertType("'abc'", $this->data); + self::noop('foo'); + assertType("'abc'", $this->data); + } + + static final public function noop(string $message): void { + file_put_contents('log file', $message); + } + +} + +final class FinalTest +{ + private string $data; + + public function __construct() { + $this->data = 'abc'; + assertType("'abc'", $this->data); + self::noop('foo'); + assertType("'abc'", $this->data); + } + + static public function noop(string $message): void { + file_put_contents('log file', $message); + } + +} + +final class PrivateTest +{ + private string $data; + + public function __construct() { + $this->data = 'abc'; + assertType("'abc'", $this->data); + self::noop('foo'); + assertType("'abc'", $this->data); + } + + static private function noop(string $message): void { + file_put_contents('log file', $message); + } + +} diff --git a/tests/PHPStan/Analyser/nsrt/bug-13735b.php b/tests/PHPStan/Analyser/nsrt/bug-13735b.php new file mode 100644 index 00000000000..3c0aee56cd4 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-13735b.php @@ -0,0 +1,421 @@ += 8.0 + +declare(strict_types = 1); + +namespace Bug13735b; + +use function PHPStan\Testing\assertType; + +class Foo +{ + public ?Bar $bar = null; +} + +class Bar +{ +} + +class HelloWorld extends ParentClass +{ + public ?Foo $foo = null; + + /** @var array */ + private array $arr = []; + + private static ?Foo $staticFoo = null; + + private static ?HelloWorld $instance = null; + + public function doNestedPropertyFetch(): void + { + $this->foo = new Foo(); + $this->foo->bar = new Bar(); + assertType('Bug13735b\Bar', $this->foo->bar); + self::sideEffect(); + assertType('Bug13735b\Bar', $this->foo->bar); + } + + public function doNullsafePropertyFetch(): void + { + if ($this->foo?->bar !== null) { + assertType('Bug13735b\Bar', $this->foo?->bar); + self::sideEffect(); + assertType('Bug13735b\Bar', $this->foo?->bar); + } + } + + public function doDynamicPropertyName(string $name): void + { + if ($this->{$name} instanceof Foo) { + assertType('Bug13735b\Foo', $this->{$name}); + self::sideEffect(); + assertType('Bug13735b\Foo', $this->{$name}); + } + } + + public function doArrayProperty(): void + { + $this->arr['x'] = 'y'; + assertType("non-empty-array&hasOffsetValue('x', 'y')", $this->arr); + assertType("'y'", $this->arr['x']); + self::sideEffect(); + assertType("non-empty-array&hasOffsetValue('x', 'y')", $this->arr); + assertType("'y'", $this->arr['x']); + } + + public function doStaticMethodCalledOnInstance(HelloWorld $other): void + { + $other->foo = new Foo(); + assertType('Bug13735b\Foo', $other->foo); + $other->sideEffect(); + assertType('Bug13735b\Foo', $other->foo); + } + + public function doStaticClosure(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $staticClosure = static function (): void { + file_put_contents('log file', 'foo'); + }; + $staticClosure(); + assertType('Bug13735b\Foo', $this->foo); + } + + public function doStaticClosureGettingThisAsArgument(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $staticClosure = static function (HelloWorld $other): void { + $other->foo = null; + }; + $staticClosure($this); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doStaticArrowFunctionGettingThisAsArgument(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $staticArrowFunction = static fn (HelloWorld $other): ?Foo => $other->foo = null; + $staticArrowFunction($this); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doStaticClosureGettingPropertyAsArgument(): void + { + $this->foo = new Foo(); + $this->foo->bar = new Bar(); + $staticClosure = static function (Foo $foo): void { + $foo->bar = null; + }; + $staticClosure($this->foo); + // a closure that writes anywhere gives the carve-out up entirely - PHPStan cannot + // tell a write through a parameter from one through a capture that aliases $this + assertType('Bug13735b\Foo|null', $this->foo); + assertType('Bug13735b\Bar|null', $this->foo->bar); + } + + public function doStaticClosureGettingResourceAsArgument(): void + { + $fh = fopen('php://memory', 'r'); + if ($fh === false) { + return; + } + + if (ftell($fh) !== false) { + assertType('int', ftell($fh)); + $staticClosure = static function ($handle): void { + fseek($handle, 10); + }; + $staticClosure($fh); + // a resource is a handle to mutable state the closure can move + assertType('int|false', ftell($fh)); + } + } + + public function doStaticClosureGettingScalarAsArgument(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $staticClosure = static function (int $i): void { + file_put_contents('log file', (string) $i); + }; + $staticClosure(1); + assertType('Bug13735b\Foo', $this->foo); + } + + /** + * 'static function () use ($this) {}' is a fatal error - 'Cannot use $this as + * lexical variable' - so a static closure reaches the object by capturing it + * under another name. PHPStan does not track that aliasing, so the write lands + * on '$self->foo' while the caller remembers '$this->foo'. + */ + public function doStaticClosureCapturingReceiver(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $self = $this; + $staticClosure = static function () use ($self): void { + $self->foo = null; + }; + $staticClosure(); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doStaticClosureCapturingReceiverByRef(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $self = $this; + $staticClosure = static function () use (&$self): void { + $self->foo = null; + }; + $staticClosure(); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doStaticClosureCallingImpureMethodOnCapturedReceiver(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $self = $this; + $staticClosure = static function () use ($self): void { + $self->nonStaticMutate($self); + }; + $staticClosure(); + assertType('Bug13735b\Foo|null', $this->foo); + } + + /** + * An arrow function captures the receiver without a 'use' clause at all, so it + * records no used variables - only the write it does gives the capture away. + */ + public function doStaticArrowFunctionCapturingReceiver(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $self = $this; + $staticArrowFunction = static fn (): ?Foo => $self->foo = null; + $staticArrowFunction(); + assertType('Bug13735b\Foo|null', $this->foo); + } + + /** A capture is enough to give the carve-out up, even one that is only read. */ + public function doStaticClosureReadingCapturedReceiver(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $self = $this; + $staticClosure = static function () use ($self): void { + file_put_contents('log file', $self->foo === null ? 'null' : 'foo'); + }; + $staticClosure(); + assertType('Bug13735b\Foo|null', $this->foo); + } + + /** ... and a capture that cannot be the object at all is not told apart either. */ + public function doStaticClosureCapturingScalar(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $message = 'foo'; + $staticClosure = static function () use ($message): void { + file_put_contents('log file', $message); + }; + $staticClosure(); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doNonStaticClosureCapturingReceiver(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $self = $this; + $closure = function () use ($self): void { + $self->foo = null; + }; + $closure(); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doStaticMethodGettingThisAsArgument(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + self::mutate($this); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doStaticMethodGettingPropertyAsArgument(): void + { + $this->foo = new Foo(); + $this->foo->bar = new Bar(); + self::mutateFoo($this->foo); + assertType('Bug13735b\Foo', $this->foo); + assertType('Bug13735b\Bar|null', $this->foo->bar); + } + + public function doLateStaticBindingGettingThisAsArgument(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + static::mutate($this); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doStaticMethodCalledOnThisGettingThisAsArgument(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $this->mutate($this); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doStaticMethodCalledOnInstanceGettingThisAsArgument(HelloWorld $other): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $other->mutate($this); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doMethodCallGettingThisAsArgument(HelloWorld $other): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $other->nonStaticMutate($this); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doMethodCallGettingPropertyAsArgument(HelloWorld $other): void + { + $this->foo = new Foo(); + $this->foo->bar = new Bar(); + $other->mutateFoo($this->foo); + // the callee can change what's inside $this->foo, not which Foo it points at + assertType('Bug13735b\Foo', $this->foo); + assertType('Bug13735b\Bar|null', $this->foo->bar); + } + + public function doNonStaticClosure(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + $closure = function (): void { + file_put_contents('log file', 'foo'); + }; + $closure(); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doMethodCallOnThis(): void + { + if ($this->getFoo() !== null) { + assertType('Bug13735b\Foo', $this->getFoo()); + self::sideEffect(); + assertType('Bug13735b\Foo|null', $this->getFoo()); + } + } + + public function doStaticProperty(): void + { + self::$staticFoo = new Foo(); + assertType('Bug13735b\Foo', self::$staticFoo); + self::sideEffect(); + assertType('Bug13735b\Foo|null', self::$staticFoo); + } + + public function doNonStaticMethod(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + self::nonStatic(); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doLateStaticBinding(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + static::sideEffect(); + assertType('Bug13735b\Foo', $this->foo); + } + + public function doLateStaticBindingNonStaticMethod(): void + { + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + static::nonStatic(); + assertType('Bug13735b\Foo|null', $this->foo); + } + + public function doParentMethod(): void + { + $this->publicFoo = new Foo(); + assertType('Bug13735b\Foo', $this->publicFoo); + parent::nonStaticParent(); + assertType('Bug13735b\Foo|null', $this->publicFoo); + } + + /** + * A static method can also reach the object through static state. PHPStan does + * not track that for any receiver - 'HelloWorld::$instance = $other; HelloWorld::mutateStored();' + * has never invalidated '$other->foo' either - so '$this' is no longer an exception. + */ + public function doReachedViaStaticProperty(): void + { + self::$instance = $this; + $this->foo = new Foo(); + assertType('Bug13735b\Foo', $this->foo); + self::mutateStored(); + assertType('Bug13735b\Foo', $this->foo); + } + + public function getFoo(): ?Foo + { + return $this->foo; + } + + public static function mutate(HelloWorld $other): void + { + $other->foo = null; + } + + public static function mutateFoo(Foo $foo): void + { + $foo->bar = null; + } + + public static function mutateStored(): void + { + if (self::$instance !== null) { + self::$instance->foo = null; + } + } + + public static function sideEffect(): void + { + file_put_contents('log file', 'foo'); + } + + public function nonStatic(): void + { + file_put_contents('log file', 'foo'); + } + + public function nonStaticMutate(HelloWorld $other): void + { + $other->foo = null; + } +} + +class ParentClass +{ + public ?Foo $publicFoo = null; + + public function nonStaticParent(): void + { + file_put_contents('log file', 'foo'); + } +} diff --git a/turbo-ext/src/ScopeOps.cpp b/turbo-ext/src/ScopeOps.cpp index a7f19c342ae..81b321419bc 100644 --- a/turbo-ext/src/ScopeOps.cpp +++ b/turbo-ext/src/ScopeOps.cpp @@ -853,7 +853,8 @@ class ScopeOps zval *invalidatingClass, zv::TableRef expressionTypes, zv::TableRef nativeExpressionTypes, - zv::TableRef conditionalExpressions) + zv::TableRef conditionalExpressions, + bool keepPropertyFetches) { InvalidationQuery query = { scope, @@ -862,6 +863,7 @@ class ScopeOps expressionToInvalidate, invalidatingClass, zend_string_equals_literal(exprStringToInvalidate, "$this"), + keepPropertyFetches, }; /* Mirrors the twin's $canUseKeyPrefilter: outside shouldInvalidate()'s @@ -1063,7 +1065,7 @@ class ScopeOps } /* Mirrors ScopeOps::shouldInvalidateExpression(). */ - static bool shouldInvalidateExpression(zval *scope, zval *exprPrinter, zend_string *exprStringToInvalidate, zval *exprToInvalidate, zend_object *expr, zend_string *exprString, bool requireMoreCharacters, zval *invalidatingClass, bool *failed) + static bool shouldInvalidateExpression(zval *scope, zval *exprPrinter, zend_string *exprStringToInvalidate, zval *exprToInvalidate, zend_object *expr, zend_string *exprString, bool requireMoreCharacters, zval *invalidatingClass, bool keepPropertyFetches, bool *failed) { InvalidationQuery query = { scope, @@ -1072,6 +1074,7 @@ class ScopeOps exprToInvalidate, invalidatingClass, zend_string_equals_literal(exprStringToInvalidate, "$this"), + keepPropertyFetches, }; return shouldInvalidate(query, exprString, expr, requireMoreCharacters, failed); } @@ -1595,6 +1598,7 @@ class ScopeOps zval *expressionToInvalidate; zval *invalidatingClass; /* may be NULL */ bool isThis; + bool keepPropertyFetches; }; static bool strContains(zend_string *haystack, const char *needle, size_t len) @@ -1744,6 +1748,69 @@ class ScopeOps return zend_string_equals(nodeKey.get(), ctx->invalidate_str); } + /* + * Mirrors ScopeOps::isPropertyFetchChainOn(): whether $expr is a chain of + * property fetches rooted at the invalidated expression, state a callee + * that never receives the object cannot change. + */ + static bool isPropertyFetchChainOn(zend_object *expr, zend_string *exprStringToInvalidate, zval *exprPrinter, bool *failed) + { + zend_class_entry *propertyFetchCe = pt_class(PT_CLASS_PROPERTY_FETCH); + zend_class_entry *nullsafeCe = pt_class(PT_CLASS_NULLSAFE_PROPERTY_FETCH); + zend_class_entry *identifierCe = pt_class(PT_CLASS_IDENTIFIER); + zend_class_entry *variableCe = pt_class(PT_CLASS_VARIABLE); + + if (UNEXPECTED(propertyFetchCe == NULL || nullsafeCe == NULL || identifierCe == NULL || variableCe == NULL)) { + *failed = true; + return false; + } + + if (!instanceof_function(expr->ce, propertyFetchCe) && !instanceof_function(expr->ce, nullsafeCe)) { + return false; + } + + while (instanceof_function(expr->ce, propertyFetchCe) || instanceof_function(expr->ce, nullsafeCe)) { + int32_t nameOffset = pt_instance_prop_offset(expr->ce, "name", sizeof("name") - 1); + int32_t varOffset = pt_instance_prop_offset(expr->ce, "var", sizeof("var") - 1); + if (UNEXPECTED(nameOffset < 0 || varOffset < 0)) { + return false; + } + + zv::Ref name = zv::ObjRef(expr).propAtOffset((uint32_t) nameOffset).deref(); + if (!name.isObject()) { + return false; + } + zend_class_entry *nameCe = name.asObject()->ce; + if (!instanceof_function(nameCe, identifierCe)) { + if (!instanceof_function(nameCe, variableCe)) { + return false; + } + pt_node_class_info *nameInfo = pt_get_node_class_info(nameCe); + if (nameInfo == NULL || nameInfo->name_offset < 0) { + return false; + } + zv::Ref variableName = zv::ObjRef(name.asObject()).propAtOffset((uint32_t) nameInfo->name_offset).deref(); + if (!variableName.isString()) { + return false; + } + } + + zv::Ref var = zv::ObjRef(expr).propAtOffset((uint32_t) varOffset).deref(); + if (!var.isObject()) { + return false; + } + expr = var.asObject(); + } + + zv::Str rootKey = zv::Str::adopt(pt_node_key(expr, exprPrinter)); + if (UNEXPECTED(rootKey.isNull())) { + *failed = true; + return false; + } + + return zend_string_equals(rootKey.get(), exprStringToInvalidate); + } + /* * The core of shouldInvalidateExpression(); $requireMoreCharacters is * per-call (the conditional-holder scan passes false). Returns false and @@ -1823,6 +1890,13 @@ class ScopeOps } } + if (query.keepPropertyFetches) { + bool isChain = isPropertyFetchChainOn(expr, query.exprStringToInvalidate, query.exprPrinter, failed); + if (UNEXPECTED(*failed) || isChain) { + return false; + } + } + /* Compositional-key substring gate */ if (!query.isThis && !keyMayHideSubExpressions(query.exprStringToInvalidate) @@ -2034,12 +2108,13 @@ void pt_register_scope_ops() ScopeOps::intersectConditionalExpressions(zv::TableRef(ours), zv::TableRef(theirs)).intoReturnValue(return_value); }); - cls.method("invalidateExpressionEntries", reg::PublicStatic, 9, { reg::objectArg("scope"), reg::objectArg("exprPrinter"), reg::stringArg("exprStringToInvalidate"), reg::objectArg("expressionToInvalidate"), reg::boolArg("requireMoreCharacters"), reg::objectArg("invalidatingClass", true), reg::arrayArg("expressionTypes"), reg::arrayArg("nativeExpressionTypes"), reg::arrayArg("conditionalExpressions") }, [](INTERNAL_FUNCTION_PARAMETERS) { + cls.method("invalidateExpressionEntries", reg::PublicStatic, 9, { reg::objectArg("scope"), reg::objectArg("exprPrinter"), reg::stringArg("exprStringToInvalidate"), reg::objectArg("expressionToInvalidate"), reg::boolArg("requireMoreCharacters"), reg::objectArg("invalidatingClass", true), reg::arrayArg("expressionTypes"), reg::arrayArg("nativeExpressionTypes"), reg::arrayArg("conditionalExpressions"), reg::boolArg("keepPropertyFetches") }, [](INTERNAL_FUNCTION_PARAMETERS) { zval *scope, *expr_printer, *expr_to_invalidate, *invalidating_class = NULL; zend_string *invalidate_str; bool require_more_characters; + bool keep_property_fetches = false; HashTable *expression_types, *native_expression_types, *conditional_expressions; - ZEND_PARSE_PARAMETERS_START(9, 9) + ZEND_PARSE_PARAMETERS_START(9, 10) Z_PARAM_OBJECT(scope) Z_PARAM_OBJECT(expr_printer) Z_PARAM_STR(invalidate_str) @@ -2049,20 +2124,23 @@ void pt_register_scope_ops() Z_PARAM_ARRAY_HT(expression_types) Z_PARAM_ARRAY_HT(native_expression_types) Z_PARAM_ARRAY_HT(conditional_expressions) + Z_PARAM_OPTIONAL + Z_PARAM_BOOL(keep_property_fetches) ZEND_PARSE_PARAMETERS_END(); pt_init_strs(); - zv::Val result = ScopeOps::invalidateExpressionEntries(scope, expr_printer, invalidate_str, expr_to_invalidate, require_more_characters, invalidating_class, zv::TableRef(expression_types), zv::TableRef(native_expression_types), zv::TableRef(conditional_expressions)); + zv::Val result = ScopeOps::invalidateExpressionEntries(scope, expr_printer, invalidate_str, expr_to_invalidate, require_more_characters, invalidating_class, zv::TableRef(expression_types), zv::TableRef(native_expression_types), zv::TableRef(conditional_expressions), keep_property_fetches); if (UNEXPECTED(result.isUndef())) { RETURN_THROWS(); } result.intoReturnValue(return_value); }); - cls.method("shouldInvalidateExpression", reg::PublicStatic, 6, { reg::objectArg("scope"), reg::objectArg("exprPrinter"), reg::stringArg("exprStringToInvalidate"), reg::objectArg("exprToInvalidate"), reg::objectArg("expr"), reg::stringArg("exprString"), reg::boolArg("requireMoreCharacters"), reg::objectArg("invalidatingClass", true) }, [](INTERNAL_FUNCTION_PARAMETERS) { + cls.method("shouldInvalidateExpression", reg::PublicStatic, 6, { reg::objectArg("scope"), reg::objectArg("exprPrinter"), reg::stringArg("exprStringToInvalidate"), reg::objectArg("exprToInvalidate"), reg::objectArg("expr"), reg::stringArg("exprString"), reg::boolArg("requireMoreCharacters"), reg::objectArg("invalidatingClass", true), reg::boolArg("keepPropertyFetches") }, [](INTERNAL_FUNCTION_PARAMETERS) { zval *scope, *expr_printer, *expr_to_invalidate, *expr, *invalidating_class = NULL; zend_string *invalidate_str, *expr_string; bool require_more_characters = false; - ZEND_PARSE_PARAMETERS_START(6, 8) + bool keep_property_fetches = false; + ZEND_PARSE_PARAMETERS_START(6, 9) Z_PARAM_OBJECT(scope) Z_PARAM_OBJECT(expr_printer) Z_PARAM_STR(invalidate_str) @@ -2072,10 +2150,11 @@ void pt_register_scope_ops() Z_PARAM_OPTIONAL Z_PARAM_BOOL(require_more_characters) Z_PARAM_OBJECT_OR_NULL(invalidating_class) + Z_PARAM_BOOL(keep_property_fetches) ZEND_PARSE_PARAMETERS_END(); pt_init_strs(); bool failed = false; - bool result = ScopeOps::shouldInvalidateExpression(scope, expr_printer, invalidate_str, expr_to_invalidate, Z_OBJ_P(expr), expr_string, require_more_characters, invalidating_class, &failed); + bool result = ScopeOps::shouldInvalidateExpression(scope, expr_printer, invalidate_str, expr_to_invalidate, Z_OBJ_P(expr), expr_string, require_more_characters, invalidating_class, keep_property_fetches, &failed); if (UNEXPECTED(failed)) { RETURN_THROWS(); } diff --git a/turbo-ext/src/support.cpp b/turbo-ext/src/support.cpp index 6f2ff142996..069102d2bf0 100644 --- a/turbo-ext/src/support.cpp +++ b/turbo-ext/src/support.cpp @@ -28,6 +28,8 @@ static const pt_class_template pt_class_templates[PT_CLASS_COUNT] = { /* PT_CLASS_NAME */ {"name", "PhpParser\\Node\\Name"}, /* PT_CLASS_EXPR */ {"expr", "PhpParser\\Node\\Expr"}, /* PT_CLASS_PROPERTY_FETCH */ {"propertyFetch", "PhpParser\\Node\\Expr\\PropertyFetch"}, + /* PT_CLASS_NULLSAFE_PROPERTY_FETCH */ {"nullsafePropertyFetch", "PhpParser\\Node\\Expr\\NullsafePropertyFetch"}, + /* PT_CLASS_IDENTIFIER */ {"identifier", "PhpParser\\Node\\Identifier"}, /* PT_CLASS_INTERTWINED_VAR */ {"intertwinedVariableByReferenceWithExpr", "PHPStan\\Node\\Expr\\IntertwinedVariableByReferenceWithExpr"}, /* PT_CLASS_ARRAY_DIM_FETCH */ {"arrayDimFetch", "PhpParser\\Node\\Expr\\ArrayDimFetch"}, /* PT_CLASS_METHOD_CALL */ {"methodCall", "PhpParser\\Node\\Expr\\MethodCall"}, diff --git a/turbo-ext/src/support.h b/turbo-ext/src/support.h index 89b2bef8ace..ae0c1295cee 100644 --- a/turbo-ext/src/support.h +++ b/turbo-ext/src/support.h @@ -60,6 +60,8 @@ enum { PT_CLASS_NAME, PT_CLASS_EXPR, PT_CLASS_PROPERTY_FETCH, + PT_CLASS_NULLSAFE_PROPERTY_FETCH, + PT_CLASS_IDENTIFIER, PT_CLASS_INTERTWINED_VAR, PT_CLASS_ARRAY_DIM_FETCH, PT_CLASS_METHOD_CALL,