[DeadCode] Handle RemoveUnusedConstructorParamRector + RemoveUnusedPr…

…ivatePropertyRector (#1714) Co-authored-by: GitHub Action <action@github.com>
rectorphp · Jan 22, 2022 · 081958a · 081958a
1 parent 88a3207
commit 081958a
Show file tree

Hide file tree

Showing 5 changed files with 153 additions and 32 deletions.
diff --git a/rules/DeadCode/Rector/ClassMethod/RemoveUnusedConstructorParamRector.php b/rules/DeadCode/Rector/ClassMethod/RemoveUnusedConstructorParamRector.php
@@ -10,6 +10,7 @@
 use Rector\Core\NodeAnalyzer\ParamAnalyzer;
 use Rector\Core\Rector\AbstractRector;
 use Rector\Core\ValueObject\MethodName;
+use Rector\Removing\NodeManipulator\ComplexNodeRemover;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
 use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
 
@@ -19,7 +20,8 @@
 final class RemoveUnusedConstructorParamRector extends AbstractRector
 {
     public function __construct(
-        private readonly ParamAnalyzer $paramAnalyzer
+        private readonly ParamAnalyzer $paramAnalyzer,
+        private readonly ComplexNodeRemover $complexNodeRemover
     ) {
     }
 
@@ -89,14 +91,28 @@ public function refactor(Node $node): ?Node
             return null;
         }
 
-        foreach ($node->params as $param) {
-            if ($this->paramAnalyzer->isParamUsedInClassMethod($node, $param)) {
+        return $this->processRemoveParams($node);
+    }
+
+    private function processRemoveParams(ClassMethod $classMethod): ?ClassMethod
+    {
+        $paramKeysToBeRemoved = [];
+
+        foreach ($classMethod->params as $key => $param) {
+            if ($this->paramAnalyzer->isParamUsedInClassMethod($classMethod, $param)) {
                 continue;
             }
 
-            $this->nodeRemover->removeParam($node, $param);
+            $paramKeysToBeRemoved[] = $key;
+        }
+
+        $removedParamKeys = $this->complexNodeRemover
+            ->processRemoveParamWithKeys($classMethod->params, $paramKeysToBeRemoved);
+
+        if ($removedParamKeys === []) {
+            return null;
         }
 
-        return null;
+        return $classMethod;
     }
 }
diff --git a/rules/Removing/NodeManipulator/ComplexNodeRemover.php b/rules/Removing/NodeManipulator/ComplexNodeRemover.php
@@ -69,6 +69,39 @@ public function removePropertyAndUsages(Property $property, array $classMethodNa
         $this->nodeRemover->removeNode($property);
     }
 
+    /**
+     * @param Param[] $params
+     * @param int[] $paramKeysToBeRemoved
+     * @return int[]
+     */
+    public function processRemoveParamWithKeys(array $params, array $paramKeysToBeRemoved): array
+    {
+        $totalKeys = count($params) - 1;
+        $removedParamKeys = [];
+
+        foreach ($paramKeysToBeRemoved as $paramKeyToBeRemoved) {
+            $startNextKey = $paramKeyToBeRemoved + 1;
+            for ($nextKey = $startNextKey; $nextKey <= $totalKeys; ++$nextKey) {
+                if (! isset($params[$nextKey])) {
+                    // no next param, break the inner loop, remove the param
+                    break;
+                }
+
+                if (in_array($nextKey, $paramKeysToBeRemoved, true)) {
+                    // keep searching next key not in $paramKeysToBeRemoved
+                    continue;
+                }
+
+                return [];
+            }
+
+            $this->nodeRemover->removeNode($params[$paramKeyToBeRemoved]);
+            $removedParamKeys[] = $paramKeyToBeRemoved;
+        }
+
+        return $removedParamKeys;
+    }
+
     /**
      * @param Assign[] $assigns
      */
@@ -216,33 +249,6 @@ private function isInVariables(array $variables, Assign $assign): bool
         return false;
     }
 
-    /**
-     * @param Param[] $params
-     * @param int[] $paramKeysToBeRemoved
-     */
-    private function processRemoveParamWithKeys(array $params, array $paramKeysToBeRemoved): void
-    {
-        $totalKeys = count($params) - 1;
-        foreach ($paramKeysToBeRemoved as $paramKeyToBeRemoved) {
-            $startNextKey = $paramKeyToBeRemoved + 1;
-            for ($nextKey = $startNextKey; $nextKey <= $totalKeys; ++$nextKey) {
-                if (! isset($params[$nextKey])) {
-                    // no next param, break the inner loop, remove the param
-                    break;
-                }
-
-                if (in_array($nextKey, $paramKeysToBeRemoved, true)) {
-                    // keep searching next key not in $paramKeysToBeRemoved
-                    continue;
-                }
-
-                return;
-            }
-
-            $this->nodeRemover->removeNode($params[$paramKeyToBeRemoved]);
-        }
-    }
-
     private function isExpressionVariableNotAssign(Node $node): bool
     {
         $expressionVariable = $node->getAttribute(AttributeKey::PARENT_NODE);

diff --git a/tests/Issues/RemoveUnusedParamInMiddle/Fixture/do_not_remove_parameter_in_middle.php.inc b/tests/Issues/RemoveUnusedParamInMiddle/Fixture/do_not_remove_parameter_in_middle.php.inc
@@ -0,0 +1,53 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\Tests\Issues\RemoveUnusedParamInMiddle\Fixture;
+
+final class DoNotRemoveParameterInMiddle
+{
+    private $propertyA;
+    private $propertyB;
+    private $propertyC;
+
+    public function __construct($propertyA, $propertyB, $propertyC)
+    {
+        $this->propertyA = $propertyA;
+        $this->propertyB = $propertyB;
+        $this->propertyC = $propertyC;
+    }
+
+    public function run()
+    {
+        echo $this->propertyA;
+        echo $this->propertyC;
+    }
+}
+
+?>
+-----
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\Tests\Issues\RemoveUnusedParamInMiddle\Fixture;
+
+final class DoNotRemoveParameterInMiddle
+{
+    private $propertyA;
+    private $propertyC;
+
+    public function __construct($propertyA, $propertyB, $propertyC)
+    {
+        $this->propertyA = $propertyA;
+        $this->propertyC = $propertyC;
+    }
+
+    public function run()
+    {
+        echo $this->propertyA;
+        echo $this->propertyC;
+    }
+}
+
+?>
diff --git a/tests/Issues/RemoveUnusedParamInMiddle/RemoveUnusedParamInMiddleTest.php b/tests/Issues/RemoveUnusedParamInMiddle/RemoveUnusedParamInMiddleTest.php
@@ -0,0 +1,33 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\Tests\Issues\RemoveUnusedParamInMiddle;
+
+use Iterator;
+use Rector\Testing\PHPUnit\AbstractRectorTestCase;
+use Symplify\SmartFileSystem\SmartFileInfo;
+
+final class RemoveUnusedParamInMiddleTest 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/RemoveUnusedParamInMiddle/config/configured_rule.php b/tests/Issues/RemoveUnusedParamInMiddle/config/configured_rule.php
@@ -0,0 +1,13 @@
+<?php
+
+declare(strict_types=1);
+
+use Rector\DeadCode\Rector\ClassMethod\RemoveUnusedConstructorParamRector;
+use Rector\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRector;
+use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
+
+return static function (ContainerConfigurator $containerConfigurator): void {
+    $services = $containerConfigurator->services();
+    $services->set(RemoveUnusedConstructorParamRector::class);
+    $services->set(RemoveUnusedPrivatePropertyRector::class);
+};