From ca0a7e9955397eef453b38c94ac67ba6faf7356b Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Fri, 4 Oct 2024 09:59:49 +0200 Subject: [PATCH] Bleeding edge - added absent type checks to AssertRuleHelper --- conf/config.neon | 4 + src/Rules/PhpDoc/AssertRuleHelper.php | 160 ++++++++++++++++-- src/Rules/PhpDoc/FunctionAssertRule.php | 2 +- src/Rules/PhpDoc/MethodAssertRule.php | 2 +- .../Rules/PhpDoc/FunctionAssertRuleTest.php | 18 +- .../Rules/PhpDoc/MethodAssertRuleTest.php | 79 ++++++++- .../Rules/PhpDoc/data/method-assert.php | 133 +++++++++++++++ 7 files changed, 376 insertions(+), 22 deletions(-) diff --git a/conf/config.neon b/conf/config.neon index 67351dc4da..aa096cc686 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -1083,6 +1083,10 @@ services: - class: PHPStan\Rules\PhpDoc\AssertRuleHelper + arguments: + checkMissingTypehints: %checkMissingTypehints% + checkClassCaseSensitivity: %checkClassCaseSensitivity% + absentTypeChecks: %featureToggles.absentTypeChecks% - class: PHPStan\Rules\PhpDoc\UnresolvableTypeHelper diff --git a/src/Rules/PhpDoc/AssertRuleHelper.php b/src/Rules/PhpDoc/AssertRuleHelper.php index 40551504fa..073d131922 100644 --- a/src/Rules/PhpDoc/AssertRuleHelper.php +++ b/src/Rules/PhpDoc/AssertRuleHelper.php @@ -2,39 +2,63 @@ namespace PHPStan\Rules\PhpDoc; +use PhpParser\Node\Stmt\ClassMethod; +use PhpParser\Node\Stmt\Function_; use PHPStan\Node\Expr\TypeExpr; +use PHPStan\PhpDoc\Tag\AssertTag; use PHPStan\Reflection\ExtendedMethodReflection; use PHPStan\Reflection\FunctionReflection; use PHPStan\Reflection\InitializerExprContext; use PHPStan\Reflection\InitializerExprTypeResolver; use PHPStan\Reflection\ParametersAcceptor; +use PHPStan\Reflection\ReflectionProvider; +use PHPStan\Rules\ClassNameCheck; +use PHPStan\Rules\ClassNameNodePair; +use PHPStan\Rules\Generics\GenericObjectTypeCheck; use PHPStan\Rules\IdentifierRuleError; +use PHPStan\Rules\MissingTypehintCheck; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\ErrorType; use PHPStan\Type\ObjectType; use PHPStan\Type\VerbosityLevel; use function array_key_exists; +use function array_merge; +use function implode; use function sprintf; use function substr; final class AssertRuleHelper { - public function __construct(private InitializerExprTypeResolver $initializerExprTypeResolver) + public function __construct( + private InitializerExprTypeResolver $initializerExprTypeResolver, + private ReflectionProvider $reflectionProvider, + private UnresolvableTypeHelper $unresolvableTypeHelper, + private ClassNameCheck $classCheck, + private MissingTypehintCheck $missingTypehintCheck, + private GenericObjectTypeCheck $genericObjectTypeCheck, + private bool $absentTypeChecks, + private bool $checkClassCaseSensitivity, + private bool $checkMissingTypehints, + ) { } /** * @return list */ - public function check(ExtendedMethodReflection|FunctionReflection $reflection, ParametersAcceptor $acceptor): array + public function check( + Function_|ClassMethod $node, + ExtendedMethodReflection|FunctionReflection $reflection, + ParametersAcceptor $acceptor, + ): array { $parametersByName = []; foreach ($acceptor->getParameters() as $parameter) { $parametersByName[$parameter->getName()] = $parameter->getType(); } - if ($reflection instanceof ExtendedMethodReflection) { + if ($reflection instanceof ExtendedMethodReflection && !$reflection->isStatic()) { $class = $reflection->getDeclaringClass(); $parametersByName['this'] = new ObjectType($class->getName(), null, $class); } @@ -57,38 +81,138 @@ public function check(ExtendedMethodReflection|FunctionReflection $reflection, P $assertedExpr = $assert->getParameter()->getExpr(new TypeExpr($parametersByName[$parameterName])); $assertedExprType = $this->initializerExprTypeResolver->getType($assertedExpr, $context); + $assertedExprString = $assert->getParameter()->describe(); if ($assertedExprType instanceof ErrorType) { + if ($this->absentTypeChecks) { + $errors[] = RuleErrorBuilder::message(sprintf('Assert references unknown %s.', $assertedExprString)) + ->identifier('assert.unknownExpr') + ->build(); + } continue; } $assertedType = $assert->getType(); + $tagName = [ + AssertTag::NULL => '@phpstan-assert', + AssertTag::IF_TRUE => '@phpstan-assert-if-true', + AssertTag::IF_FALSE => '@phpstan-assert-if-false', + ][$assert->getIf()]; + + if ($this->absentTypeChecks) { + if ($this->unresolvableTypeHelper->containsUnresolvableType($assertedType)) { + $errors[] = RuleErrorBuilder::message(sprintf( + 'PHPDoc tag %s for %s contains unresolvable type.', + $tagName, + $assertedExprString, + ))->identifier('assert.unresolvableType')->build(); + continue; + } + } + $isSuperType = $assertedType->isSuperTypeOf($assertedExprType); - if ($isSuperType->maybe()) { + if (!$isSuperType->maybe()) { + if ($assert->isNegated() ? $isSuperType->yes() : $isSuperType->no()) { + $errors[] = RuleErrorBuilder::message(sprintf( + 'Asserted %stype %s for %s with type %s can never happen.', + $assert->isNegated() ? 'negated ' : '', + $assertedType->describe(VerbosityLevel::precise()), + $assertedExprString, + $assertedExprType->describe(VerbosityLevel::precise()), + ))->identifier('assert.impossibleType')->build(); + } elseif ($assert->isNegated() ? $isSuperType->no() : $isSuperType->yes()) { + $errors[] = RuleErrorBuilder::message(sprintf( + 'Asserted %stype %s for %s with type %s does not narrow down the type.', + $assert->isNegated() ? 'negated ' : '', + $assertedType->describe(VerbosityLevel::precise()), + $assertedExprString, + $assertedExprType->describe(VerbosityLevel::precise()), + ))->identifier('assert.alreadyNarrowedType')->build(); + } + } + + if (!$this->absentTypeChecks) { continue; } - $assertedExprString = $assert->getParameter()->describe(); + foreach ($assertedType->getReferencedClasses() as $class) { + if (!$this->reflectionProvider->hasClass($class)) { + $errors[] = RuleErrorBuilder::message(sprintf( + 'PHPDoc tag %s for %s contains unknown class %s.', + $tagName, + $assertedExprString, + $class, + ))->identifier('class.notFound')->build(); + continue; + } + + $classReflection = $this->reflectionProvider->getClass($class); + if ($classReflection->isTrait()) { + $errors[] = RuleErrorBuilder::message(sprintf( + 'PHPDoc tag %s for %s contains invalid type %s.', + $tagName, + $assertedExprString, + $class, + ))->identifier('assert.trait')->build(); + continue; + } + + $errors = array_merge( + $errors, + $this->classCheck->checkClassNames([ + new ClassNameNodePair($class, $node), + ], $this->checkClassCaseSensitivity), + ); + } + + $errors = array_merge($errors, $this->genericObjectTypeCheck->check( + $assertedType, + sprintf('PHPDoc tag %s for %s contains generic type %%s but %%s %%s is not generic.', $tagName, $assertedExprString), + sprintf('Generic type %%s in PHPDoc tag %s for %s does not specify all template types of %%s %%s: %%s', $tagName, $assertedExprString), + sprintf('Generic type %%s in PHPDoc tag %s for %s specifies %%d template types, but %%s %%s supports only %%d: %%s', $tagName, $assertedExprString), + sprintf('Type %%s in generic type %%s in PHPDoc tag %s for %s is not subtype of template type %%s of %%s %%s.', $tagName, $assertedExprString), + sprintf('Call-site variance of %%s in generic type %%s in PHPDoc tag %s for %s is in conflict with %%s template type %%s of %%s %%s.', $tagName, $assertedExprString), + sprintf('Call-site variance of %%s in generic type %%s in PHPDoc tag %s for %s is redundant, template type %%s of %%s %%s has the same variance.', $tagName, $assertedExprString), + )); + + if (!$this->checkMissingTypehints) { + continue; + } - if ($assert->isNegated() ? $isSuperType->yes() : $isSuperType->no()) { + foreach ($this->missingTypehintCheck->getIterableTypesWithMissingValueTypehint($assertedType) as $iterableType) { + $iterableTypeDescription = $iterableType->describe(VerbosityLevel::typeOnly()); $errors[] = RuleErrorBuilder::message(sprintf( - 'Asserted %stype %s for %s with type %s can never happen.', - $assert->isNegated() ? 'negated ' : '', - $assertedType->describe(VerbosityLevel::precise()), + 'PHPDoc tag %s for %s has no value type specified in iterable type %s.', + $tagName, $assertedExprString, - $assertedExprType->describe(VerbosityLevel::precise()), - ))->identifier('assert.impossibleType')->build(); - } elseif ($assert->isNegated() ? $isSuperType->no() : $isSuperType->yes()) { + $iterableTypeDescription, + )) + ->tip(MissingTypehintCheck::MISSING_ITERABLE_VALUE_TYPE_TIP) + ->identifier('missingType.iterableValue') + ->build(); + } + + foreach ($this->missingTypehintCheck->getNonGenericObjectTypesWithGenericClass($assertedType) as [$innerName, $genericTypeNames]) { $errors[] = RuleErrorBuilder::message(sprintf( - 'Asserted %stype %s for %s with type %s does not narrow down the type.', - $assert->isNegated() ? 'negated ' : '', - $assertedType->describe(VerbosityLevel::precise()), + 'PHPDoc tag %s for %s contains generic %s but does not specify its types: %s', + $tagName, $assertedExprString, - $assertedExprType->describe(VerbosityLevel::precise()), - ))->identifier('assert.alreadyNarrowedType')->build(); + $innerName, + implode(', ', $genericTypeNames), + )) + ->identifier('missingType.generics') + ->build(); } - } + foreach ($this->missingTypehintCheck->getCallablesWithMissingSignature($assertedType) as $callableType) { + $errors[] = RuleErrorBuilder::message(sprintf( + 'PHPDoc tag %s for %s has no signature specified for %s.', + $tagName, + $assertedExprString, + $callableType->describe(VerbosityLevel::typeOnly()), + ))->identifier('missingType.callable')->build(); + } + } return $errors; } diff --git a/src/Rules/PhpDoc/FunctionAssertRule.php b/src/Rules/PhpDoc/FunctionAssertRule.php index 481d99f165..893192e250 100644 --- a/src/Rules/PhpDoc/FunctionAssertRule.php +++ b/src/Rules/PhpDoc/FunctionAssertRule.php @@ -31,7 +31,7 @@ public function processNode(Node $node, Scope $scope): array return []; } - return $this->helper->check($function, $variants[0]); + return $this->helper->check($node->getOriginalNode(), $function, $variants[0]); } } diff --git a/src/Rules/PhpDoc/MethodAssertRule.php b/src/Rules/PhpDoc/MethodAssertRule.php index d8be08408f..6694ad3e42 100644 --- a/src/Rules/PhpDoc/MethodAssertRule.php +++ b/src/Rules/PhpDoc/MethodAssertRule.php @@ -31,7 +31,7 @@ public function processNode(Node $node, Scope $scope): array return []; } - return $this->helper->check($method, $variants[0]); + return $this->helper->check($node->getOriginalNode(), $method, $variants[0]); } } diff --git a/tests/PHPStan/Rules/PhpDoc/FunctionAssertRuleTest.php b/tests/PHPStan/Rules/PhpDoc/FunctionAssertRuleTest.php index 3bfd8ae7a6..7c63ae9d67 100644 --- a/tests/PHPStan/Rules/PhpDoc/FunctionAssertRuleTest.php +++ b/tests/PHPStan/Rules/PhpDoc/FunctionAssertRuleTest.php @@ -3,6 +3,11 @@ namespace PHPStan\Rules\PhpDoc; use PHPStan\Reflection\InitializerExprTypeResolver; +use PHPStan\Rules\ClassCaseSensitivityCheck; +use PHPStan\Rules\ClassForbiddenNameCheck; +use PHPStan\Rules\ClassNameCheck; +use PHPStan\Rules\Generics\GenericObjectTypeCheck; +use PHPStan\Rules\MissingTypehintCheck; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; @@ -15,7 +20,18 @@ class FunctionAssertRuleTest extends RuleTestCase protected function getRule(): Rule { $initializerExprTypeResolver = self::getContainer()->getByType(InitializerExprTypeResolver::class); - return new FunctionAssertRule(new AssertRuleHelper($initializerExprTypeResolver)); + $reflectionProvider = $this->createReflectionProvider(); + return new FunctionAssertRule(new AssertRuleHelper( + $initializerExprTypeResolver, + $reflectionProvider, + new UnresolvableTypeHelper(), + new ClassNameCheck(new ClassCaseSensitivityCheck($reflectionProvider, true), new ClassForbiddenNameCheck(self::getContainer())), + new MissingTypehintCheck(true, true, true, true, []), + new GenericObjectTypeCheck(), + true, + true, + true, + )); } public function testRule(): void diff --git a/tests/PHPStan/Rules/PhpDoc/MethodAssertRuleTest.php b/tests/PHPStan/Rules/PhpDoc/MethodAssertRuleTest.php index 932a09c299..84474ea49a 100644 --- a/tests/PHPStan/Rules/PhpDoc/MethodAssertRuleTest.php +++ b/tests/PHPStan/Rules/PhpDoc/MethodAssertRuleTest.php @@ -3,6 +3,11 @@ namespace PHPStan\Rules\PhpDoc; use PHPStan\Reflection\InitializerExprTypeResolver; +use PHPStan\Rules\ClassCaseSensitivityCheck; +use PHPStan\Rules\ClassForbiddenNameCheck; +use PHPStan\Rules\ClassNameCheck; +use PHPStan\Rules\Generics\GenericObjectTypeCheck; +use PHPStan\Rules\MissingTypehintCheck; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; @@ -15,7 +20,18 @@ class MethodAssertRuleTest extends RuleTestCase protected function getRule(): Rule { $initializerExprTypeResolver = self::getContainer()->getByType(InitializerExprTypeResolver::class); - return new MethodAssertRule(new AssertRuleHelper($initializerExprTypeResolver)); + $reflectionProvider = $this->createReflectionProvider(); + return new MethodAssertRule(new AssertRuleHelper( + $initializerExprTypeResolver, + $reflectionProvider, + new UnresolvableTypeHelper(), + new ClassNameCheck(new ClassCaseSensitivityCheck($reflectionProvider, true), new ClassForbiddenNameCheck(self::getContainer())), + new MissingTypehintCheck(true, true, true, true, []), + new GenericObjectTypeCheck(), + true, + true, + true, + )); } public function testRule(): void @@ -54,6 +70,67 @@ public function testRule(): void 'Asserted negated type string for $i with type int does not narrow down the type.', 72, ], + [ + 'PHPDoc tag @phpstan-assert for $this->fooProp contains unresolvable type.', + 94, + ], + [ + 'PHPDoc tag @phpstan-assert-if-true for $a contains unresolvable type.', + 94, + ], + [ + 'PHPDoc tag @phpstan-assert for $a contains unknown class MethodAssert\Nonexistent.', + 105, + ], + [ + 'PHPDoc tag @phpstan-assert for $b contains invalid type MethodAssert\FooTrait.', + 105, + ], + [ + 'Class MethodAssert\Foo referenced with incorrect case: MethodAssert\fOO.', + 105, + ], + [ + 'Assert references unknown $this->barProp.', + 105, + ], + [ + 'Assert references unknown parameter $this.', + 113, + ], + [ + 'PHPDoc tag @phpstan-assert for $m contains generic type Exception but class Exception is not generic.', + 131, + ], + [ + 'Generic type MethodAssert\FooBar in PHPDoc tag @phpstan-assert for $m does not specify all template types of class MethodAssert\FooBar: T, TT', + 138, + ], + [ + 'Type mixed in generic type MethodAssert\FooBar in PHPDoc tag @phpstan-assert for $m is not subtype of template type T of int of class MethodAssert\FooBar.', + 138, + ], + [ + 'Generic type MethodAssert\FooBar in PHPDoc tag @phpstan-assert for $m does not specify all template types of class MethodAssert\FooBar: T, TT', + 145, + ], + [ + 'Generic type MethodAssert\FooBar in PHPDoc tag @phpstan-assert for $m specifies 3 template types, but class MethodAssert\FooBar supports only 2: T, TT', + 152, + ], + [ + 'PHPDoc tag @phpstan-assert for $m has no value type specified in iterable type array.', + 194, + 'See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type', + ], + [ + 'PHPDoc tag @phpstan-assert for $m contains generic class MethodAssert\FooBar but does not specify its types: T, TT', + 202, + ], + [ + 'PHPDoc tag @phpstan-assert for $m has no signature specified for callable.', + 210, + ], ]); } diff --git a/tests/PHPStan/Rules/PhpDoc/data/method-assert.php b/tests/PHPStan/Rules/PhpDoc/data/method-assert.php index fa603eb85f..ad07ee066d 100644 --- a/tests/PHPStan/Rules/PhpDoc/data/method-assert.php +++ b/tests/PHPStan/Rules/PhpDoc/data/method-assert.php @@ -80,3 +80,136 @@ public function negate2(int $i): void { } } + +class AbsentTypeChecks +{ + + /** @var string|null */ + public $fooProp; + + /** + * @phpstan-assert-if-true string&int $a + * @phpstan-assert string&int $this->fooProp + */ + public function doFoo($a): bool + { + + } + + /** + * @phpstan-assert Nonexistent $a + * @phpstan-assert FooTrait $b + * @phpstan-assert fOO $c + * @phpstan-assert Foo $this->barProp + */ + public function doBar($a, $b, $c): bool + { + + } + + /** + * @phpstan-assert !null $this->fooProp + */ + public static function doBaz(): void + { + + } + +} + +trait FooTrait +{ + +} + +class InvalidGenerics +{ + + /** + * @phpstan-assert \Exception $m + */ + function invalidPhpstanAssertGeneric($m) { + + } + + /** + * @phpstan-assert FooBar $m + */ + function invalidPhpstanAssertWrongGenericParams($m) { + + } + + /** + * @phpstan-assert FooBar $m + */ + function invalidPhpstanAssertNotAllGenericParams($m) { + + } + + /** + * @phpstan-assert FooBar $m + */ + function invalidPhpstanAssertMoreGenericParams($m) { + + } + +} + + +/** + * @template T of int + * @template TT of string + */ +class FooBar { + /** + * @param-out T $s + */ + function genericClassFoo(mixed &$s): void + { + } + + /** + * @template S of self + * @param-out S $s + */ + function genericSelf(mixed &$s): void + { + } + + /** + * @template S of static + * @param-out S $s + */ + function genericStatic(mixed &$s): void + { + } +} + +class MissingTypes +{ + + /** + * @phpstan-assert array $m + */ + public function doFoo($m): void + { + + } + + /** + * @phpstan-assert FooBar $m + */ + public function doBar($m): void + { + + } + + /** + * @phpstan-assert callable $m + */ + public function doBaz($m): void + { + + } + +}