From 10c6caee6c854b74b00f211bd552b0c3f7c6d835 Mon Sep 17 00:00:00 2001 From: Caleb White Date: Sat, 2 Nov 2024 20:48:21 -0500 Subject: [PATCH 1/8] test: add failing test case for bug #11857 --- tests/PHPStan/Analyser/Bug11857Test.php | 46 ++++++++++ tests/PHPStan/Analyser/bug-11857.neon | 5 ++ tests/PHPStan/Analyser/data/bug-11857.php | 101 ++++++++++++++++++++++ 3 files changed, 152 insertions(+) create mode 100644 tests/PHPStan/Analyser/Bug11857Test.php create mode 100644 tests/PHPStan/Analyser/bug-11857.neon create mode 100644 tests/PHPStan/Analyser/data/bug-11857.php diff --git a/tests/PHPStan/Analyser/Bug11857Test.php b/tests/PHPStan/Analyser/Bug11857Test.php new file mode 100644 index 0000000000..a49ecb6b95 --- /dev/null +++ b/tests/PHPStan/Analyser/Bug11857Test.php @@ -0,0 +1,46 @@ +assertNoErrors($this->runAnalyse($file)); + } + + /** @return Error[] */ + private function runAnalyse(string $file): array + { + $file = $this->getFileHelper()->normalizePath($file); + + $analyser = self::getContainer()->getByType(Analyser::class); + $fileHelper = self::getContainer()->getByType(FileHelper::class); + + $errors = $analyser->analyse([$file], null, null, true, null)->getErrors(); + + foreach ($errors as $error) { + $this->assertSame($fileHelper->normalizePath($file), $error->getFilePath()); + } + + return $errors; + } + + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/bug-11857.neon']; + } +} diff --git a/tests/PHPStan/Analyser/bug-11857.neon b/tests/PHPStan/Analyser/bug-11857.neon new file mode 100644 index 0000000000..92929b9c91 --- /dev/null +++ b/tests/PHPStan/Analyser/bug-11857.neon @@ -0,0 +1,5 @@ +services: + - + class: Bug11857\RelationDynamicMethodReturnTypeExtension + tags: + - phpstan.broker.dynamicMethodReturnTypeExtension diff --git a/tests/PHPStan/Analyser/data/bug-11857.php b/tests/PHPStan/Analyser/data/bug-11857.php new file mode 100644 index 0000000000..c84a735c24 --- /dev/null +++ b/tests/PHPStan/Analyser/data/bug-11857.php @@ -0,0 +1,101 @@ +getName() === 'belongsTo'; + } + + public function getTypeFromMethodCall(MethodReflection $methodReflection, MethodCall $methodCall, Scope $scope): Type { + $returnType = $methodReflection->getVariants()[0]->getReturnType(); + $argType = $scope->getType($methodCall->getArgs()[0]->value); + $modelClass = $argType->getClassStringObjectType()->getObjectClassNames()[0]; + + return new GenericObjectType($returnType->getObjectClassNames()[0], [ + new ObjectType($modelClass), + $scope->getType($methodCall->var), + ]); + } +} + +abstract class Model +{ + /** @return BelongsTo<*, *> */ + public function belongsTo(string $related): BelongsTo + { + return new BelongsTo(); + } +} + +/** + * @template TRelatedModel of Model + * @template TDeclaringModel of Model + */ +class BelongsTo {} + +class User extends Model {} + +class Post extends Model +{ + /** @return BelongsTo */ + public function user(): BelongsTo + { + return $this->belongsTo(User::class); + } + + /** @return BelongsTo */ + public function userSelf(): BelongsTo + { + /** @phpstan-ignore return.type */ + return $this->belongsTo(User::class); + } +} + +class ChildPost extends Post {} + +final class Comment extends Model +{ + // This model is final, so either of these + // two methods would work. It seems that + // PHPStan is automatically converting the + // `$this` to a `self` type in the user docblock, + // but it is not doing so likewise for the `$this` + // that is returned by the dynamic return extension. + + /** @return BelongsTo */ + public function user(): BelongsTo + { + return $this->belongsTo(User::class); + } + + /** @return BelongsTo */ + public function user2(): BelongsTo + { + return $this->belongsTo(User::class); + } +} + +function test(ChildPost $child): void +{ + assertType('Bug11857\BelongsTo', $child->user()); + // This demonstrates why `$this` is needed in non-final models + assertType('Bug11857\BelongsTo', $child->userSelf()); +} From 3dc9a893081c7816f0f359903128306c42397d69 Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Wed, 6 Nov 2024 11:07:58 +0100 Subject: [PATCH 2/8] rework test --- .github/workflows/e2e-tests.yml | 4 ++ e2e/bug-11857/.gitignore | 1 + e2e/bug-11857/composer.json | 5 ++ e2e/bug-11857/composer.lock | 18 ++++++++ .../bug-11857/phpstan.neon | 5 ++ e2e/bug-11857/src/extension.php | 36 +++++++++++++++ .../bug-11857/src/test.php | 32 ------------- tests/PHPStan/Analyser/Bug11857Test.php | 46 ------------------- 8 files changed, 69 insertions(+), 78 deletions(-) create mode 100644 e2e/bug-11857/.gitignore create mode 100644 e2e/bug-11857/composer.json create mode 100644 e2e/bug-11857/composer.lock rename tests/PHPStan/Analyser/bug-11857.neon => e2e/bug-11857/phpstan.neon (77%) create mode 100644 e2e/bug-11857/src/extension.php rename tests/PHPStan/Analyser/data/bug-11857.php => e2e/bug-11857/src/test.php (58%) delete mode 100644 tests/PHPStan/Analyser/Bug11857Test.php diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 8914cca904..4b90be1ec3 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -255,6 +255,10 @@ jobs: echo "$OUTPUT" ../bashunit -a contains 'Child process error (exit code 255): PHP Fatal error' "$OUTPUT" ../bashunit -a contains 'Result is incomplete because of severe errors.' "$OUTPUT" + - script: | + cd e2e/bug-11857 + composer install + ../../bin/phpstan steps: - name: "Checkout" diff --git a/e2e/bug-11857/.gitignore b/e2e/bug-11857/.gitignore new file mode 100644 index 0000000000..61ead86667 --- /dev/null +++ b/e2e/bug-11857/.gitignore @@ -0,0 +1 @@ +/vendor diff --git a/e2e/bug-11857/composer.json b/e2e/bug-11857/composer.json new file mode 100644 index 0000000000..a072011fe8 --- /dev/null +++ b/e2e/bug-11857/composer.json @@ -0,0 +1,5 @@ +{ + "autoload-dev": { + "classmap": ["src/"] + } +} diff --git a/e2e/bug-11857/composer.lock b/e2e/bug-11857/composer.lock new file mode 100644 index 0000000000..b383d88ac5 --- /dev/null +++ b/e2e/bug-11857/composer.lock @@ -0,0 +1,18 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state", + "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", + "This file is @generated automatically" + ], + "content-hash": "d751713988987e9331980363e24189ce", + "packages": [], + "packages-dev": [], + "aliases": [], + "minimum-stability": "stable", + "stability-flags": {}, + "prefer-stable": false, + "prefer-lowest": false, + "platform": {}, + "platform-dev": {}, + "plugin-api-version": "2.6.0" +} diff --git a/tests/PHPStan/Analyser/bug-11857.neon b/e2e/bug-11857/phpstan.neon similarity index 77% rename from tests/PHPStan/Analyser/bug-11857.neon rename to e2e/bug-11857/phpstan.neon index 92929b9c91..306701cd6f 100644 --- a/tests/PHPStan/Analyser/bug-11857.neon +++ b/e2e/bug-11857/phpstan.neon @@ -1,3 +1,8 @@ +parameters: + level: 8 + paths: + - src + services: - class: Bug11857\RelationDynamicMethodReturnTypeExtension diff --git a/e2e/bug-11857/src/extension.php b/e2e/bug-11857/src/extension.php new file mode 100644 index 0000000000..c24d0f3653 --- /dev/null +++ b/e2e/bug-11857/src/extension.php @@ -0,0 +1,36 @@ +getName() === 'belongsTo'; + } + + public function getTypeFromMethodCall(MethodReflection $methodReflection, MethodCall $methodCall, Scope $scope): Type { + $returnType = $methodReflection->getVariants()[0]->getReturnType(); + $argType = $scope->getType($methodCall->getArgs()[0]->value); + $modelClass = $argType->getClassStringObjectType()->getObjectClassNames()[0]; + + return new GenericObjectType($returnType->getObjectClassNames()[0], [ + new ObjectType($modelClass), + $scope->getType($methodCall->var), + ]); + } +} + diff --git a/tests/PHPStan/Analyser/data/bug-11857.php b/e2e/bug-11857/src/test.php similarity index 58% rename from tests/PHPStan/Analyser/data/bug-11857.php rename to e2e/bug-11857/src/test.php index c84a735c24..7efbf3b177 100644 --- a/tests/PHPStan/Analyser/data/bug-11857.php +++ b/e2e/bug-11857/src/test.php @@ -2,40 +2,8 @@ namespace Bug11857; -use PHPStan\Analyser\Scope; -use PHPStan\Reflection\MethodReflection; -use PHPStan\Type\DynamicMethodReturnTypeExtension; -use PHPStan\Type\Generic\GenericObjectType; -use PHPStan\Type\ObjectType; -use PHPStan\Type\Type; -use PhpParser\Node\Expr\MethodCall; - use function PHPStan\Testing\assertType; -class RelationDynamicMethodReturnTypeExtension implements DynamicMethodReturnTypeExtension -{ - public function getClass(): string - { - return Model::class; - } - - public function isMethodSupported(MethodReflection $methodReflection): bool - { - return $methodReflection->getName() === 'belongsTo'; - } - - public function getTypeFromMethodCall(MethodReflection $methodReflection, MethodCall $methodCall, Scope $scope): Type { - $returnType = $methodReflection->getVariants()[0]->getReturnType(); - $argType = $scope->getType($methodCall->getArgs()[0]->value); - $modelClass = $argType->getClassStringObjectType()->getObjectClassNames()[0]; - - return new GenericObjectType($returnType->getObjectClassNames()[0], [ - new ObjectType($modelClass), - $scope->getType($methodCall->var), - ]); - } -} - abstract class Model { /** @return BelongsTo<*, *> */ diff --git a/tests/PHPStan/Analyser/Bug11857Test.php b/tests/PHPStan/Analyser/Bug11857Test.php deleted file mode 100644 index a49ecb6b95..0000000000 --- a/tests/PHPStan/Analyser/Bug11857Test.php +++ /dev/null @@ -1,46 +0,0 @@ -assertNoErrors($this->runAnalyse($file)); - } - - /** @return Error[] */ - private function runAnalyse(string $file): array - { - $file = $this->getFileHelper()->normalizePath($file); - - $analyser = self::getContainer()->getByType(Analyser::class); - $fileHelper = self::getContainer()->getByType(FileHelper::class); - - $errors = $analyser->analyse([$file], null, null, true, null)->getErrors(); - - foreach ($errors as $error) { - $this->assertSame($fileHelper->normalizePath($file), $error->getFilePath()); - } - - return $errors; - } - - public static function getAdditionalConfigFiles(): array - { - return [__DIR__ . '/bug-11857.neon']; - } -} From 0091f49040fec358dd8dfe9b89979f47e318ccd6 Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Wed, 6 Nov 2024 11:19:47 +0100 Subject: [PATCH 3/8] fix attempts --- src/Analyser/MutatingScope.php | 3 +++ src/Analyser/NodeScopeResolver.php | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index c24defb1f8..980e2ad152 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -3012,6 +3012,9 @@ private function transformStaticType(Type $type): Type if (!$this->isInClass()) { return $type; } + if ($type instanceof ThisType) { + return $traverse($type); + } if ($type instanceof StaticType) { $classReflection = $this->getClassReflection(); $changedType = $type->changeBaseClass($classReflection); diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 4a8264b674..4f3fa9a303 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -6157,6 +6157,10 @@ public function getPhpDocs(Scope $scope, Node\FunctionLike|Node\Stmt\Property $n private function transformStaticType(ClassReflection $declaringClass, Type $type): Type { return TypeTraverser::map($type, static function (Type $type, callable $traverse) use ($declaringClass): Type { + if ($type instanceof ThisType) { + return $traverse($type); + } + if ($type instanceof StaticType) { $changedType = $type->changeBaseClass($declaringClass); if ($declaringClass->isFinal()) { From c7a5302b1621c0a03dc0f2eb8fd48945230de293 Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Wed, 6 Nov 2024 11:22:03 +0100 Subject: [PATCH 4/8] Fix test --- e2e/bug-11857/src/test.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/e2e/bug-11857/src/test.php b/e2e/bug-11857/src/test.php index 7efbf3b177..5c237f25e8 100644 --- a/e2e/bug-11857/src/test.php +++ b/e2e/bug-11857/src/test.php @@ -57,6 +57,7 @@ public function user(): BelongsTo /** @return BelongsTo */ public function user2(): BelongsTo { + /** @phpstan-ignore return.type */ return $this->belongsTo(User::class); } } @@ -65,5 +66,5 @@ function test(ChildPost $child): void { assertType('Bug11857\BelongsTo', $child->user()); // This demonstrates why `$this` is needed in non-final models - assertType('Bug11857\BelongsTo', $child->userSelf()); + assertType('Bug11857\BelongsTo', $child->userSelf()); // should be: Bug11857\BelongsTo } From 0ae429c17e729c626e4e09c049ae78040c5233d8 Mon Sep 17 00:00:00 2001 From: Caleb White Date: Wed, 6 Nov 2024 09:16:10 -0600 Subject: [PATCH 5/8] test: add builder test --- e2e/bug-11857/src/builders.php | 71 ++++++++++++++++++++++++++++++++++ e2e/bug-11857/src/test.php | 3 +- 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 e2e/bug-11857/src/builders.php diff --git a/e2e/bug-11857/src/builders.php b/e2e/bug-11857/src/builders.php new file mode 100644 index 0000000000..b3dd47cc58 --- /dev/null +++ b/e2e/bug-11857/src/builders.php @@ -0,0 +1,71 @@ + $attributes + * @return $this + */ + public function filter(array $attributes): static + { + // filter stuff + return $this; + } + + /** + * @param array $attributes + * @return $this + */ + public function filterUsingRequest(array $attributes): static + { + // request handling + return $this->filter($attributes); + } +} + +/** @template TModel of Model */ +class Builder +{ + public function __construct( + /** @var TModel */ + protected Model $model + ) { + } + +} + +/** + * @template TModel of Model + * @extends Builder + */ +class BaseBuilder extends Builder +{ + use Filterable; +} + +/** @extends BaseBuilder */ +class UserBuilder extends BaseBuilder {} + +/** + * @template TModel of Model + * @extends Builder + */ +class PackageBuilder extends Builder {} + +// this extends a Builder coming from a package +// so can't extend the BaseBuilder in our app +/** @extends PackageBuilder */ +final class CommentBuilder extends PackageBuilder +{ + use Filterable; +} + +function test(UserBuilder $user, CommentBuilder $comment): void +{ + assertType(UserBuilder::class, $user->filterUsingRequest(['foo' => 'bar'])); + assertType(CommentBuilder::class, $comment->filterUsingRequest(['foo' => 'bar'])); +} diff --git a/e2e/bug-11857/src/test.php b/e2e/bug-11857/src/test.php index 5c237f25e8..b982cb18b0 100644 --- a/e2e/bug-11857/src/test.php +++ b/e2e/bug-11857/src/test.php @@ -66,5 +66,6 @@ function test(ChildPost $child): void { assertType('Bug11857\BelongsTo', $child->user()); // This demonstrates why `$this` is needed in non-final models - assertType('Bug11857\BelongsTo', $child->userSelf()); // should be: Bug11857\BelongsTo + // Should be: Bug11857\BelongsTo + assertType('Bug11857\BelongsTo', $child->userSelf()); } From c2ae542a17d03a22546d59b8de0e8aa480c29138 Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Wed, 6 Nov 2024 19:34:13 +0100 Subject: [PATCH 6/8] Revert "test: add builder test" This reverts commit 0ae429c17e729c626e4e09c049ae78040c5233d8. --- e2e/bug-11857/src/builders.php | 71 ---------------------------------- e2e/bug-11857/src/test.php | 3 +- 2 files changed, 1 insertion(+), 73 deletions(-) delete mode 100644 e2e/bug-11857/src/builders.php diff --git a/e2e/bug-11857/src/builders.php b/e2e/bug-11857/src/builders.php deleted file mode 100644 index b3dd47cc58..0000000000 --- a/e2e/bug-11857/src/builders.php +++ /dev/null @@ -1,71 +0,0 @@ - $attributes - * @return $this - */ - public function filter(array $attributes): static - { - // filter stuff - return $this; - } - - /** - * @param array $attributes - * @return $this - */ - public function filterUsingRequest(array $attributes): static - { - // request handling - return $this->filter($attributes); - } -} - -/** @template TModel of Model */ -class Builder -{ - public function __construct( - /** @var TModel */ - protected Model $model - ) { - } - -} - -/** - * @template TModel of Model - * @extends Builder - */ -class BaseBuilder extends Builder -{ - use Filterable; -} - -/** @extends BaseBuilder */ -class UserBuilder extends BaseBuilder {} - -/** - * @template TModel of Model - * @extends Builder - */ -class PackageBuilder extends Builder {} - -// this extends a Builder coming from a package -// so can't extend the BaseBuilder in our app -/** @extends PackageBuilder */ -final class CommentBuilder extends PackageBuilder -{ - use Filterable; -} - -function test(UserBuilder $user, CommentBuilder $comment): void -{ - assertType(UserBuilder::class, $user->filterUsingRequest(['foo' => 'bar'])); - assertType(CommentBuilder::class, $comment->filterUsingRequest(['foo' => 'bar'])); -} diff --git a/e2e/bug-11857/src/test.php b/e2e/bug-11857/src/test.php index b982cb18b0..5c237f25e8 100644 --- a/e2e/bug-11857/src/test.php +++ b/e2e/bug-11857/src/test.php @@ -66,6 +66,5 @@ function test(ChildPost $child): void { assertType('Bug11857\BelongsTo', $child->user()); // This demonstrates why `$this` is needed in non-final models - // Should be: Bug11857\BelongsTo - assertType('Bug11857\BelongsTo', $child->userSelf()); + assertType('Bug11857\BelongsTo', $child->userSelf()); // should be: Bug11857\BelongsTo } From 0c18bb96216193595e2ede362277640621fb266e Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Wed, 6 Nov 2024 19:34:05 +0100 Subject: [PATCH 7/8] Simpler test --- .../Rules/Methods/ReturnTypeRuleTest.php | 5 ++ .../Rules/Methods/data/bug-11857-builder.php | 49 +++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 tests/PHPStan/Rules/Methods/data/bug-11857-builder.php diff --git a/tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php b/tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php index fcbc0643c9..eaa0465a42 100644 --- a/tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php +++ b/tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php @@ -1059,4 +1059,9 @@ public function testBug11663(): void $this->analyse([__DIR__ . '/data/bug-11663.php'], []); } + public function testBug11857(): void + { + $this->analyse([__DIR__ . '/data/bug-11857-builder.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Methods/data/bug-11857-builder.php b/tests/PHPStan/Rules/Methods/data/bug-11857-builder.php new file mode 100644 index 0000000000..89209ffe37 --- /dev/null +++ b/tests/PHPStan/Rules/Methods/data/bug-11857-builder.php @@ -0,0 +1,49 @@ += 8.0 + +namespace Bug11857Builder; + +class Foo +{ + + /** + * @param array $attributes + * @return $this + */ + public function filter(array $attributes): static + { + return $this; + } + + /** + * @param array $attributes + * @return $this + */ + public function filterUsingRequest(array $attributes): static + { + return $this->filter($attributes); + } + +} + +final class FinalFoo +{ + + /** + * @param array $attributes + * @return $this + */ + public function filter(array $attributes): static + { + return $this; + } + + /** + * @param array $attributes + * @return $this + */ + public function filterUsingRequest(array $attributes): static + { + return $this->filter($attributes); + } + +} From 3da993b046f53e1bdd36c33c815e4ae56cf128ab Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Wed, 6 Nov 2024 19:39:25 +0100 Subject: [PATCH 8/8] Better fixes --- src/Analyser/MutatingScope.php | 5 +---- src/Analyser/NodeScopeResolver.php | 6 +----- src/Type/StaticType.php | 2 +- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index 980e2ad152..df85c5fa35 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -3012,13 +3012,10 @@ private function transformStaticType(Type $type): Type if (!$this->isInClass()) { return $type; } - if ($type instanceof ThisType) { - return $traverse($type); - } if ($type instanceof StaticType) { $classReflection = $this->getClassReflection(); $changedType = $type->changeBaseClass($classReflection); - if ($classReflection->isFinal()) { + if ($classReflection->isFinal() && !$type instanceof ThisType) { $changedType = $changedType->getStaticObjectType(); } return $traverse($changedType); diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 4f3fa9a303..dda335f566 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -6157,13 +6157,9 @@ public function getPhpDocs(Scope $scope, Node\FunctionLike|Node\Stmt\Property $n private function transformStaticType(ClassReflection $declaringClass, Type $type): Type { return TypeTraverser::map($type, static function (Type $type, callable $traverse) use ($declaringClass): Type { - if ($type instanceof ThisType) { - return $traverse($type); - } - if ($type instanceof StaticType) { $changedType = $type->changeBaseClass($declaringClass); - if ($declaringClass->isFinal()) { + if ($declaringClass->isFinal() && !$type instanceof ThisType) { $changedType = $changedType->getStaticObjectType(); } return $traverse($changedType); diff --git a/src/Type/StaticType.php b/src/Type/StaticType.php index 9cf4da4dc8..5fd6f7e358 100644 --- a/src/Type/StaticType.php +++ b/src/Type/StaticType.php @@ -299,7 +299,7 @@ private function transformStaticType(Type $type, ClassMemberAccessAnswerer $scop $isFinal = $classReflection->isFinal(); } $type = $type->changeBaseClass($classReflection); - if (!$isFinal) { + if (!$isFinal || $type instanceof ThisType) { return $type; }