From 7453f4f75fae3d635063589467842aae29d88b54 Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Wed, 21 Aug 2024 13:30:18 +0200 Subject: [PATCH] Bleeding edge - check too wide private property type --- conf/bleedingEdge.neon | 1 + conf/config.level4.neon | 5 + conf/config.neon | 1 + conf/parametersSchema.neon | 1 + src/Analyser/NodeScopeResolver.php | 2 +- src/Node/ClassPropertiesNode.php | 11 ++ src/Node/ClassStatementsGatherer.php | 13 ++ src/Node/Property/PropertyAssign.php | 31 ++++ .../TooWidePropertyTypeRule.php | 132 ++++++++++++++++++ .../TooWidePropertyTypeRuleTest.php | 59 ++++++++ .../data/too-wide-property-type.php | 87 ++++++++++++ 11 files changed, 342 insertions(+), 1 deletion(-) create mode 100644 src/Node/Property/PropertyAssign.php create mode 100644 src/Rules/TooWideTypehints/TooWidePropertyTypeRule.php create mode 100644 tests/PHPStan/Rules/TooWideTypehints/TooWidePropertyTypeRuleTest.php create mode 100644 tests/PHPStan/Rules/TooWideTypehints/data/too-wide-property-type.php diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 84fd7a825..fb6a5ad96 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -59,5 +59,6 @@ parameters: preciseMissingReturn: true validatePregQuote: true noImplicitWildcard: true + tooWidePropertyType: true stubFiles: - ../stubs/bleedingEdge/Rule.stub diff --git a/conf/config.level4.neon b/conf/config.level4.neon index cb79c9cca..8f3ecc86b 100644 --- a/conf/config.level4.neon +++ b/conf/config.level4.neon @@ -54,6 +54,8 @@ conditionalTags: phpstan.rules.rule: %featureToggles.pure% PHPStan\Rules\DeadCode\PossiblyPureStaticCallCollector: phpstan.collector: %featureToggles.pure% + PHPStan\Rules\TooWideTypehints\TooWidePropertyTypeRule: + phpstan.rules.rule: %featureToggles.tooWidePropertyType% parameters: checkAdvancedIsset: true @@ -313,3 +315,6 @@ services: - class: PHPStan\Rules\TooWideTypehints\TooWideMethodParameterOutTypeRule + + - + class: PHPStan\Rules\TooWideTypehints\TooWidePropertyTypeRule diff --git a/conf/config.neon b/conf/config.neon index 517728b3c..d347559d7 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -95,6 +95,7 @@ parameters: validatePregQuote: false noImplicitWildcard: false narrowPregMatches: true + tooWidePropertyType: false fileExtensions: - php checkAdvancedIsset: false diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon index 7cfb651b7..66fc2ae42 100644 --- a/conf/parametersSchema.neon +++ b/conf/parametersSchema.neon @@ -90,6 +90,7 @@ parametersSchema: validatePregQuote: bool() noImplicitWildcard: bool() narrowPregMatches: bool() + tooWidePropertyType: bool() ]) fileExtensions: listOf(string()) checkAdvancedIsset: bool() diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index c48b2e439..f1c6b0725 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -870,7 +870,7 @@ private function processStmtNode( $this->processAttributeGroups($stmt, $stmt->attrGroups, $classScope, $classStatementsGatherer); $this->processStmtNodes($stmt, $stmt->stmts, $classScope, $classStatementsGatherer, $context); - $nodeCallback(new ClassPropertiesNode($stmt, $this->readWritePropertiesExtensionProvider, $classStatementsGatherer->getProperties(), $classStatementsGatherer->getPropertyUsages(), $classStatementsGatherer->getMethodCalls(), $classStatementsGatherer->getReturnStatementsNodes(), $classReflection), $classScope); + $nodeCallback(new ClassPropertiesNode($stmt, $this->readWritePropertiesExtensionProvider, $classStatementsGatherer->getProperties(), $classStatementsGatherer->getPropertyUsages(), $classStatementsGatherer->getMethodCalls(), $classStatementsGatherer->getReturnStatementsNodes(), $classStatementsGatherer->getPropertyAssigns(), $classReflection), $classScope); $nodeCallback(new ClassMethodsNode($stmt, $classStatementsGatherer->getMethods(), $classStatementsGatherer->getMethodCalls(), $classReflection), $classScope); $nodeCallback(new ClassConstantsNode($stmt, $classStatementsGatherer->getConstants(), $classStatementsGatherer->getConstantFetches(), $classReflection), $classScope); $classReflection->evictPrivateSymbols(); diff --git a/src/Node/ClassPropertiesNode.php b/src/Node/ClassPropertiesNode.php index 302dab88f..7afc4bc87 100644 --- a/src/Node/ClassPropertiesNode.php +++ b/src/Node/ClassPropertiesNode.php @@ -13,6 +13,7 @@ use PHPStan\Analyser\Scope; use PHPStan\Node\Expr\PropertyInitializationExpr; use PHPStan\Node\Method\MethodCall; +use PHPStan\Node\Property\PropertyAssign; use PHPStan\Node\Property\PropertyRead; use PHPStan\Node\Property\PropertyWrite; use PHPStan\Reflection\ClassReflection; @@ -40,6 +41,7 @@ class ClassPropertiesNode extends NodeAbstract implements VirtualNode * @param array $propertyUsages * @param array $methodCalls * @param array $returnStatementNodes + * @param list $propertyAssigns */ public function __construct( private ClassLike $class, @@ -48,6 +50,7 @@ public function __construct( private array $propertyUsages, private array $methodCalls, private array $returnStatementNodes, + private array $propertyAssigns, private ClassReflection $classReflection, ) { @@ -404,4 +407,12 @@ private function getInitializedProperties(Scope $scope, array $initialInitialize return $initialInitializedProperties; } + /** + * @return list + */ + public function getPropertyAssigns(): array + { + return $this->propertyAssigns; + } + } diff --git a/src/Node/ClassStatementsGatherer.php b/src/Node/ClassStatementsGatherer.php index a344e4349..55ef90779 100644 --- a/src/Node/ClassStatementsGatherer.php +++ b/src/Node/ClassStatementsGatherer.php @@ -13,6 +13,7 @@ use PhpParser\Node\Identifier; use PHPStan\Analyser\Scope; use PHPStan\Node\Constant\ClassConstantFetch; +use PHPStan\Node\Property\PropertyAssign; use PHPStan\Node\Property\PropertyRead; use PHPStan\Node\Property\PropertyWrite; use PHPStan\Reflection\ClassReflection; @@ -55,6 +56,9 @@ final class ClassStatementsGatherer /** @var array */ private array $returnStatementNodes = []; + /** @var list */ + private array $propertyAssigns = []; + /** * @param callable(Node $node, Scope $scope): void $nodeCallback */ @@ -122,6 +126,14 @@ public function getReturnStatementsNodes(): array return $this->returnStatementNodes; } + /** + * @return list + */ + public function getPropertyAssigns(): array + { + return $this->propertyAssigns; + } + public function __invoke(Node $node, Scope $scope): void { $nodeCallback = $this->nodeCallback; @@ -189,6 +201,7 @@ private function gatherNodes(Node $node, Scope $scope): void } if ($node instanceof PropertyAssignNode) { $this->propertyUsages[] = new PropertyWrite($node->getPropertyFetch(), $scope, false); + $this->propertyAssigns[] = new PropertyAssign($node, $scope); return; } if (!$node instanceof Expr) { diff --git a/src/Node/Property/PropertyAssign.php b/src/Node/Property/PropertyAssign.php new file mode 100644 index 000000000..a88d384a7 --- /dev/null +++ b/src/Node/Property/PropertyAssign.php @@ -0,0 +1,31 @@ +assign; + } + + public function getScope(): Scope + { + return $this->scope; + } + +} diff --git a/src/Rules/TooWideTypehints/TooWidePropertyTypeRule.php b/src/Rules/TooWideTypehints/TooWidePropertyTypeRule.php new file mode 100644 index 000000000..68a513f29 --- /dev/null +++ b/src/Rules/TooWideTypehints/TooWidePropertyTypeRule.php @@ -0,0 +1,132 @@ + + */ +final class TooWidePropertyTypeRule implements Rule +{ + + public function __construct( + private ReadWritePropertiesExtensionProvider $extensionProvider, + private PropertyReflectionFinder $propertyReflectionFinder, + ) + { + } + + public function getNodeType(): string + { + return ClassPropertiesNode::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $errors = []; + $classReflection = $node->getClassReflection(); + + foreach ($node->getProperties() as $property) { + if (!$property->isPrivate()) { + continue; + } + if ($property->isDeclaredInTrait()) { + continue; + } + if ($property->isPromoted()) { + continue; + } + $propertyName = $property->getName(); + if (!$classReflection->hasNativeProperty($propertyName)) { + continue; + } + + $propertyReflection = $classReflection->getNativeProperty($propertyName); + $propertyType = $propertyReflection->getWritableType(); + if (!$propertyType instanceof UnionType) { + continue; + } + foreach ($this->extensionProvider->getExtensions() as $extension) { + if ($extension->isAlwaysWritten($propertyReflection, $propertyName)) { + continue 2; + } + if ($extension->isInitialized($propertyReflection, $propertyName)) { + continue 2; + } + } + + $assignedTypes = []; + foreach ($node->getPropertyAssigns() as $assign) { + $assignNode = $assign->getAssign(); + $assignPropertyReflections = $this->propertyReflectionFinder->findPropertyReflectionsFromNode($assignNode->getPropertyFetch(), $assign->getScope()); + foreach ($assignPropertyReflections as $assignPropertyReflection) { + if ($propertyName !== $assignPropertyReflection->getName()) { + continue; + } + if ($propertyReflection->getDeclaringClass()->getName() !== $assignPropertyReflection->getDeclaringClass()->getName()) { + continue; + } + + $assignedTypes[] = $assignPropertyReflection->getScope()->getType($assignNode->getAssignedExpr()); + } + } + + if ($property->getDefault() !== null) { + $assignedTypes[] = $scope->getType($property->getDefault()); + } + + if (count($assignedTypes) === 0) { + continue; + } + + $assignedType = TypeCombinator::union(...$assignedTypes); + $propertyDescription = $this->describePropertyByName($propertyReflection, $propertyName); + $verbosityLevel = VerbosityLevel::getRecommendedLevelByType($propertyType, $assignedType); + foreach ($propertyType->getTypes() as $type) { + if (!$type->isSuperTypeOf($assignedType)->no()) { + continue; + } + + if ($property->getNativeType() === null && (new NullType())->isSuperTypeOf($type)->yes()) { + continue; + } + + $errors[] = RuleErrorBuilder::message(sprintf( + '%s (%s) is never assigned %s so it can be removed from the property type.', + $propertyDescription, + $propertyType->describe($verbosityLevel), + $type->describe($verbosityLevel), + )) + ->identifier('property.unusedType') + ->line($property->getStartLine()) + ->build(); + } + + } + return $errors; + } + + private function describePropertyByName(PropertyReflection $property, string $propertyName): string + { + if (!$property->isStatic()) { + return sprintf('Property %s::$%s', $property->getDeclaringClass()->getDisplayName(), $propertyName); + } + + return sprintf('Static property %s::$%s', $property->getDeclaringClass()->getDisplayName(), $propertyName); + } + +} diff --git a/tests/PHPStan/Rules/TooWideTypehints/TooWidePropertyTypeRuleTest.php b/tests/PHPStan/Rules/TooWideTypehints/TooWidePropertyTypeRuleTest.php new file mode 100644 index 000000000..f512d22fc --- /dev/null +++ b/tests/PHPStan/Rules/TooWideTypehints/TooWidePropertyTypeRuleTest.php @@ -0,0 +1,59 @@ + + */ +class TooWidePropertyTypeRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new TooWidePropertyTypeRule( + new DirectReadWritePropertiesExtensionProvider([]), + new PropertyReflectionFinder(), + ); + } + + public function testRule(): void + { + if (PHP_VERSION_ID < 80000) { + self::markTestSkipped('Test requires PHP 8.0.'); + } + + $this->analyse([__DIR__ . '/data/too-wide-property-type.php'], [ + [ + 'Property TooWidePropertyType\Foo::$foo (int|string) is never assigned string so it can be removed from the property type.', + 9, + ], + /*[ + 'Property TooWidePropertyType\Foo::$barr (int|null) is never assigned null so it can be removed from the property type.', + 15, + ], + [ + 'Property TooWidePropertyType\Foo::$barrr (int|null) is never assigned null so it can be removed from the property type.', + 18, + ],*/ + [ + 'Property TooWidePropertyType\Foo::$baz (int|null) is never assigned null so it can be removed from the property type.', + 20, + ], + [ + 'Property TooWidePropertyType\Bar::$c (int|null) is never assigned int so it can be removed from the property type.', + 45, + ], + [ + 'Property TooWidePropertyType\Bar::$d (int|null) is never assigned null so it can be removed from the property type.', + 47, + ], + ]); + } + +} diff --git a/tests/PHPStan/Rules/TooWideTypehints/data/too-wide-property-type.php b/tests/PHPStan/Rules/TooWideTypehints/data/too-wide-property-type.php new file mode 100644 index 000000000..b304e8934 --- /dev/null +++ b/tests/PHPStan/Rules/TooWideTypehints/data/too-wide-property-type.php @@ -0,0 +1,87 @@ += 8.0 + +namespace TooWidePropertyType; + +class Foo +{ + + /** @var int|string */ + private $foo; + + /** @var int|null */ + private $bar; // do not report "null" as not assigned + + /** @var int|null */ + private $barr = 1; // report "null" as not assigned + + /** @var int|null */ + private $barrr; // assigned in constructor - report "null" as not assigned + + private int|null $baz; // report "null" as not assigned + + public function __construct() + { + $this->barrr = 1; + } + + public function doFoo(): void + { + $this->foo = 1; + $this->bar = 1; + $this->barr = 1; + $this->barrr = 1; + $this->baz = 1; + } + +} + +class Bar +{ + + private ?int $a = null; + + private ?int $b = 1; + + private ?int $c = null; + + private ?int $d = 1; + + public function doFoo(): void + { + $this->a = 1; + $this->b = null; + } + +} + +class Baz +{ + + private ?int $a = null; + + public function doFoo(): self + { + $s = new self(); + $s->a = 1; + + return $s; + } + +} + +class Lorem +{ + + public function __construct( + private ?int $a = null + ) + { + + } + + public function doFoo(): void + { + $this->a = 1; + } + +}