diff --git a/composer.json b/composer.json index ceeff9e26e..581ac9845f 100644 --- a/composer.json +++ b/composer.json @@ -21,9 +21,9 @@ "nette/neon": "^3.0", "nette/schema": "^1.0", "nette/utils": "^3.1.3", - "nikic/php-parser": "4.12.0", + "nikic/php-parser": "dev-master as 4.12.0", "ondram/ci-detector": "^3.4.0", - "ondrejmirtes/better-reflection": "4.3.64", + "ondrejmirtes/better-reflection": "4.3.65", "phpstan/php-8-stubs": "^0.1.22", "phpstan/phpdoc-parser": "^0.5.5", "react/child-process": "^0.6.1", diff --git a/composer.lock b/composer.lock index 4913070e89..3ff4e6fe3d 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "8bb4c4f5674ba1245e1db1a1cad7f8e8", + "content-hash": "e7c69231bdd43f8e377c2fc08d85c590", "packages": [ { "name": "clue/block-react", @@ -1946,16 +1946,16 @@ }, { "name": "nikic/php-parser", - "version": "v4.12.0", + "version": "dev-master", "source": { "type": "git", "url": "https://github.com/nikic/PHP-Parser.git", - "reference": "6608f01670c3cc5079e18c1dab1104e002579143" + "reference": "9aebf377fcdf205b2156cb78c0bd6e7b2003f106" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/nikic/PHP-Parser/zipball/6608f01670c3cc5079e18c1dab1104e002579143", - "reference": "6608f01670c3cc5079e18c1dab1104e002579143", + "url": "https://api.github.com/repos/nikic/PHP-Parser/zipball/9aebf377fcdf205b2156cb78c0bd6e7b2003f106", + "reference": "9aebf377fcdf205b2156cb78c0bd6e7b2003f106", "shasum": "" }, "require": { @@ -1966,6 +1966,7 @@ "ircmaxell/php-yacc": "^0.0.7", "phpunit/phpunit": "^6.5 || ^7.0 || ^8.0 || ^9.0" }, + "default-branch": true, "bin": [ "bin/php-parse" ], @@ -1996,9 +1997,9 @@ ], "support": { "issues": "https://github.com/nikic/PHP-Parser/issues", - "source": "https://github.com/nikic/PHP-Parser/tree/v4.12.0" + "source": "https://github.com/nikic/PHP-Parser/tree/master" }, - "time": "2021-07-21T10:44:31+00:00" + "time": "2021-08-08T17:12:44+00:00" }, { "name": "ondram/ci-detector", @@ -2074,16 +2075,16 @@ }, { "name": "ondrejmirtes/better-reflection", - "version": "4.3.64", + "version": "4.3.65", "source": { "type": "git", "url": "https://github.com/ondrejmirtes/BetterReflection.git", - "reference": "737857c9777a5f6c793935ee1bb2f72017d3a976" + "reference": "5e74a9b7ccd861481c618306d6e995b738419f35" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/ondrejmirtes/BetterReflection/zipball/737857c9777a5f6c793935ee1bb2f72017d3a976", - "reference": "737857c9777a5f6c793935ee1bb2f72017d3a976", + "url": "https://api.github.com/repos/ondrejmirtes/BetterReflection/zipball/5e74a9b7ccd861481c618306d6e995b738419f35", + "reference": "5e74a9b7ccd861481c618306d6e995b738419f35", "shasum": "" }, "require": { @@ -2138,9 +2139,9 @@ ], "description": "Better Reflection - an improved code reflection API", "support": { - "source": "https://github.com/ondrejmirtes/BetterReflection/tree/4.3.64" + "source": "https://github.com/ondrejmirtes/BetterReflection/tree/4.3.65" }, - "time": "2021-08-17T15:26:08+00:00" + "time": "2021-08-18T09:23:47+00:00" }, { "name": "phpstan/php-8-stubs", @@ -6385,10 +6386,18 @@ "time": "2021-03-09T10:59:23+00:00" } ], - "aliases": [], + "aliases": [ + { + "package": "nikic/php-parser", + "version": "9999999-dev", + "alias": "4.12.0", + "alias_normalized": "4.12.0.0" + } + ], "minimum-stability": "dev", "stability-flags": { - "jetbrains/phpstorm-stubs": 20 + "jetbrains/phpstorm-stubs": 20, + "nikic/php-parser": 20 }, "prefer-stable": true, "prefer-lowest": false, diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 6120cbd670..bfdd57db17 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -26,5 +26,6 @@ parameters: finalByPhpDocTag: true classConstants: true privateStaticCall: true + overridingProperty: true stubFiles: - ../stubs/arrayFunctions.stub diff --git a/conf/config.level0.neon b/conf/config.level0.neon index c9361622fa..1ba50bab15 100644 --- a/conf/config.level0.neon +++ b/conf/config.level0.neon @@ -29,6 +29,8 @@ conditionalTags: phpstan.rules.rule: %featureToggles.fileWhitespace% PHPStan\Rules\Properties\UninitializedPropertyRule: phpstan.rules.rule: %checkUninitializedProperties% + PHPStan\Rules\Properties\OverridingPropertyRule: + phpstan.rules.rule: %featureToggles.overridingProperty% parametersSchema: missingClosureNativeReturnCheckObjectTypehint: bool() @@ -213,6 +215,11 @@ services: checkClassCaseSensitivity: %checkClassCaseSensitivity% checkThisOnly: %checkThisOnly% + - + class: PHPStan\Rules\Properties\OverridingPropertyRule + arguments: + checkPhpDocMethodSignatures: %checkPhpDocMethodSignatures% + - class: PHPStan\Rules\Properties\UninitializedPropertyRule arguments: diff --git a/conf/config.neon b/conf/config.neon index 932dd54217..11b72ad1b2 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -50,6 +50,7 @@ parameters: finalByPhpDocTag: false classConstants: false privateStaticCall: false + overridingProperty: false fileExtensions: - php checkAlwaysTrueCheckTypeFunctionCall: false @@ -229,7 +230,8 @@ parametersSchema: crossCheckInterfaces: bool(), finalByPhpDocTag: bool(), classConstants: bool(), - privateStaticCall: bool() + privateStaticCall: bool(), + overridingProperty: bool() ]) fileExtensions: listOf(string()) checkAlwaysTrueCheckTypeFunctionCall: bool() diff --git a/src/Node/ClassPropertyNode.php b/src/Node/ClassPropertyNode.php index 36bde885b1..d04f196489 100644 --- a/src/Node/ClassPropertyNode.php +++ b/src/Node/ClassPropertyNode.php @@ -99,6 +99,11 @@ public function isStatic(): bool return (bool) ($this->flags & Class_::MODIFIER_STATIC); } + public function isReadOnly(): bool + { + return (bool) ($this->flags & Class_::MODIFIER_READONLY); + } + /** * @return Identifier|Name|NullableType|UnionType|null */ diff --git a/src/Reflection/Php/PhpPropertyReflection.php b/src/Reflection/Php/PhpPropertyReflection.php index 935da1955b..077c1e58bd 100644 --- a/src/Reflection/Php/PhpPropertyReflection.php +++ b/src/Reflection/Php/PhpPropertyReflection.php @@ -97,6 +97,15 @@ public function isPublic(): bool return $this->reflection->isPublic(); } + public function isReadOnly(): bool + { + if (method_exists($this->reflection, 'isReadOnly')) { + return $this->reflection->isReadOnly(); + } + + return false; + } + public function getReadableType(): Type { if ($this->type === null) { @@ -149,6 +158,11 @@ public function getPhpDocType(): Type return new MixedType(); } + public function hasNativeType(): bool + { + return $this->nativeType !== null; + } + public function getNativeType(): Type { if ($this->finalNativeType === null) { diff --git a/src/Rules/PhpDoc/IncompatiblePropertyPhpDocTypeRule.php b/src/Rules/PhpDoc/IncompatiblePropertyPhpDocTypeRule.php index 74b813e37e..e2494d919f 100644 --- a/src/Rules/PhpDoc/IncompatiblePropertyPhpDocTypeRule.php +++ b/src/Rules/PhpDoc/IncompatiblePropertyPhpDocTypeRule.php @@ -43,7 +43,7 @@ public function processNode(Node $node, Scope $scope): array $propertyName = $node->getName(); $propertyReflection = $scope->getClassReflection()->getNativeProperty($propertyName); - if (!$propertyReflection->hasPhpDoc()) { + if (!$propertyReflection->hasPhpDocType()) { return []; } diff --git a/src/Rules/Properties/OverridingPropertyRule.php b/src/Rules/Properties/OverridingPropertyRule.php new file mode 100644 index 0000000000..c66b81b0b6 --- /dev/null +++ b/src/Rules/Properties/OverridingPropertyRule.php @@ -0,0 +1,193 @@ + + */ +class OverridingPropertyRule implements Rule +{ + + private bool $checkPhpDocMethodSignatures; + + public function __construct(bool $checkPhpDocMethodSignatures) + { + $this->checkPhpDocMethodSignatures = $checkPhpDocMethodSignatures; + } + + public function getNodeType(): string + { + return ClassPropertyNode::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if (!$scope->isInClass()) { + throw new \PHPStan\ShouldNotHappenException(); + } + + $classReflection = $scope->getClassReflection(); + $prototype = $this->findPrototype($classReflection, $node->getName()); + if ($prototype === null) { + return []; + } + + $errors = []; + if ($prototype->isStatic()) { + if (!$node->isStatic()) { + $errors[] = RuleErrorBuilder::message(sprintf( + 'Non-static property %s::$%s overrides static property %s::$%s.', + $classReflection->getDisplayName(), + $node->getName(), + $prototype->getDeclaringClass()->getDisplayName(), + $node->getName() + ))->nonIgnorable()->build(); + } + } elseif ($node->isStatic()) { + $errors[] = RuleErrorBuilder::message(sprintf( + 'Static property %s::$%s overrides non-static property %s::$%s.', + $classReflection->getDisplayName(), + $node->getName(), + $prototype->getDeclaringClass()->getDisplayName(), + $node->getName() + ))->nonIgnorable()->build(); + } + + if ($prototype->isReadOnly()) { + if (!$node->isReadOnly()) { + $errors[] = RuleErrorBuilder::message(sprintf( + 'Readwrite property %s::$%s overrides readonly property %s::$%s.', + $classReflection->getDisplayName(), + $node->getName(), + $prototype->getDeclaringClass()->getDisplayName(), + $node->getName() + ))->nonIgnorable()->build(); + } + } elseif ($node->isReadOnly()) { + $errors[] = RuleErrorBuilder::message(sprintf( + 'Readonly property %s::$%s overrides readwrite property %s::$%s.', + $classReflection->getDisplayName(), + $node->getName(), + $prototype->getDeclaringClass()->getDisplayName(), + $node->getName() + ))->nonIgnorable()->build(); + } + + if ($prototype->isPublic()) { + if (!$node->isPublic()) { + $errors[] = RuleErrorBuilder::message(sprintf( + '%s property %s::$%s overriding public property %s::$%s should also be public.', + $node->isPrivate() ? 'Private' : 'Protected', + $classReflection->getDisplayName(), + $node->getName(), + $prototype->getDeclaringClass()->getDisplayName(), + $node->getName() + ))->nonIgnorable()->build(); + } + } elseif ($node->isPrivate()) { + $errors[] = RuleErrorBuilder::message(sprintf( + 'Private property %s::$%s overriding protected property %s::$%s should be protected or public.', + $classReflection->getDisplayName(), + $node->getName(), + $prototype->getDeclaringClass()->getDisplayName(), + $node->getName() + ))->nonIgnorable()->build(); + } + + $typeErrors = []; + if ($prototype->hasNativeType()) { + if ($node->getNativeType() === null) { + $typeErrors[] = RuleErrorBuilder::message(sprintf( + 'Property %s::$%s overriding property %s::$%s (%s) should also have native type %s.', + $classReflection->getDisplayName(), + $node->getName(), + $prototype->getDeclaringClass()->getDisplayName(), + $node->getName(), + $prototype->getNativeType()->describe(VerbosityLevel::typeOnly()), + $prototype->getNativeType()->describe(VerbosityLevel::typeOnly()) + ))->nonIgnorable()->build(); + } else { + $nativeType = ParserNodeTypeToPHPStanType::resolve($node->getNativeType(), $scope->getClassReflection()->getName()); + if (!$prototype->getNativeType()->equals($nativeType)) { + $typeErrors[] = RuleErrorBuilder::message(sprintf( + 'Type %s of property %s::$%s is not the same as type %s of overridden property %s::$%s.', + $nativeType->describe(VerbosityLevel::typeOnly()), + $classReflection->getDisplayName(), + $node->getName(), + $prototype->getNativeType()->describe(VerbosityLevel::typeOnly()), + $prototype->getDeclaringClass()->getDisplayName(), + $node->getName() + ))->nonIgnorable()->build(); + } + } + } elseif ($node->getNativeType() !== null) { + $typeErrors[] = RuleErrorBuilder::message(sprintf( + 'Property %s::$%s (%s) overriding property %s::$%s should not have a native type.', + $classReflection->getDisplayName(), + $node->getName(), + ParserNodeTypeToPHPStanType::resolve($node->getNativeType(), $scope->getClassReflection()->getName())->describe(VerbosityLevel::typeOnly()), + $prototype->getDeclaringClass()->getDisplayName(), + $node->getName() + ))->nonIgnorable()->build(); + } + + $errors = array_merge($errors, $typeErrors); + + if (!$this->checkPhpDocMethodSignatures) { + return $errors; + } + + if (count($typeErrors) > 0) { + return $errors; + } + + $propertyReflection = $classReflection->getNativeProperty($node->getName()); + if ($propertyReflection->getReadableType()->equals($prototype->getReadableType())) { + return $errors; + } + + $verbosity = VerbosityLevel::getRecommendedLevelByType($prototype->getReadableType(), $propertyReflection->getReadableType()); + + $errors[] = RuleErrorBuilder::message(sprintf( + 'Type %s of property %s::$%s is not the same as type %s of overridden property %s::$%s.', + $propertyReflection->getReadableType()->describe($verbosity), + $classReflection->getDisplayName(), + $node->getName(), + $prototype->getReadableType()->describe($verbosity), + $prototype->getDeclaringClass()->getDisplayName(), + $node->getName() + ))->build(); + + return $errors; + } + + private function findPrototype(ClassReflection $classReflection, string $propertyName): ?PhpPropertyReflection + { + $parentClass = $classReflection->getParentClass(); + if ($parentClass === false) { + return null; + } + + if (!$parentClass->hasNativeProperty($propertyName)) { + return null; + } + + $property = $parentClass->getNativeProperty($propertyName); + if ($property->isPrivate()) { + return null; + } + + return $property; + } + +} diff --git a/tests/PHPStan/Rules/Properties/OverridingPropertyRuleTest.php b/tests/PHPStan/Rules/Properties/OverridingPropertyRuleTest.php new file mode 100644 index 0000000000..a05fe49500 --- /dev/null +++ b/tests/PHPStan/Rules/Properties/OverridingPropertyRuleTest.php @@ -0,0 +1,97 @@ + + */ +class OverridingPropertyRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new OverridingPropertyRule(true); + } + + public function testRule(): void + { + if (!self::$useStaticReflectionProvider) { + $this->markTestSkipped('Test requires static reflection.'); + } + + $this->analyse([__DIR__ . '/data/overriding-property.php'], [ + [ + 'Static property OverridingProperty\Bar::$protectedFoo overrides non-static property OverridingProperty\Foo::$protectedFoo.', + 25, + ], + [ + 'Non-static property OverridingProperty\Bar::$protectedStaticFoo overrides static property OverridingProperty\Foo::$protectedStaticFoo.', + 26, + ], + [ + 'Static property OverridingProperty\Bar::$publicFoo overrides non-static property OverridingProperty\Foo::$publicFoo.', + 28, + ], + [ + 'Non-static property OverridingProperty\Bar::$publicStaticFoo overrides static property OverridingProperty\Foo::$publicStaticFoo.', + 29, + ], + [ + 'Readonly property OverridingProperty\ReadonlyChild::$readWrite overrides readwrite property OverridingProperty\ReadonlyParent::$readWrite.', + 45, + ], + [ + 'Readwrite property OverridingProperty\ReadonlyChild::$readOnly overrides readonly property OverridingProperty\ReadonlyParent::$readOnly.', + 46, + ], + [ + 'Readonly property OverridingProperty\ReadonlyChild2::$readWrite overrides readwrite property OverridingProperty\ReadonlyParent::$readWrite.', + 55, + ], + [ + 'Readwrite property OverridingProperty\ReadonlyChild2::$readOnly overrides readonly property OverridingProperty\ReadonlyParent::$readOnly.', + 56, + ], + [ + 'Private property OverridingProperty\PrivateDolor::$protectedFoo overriding protected property OverridingProperty\Dolor::$protectedFoo should be protected or public.', + 76, + ], + [ + 'Private property OverridingProperty\PrivateDolor::$publicFoo overriding public property OverridingProperty\Dolor::$publicFoo should also be public.', + 77, + ], + [ + 'Private property OverridingProperty\PrivateDolor::$anotherPublicFoo overriding public property OverridingProperty\Dolor::$anotherPublicFoo should also be public.', + 78, + ], + [ + 'Protected property OverridingProperty\ProtectedDolor::$publicFoo overriding public property OverridingProperty\Dolor::$publicFoo should also be public.', + 87, + ], + [ + 'Protected property OverridingProperty\ProtectedDolor::$anotherPublicFoo overriding public property OverridingProperty\Dolor::$anotherPublicFoo should also be public.', + 88, + ], + [ + 'Property OverridingProperty\TypeChild::$withType overriding property OverridingProperty\Typed::$withType (int) should also have native type int.', + 125, + ], + [ + 'Property OverridingProperty\TypeChild::$withoutType (int) overriding property OverridingProperty\Typed::$withoutType should not have a native type.', + 126, + ], + [ + 'Type string of property OverridingProperty\Typed2Child::$foo is not the same as type int of overridden property OverridingProperty\Typed2::$foo.', + 142, + ], + [ + 'Type 4 of property OverridingProperty\Typed2WithPhpDoc::$foo is not the same as type 1|2|3 of overridden property OverridingProperty\TypedWithPhpDoc::$foo.', + 158, + ], + ]); + } + +} diff --git a/tests/PHPStan/Rules/Properties/data/overriding-property.php b/tests/PHPStan/Rules/Properties/data/overriding-property.php new file mode 100644 index 0000000000..51c56ff6a1 --- /dev/null +++ b/tests/PHPStan/Rules/Properties/data/overriding-property.php @@ -0,0 +1,160 @@ += 8.1 + +namespace OverridingProperty; + +class Foo +{ + + private $privateFoo; + private static $privateStaticFoo; + + protected $protectedFoo; + protected static $protectedStaticFoo; + + public $publicFoo; + public static $publicStaticFoo; + +} + +class Bar extends Foo +{ + + private static $privateFoo; + private $privateStaticFoo; + + protected static $protectedFoo; + protected $protectedStaticFoo; + + public static $publicFoo; + public $publicStaticFoo; + +} + +class ReadonlyParent +{ + + public mixed $readWrite; + public readonly mixed $readOnly; + public readonly mixed $readonly2; + +} + +class ReadonlyChild extends ReadonlyParent +{ + + public readonly mixed $readWrite; + public mixed $readOnly; + public readonly mixed $readonly2; + +} + +class ReadonlyChild2 extends ReadonlyParent +{ + + public function __construct( + public readonly mixed $readWrite, + public mixed $readOnly, + public readonly mixed $readonly2 + ) {} + +} + +class Dolor +{ + + private $privateFoo; + protected $protectedFoo; + public $publicFoo; + var $anotherPublicFoo; + +} + +class PrivateDolor extends Dolor +{ + + private $privateFoo; + private $protectedFoo; // error + private $publicFoo; // error + private $anotherPublicFoo; // error + +} + +class ProtectedDolor extends Dolor +{ + + protected $privateFoo; + protected $protectedFoo; + protected $publicFoo; // error + protected $anotherPublicFoo; // error + +} + +class PublicDolor extends Dolor +{ + + public $privateFoo; + public $protectedFoo; + public $publicFoo; + public $anotherPublicFoo; + +} + +class Public2Dolor extends Dolor +{ + + var $privateFoo; + var $protectedFoo; + var $publicFoo; + var $anotherPublicFoo; + +} + +class Typed +{ + + public int $withType; + public $withoutType; + public int $withType2; + public $withoutType2; + +} + +class TypeChild extends Typed +{ + + public $withType; // error + public int $withoutType; // error + public int $withType2; + public $withoutType2; + +} + +class Typed2 +{ + + protected int $foo; + +} + +class Typed2Child extends Typed2 +{ + + protected string $foo; + +} + +class TypedWithPhpDoc +{ + + /** @var 1|2|3 */ + protected int $foo; + +} + +class Typed2WithPhpDoc extends TypedWithPhpDoc +{ + + /** @var 4 */ + protected int $foo; + +}