From b571eacf502d271a43a391174bd53d66953d1a90 Mon Sep 17 00:00:00 2001 From: Can Vural Date: Fri, 7 Jun 2024 12:55:30 +0200 Subject: [PATCH 1/3] reproduce a weird bug with closures --- tests/PHPStan/Analyser/WeirdBugTest.php | 35 ++++ .../Analyser/data/weird-bug-extensions.php | 171 ++++++++++++++++++ tests/PHPStan/Analyser/data/weird-bug.php | 50 +++++ tests/PHPStan/Analyser/weird-bug-config.neon | 10 + 4 files changed, 266 insertions(+) create mode 100644 tests/PHPStan/Analyser/WeirdBugTest.php create mode 100644 tests/PHPStan/Analyser/data/weird-bug-extensions.php create mode 100644 tests/PHPStan/Analyser/data/weird-bug.php create mode 100644 tests/PHPStan/Analyser/weird-bug-config.neon diff --git a/tests/PHPStan/Analyser/WeirdBugTest.php b/tests/PHPStan/Analyser/WeirdBugTest.php new file mode 100644 index 0000000000..b60798b1a5 --- /dev/null +++ b/tests/PHPStan/Analyser/WeirdBugTest.php @@ -0,0 +1,35 @@ +gatherAssertTypes(__DIR__ . '/data/weird-bug.php'); + } + + /** + * @dataProvider dataFileAsserts + * @param mixed ...$args + */ + public function testFileAsserts( + string $assertType, + string $file, + ...$args, + ): void + { + $this->assertFileAsserts($assertType, $file, ...$args); + } + + public static function getAdditionalConfigFiles(): array + { + return [ + __DIR__ . '/weird-bug-config.neon', + ]; + } + +} diff --git a/tests/PHPStan/Analyser/data/weird-bug-extensions.php b/tests/PHPStan/Analyser/data/weird-bug-extensions.php new file mode 100644 index 0000000000..2d374cc8bb --- /dev/null +++ b/tests/PHPStan/Analyser/data/weird-bug-extensions.php @@ -0,0 +1,171 @@ +getDeclaringClass()->getName() === \WeirdBug\Builder::class && + $parameter->getName() === 'callback' && + $methodReflection->getName() === 'methodWithCallback'; + } + + public function getTypeFromMethodCall( + MethodReflection $methodReflection, + MethodCall $methodCall, + ParameterReflection $parameter, + Scope $scope + ): ?Type { + return new CallableType( + [ + new NativeParameterReflection('test', false, new GenericObjectType(\WeirdBug\Builder::class, [new ObjectType(\WeirdBug\SubModel::class)]), PassedByReference::createNo(), false, null), // @phpstan-ignore-line + ] + ); + } +} + +class BuilderForwardsCallsToAnotherBuilderExtensions implements MethodsClassReflectionExtension +{ + + public function __construct(private ReflectionProvider $reflectionProvider) + { + } + + public function hasMethod(ClassReflection $classReflection, string $methodName): bool + { + return $classReflection->getName() === \WeirdBug\Builder::class && $this->reflectionProvider->getClass(\WeirdBug\AnotherBuilder::class)->hasNativeMethod($methodName); + } + + public function getMethod( + ClassReflection $classReflection, + string $methodName + ): MethodReflection { + $ref = $this->reflectionProvider->getClass(\WeirdBug\AnotherBuilder::class)->getNativeMethod($methodName); + + /** @var ObjectType $model */ + $model = $classReflection->getActiveTemplateTypeMap()->getType('T'); + + return new BuilderMethodReflection( + $methodName, + $classReflection, + $ref->getVariants()[0]->getParameters(), + $model->getMethod('getBuilder', new OutOfClassScope())->getVariants()[0]->getReturnType(), + $ref->getVariants()[0]->isVariadic() + ); + } +} + +final class BuilderMethodReflection implements MethodReflection +{ + private Type $returnType; + + /** @param ParameterReflection[] $parameters */ + public function __construct(private string $methodName, private ClassReflection $classReflection, private array $parameters, Type|null $returnType = null, private bool $isVariadic = false) + { + $this->returnType = $returnType ?? new ObjectType(\WeirdBug\Builder::class); + } + + public function getDeclaringClass(): ClassReflection + { + return $this->classReflection; + } + + public function isStatic(): bool + { + return true; + } + + public function isPrivate(): bool + { + return false; + } + + public function isPublic(): bool + { + return true; + } + + public function getName(): string + { + return $this->methodName; + } + + public function getPrototype(): ClassMemberReflection + { + return $this; + } + + /** + * {@inheritDoc} + */ + public function getVariants(): array + { + return [ + new FunctionVariant( + TemplateTypeMap::createEmpty(), + null, + $this->parameters, + $this->isVariadic, + $this->returnType, + ), + ]; + } + + public function getDocComment(): string|null + { + return null; + } + + public function isDeprecated(): TrinaryLogic + { + return TrinaryLogic::createNo(); + } + + public function getDeprecatedDescription(): string|null + { + return null; + } + + public function isFinal(): TrinaryLogic + { + return TrinaryLogic::createNo(); + } + + public function isInternal(): TrinaryLogic + { + return TrinaryLogic::createNo(); + } + + public function getThrowType(): Type|null + { + return null; + } + + public function hasSideEffects(): TrinaryLogic + { + return TrinaryLogic::createMaybe(); + } +} diff --git a/tests/PHPStan/Analyser/data/weird-bug.php b/tests/PHPStan/Analyser/data/weird-bug.php new file mode 100644 index 0000000000..5a2b72caa0 --- /dev/null +++ b/tests/PHPStan/Analyser/data/weird-bug.php @@ -0,0 +1,50 @@ + + */ + public static function getBuilder(): Builder + { + return new Builder(new static()); // @phpstan-ignore-line + } +} + +class SubModel extends Model {} + +/** + * @template T of Model + */ +class Builder { + + /** + * @param T $model + */ + public function __construct(private Model $model) // @phpstan-ignore-line + { + } + + public function methodWithCallback(\Closure $callback): void + { + $callback($this); + } +} + +class AnotherBuilder { + public function someMethod(): self + { + return $this; + } +} + +function test(): void +{ + SubModel::getBuilder()->methodWithCallback(function (Builder $builder, $value) { + assertType('WeirdBug\Builder', $builder); + return $builder->someMethod(); + }); +} diff --git a/tests/PHPStan/Analyser/weird-bug-config.neon b/tests/PHPStan/Analyser/weird-bug-config.neon new file mode 100644 index 0000000000..f5f3ef89c9 --- /dev/null +++ b/tests/PHPStan/Analyser/weird-bug-config.neon @@ -0,0 +1,10 @@ +services: + - + class: WeirdBugExtensions\MethodParameterClosureTypeExtension + tags: + - phpstan.methodParameterClosureTypeExtension + + - + class: WeirdBugExtensions\BuilderForwardsCallsToAnotherBuilderExtensions + tags: + - phpstan.broker.methodsClassReflectionExtension From 57e8687ddda94e4e15bf9686ea2020578d1ade5a Mon Sep 17 00:00:00 2001 From: Can Vural Date: Fri, 7 Jun 2024 14:47:16 +0200 Subject: [PATCH 2/3] move the test to rule tests --- tests/PHPStan/Analyser/WeirdBugTest.php | 35 ------------------ .../PHPStan/Rules/Functions/WeirdBugTest.php | 37 +++++++++++++++++++ .../Functions/data}/weird-bug-config.neon | 0 .../Functions}/data/weird-bug-extensions.php | 0 .../Functions/data/weird-bug-test.php} | 4 +- 5 files changed, 39 insertions(+), 37 deletions(-) delete mode 100644 tests/PHPStan/Analyser/WeirdBugTest.php create mode 100644 tests/PHPStan/Rules/Functions/WeirdBugTest.php rename tests/PHPStan/{Analyser => Rules/Functions/data}/weird-bug-config.neon (100%) rename tests/PHPStan/{Analyser => Rules/Functions}/data/weird-bug-extensions.php (100%) rename tests/PHPStan/{Analyser/data/weird-bug.php => Rules/Functions/data/weird-bug-test.php} (83%) diff --git a/tests/PHPStan/Analyser/WeirdBugTest.php b/tests/PHPStan/Analyser/WeirdBugTest.php deleted file mode 100644 index b60798b1a5..0000000000 --- a/tests/PHPStan/Analyser/WeirdBugTest.php +++ /dev/null @@ -1,35 +0,0 @@ -gatherAssertTypes(__DIR__ . '/data/weird-bug.php'); - } - - /** - * @dataProvider dataFileAsserts - * @param mixed ...$args - */ - public function testFileAsserts( - string $assertType, - string $file, - ...$args, - ): void - { - $this->assertFileAsserts($assertType, $file, ...$args); - } - - public static function getAdditionalConfigFiles(): array - { - return [ - __DIR__ . '/weird-bug-config.neon', - ]; - } - -} diff --git a/tests/PHPStan/Rules/Functions/WeirdBugTest.php b/tests/PHPStan/Rules/Functions/WeirdBugTest.php new file mode 100644 index 0000000000..d2d1f1445f --- /dev/null +++ b/tests/PHPStan/Rules/Functions/WeirdBugTest.php @@ -0,0 +1,37 @@ + + */ +class WeirdBugTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new ClosureReturnTypeRule(new FunctionReturnTypeCheck(new RuleLevelHelper($this->createReflectionProvider(), true, false, true, false, false, true, false))); + } + + public function testClosureReturnTypeRule(): void + { + if (PHP_VERSION_ID < 80000) { + $this->markTestSkipped('Test requires PHP 8.0'); + } + + $this->analyse([__DIR__ . '/data/weird-bug-test.php'], []); + } + + public static function getAdditionalConfigFiles(): array + { + return [ + __DIR__ . '/data/weird-bug-config.neon', + ]; + } + +} diff --git a/tests/PHPStan/Analyser/weird-bug-config.neon b/tests/PHPStan/Rules/Functions/data/weird-bug-config.neon similarity index 100% rename from tests/PHPStan/Analyser/weird-bug-config.neon rename to tests/PHPStan/Rules/Functions/data/weird-bug-config.neon diff --git a/tests/PHPStan/Analyser/data/weird-bug-extensions.php b/tests/PHPStan/Rules/Functions/data/weird-bug-extensions.php similarity index 100% rename from tests/PHPStan/Analyser/data/weird-bug-extensions.php rename to tests/PHPStan/Rules/Functions/data/weird-bug-extensions.php diff --git a/tests/PHPStan/Analyser/data/weird-bug.php b/tests/PHPStan/Rules/Functions/data/weird-bug-test.php similarity index 83% rename from tests/PHPStan/Analyser/data/weird-bug.php rename to tests/PHPStan/Rules/Functions/data/weird-bug-test.php index 5a2b72caa0..bf7538423e 100644 --- a/tests/PHPStan/Analyser/data/weird-bug.php +++ b/tests/PHPStan/Rules/Functions/data/weird-bug-test.php @@ -10,7 +10,7 @@ class Model { */ public static function getBuilder(): Builder { - return new Builder(new static()); // @phpstan-ignore-line + return new Builder(new static()); } } @@ -24,7 +24,7 @@ class Builder { /** * @param T $model */ - public function __construct(private Model $model) // @phpstan-ignore-line + public function __construct(private Model $model) { } From ea5f4c219bb5f854b2680e129c9d1f8660943422 Mon Sep 17 00:00:00 2001 From: Can Vural Date: Fri, 7 Feb 2025 00:49:00 +0100 Subject: [PATCH 3/3] fix: use passed to type to compare return types in ClosureReturnTypeRule --- src/Analyser/NodeScopeResolver.php | 2 ++ src/Node/ClosureReturnStatementsNode.php | 7 +++++++ src/Rules/Functions/ClosureReturnTypeRule.php | 10 ++++++++-- tests/PHPStan/Rules/Functions/WeirdBugTest.php | 1 + 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index c0617856f7..3affce792d 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -4311,6 +4311,7 @@ private function processClosureNode( $statementResult, $executionEnds, array_merge($statementResult->getImpurePoints(), $closureImpurePoints), + $passedToType, ), $closureScope); return new ProcessClosureResult($scope, $statementResult->getThrowPoints(), $statementResult->getImpurePoints(), $invalidateExpressions); @@ -4357,6 +4358,7 @@ private function processClosureNode( $statementResult, $executionEnds, array_merge($statementResult->getImpurePoints(), $closureImpurePoints), + $passedToType, ), $closureScope); return new ProcessClosureResult($scope->processClosureScope($closureResultScope, null, $byRefUses), $statementResult->getThrowPoints(), $statementResult->getImpurePoints(), $invalidateExpressions); diff --git a/src/Node/ClosureReturnStatementsNode.php b/src/Node/ClosureReturnStatementsNode.php index 7231c02e5f..f0742f8198 100644 --- a/src/Node/ClosureReturnStatementsNode.php +++ b/src/Node/ClosureReturnStatementsNode.php @@ -9,6 +9,7 @@ use PhpParser\NodeAbstract; use PHPStan\Analyser\ImpurePoint; use PHPStan\Analyser\StatementResult; +use PHPStan\Type\Type; use function count; /** @@ -33,6 +34,7 @@ public function __construct( private StatementResult $statementResult, private array $executionEnds, private array $impurePoints, + private ?Type $passedToType, ) { parent::__construct($closureExpr->getAttributes()); @@ -84,6 +86,11 @@ public function returnsByRef(): bool return $this->closureExpr->byRef; } + public function getPassedToType(): ?Type + { + return $this->passedToType; + } + public function getType(): string { return 'PHPStan_Node_ClosureReturnStatementsNode'; diff --git a/src/Rules/Functions/ClosureReturnTypeRule.php b/src/Rules/Functions/ClosureReturnTypeRule.php index 218edf5734..a341727dc7 100644 --- a/src/Rules/Functions/ClosureReturnTypeRule.php +++ b/src/Rules/Functions/ClosureReturnTypeRule.php @@ -7,6 +7,7 @@ use PHPStan\Node\ClosureReturnStatementsNode; use PHPStan\Rules\FunctionReturnTypeCheck; use PHPStan\Rules\Rule; +use PHPStan\Type\CallableType; use PHPStan\Type\Type; use PHPStan\Type\TypeCombinator; @@ -31,8 +32,13 @@ public function processNode(Node $node, Scope $scope): array return []; } - /** @var Type $returnType */ - $returnType = $scope->getAnonymousFunctionReturnType(); + if ($node->getPassedToType() instanceof CallableType) { + $returnType = $node->getPassedToType()->getReturnType(); + } else { + /** @var Type $returnType */ + $returnType = $scope->getAnonymousFunctionReturnType(); + } + $containsNull = TypeCombinator::containsNull($returnType); $hasNativeTypehint = $node->getClosureExpr()->returnType !== null; diff --git a/tests/PHPStan/Rules/Functions/WeirdBugTest.php b/tests/PHPStan/Rules/Functions/WeirdBugTest.php index d2d1f1445f..d31d27a4ac 100644 --- a/tests/PHPStan/Rules/Functions/WeirdBugTest.php +++ b/tests/PHPStan/Rules/Functions/WeirdBugTest.php @@ -6,6 +6,7 @@ use PHPStan\Rules\Rule; use PHPStan\Rules\RuleLevelHelper; use PHPStan\Testing\RuleTestCase; +use const PHP_VERSION_ID; /** * @extends RuleTestCase