From fb8d3ef823742ed11ee2b1fba337ed34c8034e8d Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Sat, 24 Oct 2020 16:59:55 +0200 Subject: [PATCH] MethodSignatureRule - read PHPDoc types instead of combined types --- .../Php/PhpClassReflectionExtension.php | 70 ++++++++++++++++--- src/Rules/Methods/MethodSignatureRule.php | 64 +++++++++++------ src/Testing/TestCase.php | 2 +- .../Analyser/NodeScopeResolverTest.php | 5 ++ tests/PHPStan/Analyser/data/bug-3997.php | 15 ++++ .../Rules/Methods/MethodSignatureRuleTest.php | 12 ++++ .../Rules/Methods/ReturnTypeRuleTest.php | 22 ++++++ tests/PHPStan/Rules/Methods/data/bug-3997.php | 64 +++++++++++++++++ 8 files changed, 222 insertions(+), 32 deletions(-) create mode 100644 tests/PHPStan/Analyser/data/bug-3997.php create mode 100644 tests/PHPStan/Rules/Methods/data/bug-3997.php diff --git a/src/Reflection/Php/PhpClassReflectionExtension.php b/src/Reflection/Php/PhpClassReflectionExtension.php index 33d09b08d3..ee590610c9 100644 --- a/src/Reflection/Php/PhpClassReflectionExtension.php +++ b/src/Reflection/Php/PhpClassReflectionExtension.php @@ -33,6 +33,7 @@ use PHPStan\Type\ArrayType; use PHPStan\Type\Constant\ConstantArrayType; use PHPStan\Type\ErrorType; +use PHPStan\Type\FileTypeMapper; use PHPStan\Type\Generic\TemplateTypeHelper; use PHPStan\Type\Generic\TemplateTypeMap; use PHPStan\Type\MixedType; @@ -69,6 +70,8 @@ class PhpClassReflectionExtension private \PHPStan\Reflection\ReflectionProvider $reflectionProvider; + private FileTypeMapper $fileTypeMapper; + /** @var string[] */ private array $universalObjectCratesClasses; @@ -101,6 +104,7 @@ class PhpClassReflectionExtension * @param \PHPStan\Parser\Parser $parser * @param \PHPStan\PhpDoc\StubPhpDocProvider $stubPhpDocProvider * @param \PHPStan\Reflection\ReflectionProvider $reflectionProvider + * @param FileTypeMapper $fileTypeMapper * @param bool $inferPrivatePropertyTypeFromConstructor * @param string[] $universalObjectCratesClasses */ @@ -115,6 +119,7 @@ public function __construct( Parser $parser, StubPhpDocProvider $stubPhpDocProvider, ReflectionProvider $reflectionProvider, + FileTypeMapper $fileTypeMapper, bool $inferPrivatePropertyTypeFromConstructor, array $universalObjectCratesClasses ) @@ -129,6 +134,7 @@ public function __construct( $this->parser = $parser; $this->stubPhpDocProvider = $stubPhpDocProvider; $this->reflectionProvider = $reflectionProvider; + $this->fileTypeMapper = $fileTypeMapper; $this->inferPrivatePropertyTypeFromConstructor = $inferPrivatePropertyTypeFromConstructor; $this->universalObjectCratesClasses = $universalObjectCratesClasses; } @@ -422,9 +428,11 @@ private function createMethod( } foreach ($variantNumbers as $variantNumber) { $methodSignature = $this->signatureMapProvider->getMethodSignature($declaringClassName, $methodReflection->getName(), $reflectionMethod, $variantNumber); - $phpDocReturnType = null; + $stubPhpDocReturnType = null; $stubPhpDocParameterTypes = []; $stubPhpDocParameterVariadicity = []; + $phpDocParameterTypes = []; + $phpDocReturnType = null; if (count($variantNumbers) === 1) { $stubPhpDocPair = $this->findMethodPhpDocIncludingAncestors($declaringClass, $methodReflection->getName(), array_map(static function (ParameterSignature $parameterSignature): string { return $parameterSignature->getName(); @@ -435,9 +443,8 @@ private function createMethod( $templateTypeMap = $stubDeclaringClass->getActiveTemplateTypeMap(); $returnTag = $stubPhpDoc->getReturnTag(); if ($returnTag !== null) { - $stubPhpDocReturnType = $returnTag->getType(); - $phpDocReturnType = TemplateTypeHelper::resolveTemplateTypes( - $stubPhpDocReturnType, + $stubPhpDocReturnType = TemplateTypeHelper::resolveTemplateTypes( + $returnTag->getType(), $templateTypeMap ); } @@ -449,9 +456,27 @@ private function createMethod( ); $stubPhpDocParameterVariadicity[$name] = $paramTag->isVariadic(); } + } elseif ($reflectionMethod !== null && $reflectionMethod->getDocComment() !== false) { + $filename = $reflectionMethod->getFileName(); + if ($filename !== false) { + $phpDocBlock = $this->fileTypeMapper->getResolvedPhpDoc( + $filename, + $declaringClassName, + null, + $reflectionMethod->getName(), + $reflectionMethod->getDocComment() + ); + $returnTag = $phpDocBlock->getReturnTag(); + if ($returnTag !== null) { + $phpDocReturnType = $returnTag->getType(); + } + foreach ($phpDocBlock->getParamTags() as $name => $paramTag) { + $phpDocParameterTypes[$name] = $paramTag->getType(); + } + } } } - $variants[] = $this->createNativeMethodVariant($methodSignature, $stubPhpDocParameterTypes, $stubPhpDocParameterVariadicity, $phpDocReturnType); + $variants[] = $this->createNativeMethodVariant($methodSignature, $stubPhpDocParameterTypes, $stubPhpDocParameterVariadicity, $stubPhpDocReturnType, $phpDocParameterTypes, $phpDocReturnType); } if ($this->signatureMapProvider->hasMethodMetadata($declaringClassName, $methodReflection->getName())) { @@ -557,6 +582,8 @@ private function createMethod( * @param FunctionSignature $methodSignature * @param array $stubPhpDocParameterTypes * @param array $stubPhpDocParameterVariadicity + * @param Type|null $stubPhpDocReturnType + * @param array $phpDocParameterTypes * @param Type|null $phpDocReturnType * @return FunctionVariantWithPhpDocs */ @@ -564,16 +591,32 @@ private function createNativeMethodVariant( FunctionSignature $methodSignature, array $stubPhpDocParameterTypes, array $stubPhpDocParameterVariadicity, + ?Type $stubPhpDocReturnType, + array $phpDocParameterTypes, ?Type $phpDocReturnType ): FunctionVariantWithPhpDocs { $parameters = []; foreach ($methodSignature->getParameters() as $parameterSignature) { + $type = null; + $phpDocType = null; + + if (isset($stubPhpDocParameterTypes[$parameterSignature->getName()])) { + $type = $stubPhpDocParameterTypes[$parameterSignature->getName()]; + $phpDocType = $stubPhpDocParameterTypes[$parameterSignature->getName()]; + } elseif (isset($phpDocParameterTypes[$parameterSignature->getName()])) { + $type = TypehintHelper::decideType( + $parameterSignature->getNativeType(), + $phpDocParameterTypes[$parameterSignature->getName()] + ); + $phpDocType = $phpDocParameterTypes[$parameterSignature->getName()]; + } + $parameters[] = new NativeParameterWithPhpDocsReflection( $parameterSignature->getName(), $parameterSignature->isOptional(), - $stubPhpDocParameterTypes[$parameterSignature->getName()] ?? $parameterSignature->getType(), - $stubPhpDocParameterTypes[$parameterSignature->getName()] ?? new MixedType(), + $type ?? $parameterSignature->getType(), + $phpDocType ?? new MixedType(), $parameterSignature->getNativeType(), $parameterSignature->passedByReference(), $stubPhpDocParameterVariadicity[$parameterSignature->getName()] ?? $parameterSignature->isVariadic(), @@ -581,12 +624,23 @@ private function createNativeMethodVariant( ); } + $returnType = null; + if ($stubPhpDocReturnType !== null) { + $returnType = $stubPhpDocReturnType; + $phpDocReturnType = $stubPhpDocReturnType; + } elseif ($phpDocReturnType !== null) { + $returnType = TypehintHelper::decideType( + $methodSignature->getReturnType(), + $phpDocReturnType + ); + } + return new FunctionVariantWithPhpDocs( TemplateTypeMap::createEmpty(), null, $parameters, $methodSignature->isVariadic(), - $phpDocReturnType ?? $methodSignature->getReturnType(), + $returnType ?? $methodSignature->getReturnType(), $phpDocReturnType ?? new MixedType(), $methodSignature->getNativeReturnType() ); diff --git a/src/Rules/Methods/MethodSignatureRule.php b/src/Rules/Methods/MethodSignatureRule.php index af9bc2b862..f01dc5c417 100644 --- a/src/Rules/Methods/MethodSignatureRule.php +++ b/src/Rules/Methods/MethodSignatureRule.php @@ -6,13 +6,13 @@ use PHPStan\Analyser\Scope; use PHPStan\Node\InClassMethodNode; use PHPStan\Reflection\ClassReflection; -use PHPStan\Reflection\MethodReflection; -use PHPStan\Reflection\ParameterReflection; use PHPStan\Reflection\ParametersAcceptorSelector; +use PHPStan\Reflection\ParametersAcceptorWithPhpDocs; +use PHPStan\Reflection\Php\PhpMethodFromParserNodeReflection; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\TrinaryLogic; use PHPStan\Type\MixedType; -use PHPStan\Type\Type; +use PHPStan\Type\TypehintHelper; use PHPStan\Type\VerbosityLevel; use PHPStan\Type\VoidType; @@ -43,7 +43,7 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { $method = $scope->getFunction(); - if (!$method instanceof MethodReflection) { + if (!$method instanceof PhpMethodFromParserNodeReflection) { return []; } @@ -60,20 +60,25 @@ public function processNode(Node $node, Scope $scope): array $parameters = ParametersAcceptorSelector::selectSingle($method->getVariants()); $errors = []; - foreach ($this->collectParentMethods($methodName, $method->getDeclaringClass(), $scope) as $parentMethod) { - $parentParameters = ParametersAcceptorSelector::selectFromTypes(array_map(static function (ParameterReflection $parameter): Type { - return $parameter->getType(); - }, $parameters->getParameters()), $parentMethod->getVariants(), false); + foreach ($this->collectParentMethods($methodName, $method->getDeclaringClass()) as $parentMethod) { + $parentVariants = $parentMethod->getVariants(); + if (count($parentVariants) !== 1) { + continue; + } + $parentParameters = $parentVariants[0]; + if (!$parentParameters instanceof ParametersAcceptorWithPhpDocs) { + continue; + } - $returnTypeCompatibility = $this->checkReturnTypeCompatibility($parameters->getReturnType(), $parentParameters->getReturnType()); + $returnTypeCompatibility = $this->checkReturnTypeCompatibility($parameters, $parentParameters); if ($returnTypeCompatibility->no() || (!$returnTypeCompatibility->yes() && $this->reportMaybes)) { $errors[] = RuleErrorBuilder::message(sprintf( 'Return type (%s) of method %s::%s() should be %s with return type (%s) of method %s::%s()', - $parameters->getReturnType()->describe(VerbosityLevel::value()), + $parameters->getPhpDocReturnType()->describe(VerbosityLevel::value()), $method->getDeclaringClass()->getDisplayName(), $method->getName(), $returnTypeCompatibility->no() ? 'compatible' : 'covariant', - $parentParameters->getReturnType()->describe(VerbosityLevel::value()), + $parentParameters->getPhpDocReturnType()->describe(VerbosityLevel::value()), $parentMethod->getDeclaringClass()->getDisplayName(), $parentMethod->getName() ))->build(); @@ -111,37 +116,44 @@ public function processNode(Node $node, Scope $scope): array /** * @param string $methodName * @param \PHPStan\Reflection\ClassReflection $class - * @param \PHPStan\Analyser\Scope $scope * @return \PHPStan\Reflection\MethodReflection[] */ - private function collectParentMethods(string $methodName, ClassReflection $class, Scope $scope): array + private function collectParentMethods(string $methodName, ClassReflection $class): array { $parentMethods = []; $parentClass = $class->getParentClass(); - if ($parentClass !== false && $parentClass->hasMethod($methodName)) { - $parentMethod = $parentClass->getMethod($methodName, $scope); + if ($parentClass !== false && $parentClass->hasNativeMethod($methodName)) { + $parentMethod = $parentClass->getNativeMethod($methodName); if (!$parentMethod->isPrivate()) { $parentMethods[] = $parentMethod; } } foreach ($class->getInterfaces() as $interface) { - if (!$interface->hasMethod($methodName)) { + if (!$interface->hasNativeMethod($methodName)) { continue; } - $parentMethods[] = $interface->getMethod($methodName, $scope); + $parentMethods[] = $interface->getNativeMethod($methodName); } return $parentMethods; } private function checkReturnTypeCompatibility( - Type $returnType, - Type $parentReturnType + ParametersAcceptorWithPhpDocs $currentVariant, + ParametersAcceptorWithPhpDocs $parentVariant ): TrinaryLogic { + $returnType = TypehintHelper::decideType( + $currentVariant->getNativeReturnType(), + $currentVariant->getPhpDocReturnType() + ); + $parentReturnType = TypehintHelper::decideType( + $parentVariant->getNativeReturnType(), + $parentVariant->getPhpDocReturnType() + ); // Allow adding `void` return type hints when the parent defines no return type if ($returnType instanceof VoidType && $parentReturnType instanceof MixedType) { return TrinaryLogic::createYes(); @@ -156,8 +168,8 @@ private function checkReturnTypeCompatibility( } /** - * @param \PHPStan\Reflection\ParameterReflection[] $parameters - * @param \PHPStan\Reflection\ParameterReflection[] $parentParameters + * @param \PHPStan\Reflection\ParameterReflectionWithPhpDocs[] $parameters + * @param \PHPStan\Reflection\ParameterReflectionWithPhpDocs[] $parentParameters * @return array */ private function checkParameterTypeCompatibility( @@ -172,8 +184,14 @@ private function checkParameterTypeCompatibility( $parameter = $parameters[$i]; $parentParameter = $parentParameters[$i]; - $parameterType = $parameter->getType(); - $parentParameterType = $parentParameter->getType(); + $parameterType = TypehintHelper::decideType( + $parameter->getNativeType(), + $parameter->getPhpDocType() + ); + $parentParameterType = TypehintHelper::decideType( + $parentParameter->getNativeType(), + $parentParameter->getPhpDocType() + ); $parameterResults[] = $parameterType->isSuperTypeOf($parentParameterType); } diff --git a/src/Testing/TestCase.php b/src/Testing/TestCase.php index 431bb4cc48..0a13d666bf 100644 --- a/src/Testing/TestCase.php +++ b/src/Testing/TestCase.php @@ -334,7 +334,7 @@ public function create( $annotationsPropertiesClassReflectionExtension = new AnnotationsPropertiesClassReflectionExtension(); $signatureMapProvider = self::getContainer()->getByType(SignatureMapProvider::class); $methodReflectionFactory->reflectionProvider = $actualReflectionProvider; - $phpExtension = new PhpClassReflectionExtension(self::getContainer()->getByType(ScopeFactory::class), self::getContainer()->getByType(NodeScopeResolver::class), $methodReflectionFactory, $phpDocInheritanceResolver, $annotationsMethodsClassReflectionExtension, $annotationsPropertiesClassReflectionExtension, $signatureMapProvider, $parser, self::getContainer()->getByType(StubPhpDocProvider::class), $actualReflectionProvider, true, []); + $phpExtension = new PhpClassReflectionExtension(self::getContainer()->getByType(ScopeFactory::class), self::getContainer()->getByType(NodeScopeResolver::class), $methodReflectionFactory, $phpDocInheritanceResolver, $annotationsMethodsClassReflectionExtension, $annotationsPropertiesClassReflectionExtension, $signatureMapProvider, $parser, self::getContainer()->getByType(StubPhpDocProvider::class), $actualReflectionProvider, $fileTypeMapper, true, []); $classReflectionExtensionRegistryProvider->addPropertiesClassReflectionExtension($phpExtension); $classReflectionExtensionRegistryProvider->addPropertiesClassReflectionExtension(new UniversalObjectCratesClassReflectionExtension([\stdClass::class])); $classReflectionExtensionRegistryProvider->addPropertiesClassReflectionExtension(new MixinPropertiesClassReflectionExtension([])); diff --git a/tests/PHPStan/Analyser/NodeScopeResolverTest.php b/tests/PHPStan/Analyser/NodeScopeResolverTest.php index eeead06e09..81438fba31 100644 --- a/tests/PHPStan/Analyser/NodeScopeResolverTest.php +++ b/tests/PHPStan/Analyser/NodeScopeResolverTest.php @@ -10245,6 +10245,11 @@ public function dataBug3993(): array return $this->gatherAssertTypes(__DIR__ . '/data/bug-3993.php'); } + public function dataBug3997(): array + { + return $this->gatherAssertTypes(__DIR__ . '/data/bug-3997.php'); + } + /** * @dataProvider dataBug2574 * @dataProvider dataBug2577 diff --git a/tests/PHPStan/Analyser/data/bug-3997.php b/tests/PHPStan/Analyser/data/bug-3997.php new file mode 100644 index 0000000000..3f208a1ca0 --- /dev/null +++ b/tests/PHPStan/Analyser/data/bug-3997.php @@ -0,0 +1,15 @@ +', $c->count()); + assertType('int<0, max>', count($c)); +}; + +function (\ArrayIterator $i): void { + assertType('int<0, max>', $i->count()); + assertType('int<0, max>', count($i)); +}; diff --git a/tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php b/tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php index fbfa4bbb44..48c8e60d00 100644 --- a/tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php +++ b/tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php @@ -204,4 +204,16 @@ public function testBug2950(): void $this->analyse([__DIR__ . '/data/bug-2950.php'], []); } + public function testBug3997(): void + { + $this->reportMaybes = true; + $this->reportStatic = true; + $this->analyse([__DIR__ . '/data/bug-3997.php'], [ + [ + 'Return type (string) of method Bug3997\Ipsum::count() should be compatible with return type (int) of method Countable::count()', + 59, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php b/tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php index c38b671db4..89b3366026 100644 --- a/tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php +++ b/tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php @@ -370,4 +370,26 @@ public function testReturnTypeRulePhp70(): void ]); } + public function testBug3997(): void + { + $this->analyse([__DIR__ . '/data/bug-3997.php'], [ + [ + 'Method Bug3997\Foo::count() should return int but returns string.', + 12, + ], + [ + 'Method Bug3997\Bar::count() should return int but returns string.', + 22, + ], + [ + 'Method Bug3997\Baz::count() should return int but returns string.', + 35, + ], + [ + 'Method Bug3997\Lorem::count() should return int but returns string.', + 48, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Methods/data/bug-3997.php b/tests/PHPStan/Rules/Methods/data/bug-3997.php new file mode 100644 index 0000000000..83362ddc2c --- /dev/null +++ b/tests/PHPStan/Rules/Methods/data/bug-3997.php @@ -0,0 +1,64 @@ +