[DeadCode] Fix RemoveUnusedPrivateMethodParameterRector args keys re-…

…order (#2770) Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com> Co-authored-by: Alejo Moreira <alejo.moreira@729solutions.com> Co-authored-by: GitHub Action <action@github.com>
rectorphp · Aug 17, 2022 · 244fb1c · 244fb1c
1 parent 03ed036
commit 244fb1c
Show file tree

Hide file tree

Showing 5 changed files with 133 additions and 27 deletions.
diff --git a/rules/DeadCode/NodeCollector/UnusedParameterResolver.php b/rules/DeadCode/NodeCollector/UnusedParameterResolver.php
@@ -16,7 +16,7 @@ public function __construct(
     }
 
     /**
-     * @return Param[]
+     * @return array<int, Param>
      */
     public function resolve(ClassMethod $classMethod): array
     {

diff --git a/rules/DeadCode/Rector/ClassMethod/RemoveUnusedPrivateMethodParameterRector.php b/rules/DeadCode/Rector/ClassMethod/RemoveUnusedPrivateMethodParameterRector.php
@@ -8,7 +8,7 @@
 use PhpParser\Node\Expr\MethodCall;
 use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Param;
-use PhpParser\Node\Stmt\ClassLike;
+use PhpParser\Node\Stmt\Class_;
 use PhpParser\Node\Stmt\ClassMethod;
 use PHPStan\PhpDocParser\Ast\PhpDoc\ParamTagValueNode;
 use Rector\BetterPhpDocParser\PhpDocManipulator\PhpDocTagRemover;
@@ -65,52 +65,67 @@ private function run($value)
      */
     public function getNodeTypes(): array
     {
-        return [ClassMethod::class];
+        return [Class_::class];
     }
 
     /**
-     * @param ClassMethod $node
+     * @param Class_ $node
      */
     public function refactor(Node $node): ?Node
     {
-        if ($this->shouldSkip($node)) {
-            return null;
-        }
+        $hasChanged = false;
 
-        $unusedParameters = $this->unusedParameterResolver->resolve($node);
-        if ($unusedParameters === []) {
-            return null;
-        }
+        foreach ($node->getMethods() as $classMethod) {
+            if ($this->shouldSkipClassMethod($classMethod)) {
+                continue;
+            }
 
-        $this->nodeRemover->removeNodes($unusedParameters);
+            $unusedParameters = $this->unusedParameterResolver->resolve($classMethod);
+            if ($unusedParameters === []) {
+                continue;
+            }
+
+            $unusedParameterPositions = array_keys($unusedParameters);
+            foreach (array_keys($classMethod->params) as $key) {
+                if (! in_array($key, $unusedParameterPositions, true)) {
+                    continue;
+                }
+
+                unset($classMethod->params[$key]);
+            }
 
-        $this->clearPhpDocInfo($node, $unusedParameters);
-        $this->removeCallerArgs($node, $unusedParameters);
+            // reset param keys
+            $classMethod->params = array_values($classMethod->params);
 
-        return $node;
+            $this->clearPhpDocInfo($classMethod, $unusedParameters);
+            $this->removeCallerArgs($node, $classMethod, $unusedParameters);
+
+            $hasChanged = true;
+        }
+
+        if ($hasChanged) {
+            return $node;
+        }
+
+        return null;
     }
 
     /**
      * @param Param[] $unusedParameters
      */
-    private function removeCallerArgs(ClassMethod $classMethod, array $unusedParameters): void
+    private function removeCallerArgs(Class_ $class, ClassMethod $classMethod, array $unusedParameters): void
     {
-        $classLike = $this->betterNodeFinder->findParentType($classMethod, ClassLike::class);
-        if (! $classLike instanceof ClassLike) {
-            return;
-        }
-
-        $methods = $classLike->getMethods();
-        if ($methods === []) {
+        $classMethods = $class->getMethods();
+        if ($classMethods === []) {
             return;
         }
 
         $methodName = $this->nodeNameResolver->getName($classMethod);
         $keysArg = array_keys($unusedParameters);
 
-        foreach ($methods as $method) {
+        foreach ($classMethods as $classMethod) {
             /** @var MethodCall[] $callers */
-            $callers = $this->resolveCallers($method, $methodName);
+            $callers = $this->resolveCallers($classMethod, $methodName);
             if ($callers === []) {
                 continue;
             }
@@ -133,7 +148,8 @@ private function cleanupArgs(MethodCall $methodCall, array $keysArg): void
             }
         }
 
-        $methodCall->args = $args;
+        // reset arg keys
+        $methodCall->args = array_values($args);
     }
 
     /**
@@ -162,7 +178,7 @@ private function resolveCallers(ClassMethod $classMethod, string $methodName): a
         });
     }
 
-    private function shouldSkip(ClassMethod $classMethod): bool
+    private function shouldSkipClassMethod(ClassMethod $classMethod): bool
     {
         if (! $classMethod->isPrivate()) {
             return true;

diff --git a/tests/Issues/Issue7374/Fixture/fixture.php.inc b/tests/Issues/Issue7374/Fixture/fixture.php.inc
@@ -0,0 +1,40 @@
+<?php
+
+namespace Rector\Core\Tests\Issues\Issue7374\Fixture;
+
+final class ClassWithTwoMethods
+{
+    private function displayTree($name, $data, $other = null)
+    {
+        if (empty($data)) return false;
+    }
+
+    public function action($params, $server)
+    {
+        $bb = new BusibooksBusinesses();
+
+        $this->displayTree("Equity", $equity, array(
+            array("name" => "NET INCOME", "amount" => $net_income),
+            array("name" => "RETAINED EARNINGS", "amount" => $retained_earnings)
+        ));
+    }
+}
+-----
+<?php
+
+namespace Rector\Core\Tests\Issues\Issue7374\Fixture;
+
+final class ClassWithTwoMethods
+{
+    private function displayTree($data)
+    {
+        if (empty($data)) return false;
+    }
+
+    public function action($params, $server)
+    {
+        $busibooksBusinesses = new BusibooksBusinesses();
+
+        $this->displayTree($equity);
+    }
+}
diff --git a/tests/Issues/Issue7374/RuleCombinationShouldNotReturnGetAttributeOnNullReturnTest.php b/tests/Issues/Issue7374/RuleCombinationShouldNotReturnGetAttributeOnNullReturnTest.php
@@ -0,0 +1,36 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\Tests\Issues\Issue7374;
+
+use Iterator;
+use Rector\Testing\PHPUnit\AbstractRectorTestCase;
+use Symplify\SmartFileSystem\SmartFileInfo;
+
+/**
+ * @see https://github.com/rectorphp/rector/issues/7374
+ */
+final class RuleCombinationShouldNotReturnGetAttributeOnNullReturnTest extends AbstractRectorTestCase
+{
+    /**
+     * @dataProvider provideData()
+     */
+    public function test(SmartFileInfo $fileInfo): void
+    {
+        $this->doTestFileInfo($fileInfo);
+    }
+
+    /**
+     * @return Iterator<SmartFileInfo>
+     */
+    public function provideData(): Iterator
+    {
+        return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture');
+    }
+
+    public function provideConfigFilePath(): string
+    {
+        return __DIR__ . '/config/configured_rule.php';
+    }
+}
diff --git a/tests/Issues/Issue7374/config/configured_rule.php b/tests/Issues/Issue7374/config/configured_rule.php
@@ -0,0 +1,14 @@
+<?php
+
+declare(strict_types=1);
+
+use Rector\Config\RectorConfig;
+use Rector\DeadCode\Rector\ClassMethod\RemoveUnusedPrivateMethodParameterRector;
+use Rector\Naming\Rector\ClassMethod\RenameVariableToMatchNewTypeRector;
+
+return static function (RectorConfig $rectorConfig): void {
+    $rectorConfig->rules([
+        RemoveUnusedPrivateMethodParameterRector::class,
+        RenameVariableToMatchNewTypeRector::class,
+    ]);
+};