From fe15befd99afca99567012b894392c497d8e7e51 Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Mon, 30 Dec 2024 17:33:49 +0100 Subject: [PATCH] Reflection: proper dead transitivity (#135) --- rules.neon | 3 ++ src/Collector/ConstantFetchCollector.php | 28 +++++-------------- src/Collector/MethodCallCollector.php | 35 ++++++++---------------- src/Graph/ClassMemberUsage.php | 3 ++ src/Graph/UsageOriginDetector.php | 31 +++++++++++++++++++++ src/Provider/ReflectionUsageProvider.php | 27 +++++++++++------- tests/Rule/DeadCodeRuleTest.php | 22 +++++++++++++-- tests/Rule/data/providers/reflection.php | 9 ++++++ 8 files changed, 101 insertions(+), 57 deletions(-) create mode 100644 src/Graph/UsageOriginDetector.php diff --git a/rules.neon b/rules.neon index 5739d61..503ab3a 100644 --- a/rules.neon +++ b/rules.neon @@ -7,6 +7,9 @@ services: - class: ShipMonk\PHPStan\DeadCode\Transformer\FileSystem + - + class: ShipMonk\PHPStan\DeadCode\Graph\UsageOriginDetector + - class: ShipMonk\PHPStan\DeadCode\Provider\VendorUsageProvider tags: diff --git a/src/Collector/ConstantFetchCollector.php b/src/Collector/ConstantFetchCollector.php index 4d5600d..780a6b5 100644 --- a/src/Collector/ConstantFetchCollector.php +++ b/src/Collector/ConstantFetchCollector.php @@ -10,14 +10,13 @@ use PhpParser\Node\Name; use PHPStan\Analyser\Scope; use PHPStan\Collectors\Collector; -use PHPStan\Reflection\MethodReflection; use PHPStan\Reflection\ReflectionProvider; use PHPStan\Type\Constant\ConstantStringType; use PHPStan\Type\Type; use PHPStan\Type\TypeUtils; use ShipMonk\PHPStan\DeadCode\Graph\ClassConstantRef; use ShipMonk\PHPStan\DeadCode\Graph\ClassConstantUsage; -use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodRef; +use ShipMonk\PHPStan\DeadCode\Graph\UsageOriginDetector; use function array_map; use function count; use function current; @@ -32,17 +31,21 @@ class ConstantFetchCollector implements Collector use BufferedUsageCollector; + private UsageOriginDetector $usageOriginDetector; + private ReflectionProvider $reflectionProvider; private bool $trackMixedAccess; public function __construct( + UsageOriginDetector $usageOriginDetector, ReflectionProvider $reflectionProvider, bool $trackMixedAccess ) { $this->reflectionProvider = $reflectionProvider; $this->trackMixedAccess = $trackMixedAccess; + $this->usageOriginDetector = $usageOriginDetector; } public function getNodeType(): string @@ -109,7 +112,7 @@ private function registerFunctionCall(FuncCall $node, Scope $scope): void } $this->usageBuffer[] = new ClassConstantUsage( - $this->getCaller($scope), + $this->usageOriginDetector->detectOrigin($scope), new ClassConstantRef($className, $constantName, true), ); } @@ -137,7 +140,7 @@ private function registerFetch(ClassConstFetch $node, Scope $scope): void foreach ($this->getDeclaringTypesWithConstant($ownerType, $constantName) as $className) { $this->usageBuffer[] = new ClassConstantUsage( - $this->getCaller($scope), + $this->usageOriginDetector->detectOrigin($scope), new ClassConstantRef($className, $constantName, $possibleDescendantFetch), ); } @@ -173,21 +176,4 @@ private function getDeclaringTypesWithConstant( return $result; } - private function getCaller(Scope $scope): ?ClassMethodRef - { - if (!$scope->isInClass()) { - return null; - } - - if (!$scope->getFunction() instanceof MethodReflection) { - return null; - } - - return new ClassMethodRef( - $scope->getClassReflection()->getName(), - $scope->getFunction()->getName(), - false, - ); - } - } diff --git a/src/Collector/MethodCallCollector.php b/src/Collector/MethodCallCollector.php index e4d2ce9..2750f65 100644 --- a/src/Collector/MethodCallCollector.php +++ b/src/Collector/MethodCallCollector.php @@ -17,13 +17,13 @@ use PHPStan\Collectors\Collector; use PHPStan\Node\MethodCallableNode; use PHPStan\Node\StaticMethodCallableNode; -use PHPStan\Reflection\MethodReflection; use PHPStan\TrinaryLogic; use PHPStan\Type\Type; use PHPStan\Type\TypeCombinator; use PHPStan\Type\TypeUtils; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodRef; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodUsage; +use ShipMonk\PHPStan\DeadCode\Graph\UsageOriginDetector; /** * @implements Collector> @@ -33,10 +33,16 @@ class MethodCallCollector implements Collector use BufferedUsageCollector; + private UsageOriginDetector $usageOriginDetector; + private bool $trackMixedAccess; - public function __construct(bool $trackMixedAccess) + public function __construct( + UsageOriginDetector $usageOriginDetector, + bool $trackMixedAccess + ) { + $this->usageOriginDetector = $usageOriginDetector; $this->trackMixedAccess = $trackMixedAccess; } @@ -114,7 +120,7 @@ private function registerMethodCall( foreach ($methodNames as $methodName) { foreach ($this->getDeclaringTypesWithMethod($scope, $callerType, $methodName, TrinaryLogic::createNo()) as $className) { $this->usageBuffer[] = new ClassMethodUsage( - $this->getCaller($scope), + $this->usageOriginDetector->detectOrigin($scope), new ClassMethodRef($className, $methodName, $possibleDescendantCall), ); } @@ -140,7 +146,7 @@ private function registerStaticCall( foreach ($methodNames as $methodName) { foreach ($this->getDeclaringTypesWithMethod($scope, $callerType, $methodName, TrinaryLogic::createYes()) as $className) { $this->usageBuffer[] = new ClassMethodUsage( - $this->getCaller($scope), + $this->usageOriginDetector->detectOrigin($scope), new ClassMethodRef($className, $methodName, $possibleDescendantCall), ); } @@ -165,7 +171,7 @@ private function registerArrayCallable( foreach ($this->getDeclaringTypesWithMethod($scope, $caller, $methodName, TrinaryLogic::createMaybe()) as $className) { $this->usageBuffer[] = new ClassMethodUsage( - $this->getCaller($scope), + $this->usageOriginDetector->detectOrigin($scope), new ClassMethodRef($className, $methodName, $possibleDescendantCall), ); } @@ -189,7 +195,7 @@ private function registerClone(Clone_ $node, Scope $scope): void foreach ($this->getDeclaringTypesWithMethod($scope, $callerType, $methodName, TrinaryLogic::createNo()) as $className) { $this->usageBuffer[] = new ClassMethodUsage( - $this->getCaller($scope), + $this->usageOriginDetector->detectOrigin($scope), new ClassMethodRef($className, $methodName, true), ); } @@ -255,21 +261,4 @@ private function getDeclaringTypesWithMethod( return $result; } - private function getCaller(Scope $scope): ?ClassMethodRef - { - if (!$scope->isInClass()) { - return null; - } - - if (!$scope->getFunction() instanceof MethodReflection) { - return null; - } - - return new ClassMethodRef( - $scope->getClassReflection()->getName(), - $scope->getFunction()->getName(), - false, - ); - } - } diff --git a/src/Graph/ClassMemberUsage.php b/src/Graph/ClassMemberUsage.php index e6581ef..9cae65b 100644 --- a/src/Graph/ClassMemberUsage.php +++ b/src/Graph/ClassMemberUsage.php @@ -17,6 +17,9 @@ abstract class ClassMemberUsage /** * Origin method of the usage, "where it was called from" + * This is required for proper transitive dead code detection. + * + * @see UsageOriginDetector for typical usage */ private ?ClassMethodRef $origin; diff --git a/src/Graph/UsageOriginDetector.php b/src/Graph/UsageOriginDetector.php new file mode 100644 index 0000000..0391125 --- /dev/null +++ b/src/Graph/UsageOriginDetector.php @@ -0,0 +1,31 @@ +isInClass()) { + return null; + } + + if (!$scope->getFunction() instanceof MethodReflection) { + return null; + } + + return new ClassMethodRef( + $scope->getClassReflection()->getName(), + $scope->getFunction()->getName(), + false, + ); + } + +} diff --git a/src/Provider/ReflectionUsageProvider.php b/src/Provider/ReflectionUsageProvider.php index 94dbb23..3f279aa 100644 --- a/src/Provider/ReflectionUsageProvider.php +++ b/src/Provider/ReflectionUsageProvider.php @@ -18,6 +18,7 @@ use ShipMonk\PHPStan\DeadCode\Graph\ClassMemberUsage; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodRef; use ShipMonk\PHPStan\DeadCode\Graph\ClassMethodUsage; +use ShipMonk\PHPStan\DeadCode\Graph\UsageOriginDetector; use function array_key_first; use function count; use function in_array; @@ -25,10 +26,16 @@ class ReflectionUsageProvider implements MemberUsageProvider { + private UsageOriginDetector $usageOriginDetector; + private bool $enabled; - public function __construct(bool $enabled) + public function __construct( + UsageOriginDetector $usageOriginDetector, + bool $enabled + ) { + $this->usageOriginDetector = $usageOriginDetector; $this->enabled = $enabled; } @@ -98,7 +105,7 @@ private function extractConstantsUsedByReflection( if ($methodName === 'getConstants' || $methodName === 'getReflectionConstants') { foreach ($genericReflection->getNativeReflection()->getReflectionConstants() as $reflectionConstant) { - $usedConstants[] = $this->createConstantUsage($reflectionConstant->getDeclaringClass()->getName(), $reflectionConstant->getName()); + $usedConstants[] = $this->createConstantUsage($scope, $reflectionConstant->getDeclaringClass()->getName(), $reflectionConstant->getName()); } } @@ -106,7 +113,7 @@ private function extractConstantsUsedByReflection( $firstArg = $args[array_key_first($args)]; // @phpstan-ignore offsetAccess.notFound foreach ($scope->getType($firstArg->value)->getConstantStrings() as $constantString) { - $usedConstants[] = $this->createConstantUsage($genericReflection->getName(), $constantString->getValue()); + $usedConstants[] = $this->createConstantUsage($scope, $genericReflection->getName(), $constantString->getValue()); } } @@ -128,7 +135,7 @@ private function extractMethodsUsedByReflection( if ($methodName === 'getMethods') { foreach ($genericReflection->getNativeReflection()->getMethods() as $reflectionMethod) { - $usedMethods[] = $this->createMethodUsage($reflectionMethod->getDeclaringClass()->getName(), $reflectionMethod->getName()); + $usedMethods[] = $this->createMethodUsage($scope, $reflectionMethod->getDeclaringClass()->getName(), $reflectionMethod->getName()); } } @@ -136,7 +143,7 @@ private function extractMethodsUsedByReflection( $firstArg = $args[array_key_first($args)]; // @phpstan-ignore offsetAccess.notFound foreach ($scope->getType($firstArg->value)->getConstantStrings() as $constantString) { - $usedMethods[] = $this->createMethodUsage($genericReflection->getName(), $constantString->getValue()); + $usedMethods[] = $this->createMethodUsage($scope, $genericReflection->getName(), $constantString->getValue()); } } @@ -144,7 +151,7 @@ private function extractMethodsUsedByReflection( $constructor = $genericReflection->getNativeReflection()->getConstructor(); if ($constructor !== null) { - $usedMethods[] = $this->createMethodUsage($constructor->getDeclaringClass()->getName(), '__construct'); + $usedMethods[] = $this->createMethodUsage($scope, $constructor->getDeclaringClass()->getName(), '__construct'); } } @@ -174,10 +181,10 @@ private function getMethodNames(CallLike $call, Scope $scope): array return [$call->name->toString()]; } - private function createConstantUsage(string $className, string $constantName): ClassConstantUsage + private function createConstantUsage(Scope $scope, string $className, string $constantName): ClassConstantUsage { return new ClassConstantUsage( - null, + $this->usageOriginDetector->detectOrigin($scope), new ClassConstantRef( $className, $constantName, @@ -186,10 +193,10 @@ private function createConstantUsage(string $className, string $constantName): C ); } - private function createMethodUsage(string $className, string $methodName): ClassMethodUsage + private function createMethodUsage(Scope $scope, string $className, string $methodName): ClassMethodUsage { return new ClassMethodUsage( - null, + $this->usageOriginDetector->detectOrigin($scope), new ClassMethodRef( $className, $methodName, diff --git a/tests/Rule/DeadCodeRuleTest.php b/tests/Rule/DeadCodeRuleTest.php index 7405f27..358086c 100644 --- a/tests/Rule/DeadCodeRuleTest.php +++ b/tests/Rule/DeadCodeRuleTest.php @@ -19,6 +19,7 @@ use ShipMonk\PHPStan\DeadCode\Collector\ProvidedUsagesCollector; use ShipMonk\PHPStan\DeadCode\Compatibility\BackwardCompatibilityChecker; use ShipMonk\PHPStan\DeadCode\Formatter\RemoveDeadCodeFormatter; +use ShipMonk\PHPStan\DeadCode\Graph\UsageOriginDetector; use ShipMonk\PHPStan\DeadCode\Hierarchy\ClassHierarchy; use ShipMonk\PHPStan\DeadCode\Provider\DoctrineUsageProvider; use ShipMonk\PHPStan\DeadCode\Provider\MemberUsageProvider; @@ -76,8 +77,8 @@ protected function getCollectors(): array $this->getMemberUsageProviders(), ), new ClassDefinitionCollector(), - new MethodCallCollector($this->trackMixedAccess), - new ConstantFetchCollector($reflectionProvider, $this->trackMixedAccess), + new MethodCallCollector($this->createUsageOriginDetector(), $this->trackMixedAccess), + new ConstantFetchCollector($this->createUsageOriginDetector(), $reflectionProvider, $this->trackMixedAccess), ]; } @@ -368,7 +369,10 @@ private function getTransformedFilePath(string $file): string private function getMemberUsageProviders(): array { return [ - new ReflectionUsageProvider(true), + new ReflectionUsageProvider( + $this->createUsageOriginDetector(), + true, + ), new class extends ReflectionBasedMemberUsageProvider { @@ -421,6 +425,18 @@ static function (string $type): array { return $mock; } + private function createUsageOriginDetector(): UsageOriginDetector + { + /** @var UsageOriginDetector|null $detector */ + static $detector = null; + + if ($detector === null) { + $detector = new UsageOriginDetector(); + } + + return $detector; + } + public function gatherAnalyserErrors(array $files): array { if (!$this->unwrapGroupedErrors) { diff --git a/tests/Rule/data/providers/reflection.php b/tests/Rule/data/providers/reflection.php index 76fe99b..a9209e1 100644 --- a/tests/Rule/data/providers/reflection.php +++ b/tests/Rule/data/providers/reflection.php @@ -57,6 +57,15 @@ enum EnumHolder1 { public function used() {} } +enum TransitiveHolder { + const TRANSITIVELY_DEAD = 1; // error: Unused Reflection\TransitiveHolder::TRANSITIVELY_DEAD + + public function test() // error: Unused Reflection\TransitiveHolder::test + { + (new \ReflectionClass(self::class))->getConstant('TRANSITIVELY_DEAD'); + } +} + $reflection1 = new \ReflectionClass(Holder1::class); $reflection1->getConstants(); $reflection1->getMethod('used');