Skip to content

Commit fb8d3ef

Browse files
ondrejmirtesondrejmirtes
committed
MethodSignatureRule - read PHPDoc types instead of combined types
1 parent 26d29ec commit fb8d3ef

File tree

8 files changed

+222
-32
lines changed

8 files changed

+222
-32
lines changed

‎src/Reflection/Php/PhpClassReflectionExtension.php

+62-8
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
use PHPStan\Type\ArrayType;
3434
use PHPStan\Type\Constant\ConstantArrayType;
3535
use PHPStan\Type\ErrorType;
36+
use PHPStan\Type\FileTypeMapper;
3637
use PHPStan\Type\Generic\TemplateTypeHelper;
3738
use PHPStan\Type\Generic\TemplateTypeMap;
3839
use PHPStan\Type\MixedType;
@@ -69,6 +70,8 @@ class PhpClassReflectionExtension
6970

7071
private \PHPStan\Reflection\ReflectionProvider $reflectionProvider;
7172

73+
private FileTypeMapper $fileTypeMapper;
74+
7275
/** @var string[] */
7376
private array $universalObjectCratesClasses;
7477

@@ -101,6 +104,7 @@ class PhpClassReflectionExtension
101104
* @param \PHPStan\Parser\Parser $parser
102105
* @param \PHPStan\PhpDoc\StubPhpDocProvider $stubPhpDocProvider
103106
* @param \PHPStan\Reflection\ReflectionProvider $reflectionProvider
107+
* @param FileTypeMapper $fileTypeMapper
104108
* @param bool $inferPrivatePropertyTypeFromConstructor
105109
* @param string[] $universalObjectCratesClasses
106110
*/
@@ -115,6 +119,7 @@ public function __construct(
115119
Parser $parser,
116120
StubPhpDocProvider $stubPhpDocProvider,
117121
ReflectionProvider $reflectionProvider,
122+
FileTypeMapper $fileTypeMapper,
118123
bool $inferPrivatePropertyTypeFromConstructor,
119124
array $universalObjectCratesClasses
120125
)
@@ -129,6 +134,7 @@ public function __construct(
129134
$this->parser = $parser;
130135
$this->stubPhpDocProvider = $stubPhpDocProvider;
131136
$this->reflectionProvider = $reflectionProvider;
137+
$this->fileTypeMapper = $fileTypeMapper;
132138
$this->inferPrivatePropertyTypeFromConstructor = $inferPrivatePropertyTypeFromConstructor;
133139
$this->universalObjectCratesClasses = $universalObjectCratesClasses;
134140
}
@@ -422,9 +428,11 @@ private function createMethod(
422428
}
423429
foreach ($variantNumbers as $variantNumber) {
424430
$methodSignature = $this->signatureMapProvider->getMethodSignature($declaringClassName, $methodReflection->getName(), $reflectionMethod, $variantNumber);
425-
$phpDocReturnType = null;
431+
$stubPhpDocReturnType = null;
426432
$stubPhpDocParameterTypes = [];
427433
$stubPhpDocParameterVariadicity = [];
434+
$phpDocParameterTypes = [];
435+
$phpDocReturnType = null;
428436
if (count($variantNumbers) === 1) {
429437
$stubPhpDocPair = $this->findMethodPhpDocIncludingAncestors($declaringClass, $methodReflection->getName(), array_map(static function (ParameterSignature $parameterSignature): string {
430438
return $parameterSignature->getName();
@@ -435,9 +443,8 @@ private function createMethod(
435443
$templateTypeMap = $stubDeclaringClass->getActiveTemplateTypeMap();
436444
$returnTag = $stubPhpDoc->getReturnTag();
437445
if ($returnTag !== null) {
438-
$stubPhpDocReturnType = $returnTag->getType();
439-
$phpDocReturnType = TemplateTypeHelper::resolveTemplateTypes(
440-
$stubPhpDocReturnType,
446+
$stubPhpDocReturnType = TemplateTypeHelper::resolveTemplateTypes(
447+
$returnTag->getType(),
441448
$templateTypeMap
442449
);
443450
}
@@ -449,9 +456,27 @@ private function createMethod(
449456
);
450457
$stubPhpDocParameterVariadicity[$name] = $paramTag->isVariadic();
451458
}
459+
} elseif ($reflectionMethod !== null && $reflectionMethod->getDocComment() !== false) {
460+
$filename = $reflectionMethod->getFileName();
461+
if ($filename !== false) {
462+
$phpDocBlock = $this->fileTypeMapper->getResolvedPhpDoc(
463+
$filename,
464+
$declaringClassName,
465+
null,
466+
$reflectionMethod->getName(),
467+
$reflectionMethod->getDocComment()
468+
);
469+
$returnTag = $phpDocBlock->getReturnTag();
470+
if ($returnTag !== null) {
471+
$phpDocReturnType = $returnTag->getType();
472+
}
473+
foreach ($phpDocBlock->getParamTags() as $name => $paramTag) {
474+
$phpDocParameterTypes[$name] = $paramTag->getType();
475+
}
476+
}
452477
}
453478
}
454-
$variants[] = $this->createNativeMethodVariant($methodSignature, $stubPhpDocParameterTypes, $stubPhpDocParameterVariadicity, $phpDocReturnType);
479+
$variants[] = $this->createNativeMethodVariant($methodSignature, $stubPhpDocParameterTypes, $stubPhpDocParameterVariadicity, $stubPhpDocReturnType, $phpDocParameterTypes, $phpDocReturnType);
455480
}
456481

457482
if ($this->signatureMapProvider->hasMethodMetadata($declaringClassName, $methodReflection->getName())) {
@@ -557,36 +582,65 @@ private function createMethod(
557582
* @param FunctionSignature $methodSignature
558583
* @param array<string, Type> $stubPhpDocParameterTypes
559584
* @param array<string, bool> $stubPhpDocParameterVariadicity
585+
* @param Type|null $stubPhpDocReturnType
586+
* @param array<string, Type> $phpDocParameterTypes
560587
* @param Type|null $phpDocReturnType
561588
* @return FunctionVariantWithPhpDocs
562589
*/
563590
private function createNativeMethodVariant(
564591
FunctionSignature $methodSignature,
565592
array $stubPhpDocParameterTypes,
566593
array $stubPhpDocParameterVariadicity,
594+
?Type $stubPhpDocReturnType,
595+
array $phpDocParameterTypes,
567596
?Type $phpDocReturnType
568597
): FunctionVariantWithPhpDocs
569598
{
570599
$parameters = [];
571600
foreach ($methodSignature->getParameters() as $parameterSignature) {
601+
$type = null;
602+
$phpDocType = null;
603+
604+
if (isset($stubPhpDocParameterTypes[$parameterSignature->getName()])) {
605+
$type = $stubPhpDocParameterTypes[$parameterSignature->getName()];
606+
$phpDocType = $stubPhpDocParameterTypes[$parameterSignature->getName()];
607+
} elseif (isset($phpDocParameterTypes[$parameterSignature->getName()])) {
608+
$type = TypehintHelper::decideType(
609+
$parameterSignature->getNativeType(),
610+
$phpDocParameterTypes[$parameterSignature->getName()]
611+
);
612+
$phpDocType = $phpDocParameterTypes[$parameterSignature->getName()];
613+
}
614+
572615
$parameters[] = new NativeParameterWithPhpDocsReflection(
573616
$parameterSignature->getName(),
574617
$parameterSignature->isOptional(),
575-
$stubPhpDocParameterTypes[$parameterSignature->getName()] ?? $parameterSignature->getType(),
576-
$stubPhpDocParameterTypes[$parameterSignature->getName()] ?? new MixedType(),
618+
$type ?? $parameterSignature->getType(),
619+
$phpDocType ?? new MixedType(),
577620
$parameterSignature->getNativeType(),
578621
$parameterSignature->passedByReference(),
579622
$stubPhpDocParameterVariadicity[$parameterSignature->getName()] ?? $parameterSignature->isVariadic(),
580623
null
581624
);
582625
}
583626

627+
$returnType = null;
628+
if ($stubPhpDocReturnType !== null) {
629+
$returnType = $stubPhpDocReturnType;
630+
$phpDocReturnType = $stubPhpDocReturnType;
631+
} elseif ($phpDocReturnType !== null) {
632+
$returnType = TypehintHelper::decideType(
633+
$methodSignature->getReturnType(),
634+
$phpDocReturnType
635+
);
636+
}
637+
584638
return new FunctionVariantWithPhpDocs(
585639
TemplateTypeMap::createEmpty(),
586640
null,
587641
$parameters,
588642
$methodSignature->isVariadic(),
589-
$phpDocReturnType ?? $methodSignature->getReturnType(),
643+
$returnType ?? $methodSignature->getReturnType(),
590644
$phpDocReturnType ?? new MixedType(),
591645
$methodSignature->getNativeReturnType()
592646
);

‎src/Rules/Methods/MethodSignatureRule.php

+41-23
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,13 @@
66
use PHPStan\Analyser\Scope;
77
use PHPStan\Node\InClassMethodNode;
88
use PHPStan\Reflection\ClassReflection;
9-
use PHPStan\Reflection\MethodReflection;
10-
use PHPStan\Reflection\ParameterReflection;
119
use PHPStan\Reflection\ParametersAcceptorSelector;
10+
use PHPStan\Reflection\ParametersAcceptorWithPhpDocs;
11+
use PHPStan\Reflection\Php\PhpMethodFromParserNodeReflection;
1212
use PHPStan\Rules\RuleErrorBuilder;
1313
use PHPStan\TrinaryLogic;
1414
use PHPStan\Type\MixedType;
15-
use PHPStan\Type\Type;
15+
use PHPStan\Type\TypehintHelper;
1616
use PHPStan\Type\VerbosityLevel;
1717
use PHPStan\Type\VoidType;
1818

@@ -43,7 +43,7 @@ public function getNodeType(): string
4343
public function processNode(Node $node, Scope $scope): array
4444
{
4545
$method = $scope->getFunction();
46-
if (!$method instanceof MethodReflection) {
46+
if (!$method instanceof PhpMethodFromParserNodeReflection) {
4747
return [];
4848
}
4949

@@ -60,20 +60,25 @@ public function processNode(Node $node, Scope $scope): array
6060
$parameters = ParametersAcceptorSelector::selectSingle($method->getVariants());
6161

6262
$errors = [];
63-
foreach ($this->collectParentMethods($methodName, $method->getDeclaringClass(), $scope) as $parentMethod) {
64-
$parentParameters = ParametersAcceptorSelector::selectFromTypes(array_map(static function (ParameterReflection $parameter): Type {
65-
return $parameter->getType();
66-
}, $parameters->getParameters()), $parentMethod->getVariants(), false);
63+
foreach ($this->collectParentMethods($methodName, $method->getDeclaringClass()) as $parentMethod) {
64+
$parentVariants = $parentMethod->getVariants();
65+
if (count($parentVariants) !== 1) {
66+
continue;
67+
}
68+
$parentParameters = $parentVariants[0];
69+
if (!$parentParameters instanceof ParametersAcceptorWithPhpDocs) {
70+
continue;
71+
}
6772

68-
$returnTypeCompatibility = $this->checkReturnTypeCompatibility($parameters->getReturnType(), $parentParameters->getReturnType());
73+
$returnTypeCompatibility = $this->checkReturnTypeCompatibility($parameters, $parentParameters);
6974
if ($returnTypeCompatibility->no() || (!$returnTypeCompatibility->yes() && $this->reportMaybes)) {
7075
$errors[] = RuleErrorBuilder::message(sprintf(
7176
'Return type (%s) of method %s::%s() should be %s with return type (%s) of method %s::%s()',
72-
$parameters->getReturnType()->describe(VerbosityLevel::value()),
77+
$parameters->getPhpDocReturnType()->describe(VerbosityLevel::value()),
7378
$method->getDeclaringClass()->getDisplayName(),
7479
$method->getName(),
7580
$returnTypeCompatibility->no() ? 'compatible' : 'covariant',
76-
$parentParameters->getReturnType()->describe(VerbosityLevel::value()),
81+
$parentParameters->getPhpDocReturnType()->describe(VerbosityLevel::value()),
7782
$parentMethod->getDeclaringClass()->getDisplayName(),
7883
$parentMethod->getName()
7984
))->build();
@@ -111,37 +116,44 @@ public function processNode(Node $node, Scope $scope): array
111116
/**
112117
* @param string $methodName
113118
* @param \PHPStan\Reflection\ClassReflection $class
114-
* @param \PHPStan\Analyser\Scope $scope
115119
* @return \PHPStan\Reflection\MethodReflection[]
116120
*/
117-
private function collectParentMethods(string $methodName, ClassReflection $class, Scope $scope): array
121+
private function collectParentMethods(string $methodName, ClassReflection $class): array
118122
{
119123
$parentMethods = [];
120124

121125
$parentClass = $class->getParentClass();
122-
if ($parentClass !== false && $parentClass->hasMethod($methodName)) {
123-
$parentMethod = $parentClass->getMethod($methodName, $scope);
126+
if ($parentClass !== false && $parentClass->hasNativeMethod($methodName)) {
127+
$parentMethod = $parentClass->getNativeMethod($methodName);
124128
if (!$parentMethod->isPrivate()) {
125129
$parentMethods[] = $parentMethod;
126130
}
127131
}
128132

129133
foreach ($class->getInterfaces() as $interface) {
130-
if (!$interface->hasMethod($methodName)) {
134+
if (!$interface->hasNativeMethod($methodName)) {
131135
continue;
132136
}
133137

134-
$parentMethods[] = $interface->getMethod($methodName, $scope);
138+
$parentMethods[] = $interface->getNativeMethod($methodName);
135139
}
136140

137141
return $parentMethods;
138142
}
139143

140144
private function checkReturnTypeCompatibility(
141-
Type $returnType,
142-
Type $parentReturnType
145+
ParametersAcceptorWithPhpDocs $currentVariant,
146+
ParametersAcceptorWithPhpDocs $parentVariant
143147
): TrinaryLogic
144148
{
149+
$returnType = TypehintHelper::decideType(
150+
$currentVariant->getNativeReturnType(),
151+
$currentVariant->getPhpDocReturnType()
152+
);
153+
$parentReturnType = TypehintHelper::decideType(
154+
$parentVariant->getNativeReturnType(),
155+
$parentVariant->getPhpDocReturnType()
156+
);
145157
// Allow adding `void` return type hints when the parent defines no return type
146158
if ($returnType instanceof VoidType && $parentReturnType instanceof MixedType) {
147159
return TrinaryLogic::createYes();
@@ -156,8 +168,8 @@ private function checkReturnTypeCompatibility(
156168
}
157169

158170
/**
159-
* @param \PHPStan\Reflection\ParameterReflection[] $parameters
160-
* @param \PHPStan\Reflection\ParameterReflection[] $parentParameters
171+
* @param \PHPStan\Reflection\ParameterReflectionWithPhpDocs[] $parameters
172+
* @param \PHPStan\Reflection\ParameterReflectionWithPhpDocs[] $parentParameters
161173
* @return array<int, TrinaryLogic>
162174
*/
163175
private function checkParameterTypeCompatibility(
@@ -172,8 +184,14 @@ private function checkParameterTypeCompatibility(
172184
$parameter = $parameters[$i];
173185
$parentParameter = $parentParameters[$i];
174186

175-
$parameterType = $parameter->getType();
176-
$parentParameterType = $parentParameter->getType();
187+
$parameterType = TypehintHelper::decideType(
188+
$parameter->getNativeType(),
189+
$parameter->getPhpDocType()
190+
);
191+
$parentParameterType = TypehintHelper::decideType(
192+
$parentParameter->getNativeType(),
193+
$parentParameter->getPhpDocType()
194+
);
177195

178196
$parameterResults[] = $parameterType->isSuperTypeOf($parentParameterType);
179197
}

‎src/Testing/TestCase.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,7 @@ public function create(
334334
$annotationsPropertiesClassReflectionExtension = new AnnotationsPropertiesClassReflectionExtension();
335335
$signatureMapProvider = self::getContainer()->getByType(SignatureMapProvider::class);
336336
$methodReflectionFactory->reflectionProvider = $actualReflectionProvider;
337-
$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, []);
337+
$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, []);
338338
$classReflectionExtensionRegistryProvider->addPropertiesClassReflectionExtension($phpExtension);
339339
$classReflectionExtensionRegistryProvider->addPropertiesClassReflectionExtension(new UniversalObjectCratesClassReflectionExtension([\stdClass::class]));
340340
$classReflectionExtensionRegistryProvider->addPropertiesClassReflectionExtension(new MixinPropertiesClassReflectionExtension([]));

‎tests/PHPStan/Analyser/NodeScopeResolverTest.php

+5
Original file line numberDiff line numberDiff line change
@@ -10245,6 +10245,11 @@ public function dataBug3993(): array
1024510245
return $this->gatherAssertTypes(__DIR__ . '/data/bug-3993.php');
1024610246
}
1024710247

10248+
public function dataBug3997(): array
10249+
{
10250+
return $this->gatherAssertTypes(__DIR__ . '/data/bug-3997.php');
10251+
}
10252+
1024810253
/**
1024910254
* @dataProvider dataBug2574
1025010255
* @dataProvider dataBug2577

‎tests/PHPStan/Analyser/data/bug-3997.php

+15
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<?php
2+
3+
namespace Bug3997Type;
4+
5+
use function PHPStan\Analyser\assertType;
6+
7+
function (\Countable $c): void {
8+
assertType('int<0, max>', $c->count());
9+
assertType('int<0, max>', count($c));
10+
};
11+
12+
function (\ArrayIterator $i): void {
13+
assertType('int<0, max>', $i->count());
14+
assertType('int<0, max>', count($i));
15+
};

‎tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php

+12
Original file line numberDiff line numberDiff line change
@@ -204,4 +204,16 @@ public function testBug2950(): void
204204
$this->analyse([__DIR__ . '/data/bug-2950.php'], []);
205205
}
206206

207+
public function testBug3997(): void
208+
{
209+
$this->reportMaybes = true;
210+
$this->reportStatic = true;
211+
$this->analyse([__DIR__ . '/data/bug-3997.php'], [
212+
[
213+
'Return type (string) of method Bug3997\Ipsum::count() should be compatible with return type (int) of method Countable::count()',
214+
59,
215+
],
216+
]);
217+
}
218+
207219
}

‎tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php

+22
Original file line numberDiff line numberDiff line change
@@ -370,4 +370,26 @@ public function testReturnTypeRulePhp70(): void
370370
]);
371371
}
372372

373+
public function testBug3997(): void
374+
{
375+
$this->analyse([__DIR__ . '/data/bug-3997.php'], [
376+
[
377+
'Method Bug3997\Foo::count() should return int but returns string.',
378+
12,
379+
],
380+
[
381+
'Method Bug3997\Bar::count() should return int but returns string.',
382+
22,
383+
],
384+
[
385+
'Method Bug3997\Baz::count() should return int but returns string.',
386+
35,
387+
],
388+
[
389+
'Method Bug3997\Lorem::count() should return int but returns string.',
390+
48,
391+
],
392+
]);
393+
}
394+
373395
}

0 commit comments

Comments
 (0)