[DeadCode] Do not remove unused param in middle on RemoveUnusedConstructorParamRector (#2231)

* [DeadCode] Do not remove unused param in middle on RemoveUnusedConstructorParamRector

* more fixture

* Fixed 🎉

* Fixed 🎉

phpstan.neon

@@ -336,7 +336,6 @@ parameters:
             message: '#Class cognitive complexity is \d+, keep it under 50#'
             paths:
                 - src/PhpParser/Node/BetterNodeFinder.php
-                - rules/Removing/NodeManipulator/ComplexNodeRemover.php
 
         -
             message: '#Parameter \#2 \$length of function str_split expects int<1, max\>, int given#'

rules-tests/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/constructor_with_middle_params.php.inc

@@ -29,7 +29,7 @@ namespace Rector\Tests\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRecto
 
 final class ConstructorWithMiddleParams
 {
-    public function __construct(\stdClass $stdClass)
+    public function __construct(int $contentFinder, \stdClass $stdClass)
     {
         $this->init($stdClass);
     }

rules/Removing/NodeManipulator/ComplexNodeRemover.php

@@ -50,38 +50,42 @@ final class ComplexNodeRemover
             &$isPartoFAnotherAssign
         ) {
             // here should be checked all expr like stmts that can hold assign, e.f. if, foreach etc. etc.
-            if ($node instanceof Expression) {
-                $nodeExpr = $node->expr;
+            if (! $node instanceof Expression) {
+                return null;
+            }
 
-                // remove direct assigns
-                if ($nodeExpr instanceof Assign) {
-                    $assign = $nodeExpr;
+            $nodeExpr = $node->expr;
 
-                    // skip double assigns
-                    if ($assign->expr instanceof Assign) {
-                        $isPartoFAnotherAssign = true;
+            // remove direct assigns
+            if (! $nodeExpr instanceof Assign) {
+                return null;
+            }
+
+            $assign = $nodeExpr;
+
+            // skip double assigns
+            if ($assign->expr instanceof Assign) {
+                $isPartoFAnotherAssign = true;
+                return null;
+            }
+
+            $propertyFetches = $this->resolvePropertyFetchFromDimFetch($assign->var);
+            if ($propertyFetches === []) {
+                return null;
+            }
+
+            foreach ($propertyFetches as $propertyFetch) {
+                if (! $this->nodeNameResolver->isName($propertyFetch->var, 'this')) {
+                    continue;
+                }
+
+                if ($this->nodeNameResolver->isName($propertyFetch->name, $propertyName)) {
+                    if (! $removeAssignSideEffect && $this->sideEffectNodeDetector->detect($assign->expr)) {
+                        $hasSideEffect = true;
                         return null;
                     }
 
-                    $propertyFetches = $this->resolvePropertyFetchFromDimFetch($assign->var);
-                    if ($propertyFetches === []) {
-                        return null;
-                    }
-
-                    foreach ($propertyFetches as $propertyFetch) {
-                        if (! $this->nodeNameResolver->isName($propertyFetch->var, 'this')) {
-                            continue;
-                        }
-
-                        if ($this->nodeNameResolver->isName($propertyFetch->name, $propertyName)) {
-                            if (! $removeAssignSideEffect && $this->sideEffectNodeDetector->detect($assign->expr)) {
-                                $hasSideEffect = true;
-                                return null;
-                            }
-
-                            $this->nodeRemover->removeNode($node);
-                        }
-                    }
+                    $this->nodeRemover->removeNode($node);
                 }
             }
 
@@ -143,6 +147,7 @@ final class ComplexNodeRemover
         }
 
         $stmts = (array) $classMethod->stmts;
+        $paramKeysToBeRemoved = [];
 
         foreach ($stmts as $key => $stmt) {
             if (! $stmt instanceof Expression) {
@@ -150,17 +155,37 @@ final class ComplexNodeRemover
             }
 
             $stmtExpr = $stmt->expr;
-            if ($stmtExpr instanceof Assign && $stmtExpr->var instanceof PropertyFetch) {
-                $propertyFetch = $stmtExpr->var;
-                if ($this->nodeNameResolver->isName($propertyFetch, $propertyName)) {
-                    unset($classMethod->stmts[$key]);
 
-                    if ($stmtExpr->expr instanceof Variable) {
-                        $this->clearParamFromConstructor($classMethod, $stmtExpr->expr);
-                    }
-                }
+            if (! $stmtExpr instanceof Assign) {
+                continue;
+            }
+
+            if (! $stmtExpr->var instanceof PropertyFetch) {
+                continue;
+            }
+
+            $propertyFetch = $stmtExpr->var;
+            if (! $this->nodeNameResolver->isName($propertyFetch, $propertyName)) {
+                continue;
+            }
+
+            unset($classMethod->stmts[$key]);
+
+            if (! $stmtExpr->expr instanceof Variable) {
+                continue;
+            }
+
+            $key = $this->resolveToBeClearedParamFromConstructor($classMethod, $stmtExpr->expr);
+            if (is_int($key)) {
+                $paramKeysToBeRemoved[] = $key;
             }
         }
+
+        if ($paramKeysToBeRemoved === []) {
+            return;
+        }
+
+        $this->processRemoveParamWithKeys($classMethod->getParams(), $paramKeysToBeRemoved);
     }
 
     /**
@@ -186,7 +211,7 @@ final class ComplexNodeRemover
         return $propertyFetches;
     }
 
-    private function clearParamFromConstructor(ClassMethod $classMethod, Variable $assignedVariable): void
+    private function resolveToBeClearedParamFromConstructor(ClassMethod $classMethod, Variable $assignedVariable): ?int
     {
         // is variable used somewhere else? skip it
         $variables = $this->betterNodeFinder->findInstanceOf($classMethod, Variable::class);
@@ -198,18 +223,20 @@ final class ComplexNodeRemover
 
         // there is more than 1 use, keep it in the constructor
         if (count($paramNamedVariables) > 1) {
-            return;
+            return null;
         }
 
         $paramName = $this->nodeNameResolver->getName($assignedVariable);
         if (! is_string($paramName)) {
-            return;
+            return null;
         }
 
         foreach ($classMethod->params as $paramKey => $param) {
             if ($this->nodeNameResolver->isName($param->var, $paramName)) {
-                unset($classMethod->params[$paramKey]);
+                return $paramKey;
             }
         }
+
+        return null;
     }
 }

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;
+    }
+}
+
+?>

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';
+    }
+}

tests/Issues/RemoveUnusedParamInMiddle/config/configured_rule.php

@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+use Rector\Config\RectorConfig;
+use Rector\DeadCode\Rector\ClassMethod\RemoveUnusedConstructorParamRector;
+use Rector\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRector;
+
+return static function (RectorConfig $rectorConfig): void {
+    $rectorConfig->rule(RemoveUnusedConstructorParamRector::class);
+    $rectorConfig->rule(RemoveUnusedPrivatePropertyRector::class);
+};