From 75035dbd316b67ce1d299b8105b3cb81ae286bb7 Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Fri, 5 Dec 2025 14:25:38 +0100 Subject: [PATCH 1/9] Detect dead properties --- rules.neon | 14 ++ src/Collector/ClassDefinitionCollector.php | 63 ++++-- src/Collector/PropertyAccessCollector.php | 204 ++++++++++++++++++++ src/Enum/MemberType.php | 1 + src/Error/BlackMember.php | 4 + src/Formatter/RemoveDeadCodeFormatter.php | 1 + src/Graph/ClassPropertyRef.php | 75 +++++++ src/Graph/ClassPropertyUsage.php | 64 ++++++ src/Graph/CollectedUsage.php | 5 + src/Rule/DeadCodeRule.php | 68 ++++++- src/Visitor/PropertyWriteVisitor.php | 40 ++++ tests/AllServicesInConfigTest.php | 4 + tests/Rule/DeadCodeRuleTest.php | 27 ++- tests/Rule/data/methods/clone.php | 7 +- tests/Rule/data/methods/ctor-denied.php | 2 +- tests/Rule/data/methods/hooks-1.php | 4 +- tests/Rule/data/properties/basic.php | 41 ++++ tests/Rule/data/properties/overridden-1.php | 24 +++ tests/Rule/data/properties/overridden-2.php | 27 +++ tests/Rule/data/properties/promoted.php | 20 ++ tests/Rule/data/properties/static.php | 26 +++ tests/Rule/data/properties/traits.php | 37 ++++ tests/Rule/data/providers/behat.php | 14 +- tests/Rule/data/providers/doctrine.php | 2 +- tests/Rule/data/visitors.neon | 5 + 25 files changed, 741 insertions(+), 38 deletions(-) create mode 100644 src/Collector/PropertyAccessCollector.php create mode 100644 src/Graph/ClassPropertyRef.php create mode 100644 src/Graph/ClassPropertyUsage.php create mode 100644 src/Visitor/PropertyWriteVisitor.php create mode 100644 tests/Rule/data/properties/basic.php create mode 100644 tests/Rule/data/properties/overridden-1.php create mode 100644 tests/Rule/data/properties/overridden-2.php create mode 100644 tests/Rule/data/properties/promoted.php create mode 100644 tests/Rule/data/properties/static.php create mode 100644 tests/Rule/data/properties/traits.php create mode 100644 tests/Rule/data/visitors.neon diff --git a/rules.neon b/rules.neon index 77643c8f..b6bf6900 100644 --- a/rules.neon +++ b/rules.neon @@ -144,6 +144,13 @@ services: arguments: memberUsageExcluders: tagged(shipmonk.deadCode.memberUsageExcluder) + - + class: ShipMonk\PHPStan\DeadCode\Collector\PropertyAccessCollector + tags: + - phpstan.collector + arguments: + memberUsageExcluders: tagged(shipmonk.deadCode.memberUsageExcluder) + - class: ShipMonk\PHPStan\DeadCode\Collector\ClassDefinitionCollector tags: @@ -151,6 +158,7 @@ services: arguments: detectDeadConstants: %shipmonkDeadCode.detect.deadConstants% detectDeadEnumCases: %shipmonkDeadCode.detect.deadEnumCases% + detectDeadProperties: %shipmonkDeadCode.detect.deadProperties% - class: ShipMonk\PHPStan\DeadCode\Collector\ProvidedUsagesCollector @@ -175,6 +183,10 @@ services: servicesWithOldTag: tagged(shipmonk.deadCode.entrypointProvider) trackMixedAccessParameterValue: %shipmonkDeadCode.trackMixedAccess% + - + class: ShipMonk\PHPStan\DeadCode\Visitor\PropertyWriteVisitor + tags: + - phpstan.parser.richParserNodeVisitor parameters: parametersNotInvalidatingCache: @@ -187,6 +199,7 @@ parameters: deadMethods: true deadConstants: true deadEnumCases: false + deadProperties: false usageProviders: apiPhpDoc: enabled: true @@ -238,6 +251,7 @@ parametersSchema: deadMethods: bool() deadConstants: bool() deadEnumCases: bool() + deadProperties: bool() ]) usageProviders: structure([ apiPhpDoc: structure([ diff --git a/src/Collector/ClassDefinitionCollector.php b/src/Collector/ClassDefinitionCollector.php index e1c8a428..7fc75e1c 100644 --- a/src/Collector/ClassDefinitionCollector.php +++ b/src/Collector/ClassDefinitionCollector.php @@ -4,6 +4,7 @@ use LogicException; use PhpParser\Node; +use PhpParser\Node\Expr\Variable; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\Enum_; @@ -21,6 +22,7 @@ use function array_fill_keys; use function array_map; use function count; +use function is_string; /** * @implements Collector, * constants: array, + * properties: array, * methods: array}>, * parents: array, * traits: array, aliases?: array}>, @@ -43,15 +46,19 @@ final class ClassDefinitionCollector implements Collector private bool $detectDeadEnumCases; + private bool $detectDeadProperties; + public function __construct( ReflectionProvider $reflectionProvider, bool $detectDeadConstants, - bool $detectDeadEnumCases + bool $detectDeadEnumCases, + bool $detectDeadProperties ) { $this->reflectionProvider = $reflectionProvider; $this->detectDeadConstants = $detectDeadConstants; $this->detectDeadEnumCases = $detectDeadEnumCases; + $this->detectDeadProperties = $detectDeadProperties; } public function getNodeType(): string @@ -66,6 +73,7 @@ public function getNodeType(): string * name: string, * cases: array, * constants: array, + * properties: array, * methods: array}>, * parents: array, * traits: array, aliases?: array}>, @@ -88,40 +96,69 @@ public function processNode( $methods = []; $constants = []; $cases = []; + $properties = []; foreach ($node->getMethods() as $method) { - $methods[$method->name->toString()] = [ + $methodName = $method->name->toString(); + $methods[$methodName] = [ 'line' => $method->name->getStartLine(), 'params' => count($method->params), 'abstract' => $method->isAbstract() || $node instanceof Interface_, 'visibility' => $method->flags & (Visibility::PUBLIC | Visibility::PROTECTED | Visibility::PRIVATE), ]; - } - if ($this->detectDeadConstants) { - foreach ($node->getConstants() as $constant) { - foreach ($constant->consts as $const) { - $constants[$const->name->toString()] = [ - 'line' => $const->getStartLine(), - ]; + if ($methodName === '__construct') { + foreach ($method->getParams() as $param) { + if ($param->isPromoted() && $param->var instanceof Variable && is_string($param->var->name)) { + $properties[$param->var->name] = [ + 'line' => $param->getStartLine(), + ]; + } } } } - if ($this->detectDeadEnumCases) { - foreach ($this->getEnumCases($node) as $case) { - $cases[$case->name->toString()] = [ - 'line' => $case->name->getStartLine(), + foreach ($node->getConstants() as $constant) { + foreach ($constant->consts as $const) { + $constants[$const->name->toString()] = [ + 'line' => $const->getStartLine(), + ]; + } + } + + foreach ($this->getEnumCases($node) as $case) { + $cases[$case->name->toString()] = [ + 'line' => $case->name->getStartLine(), + ]; + } + + foreach ($node->getProperties() as $property) { + foreach ($property->props as $prop) { + $properties[$prop->name->toString()] = [ + 'line' => $prop->getStartLine(), ]; } } + if (!$this->detectDeadConstants) { + $constants = []; + } + + if (!$this->detectDeadEnumCases) { + $cases = []; + } + + if (!$this->detectDeadProperties) { + $properties = []; + } + return [ 'kind' => $kind, 'name' => $typeName, 'methods' => $methods, 'cases' => $cases, 'constants' => $constants, + 'properties' => $properties, 'parents' => $this->getParents($reflection), 'traits' => $this->getTraits($node), 'interfaces' => $this->getInterfaces($reflection), diff --git a/src/Collector/PropertyAccessCollector.php b/src/Collector/PropertyAccessCollector.php new file mode 100644 index 00000000..a2e8d0ab --- /dev/null +++ b/src/Collector/PropertyAccessCollector.php @@ -0,0 +1,204 @@ +> + */ +final class PropertyAccessCollector implements Collector +{ + + use BufferedUsageCollector; + + /** + * @var list + */ + private array $memberUsageExcluders; + + /** + * @param list $memberUsageExcluders + */ + public function __construct( + array $memberUsageExcluders + ) + { + $this->memberUsageExcluders = $memberUsageExcluders; + } + + public function getNodeType(): string + { + return Node::class; + } + + /** + * @return non-empty-list|null + */ + public function processNode( + Node $node, + Scope $scope + ): ?array + { + if ($this->isInPropertyHook($scope)) { + return null; + } + + if ($node instanceof PropertyFetch && $this->isPropertyRead($node)) { + $this->registerInstancePropertyAccess($node, $scope); + } + + if ($node instanceof StaticPropertyFetch && $this->isPropertyRead($node)) { + $this->registerStaticPropertyAccess($node, $scope); + } + + return $this->emitUsages($scope); + } + + private function registerInstancePropertyAccess( + PropertyFetch $node, + Scope $scope + ): void + { + $propertyNames = $this->getPropertyNames($node, $scope); + $callerType = $scope->getType($node->var); + + foreach ($propertyNames as $propertyName) { + foreach ($this->getDeclaringTypesWithProperty($propertyName, $callerType, null) as $propertyRef) { + $this->registerUsage( + new ClassPropertyUsage( + UsageOrigin::createRegular($node, $scope), + $propertyRef, + ), + $node, + $scope, + ); + } + } + } + + private function registerStaticPropertyAccess( + StaticPropertyFetch $node, + Scope $scope + ): void + { + $propertyNames = $this->getPropertyNames($node, $scope); + $possibleDescendant = $node->class instanceof Expr || $node->class->toString() === 'static'; + + if ($node->class instanceof Expr) { + $callerType = $scope->getType($node->class); + } else { + $callerType = $scope->resolveTypeByName($node->class); + } + + foreach ($propertyNames as $propertyName) { + foreach ($this->getDeclaringTypesWithProperty($propertyName, $callerType, $possibleDescendant) as $propertyRef) { + $this->registerUsage( + new ClassPropertyUsage( + UsageOrigin::createRegular($node, $scope), + $propertyRef, + ), + $node, + $scope, + ); + } + } + } + + /** + * @param PropertyFetch|StaticPropertyFetch $fetch + * @return list + */ + private function getPropertyNames( + Expr $fetch, + Scope $scope + ): array + { + if ($fetch->name instanceof Expr) { + $possiblePropertyNames = []; + + foreach ($scope->getType($fetch->name)->getConstantStrings() as $constantString) { + $possiblePropertyNames[] = $constantString->getValue(); + } + + return $possiblePropertyNames === [] + ? [null] // unknown property name + : $possiblePropertyNames; + } + + return [$fetch->name->toString()]; + } + + /** + * @return list> + */ + private function getDeclaringTypesWithProperty( + ?string $propertyName, + Type $callerType, + ?bool $isPossibleDescendant + ): array + { + $typeNoNull = TypeCombinator::removeNull($callerType); + $classReflections = $typeNoNull->getObjectTypeOrClassStringObjectType()->getObjectClassReflections(); + + $propertyRefs = []; + + foreach ($classReflections as $classReflection) { + $possibleDescendant = $isPossibleDescendant ?? !$classReflection->isFinalByKeyword(); + $propertyRefs[] = new ClassPropertyRef( + $classReflection->getName(), + $propertyName, + $possibleDescendant, + ); + } + + return $propertyRefs; + } + + private function registerUsage( + ClassPropertyUsage $usage, + Node $node, + Scope $scope + ): void + { + $excluderName = null; + + foreach ($this->memberUsageExcluders as $excludedUsageDecider) { + if ($excludedUsageDecider->shouldExclude($usage, $node, $scope)) { + $excluderName = $excludedUsageDecider->getIdentifier(); + break; + } + } + + $this->usages[] = new CollectedUsage($usage, $excluderName); + } + + private function isInPropertyHook(Scope $scope): bool + { + $function = $scope->getFunction(); + if ($function === null) { + return false; + } + + return $function->isMethodOrPropertyHook() && $function->isPropertyHook(); + } + + private function isPropertyRead(Node $node): bool + { + return $node->getAttribute(PropertyWriteVisitor::IS_PROPERTY_WRITE, false) !== true; + } + +} diff --git a/src/Enum/MemberType.php b/src/Enum/MemberType.php index c310ac38..68db4b2c 100644 --- a/src/Enum/MemberType.php +++ b/src/Enum/MemberType.php @@ -7,5 +7,6 @@ interface MemberType public const METHOD = 1; public const CONSTANT = 2; + public const PROPERTY = 3; } diff --git a/src/Error/BlackMember.php b/src/Error/BlackMember.php index 0bbf3b2b..2b0b17ea 100644 --- a/src/Error/BlackMember.php +++ b/src/Error/BlackMember.php @@ -7,6 +7,7 @@ use ShipMonk\PHPStan\DeadCode\Graph\ClassMemberRef; use ShipMonk\PHPStan\DeadCode\Graph\ClassMemberUsage; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodRef; +use ShipMonk\PHPStan\DeadCode\Graph\ClassPropertyRef; use ShipMonk\PHPStan\DeadCode\Graph\CollectedUsage; use ShipMonk\PHPStan\DeadCode\Rule\DeadCodeRule; use function array_keys; @@ -97,6 +98,9 @@ public function getErrorIdentifier(): string } elseif ($this->member instanceof ClassMethodRef) { return DeadCodeRule::IDENTIFIER_METHOD; + } elseif ($this->member instanceof ClassPropertyRef) { + return DeadCodeRule::IDENTIFIER_PROPERTY; + } else { throw new LogicException('Unknown member type'); } diff --git a/src/Formatter/RemoveDeadCodeFormatter.php b/src/Formatter/RemoveDeadCodeFormatter.php index 28d5498b..4e75c03d 100644 --- a/src/Formatter/RemoveDeadCodeFormatter.php +++ b/src/Formatter/RemoveDeadCodeFormatter.php @@ -56,6 +56,7 @@ public function formatErrors( $fileSpecificError->getIdentifier() !== DeadCodeRule::IDENTIFIER_METHOD && $fileSpecificError->getIdentifier() !== DeadCodeRule::IDENTIFIER_CONSTANT && $fileSpecificError->getIdentifier() !== DeadCodeRule::IDENTIFIER_ENUM_CASE + && $fileSpecificError->getIdentifier() !== DeadCodeRule::IDENTIFIER_PROPERTY ) { continue; } diff --git a/src/Graph/ClassPropertyRef.php b/src/Graph/ClassPropertyRef.php new file mode 100644 index 00000000..567be0b6 --- /dev/null +++ b/src/Graph/ClassPropertyRef.php @@ -0,0 +1,75 @@ + + */ +final class ClassPropertyRef extends ClassMemberRef +{ + + /** + * @param C $className + * @param M $propertyName + * @param bool $possibleDescendant True if the $className can be a descendant of the actual class + */ + public function __construct( + ?string $className, + ?string $propertyName, + bool $possibleDescendant + ) + { + parent::__construct($className, $propertyName, $possibleDescendant); + } + + /** + * @return list + */ + protected function getKeyPrefixes(): array + { + return ['p']; + } + + /** + * @return MemberType::PROPERTY + */ + public function getMemberType(): int + { + return MemberType::PROPERTY; + } + + public function withKnownNames( + string $className, + string $memberName + ): self + { + return new self( + $className, + $memberName, + $this->isPossibleDescendant(), + ); + } + + public function withKnownClass(string $className): self + { + return new self( + $className, + $this->getMemberName(), + $this->isPossibleDescendant(), + ); + } + + public function withKnownMember(string $memberName): self + { + return new self( + $this->getClassName(), + $memberName, + $this->isPossibleDescendant(), + ); + } + +} diff --git a/src/Graph/ClassPropertyUsage.php b/src/Graph/ClassPropertyUsage.php new file mode 100644 index 00000000..d4213b92 --- /dev/null +++ b/src/Graph/ClassPropertyUsage.php @@ -0,0 +1,64 @@ + + */ + private ClassPropertyRef $access; + + /** + * @param UsageOrigin $origin The method where the access occurs + * @param ClassPropertyRef $access The property being accessed + */ + public function __construct( + UsageOrigin $origin, + ClassPropertyRef $access + ) + { + parent::__construct($origin); + $this->access = $access; + } + + /** + * @return MemberType::PROPERTY + */ + public function getMemberType(): int + { + return MemberType::PROPERTY; + } + + /** + * @return ClassPropertyRef + */ + public function getMemberRef(): ClassPropertyRef + { + return $this->access; + } + + public function concretizeMixedClassNameUsage(string $className): self + { + if ($this->access->getClassName() !== null) { + throw new LogicException('Usage is not mixed, thus it cannot be concretized'); + } + + return new self( + $this->getOrigin(), + new ClassPropertyRef( + $className, + $this->access->getMemberName(), + $this->access->isPossibleDescendant(), + ), + ); + } + +} diff --git a/src/Graph/CollectedUsage.php b/src/Graph/CollectedUsage.php index 0179ba1a..6e770433 100644 --- a/src/Graph/CollectedUsage.php +++ b/src/Graph/CollectedUsage.php @@ -129,6 +129,11 @@ public static function deserialize( $origin, new ClassMethodRef($result['m']['c'], $result['m']['m'], $result['m']['d']), ); + } elseif ($memberType === MemberType::PROPERTY) { + $usage = new ClassPropertyUsage( + $origin, + new ClassPropertyRef($result['m']['c'], $result['m']['m'], $result['m']['d']), + ); } else { throw new LogicException('Unknown member type: ' . $memberType); } diff --git a/src/Rule/DeadCodeRule.php b/src/Rule/DeadCodeRule.php index 896e20a8..31458792 100644 --- a/src/Rule/DeadCodeRule.php +++ b/src/Rule/DeadCodeRule.php @@ -15,6 +15,7 @@ use ShipMonk\PHPStan\DeadCode\Collector\ClassDefinitionCollector; use ShipMonk\PHPStan\DeadCode\Collector\ConstantFetchCollector; use ShipMonk\PHPStan\DeadCode\Collector\MethodCallCollector; +use ShipMonk\PHPStan\DeadCode\Collector\PropertyAccessCollector; use ShipMonk\PHPStan\DeadCode\Collector\ProvidedUsagesCollector; use ShipMonk\PHPStan\DeadCode\Compatibility\BackwardCompatibilityChecker; use ShipMonk\PHPStan\DeadCode\Debug\DebugUsagePrinter; @@ -27,6 +28,7 @@ use ShipMonk\PHPStan\DeadCode\Graph\ClassMemberRef; use ShipMonk\PHPStan\DeadCode\Graph\ClassMemberUsage; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodRef; +use ShipMonk\PHPStan\DeadCode\Graph\ClassPropertyRef; use ShipMonk\PHPStan\DeadCode\Graph\CollectedUsage; use ShipMonk\PHPStan\DeadCode\Hierarchy\ClassHierarchy; use function array_key_exists; @@ -52,6 +54,7 @@ final class DeadCodeRule implements Rule, DiagnoseExtension public const IDENTIFIER_METHOD = 'shipmonk.deadMethod'; public const IDENTIFIER_CONSTANT = 'shipmonk.deadConstant'; public const IDENTIFIER_ENUM_CASE = 'shipmonk.deadEnumCase'; + public const IDENTIFIER_PROPERTY = 'shipmonk.deadProperty'; private const UNSUPPORTED_MAGIC_METHODS = [ '__invoke' => null, @@ -86,6 +89,7 @@ final class DeadCodeRule implements Rule, DiagnoseExtension * file: string, * cases: array, * constants: array, + * properties: array, * methods: array}>, * parents: array, * traits: array, aliases?: array}>, @@ -166,14 +170,15 @@ public function processNode( /** @var list $knownCollectedUsages */ $knownCollectedUsages = []; - $methodDeclarationData = $node->get(ClassDefinitionCollector::class); + $classDefinitionData = $node->get(ClassDefinitionCollector::class); $methodCallData = $node->get(MethodCallCollector::class); $constFetchData = $node->get(ConstantFetchCollector::class); + $propertyAccessData = $node->get(PropertyAccessCollector::class); $providedUsagesData = $node->get(ProvidedUsagesCollector::class); /** @var array>> $memberUseData */ - $memberUseData = array_merge_recursive($methodCallData, $providedUsagesData, $constFetchData); - unset($methodCallData, $providedUsagesData, $constFetchData); + $memberUseData = array_merge_recursive($methodCallData, $providedUsagesData, $constFetchData, $propertyAccessData); + unset($methodCallData, $providedUsagesData, $constFetchData, $propertyAccessData); foreach ($memberUseData as $file => $usesPerFile) { foreach ($usesPerFile as $useStrings) { @@ -194,7 +199,7 @@ public function processNode( } } - foreach ($methodDeclarationData as $file => $data) { + foreach ($classDefinitionData as $file => $data) { foreach ($data as $typeData) { $typeName = $typeData['name']; $this->typeDefinitions[$typeName] = [ @@ -203,6 +208,7 @@ public function processNode( 'file' => $file, 'cases' => $typeData['cases'], 'constants' => $typeData['constants'], + 'properties' => $typeData['properties'], 'methods' => $typeData['methods'], 'parents' => $typeData['parents'], 'traits' => $typeData['traits'], @@ -211,18 +217,20 @@ public function processNode( } } - unset($methodDeclarationData); + unset($classDefinitionData); foreach ($this->typeDefinitions as $typeName => $typeDefinition) { $methods = $typeDefinition['methods']; $constants = $typeDefinition['constants']; $cases = $typeDefinition['cases']; + $properties = $typeDefinition['properties']; $file = $typeDefinition['file']; $ancestorNames = $this->getAncestorNames($typeName); $this->fillTraitMethodUsages($typeName, $this->getTraitUsages($typeName), $this->getTypeMethods($typeName)); $this->fillTraitConstantUsages($typeName, $this->getTraitUsages($typeName), $this->getTypeConstants($typeName)); + $this->fillTraitPropertyUsages($typeName, $this->getTraitUsages($typeName), $this->getTypeProperties($typeName)); $this->fillClassHierarchy($typeName, $ancestorNames); foreach ($methods as $methodName => $methodData) { @@ -251,6 +259,15 @@ public function processNode( $this->blackMembers[$enumCaseKey] = new BlackMember($enumCaseRef, $file, $enumCaseData['line']); } } + + foreach ($properties as $propertyName => $propertyData) { + $propertyRef = new ClassPropertyRef($typeName, $propertyName, false); + $propertyKeys = $propertyRef->toKeys(); + + foreach ($propertyKeys as $propertyKey) { + $this->blackMembers[$propertyKey] = new BlackMember($propertyRef, $file, $propertyData['line']); + } + } } foreach ($this->typeDefinitions as $typeName => $typeDef) { @@ -260,6 +277,7 @@ public function processNode( array_keys($typeDef['constants']), array_keys($typeDef['cases']), ), + MemberType::PROPERTY => array_keys($typeDef['properties']), ]; foreach ($memberNamesForMixedExpand as $memberType => $memberNames) { @@ -421,6 +439,34 @@ private function fillTraitConstantUsages( } } + /** + * @param array, aliases?: array}> $usedTraits + * @param list $overriddenProperties + */ + private function fillTraitPropertyUsages( + string $typeName, + array $usedTraits, + array $overriddenProperties + ): void + { + foreach ($usedTraits as $traitName => $traitInfo) { + $traitProperties = $this->typeDefinitions[$traitName]['properties'] ?? []; + + $excludedProperties = $overriddenProperties; + + foreach ($traitProperties as $traitConstant => $constantInfo) { + if (in_array($traitConstant, $excludedProperties, true)) { + continue; + } + + $overriddenProperties[] = $traitConstant; + $this->traitMembers[MemberType::PROPERTY][$typeName][$traitName][$traitConstant] = $traitConstant; + } + + $this->fillTraitPropertyUsages($typeName, $this->getTraitUsages($traitName), $overriddenProperties); + } + } + /** * @param list $ancestorNames */ @@ -808,6 +854,14 @@ private function getTypeConstants(string $typeName): array return array_keys($this->typeDefinitions[$typeName]['constants'] ?? []); } + /** + * @return list + */ + private function getTypeProperties(string $typeName): array + { + return array_keys($this->typeDefinitions[$typeName]['properties'] ?? []); + } + /** * @param ClassMemberRef $memberRef */ @@ -853,7 +907,7 @@ private function getMemberNames( /** * @param ClassMemberRef $memberRef - * @return list<'methods'|'constants'|'cases'> + * @return list<'methods'|'constants'|'cases'|'properties'> */ private function getTypeDefinitionKeysForMemberType(ClassMemberRef $memberRef): array { @@ -867,6 +921,8 @@ private function getTypeDefinitionKeysForMemberType(ClassMemberRef $memberRef): } else { return ['constants', 'cases']; } + } elseif ($memberRef instanceof ClassPropertyRef) { + return ['properties']; } throw new LogicException('Invalid member type'); diff --git a/src/Visitor/PropertyWriteVisitor.php b/src/Visitor/PropertyWriteVisitor.php new file mode 100644 index 00000000..bc8e586b --- /dev/null +++ b/src/Visitor/PropertyWriteVisitor.php @@ -0,0 +1,40 @@ +var instanceof PropertyFetch || $node->var instanceof StaticPropertyFetch) // TODO nullsafe? + ) { + $node->var->setAttribute(self::IS_PROPERTY_WRITE, true); + } + + return null; + } + +} diff --git a/tests/AllServicesInConfigTest.php b/tests/AllServicesInConfigTest.php index 471d6ace..1d2fb0b9 100644 --- a/tests/AllServicesInConfigTest.php +++ b/tests/AllServicesInConfigTest.php @@ -13,6 +13,8 @@ use ShipMonk\PHPStan\DeadCode\Graph\ClassConstantUsage; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodRef; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodUsage; +use ShipMonk\PHPStan\DeadCode\Graph\ClassPropertyRef; +use ShipMonk\PHPStan\DeadCode\Graph\ClassPropertyUsage; use ShipMonk\PHPStan\DeadCode\Graph\CollectedUsage; use ShipMonk\PHPStan\DeadCode\Graph\UsageOrigin; use ShipMonk\PHPStan\DeadCode\Provider\VirtualUsageData; @@ -53,6 +55,8 @@ public function test(): void ClassMethodRef::class, ClassConstantRef::class, ClassConstantUsage::class, + ClassPropertyRef::class, + ClassPropertyUsage::class, CollectedUsage::class, BlackMember::class, RemoveDeadCodeTransformer::class, diff --git a/tests/Rule/DeadCodeRuleTest.php b/tests/Rule/DeadCodeRuleTest.php index 74e98dfd..2828a486 100644 --- a/tests/Rule/DeadCodeRuleTest.php +++ b/tests/Rule/DeadCodeRuleTest.php @@ -26,6 +26,7 @@ use ShipMonk\PHPStan\DeadCode\Collector\ClassDefinitionCollector; use ShipMonk\PHPStan\DeadCode\Collector\ConstantFetchCollector; use ShipMonk\PHPStan\DeadCode\Collector\MethodCallCollector; +use ShipMonk\PHPStan\DeadCode\Collector\PropertyAccessCollector; use ShipMonk\PHPStan\DeadCode\Collector\ProvidedUsagesCollector; use ShipMonk\PHPStan\DeadCode\Compatibility\BackwardCompatibilityChecker; use ShipMonk\PHPStan\DeadCode\Debug\DebugUsagePrinter; @@ -101,6 +102,8 @@ final class DeadCodeRuleTest extends ShipMonkRuleTestCase private bool $detectDeadEnumCases = true; + private bool $detectDeadProperties = true; + private ?DeadCodeRule $rule = null; private ?string $editorUrl = null; @@ -138,9 +141,10 @@ protected function getCollectors(): array return [ new ProvidedUsagesCollector($reflectionProvider, $this->getMemberUsageProviders(), $this->getMemberUsageExcluders()), - new ClassDefinitionCollector($reflectionProvider, $this->detectDeadConstants, $this->detectDeadEnumCases), + new ClassDefinitionCollector($reflectionProvider, $this->detectDeadConstants, $this->detectDeadEnumCases, $this->detectDeadProperties), new MethodCallCollector($this->getMemberUsageExcluders()), new ConstantFetchCollector($reflectionProvider, $this->getMemberUsageExcluders()), + new PropertyAccessCollector($this->getMemberUsageExcluders()), ]; } @@ -472,11 +476,13 @@ public function testDetectionCanBeDisabled( $this->detectDeadMethods = false; $this->detectDeadConstants = false; $this->detectDeadEnumCases = false; + $this->detectDeadProperties = false; $ownIdentifiers = [ DeadCodeRule::IDENTIFIER_METHOD, DeadCodeRule::IDENTIFIER_CONSTANT, DeadCodeRule::IDENTIFIER_ENUM_CASE, + DeadCodeRule::IDENTIFIER_PROPERTY, ]; $filterOwnErrors = static fn (Error $error): bool => in_array($error->getIdentifier(), $ownIdentifiers, true); @@ -903,6 +909,14 @@ public static function provideFiles(): Traversable yield 'enum-basic' => [__DIR__ . '/data/enums/basic.php', self::requiresPhp(8_01_00)]; yield 'enum-mixed' => [__DIR__ . '/data/enums/mixed.php', self::requiresPhp(8_01_00)]; + // properties + yield 'property-basic' => [__DIR__ . '/data/properties/basic.php']; + yield 'property-static' => [__DIR__ . '/data/properties/static.php']; + yield 'property-traits' => [__DIR__ . '/data/properties/traits.php']; + yield 'property-promoted' => [__DIR__ . '/data/properties/promoted.php']; + yield 'property-overridden-1' => [__DIR__ . '/data/properties/overridden-1.php']; + yield 'property-overridden-2' => [__DIR__ . '/data/properties/overridden-2.php']; + // mixed member yield 'mixed-member-enum' => [__DIR__ . '/data/mixed-member/enum.php', self::requiresPhp(8_01_00)]; yield 'mixed-member-full-method' => [__DIR__ . '/data/mixed-member/full-mixed-method.php']; @@ -1233,4 +1247,15 @@ private function resetInternalAnalyser(): void }, $this, OriginalRuleTestCase::class)(); } + /** + * @return string[] + */ + public static function getAdditionalConfigFiles(): array + { + return array_merge( + parent::getAdditionalConfigFiles(), + [__DIR__ . '/data/visitors.neon'], + ); + } + } diff --git a/tests/Rule/data/methods/clone.php b/tests/Rule/data/methods/clone.php index ebf6d4c1..cc2b83b1 100644 --- a/tests/Rule/data/methods/clone.php +++ b/tests/Rule/data/methods/clone.php @@ -26,10 +26,7 @@ public function __clone() { class Coord { - public function __construct( - public int $x, - public int $y, - ) {} + public function __construct() {} public function __clone() {} } @@ -37,6 +34,6 @@ public function __clone() {} clone new CloneClass1(); clone new ChildClass(); -clone(new Coord(1, 2), [ // PHP 8.5 clone with +clone(new Coord(), [ // PHP 8.5 clone with 'y' => 1, ]); diff --git a/tests/Rule/data/methods/ctor-denied.php b/tests/Rule/data/methods/ctor-denied.php index 331367c6..cc72e4e1 100644 --- a/tests/Rule/data/methods/ctor-denied.php +++ b/tests/Rule/data/methods/ctor-denied.php @@ -11,7 +11,7 @@ private function __construct() class NotUtility { - private function __construct(private int $foo) // error: Unused CtorDenied\NotUtility::__construct + private function __construct(int $foo) // error: Unused CtorDenied\NotUtility::__construct { } diff --git a/tests/Rule/data/methods/hooks-1.php b/tests/Rule/data/methods/hooks-1.php index a166cc3e..85e43ef5 100644 --- a/tests/Rule/data/methods/hooks-1.php +++ b/tests/Rule/data/methods/hooks-1.php @@ -14,7 +14,7 @@ public static function empty(): self final class Test { - public State $state { - get => State::empty(); // until we implement dead property detection, hooks are considered always called, thus currently not part of the transitivity chain + public State $state { // error: Unused Hooks1\Test::state + get => State::empty(); // hooks are considered always called, thus currently not part of the transitivity chain } } diff --git a/tests/Rule/data/properties/basic.php b/tests/Rule/data/properties/basic.php new file mode 100644 index 00000000..5c7e73ee --- /dev/null +++ b/tests/Rule/data/properties/basic.php @@ -0,0 +1,41 @@ +usedPublicProperty = 'used'; + $this->unusedPublicProperty = 'assigned but unused'; + } + + public function useProperty(): void + { + echo $this->usedPublicProperty; + } +} + +class ChildClass extends TestClass { + + public string $childUsedProperty; + + public function useChildProperty(): void + { + echo $this->childUsedProperty; + echo $this->usedPublicPropertyByChild; + } +} + +function test() { + $obj = new TestClass(); + echo $obj->usedPublicProperty; + $obj->useProperty(); + + $child = new ChildClass(); + $child->useChildProperty(); +} diff --git a/tests/Rule/data/properties/overridden-1.php b/tests/Rule/data/properties/overridden-1.php new file mode 100644 index 00000000..747acadf --- /dev/null +++ b/tests/Rule/data/properties/overridden-1.php @@ -0,0 +1,24 @@ + $class + */ + public static function test(string $class): void + { + echo $class::$one; + echo $class::$two; + } +} + +class ChildClass extends ParentClass { + public static string $one = 'child'; + public static string $two = 'child'; +} + +function test() { + ParentClass::test(); +} diff --git a/tests/Rule/data/properties/promoted.php b/tests/Rule/data/properties/promoted.php new file mode 100644 index 00000000..725cadc8 --- /dev/null +++ b/tests/Rule/data/properties/promoted.php @@ -0,0 +1,20 @@ +usedPrivatePromotedProperty; + } +} + +function test() { + $obj = new TestClass('used1', 'unused1', 'used2', 'unused2'); + echo $obj->usedPromotedProperty; +} diff --git a/tests/Rule/data/properties/static.php b/tests/Rule/data/properties/static.php new file mode 100644 index 00000000..1d3f1e6d --- /dev/null +++ b/tests/Rule/data/properties/static.php @@ -0,0 +1,26 @@ +traitUsedProperty = 'trait value'; + echo $this->traitUsedProperty; + } +} + +class TestClass { + + use TestTrait; + + public string $classUsedProperty; + + public string $classUnusedProperty; // error: Unused DeadPropertyTraits\TestClass::classUnusedProperty + + public function __construct() + { + $this->classUsedProperty = 'class value'; + $this->useTraitProperty(); + } +} + +function test() { + $obj = new TestClass(); + echo $obj->classUsedProperty; + echo $obj->traitUsedProperty; +} diff --git a/tests/Rule/data/providers/behat.php b/tests/Rule/data/providers/behat.php index aec33dc0..4b60f205 100644 --- a/tests/Rule/data/providers/behat.php +++ b/tests/Rule/data/providers/behat.php @@ -13,19 +13,19 @@ interface Context #[Attribute(Attribute::TARGET_METHOD)] class Given { - public function __construct(public string $pattern) {} + public function __construct() {} } #[Attribute(Attribute::TARGET_METHOD)] class When { - public function __construct(public string $pattern) {} + public function __construct() {} } #[Attribute(Attribute::TARGET_METHOD)] class Then { - public function __construct(public string $pattern) {} + public function __construct() {} } namespace Behat\Hook; @@ -85,13 +85,9 @@ class Transform use Behat\Step\When; use Behat\Transformation\Transform; -class SomeService -{ -} - class FeatureContext implements Context { - public function __construct(private SomeService $service) + public function __construct() { } @@ -205,7 +201,7 @@ public function iClickOn(string $element): void // Context using PHP 8 attributes class AttributeContext implements Context { - public function __construct(private SomeService $service) + public function __construct() { } diff --git a/tests/Rule/data/providers/doctrine.php b/tests/Rule/data/providers/doctrine.php index 55497bf3..3c634d2e 100644 --- a/tests/Rule/data/providers/doctrine.php +++ b/tests/Rule/data/providers/doctrine.php @@ -17,7 +17,7 @@ class MyEntity { #[\Doctrine\ORM\Mapping\Column(type: Types::STRING, enumType: InvoiceStatus::class)] - private InvoiceStatus $status; + private InvoiceStatus $status; // error: Unused Doctrine\MyEntity::status #[\Doctrine\ORM\Mapping\PreUpdate] public function onUpdate(PreUpdateEventArgs $args): void {} diff --git a/tests/Rule/data/visitors.neon b/tests/Rule/data/visitors.neon new file mode 100644 index 00000000..d9394c5b --- /dev/null +++ b/tests/Rule/data/visitors.neon @@ -0,0 +1,5 @@ +services: + - + class: ShipMonk\PHPStan\DeadCode\Visitor\PropertyWriteVisitor + tags: + - phpstan.parser.richParserNodeVisitor From cecbb3e62b32e8e21d59c0fcd261b39197b44233 Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Fri, 5 Dec 2025 16:08:25 +0100 Subject: [PATCH 2/9] Proper write detection; more tests --- src/Graph/ClassPropertyUsage.php | 4 +- src/Graph/UsageOrigin.php | 3 +- src/Visitor/PropertyWriteVisitor.php | 64 +++++++++++++++---- tests/Rule/DeadCodeRuleTest.php | 8 +++ tests/Rule/data/properties/hooks-1.php | 34 ++++++++++ tests/Rule/data/properties/hooks-2.php | 17 +++++ tests/Rule/data/properties/hooks-3.php | 18 ++++++ tests/Rule/data/properties/nullsafe.php | 12 ++++ tests/Rule/data/properties/write-array.php | 16 +++++ tests/Rule/data/properties/write-coalesce.php | 12 ++++ tests/Rule/data/properties/write-inc-dec.php | 12 ++++ .../Rule/data/properties/write-increment.php | 17 +++++ tests/Rule/data/properties/write-multi.php | 17 +++++ 13 files changed, 216 insertions(+), 18 deletions(-) create mode 100644 tests/Rule/data/properties/hooks-1.php create mode 100644 tests/Rule/data/properties/hooks-2.php create mode 100644 tests/Rule/data/properties/hooks-3.php create mode 100644 tests/Rule/data/properties/nullsafe.php create mode 100644 tests/Rule/data/properties/write-array.php create mode 100644 tests/Rule/data/properties/write-coalesce.php create mode 100644 tests/Rule/data/properties/write-inc-dec.php create mode 100644 tests/Rule/data/properties/write-increment.php create mode 100644 tests/Rule/data/properties/write-multi.php diff --git a/src/Graph/ClassPropertyUsage.php b/src/Graph/ClassPropertyUsage.php index d4213b92..6e06b0b6 100644 --- a/src/Graph/ClassPropertyUsage.php +++ b/src/Graph/ClassPropertyUsage.php @@ -17,8 +17,8 @@ final class ClassPropertyUsage extends ClassMemberUsage private ClassPropertyRef $access; /** - * @param UsageOrigin $origin The method where the access occurs - * @param ClassPropertyRef $access The property being accessed + * @param UsageOrigin $origin The method where the read occurs + * @param ClassPropertyRef $access The property being read */ public function __construct( UsageOrigin $origin, diff --git a/src/Graph/UsageOrigin.php b/src/Graph/UsageOrigin.php index feeb97ac..02b949e1 100644 --- a/src/Graph/UsageOrigin.php +++ b/src/Graph/UsageOrigin.php @@ -5,7 +5,6 @@ use LogicException; use PhpParser\Node; use PHPStan\Analyser\Scope; -use PHPStan\Reflection\Php\PhpMethodFromParserNodeReflection; use ShipMonk\PHPStan\DeadCode\Provider\MemberUsageProvider; use ShipMonk\PHPStan\DeadCode\Provider\VirtualUsageData; use function get_class; @@ -79,7 +78,7 @@ public static function createRegular( : $scope->getFile(); $function = $scope->getFunction(); - $isRegularMethod = $function instanceof PhpMethodFromParserNodeReflection && !$function->isPropertyHook(); // @phpstan-ignore phpstanApi.instanceofAssumption + $isRegularMethod = $function !== null && $function->isMethodOrPropertyHook() && !$function->isPropertyHook(); if (!$scope->isInClass() || !$isRegularMethod) { return new self( diff --git a/src/Visitor/PropertyWriteVisitor.php b/src/Visitor/PropertyWriteVisitor.php index bc8e586b..97761e2c 100644 --- a/src/Visitor/PropertyWriteVisitor.php +++ b/src/Visitor/PropertyWriteVisitor.php @@ -3,8 +3,16 @@ namespace ShipMonk\PHPStan\DeadCode\Visitor; use PhpParser\Node; +use PhpParser\Node\Expr; +use PhpParser\Node\Expr\ArrayDimFetch; use PhpParser\Node\Expr\Assign; +use PhpParser\Node\Expr\AssignOp; use PhpParser\Node\Expr\AssignRef; +use PhpParser\Node\Expr\List_; +use PhpParser\Node\Expr\PostDec; +use PhpParser\Node\Expr\PostInc; +use PhpParser\Node\Expr\PreDec; +use PhpParser\Node\Expr\PreInc; use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Expr\StaticPropertyFetch; use PhpParser\NodeVisitorAbstract; @@ -14,27 +22,55 @@ final class PropertyWriteVisitor extends NodeVisitorAbstract public const IS_PROPERTY_WRITE = '_sm_is_property_write'; - - /** - * @param Node[] $nodes - * @return Node[]|null - */ - public function beforeTraverse(array $nodes): ?array - { - return null; - } - public function enterNode(Node $node): ?Node { if ( - ($node instanceof Assign || $node instanceof AssignRef) - && - ($node->var instanceof PropertyFetch || $node->var instanceof StaticPropertyFetch) // TODO nullsafe? + !$node instanceof Assign + && !$node instanceof AssignRef + && !$node instanceof AssignOp + && !$node instanceof PostInc + && !$node instanceof PostDec + && !$node instanceof PreInc + && !$node instanceof PreDec ) { - $node->var->setAttribute(self::IS_PROPERTY_WRITE, true); + return null; } + $this->markPropertyWrites($node->var); + return null; } + private function markPropertyWrites(Expr $expr): void + { + if ($this->isFetch($expr)) { // $this->prop = + $expr->setAttribute(self::IS_PROPERTY_WRITE, true); + } + + if ($expr instanceof List_) { // [$this->first, $this->last] = + foreach ($expr->items as $item) { + if ($item === null) { + continue; + } + if ($this->isFetch($item->value)) { + $item->value->setAttribute(self::IS_PROPERTY_WRITE, true); + } + } + } + + while ($expr instanceof ArrayDimFetch) { // $this->array[] = + if ($this->isFetch($expr->var)) { + $expr->var->setAttribute(self::IS_PROPERTY_WRITE, true); + break; + } + $expr = $expr->var; + } + } + + private function isFetch(Expr $expr): bool + { + // NullsafePropertyFetch not needed: Can't use nullsafe operator in write context + return $expr instanceof PropertyFetch || $expr instanceof StaticPropertyFetch; + } + } diff --git a/tests/Rule/DeadCodeRuleTest.php b/tests/Rule/DeadCodeRuleTest.php index 2828a486..df0ede54 100644 --- a/tests/Rule/DeadCodeRuleTest.php +++ b/tests/Rule/DeadCodeRuleTest.php @@ -914,8 +914,16 @@ public static function provideFiles(): Traversable yield 'property-static' => [__DIR__ . '/data/properties/static.php']; yield 'property-traits' => [__DIR__ . '/data/properties/traits.php']; yield 'property-promoted' => [__DIR__ . '/data/properties/promoted.php']; + yield 'property-hooks-1' => [__DIR__ . '/data/properties/hooks-1.php']; + yield 'property-hooks-2' => [__DIR__ . '/data/properties/hooks-2.php']; + yield 'property-hooks-3' => [__DIR__ . '/data/properties/hooks-3.php']; yield 'property-overridden-1' => [__DIR__ . '/data/properties/overridden-1.php']; yield 'property-overridden-2' => [__DIR__ . '/data/properties/overridden-2.php']; + yield 'property-nullsafe' => [__DIR__ . '/data/properties/nullsafe.php']; + yield 'property-write-array' => [__DIR__ . '/data/properties/write-array.php']; + yield 'property-write-multi' => [__DIR__ . '/data/properties/write-multi.php']; + yield 'property-write-coalesce' => [__DIR__ . '/data/properties/write-coalesce.php']; + yield 'property-write-inc-dec' => [__DIR__ . '/data/properties/write-inc-dec.php']; // mixed member yield 'mixed-member-enum' => [__DIR__ . '/data/mixed-member/enum.php', self::requiresPhp(8_01_00)]; diff --git a/tests/Rule/data/properties/hooks-1.php b/tests/Rule/data/properties/hooks-1.php new file mode 100644 index 00000000..d2822a63 --- /dev/null +++ b/tests/Rule/data/properties/hooks-1.php @@ -0,0 +1,34 @@ +first $this->last"; + } + set(string $value) { + [$this->first, $this->last] = explode(' ', $value, 2); + } + } + + public string $city = 'default value' { // error: Unused PropertyHooks1\Person::city + get => $this->city; + + set { + $this->city = strtolower($value); + } + } +} + +function test(Person $person) { + echo $person->fullName; +} diff --git a/tests/Rule/data/properties/hooks-2.php b/tests/Rule/data/properties/hooks-2.php new file mode 100644 index 00000000..bed48259 --- /dev/null +++ b/tests/Rule/data/properties/hooks-2.php @@ -0,0 +1,17 @@ +name; +} diff --git a/tests/Rule/data/properties/hooks-3.php b/tests/Rule/data/properties/hooks-3.php new file mode 100644 index 00000000..8695dc86 --- /dev/null +++ b/tests/Rule/data/properties/hooks-3.php @@ -0,0 +1,18 @@ +name; +} diff --git a/tests/Rule/data/properties/nullsafe.php b/tests/Rule/data/properties/nullsafe.php new file mode 100644 index 00000000..9be6c152 --- /dev/null +++ b/tests/Rule/data/properties/nullsafe.php @@ -0,0 +1,12 @@ +name; +} diff --git a/tests/Rule/data/properties/write-array.php b/tests/Rule/data/properties/write-array.php new file mode 100644 index 00000000..4f37b508 --- /dev/null +++ b/tests/Rule/data/properties/write-array.php @@ -0,0 +1,16 @@ +array['name'] = 'John'; + $test->array2['more']['name'] = 'John'; +} diff --git a/tests/Rule/data/properties/write-coalesce.php b/tests/Rule/data/properties/write-coalesce.php new file mode 100644 index 00000000..035715fc --- /dev/null +++ b/tests/Rule/data/properties/write-coalesce.php @@ -0,0 +1,12 @@ +prop ??= 1; +} diff --git a/tests/Rule/data/properties/write-inc-dec.php b/tests/Rule/data/properties/write-inc-dec.php new file mode 100644 index 00000000..a51180c4 --- /dev/null +++ b/tests/Rule/data/properties/write-inc-dec.php @@ -0,0 +1,12 @@ +array['name'] = 'John'; + $test->array2['more']['name'] = 'John'; +} diff --git a/tests/Rule/data/properties/write-multi.php b/tests/Rule/data/properties/write-multi.php new file mode 100644 index 00000000..96399d29 --- /dev/null +++ b/tests/Rule/data/properties/write-multi.php @@ -0,0 +1,17 @@ +first, $this->last] = explode(' ', $name, 2); + } +} + +function test(): void { + new Person('John Doe'); +} From 166a436094e06bd4f614692383858f0575d70fee Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Fri, 5 Dec 2025 16:12:37 +0100 Subject: [PATCH 3/9] No hook tests on old PHP --- tests/Rule/DeadCodeRuleTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Rule/DeadCodeRuleTest.php b/tests/Rule/DeadCodeRuleTest.php index df0ede54..2ec70a8b 100644 --- a/tests/Rule/DeadCodeRuleTest.php +++ b/tests/Rule/DeadCodeRuleTest.php @@ -914,9 +914,9 @@ public static function provideFiles(): Traversable yield 'property-static' => [__DIR__ . '/data/properties/static.php']; yield 'property-traits' => [__DIR__ . '/data/properties/traits.php']; yield 'property-promoted' => [__DIR__ . '/data/properties/promoted.php']; - yield 'property-hooks-1' => [__DIR__ . '/data/properties/hooks-1.php']; - yield 'property-hooks-2' => [__DIR__ . '/data/properties/hooks-2.php']; - yield 'property-hooks-3' => [__DIR__ . '/data/properties/hooks-3.php']; + yield 'property-hooks-1' => [__DIR__ . '/data/properties/hooks-1.php', self::requiresPhp(8_00_00)]; + yield 'property-hooks-2' => [__DIR__ . '/data/properties/hooks-2.php', self::requiresPhp(8_00_00)]; + yield 'property-hooks-3' => [__DIR__ . '/data/properties/hooks-3.php', self::requiresPhp(8_00_00)]; yield 'property-overridden-1' => [__DIR__ . '/data/properties/overridden-1.php']; yield 'property-overridden-2' => [__DIR__ . '/data/properties/overridden-2.php']; yield 'property-nullsafe' => [__DIR__ . '/data/properties/nullsafe.php']; From 62b62da1fb01ac6f8c75f62f50f325a1a09eda7a Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Fri, 5 Dec 2025 16:38:33 +0100 Subject: [PATCH 4/9] Reflection support for properties --- src/Provider/ReflectionUsageProvider.php | 62 ++++++++++++ tests/Rule/DeadCodeRuleTest.php | 1 + .../data/providers/reflection-properties.php | 96 +++++++++++++++++++ 3 files changed, 159 insertions(+) create mode 100644 tests/Rule/data/providers/reflection-properties.php diff --git a/src/Provider/ReflectionUsageProvider.php b/src/Provider/ReflectionUsageProvider.php index 9e2f9309..8e105a37 100644 --- a/src/Provider/ReflectionUsageProvider.php +++ b/src/Provider/ReflectionUsageProvider.php @@ -18,6 +18,8 @@ use ShipMonk\PHPStan\DeadCode\Graph\ClassMemberUsage; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodRef; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodUsage; +use ShipMonk\PHPStan\DeadCode\Graph\ClassPropertyRef; +use ShipMonk\PHPStan\DeadCode\Graph\ClassPropertyUsage; use ShipMonk\PHPStan\DeadCode\Graph\UsageOrigin; use function array_filter; use function array_key_first; @@ -67,6 +69,7 @@ private function processMethodCall( $usedConstants = []; $usedMethods = []; $usedEnumCases = []; + $usedProperties = []; foreach ($methodNames as $methodName) { foreach ($callerType->getObjectClassReflections() as $reflection) { @@ -94,6 +97,10 @@ private function processMethodCall( ...$usedEnumCases, ...$this->extractEnumCasesUsedByReflection($genericClassName, $methodName, $node->getArgs(), $node, $scope), ]; + $usedProperties = [ + ...$usedProperties, + ...$this->extractPropertiesUsedByReflection($genericClassName, $methodName, $node->getArgs(), $node, $scope), + ]; } } } @@ -103,6 +110,7 @@ private function processMethodCall( ...$usedConstants, ...$usedMethods, ...$usedEnumCases, + ...$usedProperties, ], static fn (?ClassMemberUsage $usage): bool => $usage !== null)); } @@ -197,6 +205,39 @@ private function extractMethodsUsedByReflection( return $usedMethods; } + /** + * @param array $args + * @return list + */ + private function extractPropertiesUsedByReflection( + ?string $genericClassName, + string $methodName, + array $args, + Node $node, + Scope $scope + ): array + { + $usedProperties = []; + + if ( + $methodName === 'getProperties' + || $methodName === 'getDefaultProperties' // simplified, ideally should mark white only default properties + || $methodName === 'getStaticProperties' // simplified, ideally should mark white only static properties + ) { + $usedProperties[] = $this->createPropertyUsage($node, $scope, $genericClassName, null); + } + + if (in_array($methodName, ['getProperty', 'getStaticPropertyValue'], true) && count($args) >= 1) { + $firstArg = $args[array_key_first($args)]; + + foreach ($scope->getType($firstArg->value)->getConstantStrings() as $constantString) { + $usedProperties[] = $this->createPropertyUsage($node, $scope, $genericClassName, $constantString->getValue()); + } + } + + return $usedProperties; + } + /** * @param NullsafeMethodCall|MethodCall|StaticCall|New_ $call * @return list @@ -288,4 +329,25 @@ private function createMethodUsage( ); } + private function createPropertyUsage( + Node $node, + Scope $scope, + ?string $className, + ?string $propertyName + ): ?ClassPropertyUsage + { + if ($className === null && $propertyName === null) { + return null; + } + + return new ClassPropertyUsage( + UsageOrigin::createRegular($node, $scope), + new ClassPropertyRef( + $className, + $propertyName, + true, + ), + ); + } + } diff --git a/tests/Rule/DeadCodeRuleTest.php b/tests/Rule/DeadCodeRuleTest.php index 2ec70a8b..dc5cea9a 100644 --- a/tests/Rule/DeadCodeRuleTest.php +++ b/tests/Rule/DeadCodeRuleTest.php @@ -861,6 +861,7 @@ public static function provideFiles(): Traversable yield 'provider-reflection' => [__DIR__ . '/data/providers/reflection.php', self::requiresPhp(8_01_00)]; yield 'provider-reflection-enums' => [__DIR__ . '/data/providers/reflection-enums.php', self::requiresPhp(8_01_00)]; yield 'provider-reflection-no-t' => [__DIR__ . '/data/providers/reflection-no-generics.php']; + yield 'provider-reflection-properties' => [__DIR__ . '/data/providers/reflection-properties.php', self::requiresPhp(8_01_00)]; yield 'provider-symfony' => [__DIR__ . '/data/providers/symfony.php', self::requiresPhp(8_00_00)]; yield 'provider-symfony-7.1' => [__DIR__ . '/data/providers/symfony-gte71.php', self::requiresPhp(8_00_00) && self::requiresPackage('symfony/dependency-injection', '>= 7.1')]; yield 'provider-twig' => [__DIR__ . '/data/providers/twig.php', self::requiresPhp(8_00_00)]; diff --git a/tests/Rule/data/providers/reflection-properties.php b/tests/Rule/data/providers/reflection-properties.php new file mode 100644 index 00000000..844ed801 --- /dev/null +++ b/tests/Rule/data/providers/reflection-properties.php @@ -0,0 +1,96 @@ +getProperty('transitivelyDead'); + } +} + +abstract class GetAllPropertiesParent { + public static function getProperties() + { + return (new \ReflectionClass(static::class))->getProperties(); + } + + /** + * @param \ReflectionClass $reflection + */ + public static function getProperties2(\ReflectionClass $reflection) + { + return $reflection->getProperties(); + } +} + +class GetAllPropertiesChild extends GetAllPropertiesParent { + public string $property; +} + +function test() { + GetAllPropertiesChild::getProperties(); + GetAllPropertiesChild::getProperties2(); + + $reflection1 = new \ReflectionClass(Holder1::class); + $reflection1->getProperty('used'); + + $reflection2 = new \ReflectionClass(Holder2::class); + $reflection2->getProperty('property1'); + $reflection2->getProperty('property3'); + + $reflection3 = new \ReflectionClass(Holder3::class); + $reflection3->getProperties(); + + $reflection5 = new \ReflectionClass(Holder5::class); + $reflection5->getProperty('property'); +} + +/** + * @param class-string $fqn + */ +function testPropertyOnlyInDescendant(string $fqn) { + $classReflection = new \ReflectionClass($fqn); + + if ($classReflection->hasProperty('notInParent')) { + $classReflection->getProperty('notInParent'); + } +} From fa76f3a7d2adae9fd579f6e86f5964c42a92c2da Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Fri, 5 Dec 2025 16:40:11 +0100 Subject: [PATCH 5/9] Readme --- README.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 6d6d383d..185a490e 100644 --- a/README.md +++ b/README.md @@ -107,7 +107,7 @@ parameters: ## Generic usage providers: #### Reflection: -- Any enum, constant or method accessed via `ReflectionClass` is detected as used +- Any property, enum, constant or method accessed via `ReflectionClass` is detected as used - e.g. `$reflection->getConstructor()`, `$reflection->getConstant('NAME')`, `$reflection->getMethods()`, `$reflection->getCases()`... #### Vendor: @@ -376,10 +376,12 @@ parameters: detect: deadMethods: true deadConstants: true - deadEnumCases: false + deadProperties: false # opt-in + deadEnumCases: false # opt-in ``` -Enum cases are disabled by default as those are often used in API input objects (using custom deserialization, which typically require custom usage provider). +Enum cases and properties are disabled by default as those are often used in API input objects (using custom deserialization, which typically require custom usage provider). +But libraries should be able to enable those easily. ## Comparison with tomasvotruba/unused-public @@ -477,8 +479,9 @@ If you set up `editorUrl` [parameter](https://phpstan.org/user-guide/output-form - You can also mark whole class or interface with `@api` to mark all its methods as entrypoints ## Future scope: -- Dead class property detection - Dead class detection +- Dead parameters detection +- Useless public/protected visibility ## Contributing - Check your code by `composer check` From 8342c12fc68d15da81b7e6922d7ca4797be173a1 Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Fri, 5 Dec 2025 17:09:30 +0100 Subject: [PATCH 6/9] properties extension point to ReflectionBasedMemberUsageProvider --- .../ReflectionBasedMemberUsageProvider.php | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/src/Provider/ReflectionBasedMemberUsageProvider.php b/src/Provider/ReflectionBasedMemberUsageProvider.php index 46a88d6b..a51590fb 100644 --- a/src/Provider/ReflectionBasedMemberUsageProvider.php +++ b/src/Provider/ReflectionBasedMemberUsageProvider.php @@ -11,11 +11,14 @@ use ReflectionEnum; use ReflectionEnumUnitCase; use ReflectionMethod; +use ReflectionProperty; use ShipMonk\PHPStan\DeadCode\Graph\ClassConstantRef; use ShipMonk\PHPStan\DeadCode\Graph\ClassConstantUsage; use ShipMonk\PHPStan\DeadCode\Graph\ClassMemberUsage; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodRef; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodUsage; +use ShipMonk\PHPStan\DeadCode\Graph\ClassPropertyRef; +use ShipMonk\PHPStan\DeadCode\Graph\ClassPropertyUsage; use ShipMonk\PHPStan\DeadCode\Graph\UsageOrigin; use function array_merge; @@ -37,6 +40,7 @@ public function getUsages( $this->getMethodUsages($classReflection), $this->getConstantUsages($classReflection), $this->getEnumCaseUsages($classReflection), + $this->getPropertyUsages($classReflection), ); } @@ -58,6 +62,11 @@ protected function shouldMarkEnumCaseAsUsed(ReflectionEnumUnitCase $enumCase): ? return null; // Expected to be overridden by subclasses. } + protected function shouldMarkPropertyAsUsed(ReflectionProperty $property): ?VirtualUsageData + { + return null; // Expected to be overridden by subclasses. + } + /** * @return list */ @@ -134,6 +143,30 @@ private function getEnumCaseUsages(ClassReflection $classReflection): array return $usages; } + /** + * @return list + */ + private function getPropertyUsages(ClassReflection $classReflection): array + { + $nativeClassReflection = $classReflection->getNativeReflection(); + + $usages = []; + + foreach ($nativeClassReflection->getProperties() as $nativePropertyReflection) { + if ($nativePropertyReflection->getDeclaringClass()->getName() !== $nativeClassReflection->getName()) { + continue; // skip properties from ancestors + } + + $usage = $this->shouldMarkPropertyAsUsed($nativePropertyReflection); + + if ($usage !== null) { + $usages[] = $this->createPropertyUsage($nativePropertyReflection, $usage); + } + } + + return $usages; + } + private function createConstantUsage( ReflectionClassConstant $constantReflection, VirtualUsageData $data @@ -181,4 +214,19 @@ private function createEnumCaseUsage( ); } + private function createPropertyUsage( + ReflectionProperty $propertyReflection, + VirtualUsageData $data + ): ClassPropertyUsage + { + return new ClassPropertyUsage( + UsageOrigin::createVirtual($this, $data), + new ClassPropertyRef( + $propertyReflection->getDeclaringClass()->getName(), + $propertyReflection->getName(), + false, + ), + ); + } + } From 6f431d4bb73d6c608bd0996095589be961611759 Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Fri, 5 Dec 2025 17:40:56 +0100 Subject: [PATCH 7/9] properties in providers: apidoc, builtin, vendor --- src/Provider/ApiPhpDocUsageProvider.php | 29 ++++++++++++++++++++---- src/Provider/BuiltinUsageProvider.php | 24 ++++++++++++++++---- src/Provider/VendorUsageProvider.php | 22 +++++++++++++++--- src/Reflection/ReflectionHelper.php | 18 +++++++++++++++ tests/Rule/DeadCodeRuleTest.php | 1 + tests/Rule/data/providers/api-phpdoc.php | 11 +++++++++ tests/Rule/data/providers/builtin.php | 17 ++++++++++++++ tests/Rule/data/providers/vendor.php | 6 +++++ 8 files changed, 117 insertions(+), 11 deletions(-) create mode 100644 tests/Rule/data/providers/builtin.php diff --git a/src/Provider/ApiPhpDocUsageProvider.php b/src/Provider/ApiPhpDocUsageProvider.php index 08b938a2..649b2bbe 100644 --- a/src/Provider/ApiPhpDocUsageProvider.php +++ b/src/Provider/ApiPhpDocUsageProvider.php @@ -7,6 +7,7 @@ use ReflectionClassConstant; use ReflectionEnumUnitCase; use ReflectionMethod; +use ReflectionProperty; use ShipMonk\PHPStan\DeadCode\Reflection\ReflectionHelper; use function strpos; @@ -41,13 +42,18 @@ public function shouldMarkEnumCaseAsUsed(ReflectionEnumUnitCase $enumCase): ?Vir return $this->enabled ? $this->shouldMarkMemberAsUsed($enumCase) : null; } + public function shouldMarkPropertyAsUsed(ReflectionProperty $property): ?VirtualUsageData + { + return $this->enabled ? $this->shouldMarkMemberAsUsed($property) : null; + } + /** - * @param ReflectionClassConstant|ReflectionMethod $member + * @param ReflectionClassConstant|ReflectionMethod|ReflectionProperty $member */ public function shouldMarkMemberAsUsed(object $member): ?VirtualUsageData { $reflectionClass = $this->reflectionProvider->getClass($member->getDeclaringClass()->getName()); - $memberType = $member instanceof ReflectionClassConstant ? 'constant' : 'method'; + $memberType = ReflectionHelper::getMemberType($member); $memberName = $member->getName(); if ($this->isApiMember($reflectionClass, $member)) { @@ -74,7 +80,7 @@ public function shouldMarkMemberAsUsed(object $member): ?VirtualUsageData } /** - * @param ReflectionClassConstant|ReflectionMethod $member + * @param ReflectionClassConstant|ReflectionMethod|ReflectionProperty $member */ private function isApiMember( ClassReflection $reflection, @@ -100,6 +106,17 @@ private function isApiMember( return false; } + if ($member instanceof ReflectionProperty) { + $property = $reflection->getNativeProperty($member->getName()); + $phpDoc = $property->getDocComment(); + + if ($this->isApiPhpDoc($phpDoc)) { + return true; + } + + return false; + } + $phpDoc = $reflection->getNativeMethod($member->getName())->getDocComment(); if ($this->isApiPhpDoc($phpDoc)) { @@ -110,7 +127,7 @@ private function isApiMember( } /** - * @param ReflectionClassConstant|ReflectionMethod $member + * @param ReflectionClassConstant|ReflectionMethod|ReflectionProperty $member */ private function hasOwnMember( ClassReflection $reflection, @@ -125,6 +142,10 @@ private function hasOwnMember( return ReflectionHelper::hasOwnConstant($reflection, $member->getName()); } + if ($member instanceof ReflectionProperty) { + return ReflectionHelper::hasOwnProperty($reflection, $member->getName()); + } + return ReflectionHelper::hasOwnMethod($reflection, $member->getName()); } diff --git a/src/Provider/BuiltinUsageProvider.php b/src/Provider/BuiltinUsageProvider.php index 8051f081..e987393d 100644 --- a/src/Provider/BuiltinUsageProvider.php +++ b/src/Provider/BuiltinUsageProvider.php @@ -5,7 +5,10 @@ use ReflectionClass; use ReflectionClassConstant; use ReflectionMethod; +use ReflectionProperty; use Reflector; +use ShipMonk\PHPStan\DeadCode\Reflection\ReflectionHelper; +use function ucfirst; final class BuiltinUsageProvider extends ReflectionBasedMemberUsageProvider { @@ -35,8 +38,17 @@ protected function shouldMarkConstantAsUsed(ReflectionClassConstant $constant): return $this->shouldMarkMemberAsUsed($constant); } + protected function shouldMarkPropertyAsUsed(ReflectionProperty $property): ?VirtualUsageData + { + if (!$this->enabled) { + return null; + } + + return $this->shouldMarkMemberAsUsed($property); + } + /** - * @param ReflectionMethod|ReflectionClassConstant $member + * @param ReflectionMethod|ReflectionClassConstant|ReflectionProperty $member */ private function shouldMarkMemberAsUsed(Reflector $member): ?VirtualUsageData { @@ -66,7 +78,7 @@ private function shouldMarkMemberAsUsed(Reflector $member): ?VirtualUsageData } /** - * @param ReflectionMethod|ReflectionClassConstant $member + * @param ReflectionMethod|ReflectionClassConstant|ReflectionProperty $member * @param ReflectionClass $reflectionClass */ private function isBuiltinMember( @@ -82,15 +94,19 @@ private function isBuiltinMember( return false; } + if ($member instanceof ReflectionProperty && !$reflectionClass->hasProperty($member->getName())) { + return false; + } + return $reflectionClass->getExtensionName() !== false; } /** - * @param ReflectionMethod|ReflectionClassConstant $member + * @param ReflectionMethod|ReflectionClassConstant|ReflectionProperty $member */ private function createUsageNote(Reflector $member): VirtualUsageData { - $memberString = $member instanceof ReflectionMethod ? 'Method' : 'Constant'; + $memberString = ucfirst(ReflectionHelper::getMemberType($member)); return VirtualUsageData::withNote("$memberString overrides builtin one, thus is assumed to be used by some PHP code."); } diff --git a/src/Provider/VendorUsageProvider.php b/src/Provider/VendorUsageProvider.php index 74db0262..2ae457a2 100644 --- a/src/Provider/VendorUsageProvider.php +++ b/src/Provider/VendorUsageProvider.php @@ -6,11 +6,14 @@ use ReflectionClass; use ReflectionClassConstant; use ReflectionMethod; +use ReflectionProperty; use Reflector; +use ShipMonk\PHPStan\DeadCode\Reflection\ReflectionHelper; use function array_keys; use function strlen; use function strpos; use function substr; +use function ucfirst; final class VendorUsageProvider extends ReflectionBasedMemberUsageProvider { @@ -46,13 +49,22 @@ protected function shouldMarkConstantAsUsed(ReflectionClassConstant $constant): return $this->shouldMarkMemberAsUsed($constant); } + protected function shouldMarkPropertyAsUsed(ReflectionProperty $property): ?VirtualUsageData + { + if (!$this->enabled) { + return null; + } + + return $this->shouldMarkMemberAsUsed($property); + } + /** - * @param ReflectionMethod|ReflectionClassConstant $member + * @param ReflectionMethod|ReflectionClassConstant|ReflectionProperty $member */ private function shouldMarkMemberAsUsed(Reflector $member): ?VirtualUsageData { $reflectionClass = $member->getDeclaringClass(); - $memberString = $member instanceof ReflectionMethod ? 'Method' : 'Constant'; + $memberString = ucfirst(ReflectionHelper::getMemberType($member)); $usage = VirtualUsageData::withNote($memberString . ' overrides vendor one, thus is expected to be used by vendor code'); do { @@ -79,7 +91,7 @@ private function shouldMarkMemberAsUsed(Reflector $member): ?VirtualUsageData } /** - * @param ReflectionMethod|ReflectionClassConstant $member + * @param ReflectionMethod|ReflectionClassConstant|ReflectionProperty $member * @param ReflectionClass $reflectionClass */ private function isForeignMember( @@ -95,6 +107,10 @@ private function isForeignMember( return false; } + if ($member instanceof ReflectionProperty && !$reflectionClass->hasProperty($member->getName())) { + return false; + } + if ($reflectionClass->getExtensionName() !== false) { return false; // many built-in classes have stubs in PHPStan (with filepath in vendor); BuiltinUsageProvider will handle them } diff --git a/src/Reflection/ReflectionHelper.php b/src/Reflection/ReflectionHelper.php index a179a1c2..66a54758 100644 --- a/src/Reflection/ReflectionHelper.php +++ b/src/Reflection/ReflectionHelper.php @@ -3,11 +3,29 @@ namespace ShipMonk\PHPStan\DeadCode\Reflection; use PHPStan\Reflection\ClassReflection; +use ReflectionClassConstant; use ReflectionException; +use ReflectionMethod; +use ReflectionProperty; final class ReflectionHelper { + /** + * @param ReflectionClassConstant|ReflectionMethod|ReflectionProperty $member + * @return 'constant'|'property'|'method' + */ + public static function getMemberType(object $member): string + { + if ($member instanceof ReflectionClassConstant) { + return 'constant'; + } elseif ($member instanceof ReflectionProperty) { + return 'property'; + } else { + return 'method'; + } + } + public static function hasOwnMethod( ClassReflection $classReflection, string $methodName diff --git a/tests/Rule/DeadCodeRuleTest.php b/tests/Rule/DeadCodeRuleTest.php index dc5cea9a..c095facd 100644 --- a/tests/Rule/DeadCodeRuleTest.php +++ b/tests/Rule/DeadCodeRuleTest.php @@ -873,6 +873,7 @@ public static function provideFiles(): Traversable yield 'provider-nette' => [__DIR__ . '/data/providers/nette.php']; yield 'provider-apiphpdoc' => [__DIR__ . '/data/providers/api-phpdoc.php', self::requiresPhp(8_01_00)]; yield 'provider-enum' => [__DIR__ . '/data/providers/enum.php', self::requiresPhp(8_01_00)]; + yield 'provider-builtin' => [__DIR__ . '/data/providers/builtin.php']; yield 'provider-stream-wrapper' => [__DIR__ . '/data/providers/stream-wrapper.php']; // excluders diff --git a/tests/Rule/data/providers/api-phpdoc.php b/tests/Rule/data/providers/api-phpdoc.php index 048cc522..b81c6fc4 100644 --- a/tests/Rule/data/providers/api-phpdoc.php +++ b/tests/Rule/data/providers/api-phpdoc.php @@ -20,6 +20,9 @@ class PublicApi { const CONST1 = 1; const CONST2 = 2; + public string $property1; + public string $property2; + public function method1() {} public function method2() {} @@ -31,6 +34,10 @@ class PartialPublicApi { const CONST1 = 1; const CONST2 = 2; // error: Unused ApiPhpdoc1\PartialPublicApi::CONST2 + /** @api */ + public string $property1; + public string $property2; // error: Unused ApiPhpdoc1\PartialPublicApi::property2 + /** @api */ public function method1() {} public function method2() {} // error: Unused ApiPhpdoc1\PartialPublicApi::method2 @@ -56,6 +63,10 @@ class InheritedPublicApi1 extends PublicApi { const CONST2 = 2; const CONST3 = 3; // error: Unused ApiPhpdoc1\InheritedPublicApi1::CONST3 + public string $property1; + public string $property2; + public string $property3; // error: Unused ApiPhpdoc1\InheritedPublicApi1::property3 + public function method1() {} public function method2() {} public function method3() {} // error: Unused ApiPhpdoc1\InheritedPublicApi1::method3 diff --git a/tests/Rule/data/providers/builtin.php b/tests/Rule/data/providers/builtin.php new file mode 100644 index 00000000..a610a956 --- /dev/null +++ b/tests/Rule/data/providers/builtin.php @@ -0,0 +1,17 @@ + Date: Fri, 5 Dec 2025 19:45:56 +0100 Subject: [PATCH 8/9] Support auto-removal of dead properties --- src/Formatter/RemoveDeadCodeFormatter.php | 11 ++++- src/Transformer/RemoveClassMemberVisitor.php | 49 ++++++++++++++++++- src/Transformer/RemoveDeadCodeTransformer.php | 6 ++- tests/Rule/DeadCodeRuleTest.php | 2 + .../removing/promoted-properties.output.txt | 4 ++ .../data/removing/promoted-properties.php | 24 +++++++++ .../promoted-properties.transformed.php | 22 +++++++++ .../Rule/data/removing/properties.output.txt | 6 +++ tests/Rule/data/removing/properties.php | 34 +++++++++++++ .../data/removing/properties.transformed.php | 24 +++++++++ 10 files changed, 177 insertions(+), 5 deletions(-) create mode 100644 tests/Rule/data/removing/promoted-properties.output.txt create mode 100644 tests/Rule/data/removing/promoted-properties.php create mode 100644 tests/Rule/data/removing/promoted-properties.transformed.php create mode 100644 tests/Rule/data/removing/properties.output.txt create mode 100644 tests/Rule/data/removing/properties.php create mode 100644 tests/Rule/data/removing/properties.transformed.php diff --git a/src/Formatter/RemoveDeadCodeFormatter.php b/src/Formatter/RemoveDeadCodeFormatter.php index 4e75c03d..cbec8bf4 100644 --- a/src/Formatter/RemoveDeadCodeFormatter.php +++ b/src/Formatter/RemoveDeadCodeFormatter.php @@ -79,10 +79,12 @@ public function formatErrors( $deadConstants = $deadMembersByType[MemberType::CONSTANT] ?? []; /** @var array> $deadMethods */ $deadMethods = $deadMembersByType[MemberType::METHOD] ?? []; + /** @var array> $deadProperties */ + $deadProperties = $deadMembersByType[MemberType::PROPERTY] ?? []; - $membersCount += count($deadConstants) + count($deadMethods); + $membersCount += count($deadConstants) + count($deadMethods) + count($deadProperties); - $transformer = new RemoveDeadCodeTransformer(array_keys($deadMethods), array_keys($deadConstants)); + $transformer = new RemoveDeadCodeTransformer(array_keys($deadMethods), array_keys($deadConstants), array_keys($deadProperties)); $oldCode = $this->fileSystem->read($file); $newCode = $transformer->transformCode($oldCode); $this->fileSystem->write($file, $newCode); @@ -96,6 +98,11 @@ public function formatErrors( $output->writeLineFormatted(" • Removed method $method"); $this->printExcludedUsages($output, $excludedUsages); } + + foreach ($deadProperties as $property => $excludedUsages) { + $output->writeLineFormatted(" • Removed property $property"); + $this->printExcludedUsages($output, $excludedUsages); + } } $memberPlural = $membersCount === 1 ? '' : 's'; diff --git a/src/Transformer/RemoveClassMemberVisitor.php b/src/Transformer/RemoveClassMemberVisitor.php index 1ed32c0a..cd1dd0d9 100644 --- a/src/Transformer/RemoveClassMemberVisitor.php +++ b/src/Transformer/RemoveClassMemberVisitor.php @@ -4,16 +4,21 @@ use PhpParser\Node; use PhpParser\Node\Const_; +use PhpParser\Node\Expr\Variable; use PhpParser\Node\Identifier; use PhpParser\Node\Name; +use PhpParser\Node\Param; use PhpParser\Node\Stmt\ClassConst; use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\ClassMethod; use PhpParser\Node\Stmt\EnumCase; use PhpParser\Node\Stmt\Namespace_; +use PhpParser\Node\Stmt\Property; use PhpParser\NodeTraverser; use PhpParser\NodeVisitorAbstract; use function array_fill_keys; +use function array_filter; +use function is_string; use function ltrim; final class RemoveClassMemberVisitor extends NodeVisitorAbstract @@ -33,17 +38,25 @@ final class RemoveClassMemberVisitor extends NodeVisitorAbstract */ private array $deadConstants; + /** + * @var array + */ + private array $deadProperties; + /** * @param list $deadMethods * @param list $deadConstants + * @param list $deadProperties */ public function __construct( array $deadMethods, - array $deadConstants + array $deadConstants, + array $deadProperties ) { $this->deadMethods = array_fill_keys($deadMethods, true); $this->deadConstants = array_fill_keys($deadConstants, true); + $this->deadProperties = array_fill_keys($deadProperties, true); } public function enterNode(Node $node): ?Node @@ -66,6 +79,23 @@ public function leaveNode(Node $node): ?int if (isset($this->deadMethods[$methodKey])) { return NodeTraverser::REMOVE_NODE; } + + // Handle promoted properties in constructor parameters + $node->params = array_filter($node->params, function (Param $param): bool { + if (!$param->isPromoted() || !$param->var instanceof Variable) { + return true; + } + + $paramName = $param->var->name; + + if (!is_string($paramName)) { + return true; + } + + $propertyKey = ltrim($this->currentNamespace . '\\' . $this->currentClass, '\\') . '::' . $paramName; + + return !isset($this->deadProperties[$propertyKey]); + }); } if ($node instanceof ClassConst) { @@ -101,6 +131,23 @@ public function leaveNode(Node $node): ?int } } + if ($node instanceof Property) { + $allDead = true; + + foreach ($node->props as $prop) { + $propertyKey = $this->getNamespacedName($prop->name); + + if (!isset($this->deadProperties[$propertyKey])) { + $allDead = false; + break; + } + } + + if ($allDead) { + return NodeTraverser::REMOVE_NODE; + } + } + return null; } diff --git a/src/Transformer/RemoveDeadCodeTransformer.php b/src/Transformer/RemoveDeadCodeTransformer.php index ae62da9d..ad10d12d 100644 --- a/src/Transformer/RemoveDeadCodeTransformer.php +++ b/src/Transformer/RemoveDeadCodeTransformer.php @@ -26,10 +26,12 @@ final class RemoveDeadCodeTransformer /** * @param list $deadMethods * @param list $deadConstants + * @param list $deadProperties */ public function __construct( array $deadMethods, - array $deadConstants + array $deadConstants, + array $deadProperties ) { $this->phpLexer = new Lexer(); @@ -39,7 +41,7 @@ public function __construct( $this->cloningTraverser->addVisitor(new CloningVisitor()); $this->removingTraverser = new PhpTraverser(); - $this->removingTraverser->addVisitor(new RemoveClassMemberVisitor($deadMethods, $deadConstants)); + $this->removingTraverser->addVisitor(new RemoveClassMemberVisitor($deadMethods, $deadConstants, $deadProperties)); $this->phpPrinter = new PhpPrinter(); } diff --git a/tests/Rule/DeadCodeRuleTest.php b/tests/Rule/DeadCodeRuleTest.php index c095facd..3e412e7c 100644 --- a/tests/Rule/DeadCodeRuleTest.php +++ b/tests/Rule/DeadCodeRuleTest.php @@ -989,6 +989,8 @@ public static function provideAutoRemoveFiles(): Traversable { yield 'sample' => [__DIR__ . '/data/removing/sample.php']; yield 'no-namespace' => [__DIR__ . '/data/removing/no-namespace.php']; + yield 'properties' => [__DIR__ . '/data/removing/properties.php']; + yield 'promoted-properties' => [__DIR__ . '/data/removing/promoted-properties.php']; } private function getAutoremoveTransformedFilePath(string $file): string diff --git a/tests/Rule/data/removing/promoted-properties.output.txt b/tests/Rule/data/removing/promoted-properties.output.txt new file mode 100644 index 00000000..f44f0867 --- /dev/null +++ b/tests/Rule/data/removing/promoted-properties.output.txt @@ -0,0 +1,4 @@ + • Removed property Removal\ClassWithPromotedProperties::deadPromoted + • Removed property Removal\ClassWithPromotedProperties::deadPrivatePromoted + +Removed 2 dead members in 1 file. diff --git a/tests/Rule/data/removing/promoted-properties.php b/tests/Rule/data/removing/promoted-properties.php new file mode 100644 index 00000000..e8b5627d --- /dev/null +++ b/tests/Rule/data/removing/promoted-properties.php @@ -0,0 +1,24 @@ +usedPromoted; + } +} + +function useClassWithPromotedProps(): void +{ + $obj = new ClassWithPromotedProperties('used', 'dead', 42); + echo $obj->getUsed(); +} diff --git a/tests/Rule/data/removing/promoted-properties.transformed.php b/tests/Rule/data/removing/promoted-properties.transformed.php new file mode 100644 index 00000000..3f228382 --- /dev/null +++ b/tests/Rule/data/removing/promoted-properties.transformed.php @@ -0,0 +1,22 @@ +usedPromoted; + } +} + +function useClassWithPromotedProps(): void +{ + $obj = new ClassWithPromotedProperties('used', 'dead', 42); + echo $obj->getUsed(); +} diff --git a/tests/Rule/data/removing/properties.output.txt b/tests/Rule/data/removing/properties.output.txt new file mode 100644 index 00000000..8ca94de9 --- /dev/null +++ b/tests/Rule/data/removing/properties.output.txt @@ -0,0 +1,6 @@ + • Removed property Removal\ClassWithProperties::deadProperty + • Removed property Removal\ClassWithProperties::deadProperty1 + • Removed property Removal\ClassWithProperties::deadProperty2 + • Removed property Removal\ClassWithProperties::deadStaticProperty + +Removed 4 dead members in 1 file. diff --git a/tests/Rule/data/removing/properties.php b/tests/Rule/data/removing/properties.php new file mode 100644 index 00000000..7ad9555a --- /dev/null +++ b/tests/Rule/data/removing/properties.php @@ -0,0 +1,34 @@ +deadProperty); + } + } + + public int $deadProperty1, $deadProperty2; + + private static string $deadStaticProperty; + + public function __construct() + { + $this->usedProperty = 'used'; + } + + public function getUsed(): string + { + return $this->usedProperty; + } +} + +function useClass(): void +{ + $obj = new ClassWithProperties(); + echo $obj->getUsed(); +} diff --git a/tests/Rule/data/removing/properties.transformed.php b/tests/Rule/data/removing/properties.transformed.php new file mode 100644 index 00000000..f9a1eecd --- /dev/null +++ b/tests/Rule/data/removing/properties.transformed.php @@ -0,0 +1,24 @@ +usedProperty = 'used'; + } + + public function getUsed(): string + { + return $this->usedProperty; + } +} + +function useClass(): void +{ + $obj = new ClassWithProperties(); + echo $obj->getUsed(); +} From 4e7c0cf1d05c525c0890b33d33165a3dc147b932 Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Mon, 8 Dec 2025 09:20:37 +0100 Subject: [PATCH 9/9] Also allow property debug --- src/Debug/DebugUsagePrinter.php | 75 +++++++++++++++++++---- tests/Rule/DeadCodeRuleTest.php | 2 + tests/Rule/data/debug/expected_output.txt | 10 +++ tests/Rule/data/debug/property.php | 13 ++++ 4 files changed, 89 insertions(+), 11 deletions(-) create mode 100644 tests/Rule/data/debug/property.php diff --git a/src/Debug/DebugUsagePrinter.php b/src/Debug/DebugUsagePrinter.php index 0517e86b..3719b3ef 100644 --- a/src/Debug/DebugUsagePrinter.php +++ b/src/Debug/DebugUsagePrinter.php @@ -12,6 +12,7 @@ use ShipMonk\PHPStan\DeadCode\Graph\ClassConstantRef; use ShipMonk\PHPStan\DeadCode\Graph\ClassMemberUsage; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodRef; +use ShipMonk\PHPStan\DeadCode\Graph\ClassPropertyRef; use ShipMonk\PHPStan\DeadCode\Graph\CollectedUsage; use ShipMonk\PHPStan\DeadCode\Output\OutputEnhancer; use ShipMonk\PHPStan\DeadCode\Reflection\ReflectionHelper; @@ -20,11 +21,13 @@ use function count; use function explode; use function implode; +use function in_array; use function ltrim; use function next; use function reset; use function sprintf; use function str_repeat; +use function strlen; use function strpos; use function substr; @@ -115,8 +118,8 @@ private function printMixedClassNameUsages( foreach ($mixedMemberUsages as $memberType => $collectedUsages) { foreach ($collectedUsages as $memberName => $usages) { $examplesShown++; - $memberAccessString = $memberType === MemberType::METHOD ? 'method' : 'constant'; - $output->writeFormatted(sprintf(' • %s %s', $memberName, $memberAccessString)); + $memberTypeString = $this->getMemberTypeString($memberType); + $output->writeFormatted(sprintf(' • %s %s', $memberName, $memberTypeString)); $exampleCaller = $this->getExampleCaller($usages); $output->writeFormatted(sprintf(', for example in %s', $exampleCaller)); @@ -152,17 +155,27 @@ private function printMixedEverythingUsages( foreach ($fullyMixedUsages as $memberType => $collectedUsages) { $fullyMixedCount = count($collectedUsages); + $memberTypeString = $this->getMemberTypeString($memberType); + + if ($memberType === MemberType::METHOD) { + $memberAccessString = 'call'; + $memberAccessPastTense = 'called'; + } elseif ($memberType === MemberType::PROPERTY) { + $memberAccessString = 'read'; + $memberAccessPastTense = 'read'; + } else { + $memberAccessString = 'fetch'; + $memberAccessPastTense = 'fetched'; + } - $memberTypeString = $memberType === MemberType::METHOD ? 'method' : 'constant'; - $memberAccessString = $memberType === MemberType::METHOD ? 'call' : 'fetch'; - $fullyMixedPlural = $fullyMixedCount > 1 ? ($memberType === MemberType::METHOD ? 's' : 'es') : ''; - $output->writeLineFormatted(sprintf('Found %d UNKNOWN %s%s over UNKNOWN type!!', $fullyMixedCount, $memberAccessString, $fullyMixedPlural)); + $output->writeLineFormatted(sprintf('Found %d UNKNOWN %s over UNKNOWN type!!', $fullyMixedCount, $this->pluralize($fullyMixedCount, $memberAccessString))); foreach ($collectedUsages as $usages) { $output->writeLineFormatted( sprintf( - ' • %s in %s', - $memberType === MemberType::METHOD ? 'method call' : 'constant fetch', + ' • %s %s in %s', + $memberTypeString, + $memberAccessString, $this->getExampleCaller([$usages]), ), ); @@ -170,9 +183,9 @@ private function printMixedEverythingUsages( $output->writeLineFormatted(''); $output->writeLineFormatted(sprintf( - 'Such usages basically break whole dead code analysis, because any %s on any class can be %sed there!', + 'Such usages basically break whole dead code analysis, because any %s on any class can be %s there!', $memberTypeString, - $memberAccessString, + $memberAccessPastTense, )); $output->writeLineFormatted('All those usages were ignored!'); $output->writeLineFormatted(''); @@ -285,6 +298,7 @@ private function prettyMemberKey(string $memberKey): string strpos($memberKey, 'm/') === false && strpos($memberKey, 'c/') === false && strpos($memberKey, 'e/') === false + && strpos($memberKey, 'p/') === false ) { throw new LogicException("Invalid member key format: '$memberKey'"); } @@ -369,6 +383,7 @@ private function buildDebugMemberKeys(array $debugMembers): array [$class, $memberName] = explode('::', $debugMember); // @phpstan-ignore offsetAccess.notFound $normalizedClass = ltrim($class, '\\'); + $memberName = ltrim($memberName, '$'); if (!$this->reflectionProvider->hasClass($normalizedClass)) { throw new LogicException("Class '$normalizedClass' does not exist"); @@ -386,7 +401,7 @@ private function buildDebugMemberKeys(array $debugMembers): array $keys = (new ClassConstantRef($normalizedClass, $memberName, false, TrinaryLogic::createYes()))->toKeys(); } elseif (ReflectionHelper::hasOwnProperty($classReflection, $memberName)) { - throw new LogicException("Cannot debug '$debugMember', properties are not supported yet"); + $keys = (new ClassPropertyRef($normalizedClass, $memberName, false))->toKeys(); } else { throw new LogicException("Member '$memberName' does not exist directly in '$normalizedClass'"); @@ -427,6 +442,44 @@ private function getUsageWord(int $memberType): string return 'calls'; } elseif ($memberType === MemberType::CONSTANT) { return 'fetches'; + } elseif ($memberType === MemberType::PROPERTY) { + return 'reads'; + } else { + throw new LogicException("Unsupported member type: $memberType"); + } + } + + private function pluralize( + int $count, + string $singular + ): string + { + if ($count === 1) { + return $singular; + } + + if (substr($singular, -1) === 's' || substr($singular, -1) === 'x' || substr($singular, -2) === 'sh' || substr($singular, -2) === 'ch') { + return $singular . 'es'; + } + + if (substr($singular, -1) === 'y' && !in_array($singular[strlen($singular) - 2], ['a', 'e', 'i', 'o', 'u'], true)) { + return substr($singular, 0, -1) . 'ies'; + } + + return $singular . 's'; + } + + /** + * @param MemberType::* $memberType + */ + private function getMemberTypeString(int $memberType): string + { + if ($memberType === MemberType::METHOD) { + return 'method'; + } elseif ($memberType === MemberType::CONSTANT) { + return 'constant'; + } elseif ($memberType === MemberType::PROPERTY) { + return 'property'; } else { throw new LogicException("Unsupported member type: $memberType"); } diff --git a/tests/Rule/DeadCodeRuleTest.php b/tests/Rule/DeadCodeRuleTest.php index 3e412e7c..9e693d5a 100644 --- a/tests/Rule/DeadCodeRuleTest.php +++ b/tests/Rule/DeadCodeRuleTest.php @@ -340,6 +340,7 @@ public function testDebugUsage(): void 'DebugRegular\Another::call', 'DebugUnsupported\Foo::notDead', 'DebugZero\Foo::__construct', + 'DebugProperty\Foo::prop', ]; $this->analyzeFiles([ __DIR__ . '/data/debug/alternative.php', @@ -353,6 +354,7 @@ public function testDebugUsage(): void __DIR__ . '/data/debug/mixed.php', __DIR__ . '/data/debug/mixed-member.php', __DIR__ . '/data/debug/never.php', + __DIR__ . '/data/debug/property.php', __DIR__ . '/data/debug/reflection.php', __DIR__ . '/data/debug/regular.php', __DIR__ . '/data/debug/unsupported.php', diff --git a/tests/Rule/data/debug/expected_output.txt b/tests/Rule/data/debug/expected_output.txt index 9a1174cf..520420c8 100644 --- a/tests/Rule/data/debug/expected_output.txt +++ b/tests/Rule/data/debug/expected_output.txt @@ -151,3 +151,13 @@ DebugUnsupported\Foo::notDead DebugZero\Foo::__construct | | No usages found + + +DebugProperty\Foo::prop +| +| Marked as alive at: +| entry data/debug/property.php:12 +| reads DebugProperty\Foo::prop +| +| Found 1 usage: +| • data/debug/property.php:12 diff --git a/tests/Rule/data/debug/property.php b/tests/Rule/data/debug/property.php new file mode 100644 index 00000000..55476e0c --- /dev/null +++ b/tests/Rule/data/debug/property.php @@ -0,0 +1,13 @@ +prop; +}