diff --git a/README.md b/README.md index e35a0b3a..d71fae11 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,32 @@ class ApiOutputEntrypointProvider extends SimpleMethodEntrypointProvider } ``` +## Dead cycles & transitively dead methods +- This library automatically detects dead cycles and transitively dead methods (methods that are only called from dead methods) +- By default, it reports only the first dead method in the subtree and the rest as a tip: + +``` + ------ ------------------------------------------------------------------------ + Line src/App/Facade/UserFacade.php + ------ ------------------------------------------------------------------------ + 26 Unused App\Facade\UserFacade::updateUserAddress + 🪪 shipmonk.deadMethod + 💡 Thus App\Entity\User::updateAddress is transitively also unused + 💡 Thus App\Entity\Address::setPostalCode is transitively also unused + 💡 Thus App\Entity\Address::setCountry is transitively also unused + 💡 Thus App\Entity\Address::setStreet is transitively also unused + 💡 Thus App\Entity\Address::setZip is transitively also unused + ------ ------------------------------------------------------------------------ +``` + +- If you want to report all dead methods individually, you can enable it in your `phpstan.neon.dist`: + +```neon +parameters: + shipmonkDeadCode: + reportTransitivelyDeadMethodAsSeparateError: true +``` + ## Comparison with tomasvotruba/unused-public - You can see [detailed comparison PR](https://github.com/shipmonk-rnd/dead-code-detector/pull/53) - Basically, their analysis is less precise and less flexible. Mainly: @@ -104,11 +130,8 @@ class ApiOutputEntrypointProvider extends SimpleMethodEntrypointProvider - Only method calls are detected so far - Including **constructors**, static methods, trait methods, interface methods, first class callables, clone, etc. - Any calls on mixed types are not detected, e.g. `$unknownClass->method()` - - Anonymous classes are ignored ([PHPStan limitation](https://github.com/phpstan/phpstan/issues/8410)) - - Does not check most magic methods (`__get`, `__set` etc) - - Call-graph not implemented so far - - No transitive check is performed (dead method called only from dead method) - - No dead cycles are detected (e.g. dead method calling itself) + - Methods of anonymous classes are never reported as dead ([PHPStan limitation](https://github.com/phpstan/phpstan/issues/8410)) + - Most magic methods (e.g. `__get`, `__set` etc) are never reported as dead ## Contributing - Check your code by `composer check` diff --git a/rules.neon b/rules.neon index 81646868..634276fa 100644 --- a/rules.neon +++ b/rules.neon @@ -64,10 +64,13 @@ services: class: ShipMonk\PHPStan\DeadCode\Rule\DeadMethodRule tags: - phpstan.rules.rule + arguments: + reportTransitivelyDeadMethodAsSeparateError: %shipmonkDeadCode.reportTransitivelyDeadMethodAsSeparateError% parameters: shipmonkDeadCode: + reportTransitivelyDeadMethodAsSeparateError: false entrypoints: vendor: enabled: true @@ -84,6 +87,7 @@ parameters: parametersSchema: shipmonkDeadCode: structure([ + reportTransitivelyDeadMethodAsSeparateError: bool() entrypoints: structure([ vendor: structure([ enabled: bool() diff --git a/src/Collector/ClassDefinitionCollector.php b/src/Collector/ClassDefinitionCollector.php index cffff98e..d34bef57 100644 --- a/src/Collector/ClassDefinitionCollector.php +++ b/src/Collector/ClassDefinitionCollector.php @@ -7,7 +7,6 @@ use PhpParser\Node\Name; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassLike; -use PhpParser\Node\Stmt\ClassMethod; use PhpParser\Node\Stmt\Enum_; use PhpParser\Node\Stmt\Interface_; use PhpParser\Node\Stmt\Trait_; @@ -16,15 +15,15 @@ use PHPStan\Analyser\Scope; use PHPStan\Collectors\Collector; use ShipMonk\PHPStan\DeadCode\Crate\Kind; +use ShipMonk\PHPStan\DeadCode\Crate\Visibility; use function array_fill_keys; use function array_map; -use function strpos; /** * @implements Collector, + * methods: array}>, * parents: array, * traits: array, aliases?: array}>, * interfaces: array, @@ -43,7 +42,7 @@ public function getNodeType(): string * @return array{ * kind: string, * name: string, - * methods: array, + * methods: array}>, * parents: array, * traits: array, aliases?: array}>, * interfaces: array, @@ -64,13 +63,10 @@ public function processNode( $methods = []; foreach ($node->getMethods() as $method) { - if ($this->isUnsupportedMethod($node, $method)) { - continue; - } - $methods[$method->name->toString()] = [ 'line' => $method->getStartLine(), 'abstract' => $method->isAbstract() || $node instanceof Interface_, + 'visibility' => $method->flags & (Visibility::PUBLIC | Visibility::PROTECTED | Visibility::PRIVATE), ]; } @@ -163,35 +159,6 @@ private function getTraits(ClassLike $node): array return $traits; } - private function isUnsupportedMethod(ClassLike $class, ClassMethod $method): bool - { - $methodName = $method->name->toString(); - - if ($methodName === '__destruct') { - return true; - } - - if ( - strpos($methodName, '__') === 0 - && $methodName !== '__construct' - && $methodName !== '__clone' - ) { - return true; // magic methods like __toString, __get, __set etc - } - - if ($methodName === '__construct' && $method->isPrivate()) { // e.g. classes with "denied" instantiation - return true; - } - - // abstract methods in traits make sense (not dead) only when called within the trait itself, but that is hard to detect for now, so lets ignore them completely - // the difference from interface methods (or abstract methods) is that those methods can be called over the interface, but you cannot call method over trait - if ($class instanceof Trait_ && $method->isAbstract()) { - return true; - } - - return false; - } - private function getKind(ClassLike $node): string { if ($node instanceof Class_) { diff --git a/src/Collector/EntrypointCollector.php b/src/Collector/EntrypointCollector.php index 80ac8ed3..70b94b77 100644 --- a/src/Collector/EntrypointCollector.php +++ b/src/Collector/EntrypointCollector.php @@ -7,6 +7,7 @@ use PHPStan\Collectors\Collector; use PHPStan\Node\InClassNode; use ShipMonk\PHPStan\DeadCode\Crate\Call; +use ShipMonk\PHPStan\DeadCode\Crate\Method; use ShipMonk\PHPStan\DeadCode\Provider\MethodEntrypointProvider; /** @@ -48,7 +49,12 @@ public function processNode( foreach ($this->entrypointProviders as $entrypointProvider) { foreach ($entrypointProvider->getEntrypoints($node->getClassReflection()) as $entrypointMethod) { - $entrypoints[] = (new Call($entrypointMethod->getDeclaringClass()->getName(), $entrypointMethod->getName(), false))->toString(); + $call = new Call( + null, + new Method($entrypointMethod->getDeclaringClass()->getName(), $entrypointMethod->getName()), + false, + ); + $entrypoints[] = $call->toString(); } } diff --git a/src/Collector/MethodCallCollector.php b/src/Collector/MethodCallCollector.php index df35715c..14290df1 100644 --- a/src/Collector/MethodCallCollector.php +++ b/src/Collector/MethodCallCollector.php @@ -19,9 +19,11 @@ use PHPStan\Node\MethodCallableNode; use PHPStan\Node\StaticMethodCallableNode; use PHPStan\Reflection\ClassReflection; +use PHPStan\Reflection\MethodReflection; use PHPStan\Type\Type; use PHPStan\Type\TypeCombinator; use ShipMonk\PHPStan\DeadCode\Crate\Call; +use ShipMonk\PHPStan\DeadCode\Crate\Method; use function array_map; /** @@ -126,7 +128,11 @@ private function registerMethodCall( } $className = $classWithMethod->getMethod($methodName, $scope)->getDeclaringClass()->getName(); - $this->callsBuffer[] = new Call($className, $methodName, $possibleDescendantCall); + $this->callsBuffer[] = new Call( + $this->getCaller($scope), + new Method($className, $methodName), + $possibleDescendantCall, + ); } } } @@ -154,7 +160,11 @@ private function registerStaticCall( } $className = $classReflection->getMethod($methodName, $scope)->getDeclaringClass()->getName(); - $this->callsBuffer[] = new Call($className, $methodName, $possibleDescendantCall); + $this->callsBuffer[] = new Call( + $this->getCaller($scope), + new Method($className, $methodName), + $possibleDescendantCall, + ); } } } @@ -177,7 +187,11 @@ private function registerArrayCallable( foreach ($this->getReflectionsWithMethod($caller, $methodName) as $classWithMethod) { $className = $classWithMethod->getMethod($methodName, $scope)->getDeclaringClass()->getName(); - $this->callsBuffer[] = new Call($className, $methodName, $possibleDescendantCall); + $this->callsBuffer[] = new Call( + $this->getCaller($scope), + new Method($className, $methodName), + $possibleDescendantCall, + ); } } } @@ -186,7 +200,11 @@ private function registerArrayCallable( private function registerAttribute(Attribute $node, Scope $scope): void { - $this->callsBuffer[] = new Call($scope->resolveName($node->name), '__construct', false); + $this->callsBuffer[] = new Call( + null, + new Method($scope->resolveName($node->name), '__construct'), + false, + ); } private function registerClone(Clone_ $node, Scope $scope): void @@ -196,7 +214,11 @@ private function registerClone(Clone_ $node, Scope $scope): void foreach ($this->getReflectionsWithMethod($callerType, $methodName) as $classWithMethod) { $className = $classWithMethod->getMethod($methodName, $scope)->getDeclaringClass()->getName(); - $this->callsBuffer[] = new Call($className, $methodName, true); + $this->callsBuffer[] = new Call( + $this->getCaller($scope), + new Method($className, $methodName), + true, + ); } } @@ -239,4 +261,17 @@ private function getReflectionsWithMethod(Type $type, string $methodName): itera } } + private function getCaller(Scope $scope): ?Method + { + if (!$scope->isInClass()) { + return null; + } + + if (!$scope->getFunction() instanceof MethodReflection) { + return null; + } + + return new Method($scope->getClassReflection()->getName(), $scope->getFunction()->getName()); + } + } diff --git a/src/Crate/Call.php b/src/Crate/Call.php index bd33bc80..38f8fcf7 100644 --- a/src/Crate/Call.php +++ b/src/Crate/Call.php @@ -7,43 +7,81 @@ use function explode; /** - * @readonly + * @immutable */ class Call { - public string $className; + public ?Method $caller; - public string $methodName; + public Method $callee; public bool $possibleDescendantCall; public function __construct( - string $className, - string $methodName, + ?Method $caller, + Method $callee, bool $possibleDescendantCall ) { - $this->className = $className; - $this->methodName = $methodName; + $this->caller = $caller; + $this->callee = $callee; $this->possibleDescendantCall = $possibleDescendantCall; } public function toString(): string { - return "{$this->className}::{$this->methodName}::" . ($this->possibleDescendantCall ? '1' : ''); + $callerRef = $this->caller === null ? '' : "{$this->caller->className}::{$this->caller->methodName}"; + $calleeRef = "{$this->callee->className}::{$this->callee->methodName}"; + + return "{$callerRef}->$calleeRef;" . ($this->possibleDescendantCall ? '1' : ''); } - public static function fromString(string $methodKey): self + public static function fromString(string $callKey): self { - $exploded = explode('::', $methodKey); + $split1 = explode(';', $callKey); + + if (count($split1) !== 2) { + throw new LogicException("Invalid method key: $callKey"); + } + + [$edgeKey, $possibleDescendantCall] = $split1; + + $split2 = explode('->', $edgeKey); + + if (count($split2) !== 2) { + throw new LogicException("Invalid method key: $callKey"); + } + + [$callerKey, $calleeKey] = $split2; + + $calleeSplit = explode('::', $calleeKey); + + if (count($calleeSplit) !== 2) { + throw new LogicException("Invalid method key: $callKey"); + } + + [$calleeClassName, $calleeMethodName] = $calleeSplit; + $callee = new Method($calleeClassName, $calleeMethodName); + + if ($callerKey === '') { + $caller = null; + } else { + $callerSplit = explode('::', $callerKey); + + if (count($callerSplit) !== 2) { + throw new LogicException("Invalid method key: $callKey"); + } - if (count($exploded) !== 3) { - throw new LogicException("Invalid method key: $methodKey"); + [$callerClassName, $callerMethodName] = $callerSplit; + $caller = new Method($callerClassName, $callerMethodName); } - [$className, $methodName, $possibleDescendantCall] = $exploded; - return new self($className, $methodName, $possibleDescendantCall === '1'); + return new self( + $caller, + $callee, + $possibleDescendantCall === '1', + ); } } diff --git a/src/Crate/Method.php b/src/Crate/Method.php new file mode 100644 index 00000000..9e06b488 --- /dev/null +++ b/src/Crate/Method.php @@ -0,0 +1,29 @@ +className = $className; + $this->methodName = $methodName; + } + + public function toString(): string + { + return $this->className . '::' . $this->methodName; + } + +} diff --git a/src/Crate/Visibility.php b/src/Crate/Visibility.php new file mode 100644 index 00000000..17d43447 --- /dev/null +++ b/src/Crate/Visibility.php @@ -0,0 +1,12 @@ +declaringTraits[$definition] ?? null; + return $this->declaringTraits[$methodKey] ?? null; } } diff --git a/src/Rule/DeadMethodRule.php b/src/Rule/DeadMethodRule.php index 3d00c389..86ea626e 100644 --- a/src/Rule/DeadMethodRule.php +++ b/src/Rule/DeadMethodRule.php @@ -12,9 +12,14 @@ use ShipMonk\PHPStan\DeadCode\Collector\EntrypointCollector; use ShipMonk\PHPStan\DeadCode\Collector\MethodCallCollector; use ShipMonk\PHPStan\DeadCode\Crate\Call; +use ShipMonk\PHPStan\DeadCode\Crate\Kind; +use ShipMonk\PHPStan\DeadCode\Crate\Method; +use ShipMonk\PHPStan\DeadCode\Crate\Visibility; use ShipMonk\PHPStan\DeadCode\Hierarchy\ClassHierarchy; +use function array_key_exists; use function array_keys; use function array_merge; +use function explode; use function in_array; use function strpos; @@ -24,6 +29,24 @@ class DeadMethodRule implements Rule { + private const UNSUPPORTED_MAGIC_METHODS = [ + '__invoke' => null, + '__toString' => null, + '__destruct' => null, + '__call' => null, + '__callStatic' => null, + '__get' => null, + '__set' => null, + '__isset' => null, + '__unset' => null, + '__sleep' => null, + '__wakeup' => null, + '__serialize' => null, + '__unserialize' => null, + '__set_state' => null, + '__debugInfo' => null, + ]; + private ClassHierarchy $classHierarchy; /** @@ -33,7 +56,7 @@ class DeadMethodRule implements Rule * kind: string, * name: string, * file: string, - * methods: array, + * methods: array}>, * parents: array, * traits: array, aliases?: array}>, * interfaces: array @@ -44,13 +67,27 @@ class DeadMethodRule implements Rule /** * @var array> */ - private array $methodsToMarkAsUsedCache = []; + private array $methodAlternativesCache = []; + + private bool $reportTransitivelyDeadMethodAsSeparateError; + + /** + * @var array methodKey => [file, line] + */ + private array $blackMethods = []; + + /** + * @var array> caller => callee[] + */ + private array $callGraph = []; public function __construct( - ClassHierarchy $classHierarchy + ClassHierarchy $classHierarchy, + bool $reportTransitivelyDeadMethodAsSeparateError = false ) { $this->classHierarchy = $classHierarchy; + $this->reportTransitivelyDeadMethodAsSeparateError = $reportTransitivelyDeadMethodAsSeparateError; } public function getNodeType(): string @@ -75,8 +112,6 @@ public function processNode( $methodCallData = $node->get(MethodCallCollector::class); $entrypointData = $node->get(EntrypointCollector::class); - $declaredMethods = []; - foreach ($methodDeclarationData as $file => $data) { foreach ($data as $typeData) { $typeName = $typeData['name']; @@ -103,23 +138,31 @@ public function processNode( foreach ($methods as $methodName => $methodData) { $definition = $this->getMethodKey($typeName, $methodName); - $declaredMethods[$definition] = [$file, $methodData['line']]; + $this->blackMethods[$definition] = [$file, $methodData['line']]; } } unset($methodDeclarationData); + $whiteCallees = []; + foreach ($methodCallData as $file => $callsInFile) { foreach ($callsInFile as $calls) { foreach ($calls as $callString) { $call = Call::fromString($callString); + $isWhite = $this->isConsideredWhite($call); - if ($this->isAnonymousClass($call)) { - continue; - } + $alternativeCalleeKeys = $this->getAlternativeMethodKeys($call->callee, $call->possibleDescendantCall); + $alternativeCallerKeys = $call->caller !== null ? $this->getAlternativeMethodKeys($call->caller, false) : []; - foreach ($this->getMethodsToMarkAsUsed($call) as $methodDefinitionToMarkAsUsed) { - unset($declaredMethods[$methodDefinitionToMarkAsUsed]); + foreach ($alternativeCalleeKeys as $alternativeCalleeKey) { + foreach ($alternativeCallerKeys as $alternativeCallerKey) { + $this->callGraph[$alternativeCallerKey][] = $alternativeCalleeKey; + } + + if ($isWhite) { + $whiteCallees[] = $alternativeCalleeKey; + } } } } @@ -127,22 +170,51 @@ public function processNode( unset($methodCallData); + foreach ($whiteCallees as $whiteCalleeKey) { + $this->markTransitiveCallsWhite($whiteCalleeKey); + } + foreach ($entrypointData as $file => $entrypointsInFile) { foreach ($entrypointsInFile as $entrypoints) { foreach ($entrypoints as $entrypoint) { $call = Call::fromString($entrypoint); - foreach ($this->getMethodsToMarkAsUsed($call) as $methodDefinition) { - unset($declaredMethods[$methodDefinition]); + foreach ($this->getAlternativeMethodKeys($call->callee, $call->possibleDescendantCall) as $alternativeCalleeKey) { + unset($this->blackMethods[$alternativeCalleeKey]); } + + $this->markTransitiveCallsWhite($call->callee->toString()); } } } + foreach ($this->blackMethods as $blackMethodKey => $_) { + if ($this->isNeverReportedAsDead($blackMethodKey)) { + unset($this->blackMethods[$blackMethodKey]); + } + } + $errors = []; - foreach ($declaredMethods as $definitionString => [$file, $line]) { - $errors[] = $this->buildError($definitionString, $file, $line); + if ($this->reportTransitivelyDeadMethodAsSeparateError) { + foreach ($this->blackMethods as $deadMethodKey => [$file, $line]) { + $errors[] = $this->buildError($deadMethodKey, [], $file, $line); + } + + return $errors; + } + + $deadGroups = $this->groupDeadMethods(); + + foreach ($deadGroups as $deadGroupKey => $deadSubgroupKeys) { + [$file, $line] = $this->blackMethods[$deadGroupKey]; // @phpstan-ignore offsetAccess.notFound + $subGroupMap = []; + + foreach ($deadSubgroupKeys as $deadSubgroupKey) { + $subGroupMap[$deadSubgroupKey] = $this->blackMethods[$deadSubgroupKey]; // @phpstan-ignore offsetAccess.notFound + } + + $errors[] = $this->buildError($deadGroupKey, $subGroupMap, $file, $line); } return $errors; @@ -197,54 +269,192 @@ private function fillClassHierarchy(string $typeName, array $ancestorNames): voi } } - private function isAnonymousClass(Call $call): bool + private function isAnonymousClass(string $className): bool { // https://github.com/phpstan/phpstan/issues/8410 workaround, ideally this should not be ignored - return strpos($call->className, 'AnonymousClass') === 0; + return strpos($className, 'AnonymousClass') === 0; } /** * @return list */ - private function getMethodsToMarkAsUsed(Call $call): array + private function getAlternativeMethodKeys(Method $method, bool $possibleDescendant): array { - if (isset($this->methodsToMarkAsUsedCache[$call->toString()])) { - return $this->methodsToMarkAsUsedCache[$call->toString()]; + $methodKey = $method->toString(); + $cacheKey = $methodKey . ';' . ($possibleDescendant ? '1' : '0'); + + if (isset($this->methodAlternativesCache[$cacheKey])) { + return $this->methodAlternativesCache[$cacheKey]; } - $result = [$this->getMethodKey($call->className, $call->methodName)]; + $result = [$methodKey]; - if ($call->possibleDescendantCall) { - foreach ($this->classHierarchy->getClassDescendants($call->className) as $descendantName) { - $result[] = $this->getMethodKey($descendantName, $call->methodName); + if ($possibleDescendant) { + foreach ($this->classHierarchy->getClassDescendants($method->className) as $descendantName) { + $result[] = $this->getMethodKey($descendantName, $method->methodName); } } // each descendant can be a trait user - foreach ($result as $methodDefinition) { - $traitMethodKey = $this->classHierarchy->getDeclaringTraitMethodKey($methodDefinition); + foreach ($result as $resultKey) { + $traitMethodKey = $this->classHierarchy->getDeclaringTraitMethodKey($resultKey); if ($traitMethodKey !== null) { $result[] = $traitMethodKey; } } - $this->methodsToMarkAsUsedCache[$call->toString()] = $result; + $this->methodAlternativesCache[$cacheKey] = $result; + + return $result; + } + + /** + * @param array $visitedKeys + */ + private function markTransitiveCallsWhite(string $callerKey, array $visitedKeys = []): void + { + $visitedKeys = $visitedKeys === [] ? [$callerKey => null] : $visitedKeys; + $calleeKeys = $this->callGraph[$callerKey] ?? []; + + unset($this->blackMethods[$callerKey]); + + foreach ($calleeKeys as $calleeKey) { + if (array_key_exists($calleeKey, $visitedKeys)) { + continue; + } + + if (!isset($this->blackMethods[$calleeKey])) { + continue; + } + + $this->markTransitiveCallsWhite($calleeKey, array_merge($visitedKeys, [$calleeKey => null])); + } + } + + /** + * @param array $visitedKeys + * @return list + */ + private function getTransitiveDeadCalls(string $callerKey, array $visitedKeys = []): array + { + $visitedKeys = $visitedKeys === [] ? [$callerKey => null] : $visitedKeys; + $calleeKeys = $this->callGraph[$callerKey] ?? []; + + $result = []; + + foreach ($calleeKeys as $calleeKey) { + if (array_key_exists($calleeKey, $visitedKeys)) { + continue; + } + + if (!isset($this->blackMethods[$calleeKey])) { + continue; + } + + $result[] = $calleeKey; + + foreach ($this->getTransitiveDeadCalls($calleeKey, array_merge($visitedKeys, [$calleeKey => null])) as $transitiveDead) { + $result[] = $transitiveDead; + } + } return $result; } + /** + * @return array> + */ + private function groupDeadMethods(): array + { + $deadGroups = []; + + /** @var array $deadMethodsWithCaller */ + $deadMethodsWithCaller = []; + + foreach ($this->callGraph as $caller => $callees) { + if (!array_key_exists($caller, $this->blackMethods)) { + continue; + } + + foreach ($callees as $callee) { + if (array_key_exists($callee, $this->blackMethods)) { + $deadMethodsWithCaller[$callee] = true; + } + } + } + + $methodsGrouped = []; + + foreach ($this->blackMethods as $deadMethodKey => $_) { + if (isset($methodsGrouped[$deadMethodKey])) { + continue; + } + + if (isset($deadMethodsWithCaller[$deadMethodKey])) { + continue; // has a caller, thus should be part of a group, not a group representative + } + + $deadGroups[$deadMethodKey] = []; + $methodsGrouped[$deadMethodKey] = true; + + $transitiveMethodKeys = $this->getTransitiveDeadCalls($deadMethodKey); + + foreach ($transitiveMethodKeys as $transitiveMethodKey) { + $deadGroups[$deadMethodKey][] = $transitiveMethodKey; + $methodsGrouped[$transitiveMethodKey] = true; + } + } + + // now only cycles remain, lets pick group representatives based on first occurrence + foreach ($this->blackMethods as $deadMethodKey => $_) { + if (isset($methodsGrouped[$deadMethodKey])) { + continue; + } + + $transitiveDeadMethods = $this->getTransitiveDeadCalls($deadMethodKey); + + $deadGroups[$deadMethodKey] = []; + $methodsGrouped[$deadMethodKey] = true; + + foreach ($transitiveDeadMethods as $transitiveDeadMethodKey) { + $deadGroups[$deadMethodKey][] = $transitiveDeadMethodKey; + $methodsGrouped[$transitiveDeadMethodKey] = true; + } + } + + return $deadGroups; + } + + /** + * @param array $transitiveDeadMethodKeys + */ private function buildError( - string $methodKey, + string $deadMethodKey, + array $transitiveDeadMethodKeys, string $file, int $line ): IdentifierRuleError { - return RuleErrorBuilder::message('Unused ' . $methodKey) + $builder = RuleErrorBuilder::message('Unused ' . $deadMethodKey) ->file($file) ->line($line) - ->identifier('shipmonk.deadMethod') - ->build(); + ->identifier('shipmonk.deadMethod'); + + $metadata = []; + + foreach ($transitiveDeadMethodKeys as $transitiveDeadMethodKey => [$transitiveDeadMethodFile, $transitiveDeadMethodLine]) { + $builder->addTip("Thus $transitiveDeadMethodKey is transitively also unused"); + + $metadata[$transitiveDeadMethodKey] = [ + 'file' => $transitiveDeadMethodFile, + 'line' => $transitiveDeadMethodLine, + ]; + } + + $builder->metadata($metadata); + + return $builder->build(); } /** @@ -280,4 +490,37 @@ private function getMethodKey(string $typeName, string $methodName): string return $typeName . '::' . $methodName; } + private function isConsideredWhite(Call $call): bool + { + return $call->caller === null + || $this->isAnonymousClass($call->caller->className) + || array_key_exists($call->caller->methodName, self::UNSUPPORTED_MAGIC_METHODS); + } + + private function isNeverReportedAsDead(string $methodKey): bool + { + [$typeName, $methodName] = explode('::', $methodKey); // @phpstan-ignore offsetAccess.notFound + + $kind = $this->typeDefinitions[$typeName]['kind'] ?? null; + $abstract = $this->typeDefinitions[$typeName]['methods'][$methodName]['abstract'] ?? false; + $visibility = $this->typeDefinitions[$typeName]['methods'][$methodName]['visibility'] ?? 0; + + if ($kind === Kind::TRAIT && $abstract) { + // abstract methods in traits make sense (not dead) only when called within the trait itself, but that is hard to detect for now, so lets ignore them completely + // the difference from interface methods (or abstract methods) is that those methods can be called over the interface, but you cannot call method over trait + return true; + } + + if ($methodName === '__construct' && ($visibility & Visibility::PRIVATE) !== 0) { + // private constructors are often used to deny instantiation + return true; + } + + if (array_key_exists($methodName, self::UNSUPPORTED_MAGIC_METHODS)) { + return true; + } + + return false; + } + } diff --git a/tests/AllServicesInConfigTest.php b/tests/AllServicesInConfigTest.php index 19fd2b2e..2d97dfa2 100644 --- a/tests/AllServicesInConfigTest.php +++ b/tests/AllServicesInConfigTest.php @@ -9,6 +9,8 @@ use RecursiveIteratorIterator; use ShipMonk\PHPStan\DeadCode\Crate\Call; use ShipMonk\PHPStan\DeadCode\Crate\Kind; +use ShipMonk\PHPStan\DeadCode\Crate\Method; +use ShipMonk\PHPStan\DeadCode\Crate\Visibility; use ShipMonk\PHPStan\DeadCode\Provider\MethodEntrypointProvider; use ShipMonk\PHPStan\DeadCode\Provider\SimpleMethodEntrypointProvider; use function array_keys; @@ -42,7 +44,9 @@ public function test(): void $missingClassNames = []; $excluded = [ Call::class, + Method::class, Kind::class, + Visibility::class, MethodEntrypointProvider::class, SimpleMethodEntrypointProvider::class, ]; diff --git a/tests/Rule/DeadMethodRuleTest.php b/tests/Rule/DeadMethodRuleTest.php index 9686af77..bbd5994a 100644 --- a/tests/Rule/DeadMethodRuleTest.php +++ b/tests/Rule/DeadMethodRuleTest.php @@ -3,6 +3,7 @@ namespace ShipMonk\PHPStan\DeadCode\Rule; use PhpParser\Node; +use PHPStan\Analyser\Error; use PHPStan\Collectors\Collector; use PHPStan\DependencyInjection\Container; use PHPStan\PhpDocParser\Lexer\Lexer; @@ -33,10 +34,17 @@ class DeadMethodRuleTest extends RuleTestCase { + private ?bool $emitErrorsInGroups = null; + + private bool $unwrapGroupedErrors = true; + protected function getRule(): DeadMethodRule { + self::assertNotNull($this->emitErrorsInGroups); + return new DeadMethodRule( new ClassHierarchy(), + !$this->emitErrorsInGroups, ); } @@ -57,6 +65,25 @@ protected function getCollectors(): array * @dataProvider provideFiles */ public function testDead($files, ?int $lowestPhpVersion = null): void + { + $this->emitErrorsInGroups = false; + $this->doTestDead($files, $lowestPhpVersion); + } + + /** + * @param string|non-empty-list $files + * @dataProvider provideFiles + */ + public function testDeadWithGroups($files, ?int $lowestPhpVersion = null): void + { + $this->emitErrorsInGroups = true; + $this->doTestDead($files, $lowestPhpVersion); + } + + /** + * @param string|non-empty-list $files + */ + private function doTestDead($files, ?int $lowestPhpVersion = null): void { if ($lowestPhpVersion !== null && PHP_VERSION_ID < $lowestPhpVersion) { self::markTestSkipped('Requires PHP ' . $lowestPhpVersion); @@ -65,24 +92,62 @@ public function testDead($files, ?int $lowestPhpVersion = null): void $this->analyseFiles(is_array($files) ? $files : [$files]); } + public function testGrouping(): void + { + $this->emitErrorsInGroups = true; + $this->unwrapGroupedErrors = false; + + $this->analyse([__DIR__ . '/data/DeadMethodRule/grouping/default.php'], [ + [ + 'Unused Grouping\Example::foo', + 20, + "• Thus Grouping\Example::bar is transitively also unused\n" . + '• Thus Grouping\Example::bag is transitively also unused', + ], + [ + 'Unused Grouping\Example::boo', + 26, + "• Thus Grouping\Example::bag is transitively also unused\n" . + '• Thus Grouping\Example::bar is transitively also unused', + ], + [ + 'Unused Grouping\Example::recur', + 43, + ], + [ + 'Unused Grouping\Example::recur1', + 50, + 'Thus Grouping\Example::recur2 is transitively also unused', + ], + ]); + } + /** * @return array, 1?: int}> */ public static function provideFiles(): iterable { + yield 'anonym' => [__DIR__ . '/data/DeadMethodRule/anonym.php']; yield 'enum' => [__DIR__ . '/data/DeadMethodRule/enum.php', 8_01_00]; + yield 'callables' => [__DIR__ . '/data/DeadMethodRule/callables.php']; yield 'code' => [__DIR__ . '/data/DeadMethodRule/basic.php']; yield 'ctor' => [__DIR__ . '/data/DeadMethodRule/ctor.php']; yield 'ctor-interface' => [__DIR__ . '/data/DeadMethodRule/ctor-interface.php']; + yield 'ctor-private' => [__DIR__ . '/data/DeadMethodRule/ctor-private.php']; + yield 'ctor-denied' => [__DIR__ . '/data/DeadMethodRule/ctor-denied.php']; + yield 'cycles' => [__DIR__ . '/data/DeadMethodRule/cycles.php']; yield 'abstract-1' => [__DIR__ . '/data/DeadMethodRule/abstract-1.php']; yield 'entrypoint' => [__DIR__ . '/data/DeadMethodRule/entrypoint.php']; yield 'clone' => [__DIR__ . '/data/DeadMethodRule/clone.php']; + yield 'magic' => [__DIR__ . '/data/DeadMethodRule/magic.php']; + yield 'new-in-initializers' => [__DIR__ . '/data/DeadMethodRule/new-in-initializers.php']; yield 'first-class-callable' => [__DIR__ . '/data/DeadMethodRule/first-class-callable.php']; yield 'overwriting-1' => [__DIR__ . '/data/DeadMethodRule/overwriting-methods-1.php']; yield 'overwriting-2' => [__DIR__ . '/data/DeadMethodRule/overwriting-methods-2.php']; yield 'overwriting-3' => [__DIR__ . '/data/DeadMethodRule/overwriting-methods-3.php']; yield 'overwriting-4' => [__DIR__ . '/data/DeadMethodRule/overwriting-methods-4.php']; yield 'overwriting-5' => [__DIR__ . '/data/DeadMethodRule/overwriting-methods-5.php']; + yield 'trait-abstract' => [__DIR__ . '/data/DeadMethodRule/traits-abstract-method.php']; yield 'trait-1' => [__DIR__ . '/data/DeadMethodRule/traits-1.php']; yield 'trait-2' => [__DIR__ . '/data/DeadMethodRule/traits-2.php']; yield 'trait-3' => [__DIR__ . '/data/DeadMethodRule/traits-3.php']; @@ -105,6 +170,7 @@ public static function provideFiles(): iterable yield 'trait-20' => [__DIR__ . '/data/DeadMethodRule/traits-20.php']; yield 'trait-21' => [__DIR__ . '/data/DeadMethodRule/traits-21.php']; yield 'trait-22' => [__DIR__ . '/data/DeadMethodRule/traits-22.php']; + yield 'trait-23' => [__DIR__ . '/data/DeadMethodRule/traits-23.php']; yield 'nullsafe' => [__DIR__ . '/data/DeadMethodRule/nullsafe.php']; yield 'dead-in-parent-1' => [__DIR__ . '/data/DeadMethodRule/dead-in-parent-1.php']; yield 'indirect-interface' => [__DIR__ . '/data/DeadMethodRule/indirect-interface.php']; @@ -205,4 +271,39 @@ private function createServiceMapFactoryMock(): ServiceMapFactory return $factoryMock; } + public function gatherAnalyserErrors(array $files): array + { + if (!$this->unwrapGroupedErrors) { + return parent::gatherAnalyserErrors($files); + } + + $result = []; + $errors = parent::gatherAnalyserErrors($files); + + foreach ($errors as $error) { + $result[] = $error; + + /** @var array $metadata */ + $metadata = $error->getMetadata(); + + foreach ($metadata as $alsoDead => ['file' => $file, 'line' => $line]) { + // @phpstan-ignore phpstanApi.constructor + $result[] = new Error( + "Unused $alsoDead", + $file, + $line, + true, + null, + null, + null, + null, + null, + $error->getIdentifier(), + ); + } + } + + return $result; + } + } diff --git a/tests/Rule/data/DeadMethodRule/anonym.php b/tests/Rule/data/DeadMethodRule/anonym.php new file mode 100644 index 00000000..5266885d --- /dev/null +++ b/tests/Rule/data/DeadMethodRule/anonym.php @@ -0,0 +1,17 @@ +commonMethod(); $class->differentMethod(); $this->overwrittenParentMethodUsedByChild(); + $this->childMethodNowUsed(); } - public function childMethodUnused(TestInterface|TestA $class, TestInterface $interface): void // error: Unused DeadBasic\TestChild::childMethodUnused + public function childMethodNowUsed(TestInterface|TestA $class, TestInterface $interface): void { - $class->differentMethod(); // TODO this is unused since the caller is unused + $class->differentMethod(); $interface->interfaceMethod(); } + public function childMethodUnused(): void // error: Unused DeadBasic\TestChild::childMethodUnused + { + + } + public function childMethodUsed(): void { - $this->parentMethodUsed($this); + } public function overwrittenParentMethodUsedByChild(): void { - + $this->parentMethodUsed($this); } } @@ -100,11 +106,4 @@ public function traitMethodUnused(): void // error: Unused DeadBasic\TestTrait:: } } -class StaticClass { - - private function __construct() - { - } -} - new TestChild(); diff --git a/tests/Rule/data/DeadMethodRule/callables.php b/tests/Rule/data/DeadMethodRule/callables.php new file mode 100644 index 00000000..96612d80 --- /dev/null +++ b/tests/Rule/data/DeadMethodRule/callables.php @@ -0,0 +1,22 @@ +transitive(); + }; + } + + public function transitive(): void // error: Unused Callables\Foo::transitive + { + } + +} + diff --git a/tests/Rule/data/DeadMethodRule/ctor-denied.php b/tests/Rule/data/DeadMethodRule/ctor-denied.php new file mode 100644 index 00000000..fdcec087 --- /dev/null +++ b/tests/Rule/data/DeadMethodRule/ctor-denied.php @@ -0,0 +1,11 @@ +transitive(); + } + + public static function create(): self + { + return new self(); + } + + private function transitive() + { + } +} + +StaticCtor::create(); diff --git a/tests/Rule/data/DeadMethodRule/cycles.php b/tests/Rule/data/DeadMethodRule/cycles.php new file mode 100644 index 00000000..87254157 --- /dev/null +++ b/tests/Rule/data/DeadMethodRule/cycles.php @@ -0,0 +1,32 @@ +recursion1(); + } + + public function recursion2(): void // error: Unused Cycles\Foo::recursion2 + { + $this->recursion2(); + } +} + +class A { + public function a(B $b): void // error: Unused Cycles\A::a + { + $b->b(); + } +} + +class B { + public function b(A $a): void // error: Unused Cycles\B::b + { + $a->a(); + } +} + +(new Foo())->recursion1(); diff --git a/tests/Rule/data/DeadMethodRule/dead-in-parent-1.php b/tests/Rule/data/DeadMethodRule/dead-in-parent-1.php index 5d0e3cdd..b20ef6cc 100644 --- a/tests/Rule/data/DeadMethodRule/dead-in-parent-1.php +++ b/tests/Rule/data/DeadMethodRule/dead-in-parent-1.php @@ -20,9 +20,11 @@ class B extends A /** * @inheritDoc */ - public function __construct() // error: Unused DeadParent\B::__construct + public function __construct() { parent::__construct(); } } + +new B(); diff --git a/tests/Rule/data/DeadMethodRule/first-class-callable.php b/tests/Rule/data/DeadMethodRule/first-class-callable.php index 3582dc5a..e0c0e17c 100644 --- a/tests/Rule/data/DeadMethodRule/first-class-callable.php +++ b/tests/Rule/data/DeadMethodRule/first-class-callable.php @@ -4,7 +4,7 @@ class A { - public function unused(): void // error: Unused DeadFirstClassCallable\A::unused + public function used(): void { $callback = $this->usedByFirstClassCallable(...); } @@ -14,3 +14,5 @@ public function usedByFirstClassCallable(): void } } + +(new A())->used(); diff --git a/tests/Rule/data/DeadMethodRule/grouping/default.php b/tests/Rule/data/DeadMethodRule/grouping/default.php new file mode 100644 index 00000000..960cc510 --- /dev/null +++ b/tests/Rule/data/DeadMethodRule/grouping/default.php @@ -0,0 +1,62 @@ +baz(); + } + + public function baz(): void // the only used + { + + } + + + public function foo(): void // group1 entrypoint + { + $this->baz(); + $this->bar(); + } + + public function boo(): void // group2 entrypoint + { + $this->bag(); + } + + public function bar(): void + { + $this->bag(); + } + + public function bag(): void + { + $this->bar(); // loop back + } + + + + public function recur(): void // self-cycle group + { + $this->recur(); + } + + + + public function recur1(): void // cycle group + { + $this->recur2(); + } + + public function recur2(): void + { + $this->recur1(); + } + +} + +new Example(); diff --git a/tests/Rule/data/DeadMethodRule/indirect-interface.php b/tests/Rule/data/DeadMethodRule/indirect-interface.php index 8b6a804a..4c15f910 100644 --- a/tests/Rule/data/DeadMethodRule/indirect-interface.php +++ b/tests/Rule/data/DeadMethodRule/indirect-interface.php @@ -9,10 +9,14 @@ public function foo(): void; // error: Unused DeadIndirect\FooInterface::foo abstract class FooAbstract { + public function __construct() + { + $this->foo(); + } public function foo(): void { - $this->foo(); + } } @@ -20,3 +24,5 @@ class Foo extends FooAbstract implements FooInterface { } + +new Foo(); diff --git a/tests/Rule/data/DeadMethodRule/magic.php b/tests/Rule/data/DeadMethodRule/magic.php new file mode 100644 index 00000000..2ac3a58a --- /dev/null +++ b/tests/Rule/data/DeadMethodRule/magic.php @@ -0,0 +1,39 @@ +calledFromPrivateConstruct(); + } + + public function __invoke() { + $this->calledFromInvoke(); + } + + public function __destruct() + { + $this->calledFromDestruct(); + } + + public function __get() + { + $this->calledFromGet(); + } + + public function calledFromInvoke() {} + public function calledFromDestruct() {} + public function calledFromPrivateConstruct() {} + public function calledFromGet() {} + +} + +$invokable = Magic::create(); +$invokable->magic(); +$invokable(); diff --git a/tests/Rule/data/DeadMethodRule/new-in-initializers.php b/tests/Rule/data/DeadMethodRule/new-in-initializers.php new file mode 100644 index 00000000..fc6ede9c --- /dev/null +++ b/tests/Rule/data/DeadMethodRule/new-in-initializers.php @@ -0,0 +1,17 @@ +traitMethod(); + } +} + +(new Tester())->test(); diff --git a/tests/Rule/data/DeadMethodRule/traits-abstract-method.php b/tests/Rule/data/DeadMethodRule/traits-abstract-method.php new file mode 100644 index 00000000..d78a6138 --- /dev/null +++ b/tests/Rule/data/DeadMethodRule/traits-abstract-method.php @@ -0,0 +1,24 @@ +abstractInTrait(); + } + + protected abstract function abstractInTrait(): void; +} + +class User { + use MyTrait; + + protected function abstractInTrait(): void + { + } +} + + +(new User())->test();