From 09c002af22947697e7ed6382867b030ece421a90 Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Mon, 11 Nov 2024 13:01:59 +0100 Subject: [PATCH] Drop UselessPrivatePropertyNullabilityRule (#283) --- README.md | 27 +--- rules.neon | 13 -- .../UselessPrivatePropertyNullabilityRule.php | 119 ------------------ .../ClassPropertyAssignmentVisitor.php | 66 ---------- ...lessPrivatePropertyNullabilityRuleTest.php | 36 ------ .../class-property-assignment-visitor.neon | 5 - .../code.php | 52 -------- 7 files changed, 2 insertions(+), 316 deletions(-) delete mode 100644 src/Rule/UselessPrivatePropertyNullabilityRule.php delete mode 100644 src/Visitor/ClassPropertyAssignmentVisitor.php delete mode 100644 tests/Rule/UselessPrivatePropertyNullabilityRuleTest.php delete mode 100644 tests/Rule/data/UselessPrivatePropertyNullabilityRule/class-property-assignment-visitor.neon delete mode 100644 tests/Rule/data/UselessPrivatePropertyNullabilityRule/code.php diff --git a/README.md b/README.md index 52e5221..10d785d 100644 --- a/README.md +++ b/README.md @@ -592,7 +592,7 @@ function example(MyClass $class) { ### forbidUselessNullableReturn - Denies marking closure/function/method return type as nullable when null is never returned -- Recommended to be used together with `uselessPrivatePropertyDefaultValue` and `UselessPrivatePropertyNullabilityRule` +- Recommended to be used together with `uselessPrivatePropertyDefaultValue` ```php public function example(int $foo): ?int { // null never returned if ($foo < 0) { @@ -664,7 +664,7 @@ try { - Detects useless default value of a private property that is always initialized in constructor. - Cannot handle conditions or private method calls within constructor. - When enabled, return statements in constructors are denied to avoid false positives -- Recommended to be used with `uselessPrivatePropertyNullability` and `forbidUselessNullableReturn` +- Recommended to be used with `forbidUselessNullableReturn` ```php class Example { @@ -677,29 +677,6 @@ class Example } ``` -### uselessPrivatePropertyNullability: -- Detects useless nullability of a private property by checking type of all assignments. -- Works only with natively typehinted properties -- Recommended to be used with `uselessPrivatePropertyNullability` and `forbidUselessNullableReturn` as removing useless default value may cause useless nullability to be detected -- PHPStan 1.12 with bleeding edge contains more generic version of this rule under `property.unusedType` error identifier - -```php -class Example -{ - private ?int $field; // useless nullability - - public function __construct() - { - $this->field = 1; - } - - public function setField(int $value) - { - $this->field = $value; - } -} -``` - ## Native PHPStan extra strictness Some strict behaviour in PHPStan is not enabled by default, consider enabling extra strictness even there: diff --git a/rules.neon b/rules.neon index 82b4acf..a7b6380 100644 --- a/rules.neon +++ b/rules.neon @@ -86,8 +86,6 @@ parameters: reportEvenIfExceptionIsNotAcceptableByRethrownOne: true uselessPrivatePropertyDefaultValue: enabled: %shipmonkRules.enableAllRules% - uselessPrivatePropertyNullability: - enabled: %shipmonkRules.enableAllRules% parametersSchema: shipmonkRules: structure([ @@ -212,9 +210,6 @@ parametersSchema: uselessPrivatePropertyDefaultValue: structure([ enabled: bool() ]) - uselessPrivatePropertyNullability: structure([ - enabled: bool() - ]) ]) conditionalTags: @@ -290,8 +285,6 @@ conditionalTags: phpstan.rules.rule: %shipmonkRules.requirePreviousExceptionPass.enabled% ShipMonk\PHPStan\Rule\UselessPrivatePropertyDefaultValueRule: phpstan.rules.rule: %shipmonkRules.uselessPrivatePropertyDefaultValue.enabled% - ShipMonk\PHPStan\Rule\UselessPrivatePropertyNullabilityRule: - phpstan.rules.rule: %shipmonkRules.uselessPrivatePropertyNullability.enabled% ShipMonk\PHPStan\Visitor\UnusedExceptionVisitor: phpstan.parser.richParserNodeVisitor: %shipmonkRules.forbidUnusedException.enabled% @@ -299,8 +292,6 @@ conditionalTags: phpstan.parser.richParserNodeVisitor: %shipmonkRules.forbidUnusedMatchResult.enabled% ShipMonk\PHPStan\Visitor\TopLevelConstructorPropertyFetchMarkingVisitor: phpstan.parser.richParserNodeVisitor: %shipmonkRules.uselessPrivatePropertyDefaultValue.enabled% - ShipMonk\PHPStan\Visitor\ClassPropertyAssignmentVisitor: - phpstan.parser.richParserNodeVisitor: %shipmonkRules.uselessPrivatePropertyNullability.enabled% services: - @@ -405,8 +396,6 @@ services: class: ShipMonk\PHPStan\Rule\ForbidReturnInConstructorRule - class: ShipMonk\PHPStan\Rule\UselessPrivatePropertyDefaultValueRule - - - class: ShipMonk\PHPStan\Rule\UselessPrivatePropertyNullabilityRule - class: ShipMonk\PHPStan\Rule\RequirePreviousExceptionPassRule @@ -418,5 +407,3 @@ services: class: ShipMonk\PHPStan\Visitor\UnusedMatchVisitor - class: ShipMonk\PHPStan\Visitor\TopLevelConstructorPropertyFetchMarkingVisitor - - - class: ShipMonk\PHPStan\Visitor\ClassPropertyAssignmentVisitor diff --git a/src/Rule/UselessPrivatePropertyNullabilityRule.php b/src/Rule/UselessPrivatePropertyNullabilityRule.php deleted file mode 100644 index 28212dc..0000000 --- a/src/Rule/UselessPrivatePropertyNullabilityRule.php +++ /dev/null @@ -1,119 +0,0 @@ - - */ -class UselessPrivatePropertyNullabilityRule implements Rule -{ - - public function getNodeType(): string - { - return ClassPropertiesNode::class; - } - - /** - * @param ClassPropertiesNode $node - * @return list - */ - public function processNode(Node $node, Scope $scope): array - { - $classReflection = $scope->getClassReflection(); - - if ($classReflection === null) { - return []; - } - - $className = $classReflection->getName(); - - $nullabilityNeeded = []; - - foreach ($node->getPropertyUsages() as $propertyUsage) { - if (!$propertyUsage instanceof PropertyWrite) { - continue; - } - - $fetch = $propertyUsage->getFetch(); - - if ($fetch->name instanceof Expr) { - continue; - } - - $propertyName = $fetch->name->toString(); - - /** @var Expr|null $assignedExpr */ - $assignedExpr = $fetch->getAttribute(ClassPropertyAssignmentVisitor::ASSIGNED_EXPR); - - if ($assignedExpr === null) { // cases like object->array[] = value etc - continue; - } - - $nullType = new NullType(); - $assignedType = $propertyUsage->getScope()->getType($assignedExpr); - - if ($assignedType->accepts($nullType, $scope->isDeclareStrictTypes())->yes()) { - $nullabilityNeeded[$propertyName] = true; - } - } - - [$uninitializedProperties] = $node->getUninitializedProperties($scope, $this->getConstructors($classReflection)); - - $errors = []; - - foreach ($node->getProperties() as $property) { - $shouldBeChecked = ($property->isPrivate() || $property->isReadOnly()) && !$property->isPromoted(); - - if (!$shouldBeChecked) { - continue; - } - - $propertyName = $property->getName(); - $defaultValueNode = $property->getDefault(); - $propertyReflection = $classReflection->getProperty($propertyName, $scope); - $definitionHasTypehint = $property->getNativeType() !== null; - $definitionIsNullable = TypeCombinator::containsNull($propertyReflection->getWritableType()); - $nullIsAssigned = $nullabilityNeeded[$propertyName] ?? false; - $hasNullDefaultValue = $defaultValueNode instanceof ConstFetch && $scope->resolveName($defaultValueNode->name) === 'null'; - $isUninitialized = isset($uninitializedProperties[$propertyName]); - - if ($definitionHasTypehint && $definitionIsNullable && !$nullIsAssigned && !$hasNullDefaultValue && !$isUninitialized) { - $errors[] = RuleErrorBuilder::message("Property {$className}::{$propertyName} is defined as nullable, but null is never assigned") - ->line($property->getStartLine()) - ->identifier('shipmonk.uselessPrivatePropertyNullability') - ->build(); - } - } - - return $errors; - } - - /** - * @return list - */ - private function getConstructors(ClassReflection $classReflection): array - { - $constructors = []; - - if ($classReflection->hasConstructor()) { - $constructors[] = $classReflection->getConstructor()->getName(); - } - - return $constructors; - } - -} diff --git a/src/Visitor/ClassPropertyAssignmentVisitor.php b/src/Visitor/ClassPropertyAssignmentVisitor.php deleted file mode 100644 index 107d389..0000000 --- a/src/Visitor/ClassPropertyAssignmentVisitor.php +++ /dev/null @@ -1,66 +0,0 @@ - - */ - private array $stack = []; - - /** - * @param Node[] $nodes - * @return Node[]|null - */ - public function beforeTraverse(array $nodes): ?array - { - $this->stack = []; - return null; - } - - public function enterNode(Node $node): ?Node - { - if ($this->stack !== []) { - $parent = end($this->stack); - - if ( - $parent instanceof Assign - && ($node instanceof PropertyFetch || $node instanceof StaticPropertyFetch) - ) { - $node->setAttribute(self::ASSIGNED_EXPR, $parent->expr); - } - - $this->stack[] = $node; - } - - if ($this->shouldBuildStack($node)) { - $this->stack[] = $node; - } - - return null; - } - - private function shouldBuildStack(Node $node): bool - { - return $this->stack !== [] || $node instanceof Assign; - } - - public function leaveNode(Node $node): ?Node - { - array_pop($this->stack); - return null; - } - -} diff --git a/tests/Rule/UselessPrivatePropertyNullabilityRuleTest.php b/tests/Rule/UselessPrivatePropertyNullabilityRuleTest.php deleted file mode 100644 index 1e9e9b8..0000000 --- a/tests/Rule/UselessPrivatePropertyNullabilityRuleTest.php +++ /dev/null @@ -1,36 +0,0 @@ - - */ -class UselessPrivatePropertyNullabilityRuleTest extends RuleTestCase -{ - - protected function getRule(): Rule - { - return new UselessPrivatePropertyNullabilityRule(); - } - - /** - * @return string[] - */ - public static function getAdditionalConfigFiles(): array - { - return array_merge( - parent::getAdditionalConfigFiles(), - [__DIR__ . '/data/UselessPrivatePropertyNullabilityRule/class-property-assignment-visitor.neon'], - ); - } - - public function testClass(): void - { - $this->analyseFile(__DIR__ . '/data/UselessPrivatePropertyNullabilityRule/code.php'); - } - -} diff --git a/tests/Rule/data/UselessPrivatePropertyNullabilityRule/class-property-assignment-visitor.neon b/tests/Rule/data/UselessPrivatePropertyNullabilityRule/class-property-assignment-visitor.neon deleted file mode 100644 index aaaa440..0000000 --- a/tests/Rule/data/UselessPrivatePropertyNullabilityRule/class-property-assignment-visitor.neon +++ /dev/null @@ -1,5 +0,0 @@ -services: - - - class: ShipMonk\PHPStan\Visitor\ClassPropertyAssignmentVisitor - tags: - - phpstan.parser.richParserNodeVisitor diff --git a/tests/Rule/data/UselessPrivatePropertyNullabilityRule/code.php b/tests/Rule/data/UselessPrivatePropertyNullabilityRule/code.php deleted file mode 100644 index 3499f1d..0000000 --- a/tests/Rule/data/UselessPrivatePropertyNullabilityRule/code.php +++ /dev/null @@ -1,52 +0,0 @@ -isPublic = $isPublic; - $this->isProtected = $isProtected; - $this->isPrivate = $isPrivate; - $this->isPrivateMixedAssigned = $mixed; - $this->isPrivateWithConditionalAssignment = $isPrivateWithConditionalAssignment === 0 ? null : 1; - $this->isPrivateWithDefaultNull = $isPrivateWithDefaultNull; - $this->isPrivateWithDefaultNotNull = $isPrivateWithDefaultNotNull; - } - - public function setIsPrivateAssigned(?int $isPrivateAssigned): void - { - $this->isPrivateAssigned = $isPrivateAssigned; - } - -}