From bb2f77f4b063ffd0f530cdb015bc1e61eb7449e8 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Tue, 9 Jul 2019 19:36:42 +0200 Subject: [PATCH] [DeadCode] Keep array method call in RemoveUnusedPrivateMethodRector --- ecs.yaml | 1 + .../Fixture/string.php.inc | 4 +- .../RemoveUnusedPrivateMethodRector.php | 50 +++++++------ .../Fixture/skip_array_callables.php.inc | 20 ++++++ .../RemoveUnusedPrivateMethodRectorTest.php | 1 + src/NodeContainer/ParsedNodesByType.php | 72 +++++++++++++++++-- 6 files changed, 120 insertions(+), 28 deletions(-) create mode 100644 packages/DeadCode/tests/Rector/ClassMethod/RemoveUnusedPrivateMethodRector/Fixture/skip_array_callables.php.inc diff --git a/ecs.yaml b/ecs.yaml index 8d71439cb9c..54dae5f54f8 100644 --- a/ecs.yaml +++ b/ecs.yaml @@ -102,6 +102,7 @@ parameters: Symplify\CodingStandard\Sniffs\CleanCode\CognitiveComplexitySniff: # tough logic + - 'src/NodeContainer/ParsedNodesByType.php' - 'packages/PHPStan/src/Rector/Node/RemoveNonExistingVarAnnotationRector.php' - 'packages/Architecture/src/Rector/Class_/ConstructorInjectionToActionInjectionRector.php' - 'src/PhpParser/Node/Commander/NodeRemovingCommander.php' diff --git a/packages/CodeQuality/tests/Rector/If_/ExplicitBoolCompareRector/Fixture/string.php.inc b/packages/CodeQuality/tests/Rector/If_/ExplicitBoolCompareRector/Fixture/string.php.inc index 0ea665186d3..64a81fb7d95 100644 --- a/packages/CodeQuality/tests/Rector/If_/ExplicitBoolCompareRector/Fixture/string.php.inc +++ b/packages/CodeQuality/tests/Rector/If_/ExplicitBoolCompareRector/Fixture/string.php.inc @@ -2,7 +2,7 @@ namespace Rector\CodeQuality\Tests\Rector\If_\ExplicitBoolCompareRector\Fixture; -final class String_ +final class ExplicitString { public function run(string $item) { @@ -22,7 +22,7 @@ final class String_ namespace Rector\CodeQuality\Tests\Rector\If_\ExplicitBoolCompareRector\Fixture; -final class String_ +final class ExplicitString { public function run(string $item) { diff --git a/packages/DeadCode/src/Rector/ClassMethod/RemoveUnusedPrivateMethodRector.php b/packages/DeadCode/src/Rector/ClassMethod/RemoveUnusedPrivateMethodRector.php index 35add03817e..06a19302517 100644 --- a/packages/DeadCode/src/Rector/ClassMethod/RemoveUnusedPrivateMethodRector.php +++ b/packages/DeadCode/src/Rector/ClassMethod/RemoveUnusedPrivateMethodRector.php @@ -4,8 +4,8 @@ namespace Rector\DeadCode\Rector\ClassMethod; use PhpParser\Node; use PhpParser\Node\Stmt\Class_; -use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\ClassMethod; +use PhpParser\Node\Stmt\Interface_; use PhpParser\Node\Stmt\Trait_; use Rector\NodeContainer\ParsedNodesByType; use Rector\NodeTypeResolver\Node\AttributeKey; @@ -70,27 +70,7 @@ CODE_SAMPLE */ public function refactor(Node $node): ?Node { - /** @var ClassLike|null $classNode */ - $classNode = $node->getAttribute(AttributeKey::CLASS_NODE); - if ($classNode === null) { - return null; - } - - // unreliable to detect trait method usage - if ($classNode instanceof Trait_) { - return null; - } - - if ($classNode instanceof Class_ && $classNode->isAnonymous()) { - return null; - } - - // skips interfaces by default too - if (! $node->isPrivate()) { - return null; - } - - if ($this->isName($node, '__*')) { + if ($this->shouldSkip($node)) { return null; } @@ -103,4 +83,30 @@ CODE_SAMPLE return $node; } + + private function shouldSkip(ClassMethod $classMethod): bool + { + /** @var Class_|Interface_|Trait_|null $classNode */ + $classNode = $classMethod->getAttribute(AttributeKey::CLASS_NODE); + if ($classNode === null) { + return true; + } + + // unreliable to detect trait, interface doesn't make sense + if ($classNode instanceof Trait_ || $classNode instanceof Interface_) { + return true; + } + + if ($classNode->isAnonymous()) { + return true; + } + + // skips interfaces by default too + if (! $classMethod->isPrivate()) { + return true; + } + + // skip magic methods - @see https://www.php.net/manual/en/language.oop5.magic.php + return $this->isName($classMethod, '__*'); + } } diff --git a/packages/DeadCode/tests/Rector/ClassMethod/RemoveUnusedPrivateMethodRector/Fixture/skip_array_callables.php.inc b/packages/DeadCode/tests/Rector/ClassMethod/RemoveUnusedPrivateMethodRector/Fixture/skip_array_callables.php.inc new file mode 100644 index 00000000000..9d6f6e18dc3 --- /dev/null +++ b/packages/DeadCode/tests/Rector/ClassMethod/RemoveUnusedPrivateMethodRector/Fixture/skip_array_callables.php.inc @@ -0,0 +1,20 @@ + $b; + } +} diff --git a/packages/DeadCode/tests/Rector/ClassMethod/RemoveUnusedPrivateMethodRector/RemoveUnusedPrivateMethodRectorTest.php b/packages/DeadCode/tests/Rector/ClassMethod/RemoveUnusedPrivateMethodRector/RemoveUnusedPrivateMethodRectorTest.php index 9a27b9f3b3a..279c23e565b 100644 --- a/packages/DeadCode/tests/Rector/ClassMethod/RemoveUnusedPrivateMethodRector/RemoveUnusedPrivateMethodRectorTest.php +++ b/packages/DeadCode/tests/Rector/ClassMethod/RemoveUnusedPrivateMethodRector/RemoveUnusedPrivateMethodRectorTest.php @@ -20,6 +20,7 @@ final class RemoveUnusedPrivateMethodRectorTest extends AbstractRectorTestCase __DIR__ . '/Fixture/keep_used_method.php.inc', __DIR__ . '/Fixture/keep_used_method_static.php.inc', __DIR__ . '/Fixture/skip_anonymous_class.php.inc', + __DIR__ . '/Fixture/skip_array_callables.php.inc', ]); } diff --git a/src/NodeContainer/ParsedNodesByType.php b/src/NodeContainer/ParsedNodesByType.php index c8613d3dc71..ee762c4590c 100644 --- a/src/NodeContainer/ParsedNodesByType.php +++ b/src/NodeContainer/ParsedNodesByType.php @@ -4,6 +4,7 @@ namespace Rector\NodeContainer; use Nette\Utils\Strings; use PhpParser\Node; +use PhpParser\Node\Expr\Array_; use PhpParser\Node\Expr\ClassConstFetch; use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\New_; @@ -43,6 +44,8 @@ final class ParsedNodesByType New_::class, StaticCall::class, MethodCall::class, + // for array callable - [$this, 'someCall'] + Array_::class, ]; /** @@ -81,10 +84,16 @@ final class ParsedNodesByType private $simpleParsedNodesByType = []; /** - * @var mixed[] + * @var MethodCall[][][]|StaticCall[][][] */ private $methodsCallsByTypeAndMethod = []; + /** + * E.g. [$this, 'someLocalMethod'] + * @var Array_[][][] + */ + private $arrayCallablesByTypeAndMethod = []; + public function __construct(NameResolver $nameResolver) { $this->nameResolver = $nameResolver; @@ -388,6 +397,22 @@ final class ParsedNodesByType return; } + // array callable - [$this, 'someCall'] + if ($node instanceof Array_) { + $arrayCallableClassAndMethod = $this->matchArrayCallableClassAndMethod($node); + if ($arrayCallableClassAndMethod === null) { + return; + } + + [$className, $methodName] = $arrayCallableClassAndMethod; + if (! method_exists($className, $methodName)) { + return; + } + + $this->arrayCallablesByTypeAndMethod[$className][$methodName][] = $node; + return; + } + if ($node instanceof MethodCall || $node instanceof StaticCall) { $this->addCall($node); return; @@ -399,7 +424,7 @@ final class ParsedNodesByType } /** - * @return MethodCall[] + * @return MethodCall[]|StaticCall[]|Array_[] */ public function findClassMethodCalls(ClassMethod $classMethod): array { @@ -413,11 +438,11 @@ final class ParsedNodesByType return []; } - return $this->methodsCallsByTypeAndMethod[$className][$methodName] ?? []; + return $this->methodsCallsByTypeAndMethod[$className][$methodName] ?? $this->arrayCallablesByTypeAndMethod[$className][$methodName] ?? []; } /** - * @return MethodCall[]|StaticCall[] + * @return MethodCall[][]|StaticCall[][] */ public function findMethodCallsOnClass(string $className): array { @@ -575,4 +600,43 @@ final class ParsedNodesByType $this->methodsCallsByTypeAndMethod[$className][$methodName][] = $node; } + + /** + * Matches array like: "[$this, 'methodName']" → ['ClassName', 'methodName'] + * @return string[]|null + */ + private function matchArrayCallableClassAndMethod(Array_ $array): ?array + { + if (count($array->items) !== 2) { + return null; + } + + if ($array->items[0] === null) { + return null; + } + + if (! $array->items[0]->value instanceof Variable) { + return null; + } + + if (! $this->nameResolver->isName($array->items[0]->value, 'this')) { + return null; + } + + if ($array->items[1] === null) { + return null; + } + + if (! $array->items[1]->value instanceof Node\Scalar\String_) { + return null; + } + + /** @var Node\Scalar\String_ $string */ + $string = $array->items[1]->value; + + $methodName = $string->value; + $className = $array->getAttribute(AttributeKey::CLASS_NAME); + + return [$className, $methodName]; + } }