[DX] Cleanup ComplexNodeRemover (#2230)

* misc

* final

* cleanup

* [ci-review] Rector Rectify

Co-authored-by: GitHub Action <action@github.com>

phpstan.neon

@@ -588,5 +588,6 @@ parameters:
 
         # cleanup after merge of assing remover refactoring
         - '#Cognitive complexity for "Rector\\Removing\\NodeManipulator\\ComplexNodeRemover\:\:removePropertyAndUsages\(\)" is \d+, keep it under 10#'
+        - '#Cognitive complexity for "Rector\\Removing\\NodeManipulator\\ComplexNodeRemover\:\:removeConstructorDependency\(\)" is \d+, keep it under 10#'
         - '#Class "Rector\\DeadCode\\Rector\\Property\\RemoveUnusedPrivatePropertyRector" has invalid namespace category "Property"\. Pick one of\: "Class_"#'
         - '#Cognitive complexity for "Rector\\ReadWrite\\NodeAnalyzer\\ReadWritePropertyAnalyzer\:\:isRead\(\)" is 14, keep it under 10#'

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

@@ -2,7 +2,7 @@
 
 namespace Rector\Tests\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;
 
-class DoNotRemoveParameterOnConstructorOnlyInMiddleParams
+final class ConstructorWithMiddleParams
 {
     /**
      * @var int
@@ -27,9 +27,9 @@ class DoNotRemoveParameterOnConstructorOnlyInMiddleParams
 
 namespace Rector\Tests\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;
 
-class DoNotRemoveParameterOnConstructorOnlyInMiddleParams
+final class ConstructorWithMiddleParams
 {
-    public function __construct(int $contentFinder, \stdClass $stdClass)
+    public function __construct(\stdClass $stdClass)
     {
         $this->init($stdClass);
     }

rules/Removing/NodeAnalyzer/ForbiddenPropertyRemovalAnalyzer.php

@@ -1,126 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace Rector\Removing\NodeAnalyzer;
-
-use PhpParser\Node;
-use PhpParser\Node\Expr\Assign;
-use PhpParser\Node\Expr\Clone_;
-use PhpParser\Node\Expr\New_;
-use PhpParser\Node\Expr\PropertyFetch;
-use PhpParser\Node\Expr\StaticPropertyFetch;
-use PhpParser\Node\Stmt\ClassLike;
-use PHPStan\Type\ObjectType;
-use PHPStan\Type\ThisType;
-use Rector\Core\NodeAnalyzer\PropertyFetchAnalyzer;
-use Rector\Core\PhpParser\Comparing\NodeComparator;
-use Rector\Core\PhpParser\Node\BetterNodeFinder;
-use Rector\NodeNameResolver\NodeNameResolver;
-use Rector\NodeTypeResolver\NodeTypeResolver;
-
-final class ForbiddenPropertyRemovalAnalyzer
-{
-    public function __construct(
-        private readonly BetterNodeFinder $betterNodeFinder,
-        private readonly NodeComparator $nodeComparator,
-        private readonly NodeNameResolver $nodeNameResolver,
-        private readonly NodeTypeResolver $nodeTypeResolver,
-        private readonly PropertyFetchAnalyzer $propertyFetchAnalyzer
-    ) {
-    }
-
-    public function isForbiddenInNewCurrentClassNameSelfClone(string $propertyName, ?ClassLike $classLike): bool
-    {
-        if (! $classLike instanceof ClassLike) {
-            return false;
-        }
-
-        $methods = $classLike->getMethods();
-        foreach ($methods as $method) {
-            $isInNewCurrentClassNameSelfClone = (bool) $this->betterNodeFinder->findFirst(
-                (array) $method->getStmts(),
-                function (Node $subNode) use ($classLike, $propertyName): bool {
-                    if ($subNode instanceof New_) {
-                        return $this->isPropertyNameUsedAfterNewOrClone($subNode, $classLike, $propertyName);
-                    }
-
-                    if ($subNode instanceof Clone_) {
-                        return $this->isPropertyNameUsedAfterNewOrClone($subNode, $classLike, $propertyName);
-                    }
-
-                    return false;
-                }
-            );
-
-            if ($isInNewCurrentClassNameSelfClone) {
-                return true;
-            }
-        }
-
-        return false;
-    }
-
-    private function isPropertyNameUsedAfterNewOrClone(
-        New_|Clone_ $expr,
-        ClassLike $classLike,
-        string $propertyName
-    ): bool {
-        $parentAssign = $this->betterNodeFinder->findParentType($expr, Assign::class);
-        if (! $parentAssign instanceof Assign) {
-            return false;
-        }
-
-        $className = (string) $this->nodeNameResolver->getName($classLike);
-        $type = $expr instanceof New_
-            ? $this->nodeTypeResolver->getType($expr->class)
-            : $this->nodeTypeResolver->getType($expr->expr);
-
-        if ($expr instanceof Clone_ && $type instanceof ThisType) {
-            $type = $type->getStaticObjectType();
-        }
-
-        if ($type instanceof ObjectType) {
-            return $this->isFoundAfterCloneOrNew($type, $expr, $parentAssign, $className, $propertyName);
-        }
-
-        return false;
-    }
-
-    private function isFoundAfterCloneOrNew(
-        ObjectType $objectType,
-        Clone_|New_ $expr,
-        Assign $parentAssign,
-        string $className,
-        string $propertyName
-    ): bool {
-        if ($objectType->getClassName() !== $className) {
-            return false;
-        }
-
-        return (bool) $this->betterNodeFinder->findFirstNext($expr, function (Node $subNode) use (
-            $parentAssign,
-            $propertyName
-        ): bool {
-            if (! $this->propertyFetchAnalyzer->isPropertyFetch($subNode)) {
-                return false;
-            }
-
-            /** @var PropertyFetch|StaticPropertyFetch $subNode */
-            $propertyFetchName = (string) $this->nodeNameResolver->getName($subNode);
-            if ($subNode instanceof PropertyFetch) {
-                if (! $this->nodeComparator->areNodesEqual($subNode->var, $parentAssign->var)) {
-                    return false;
-                }
-
-                return $propertyFetchName === $propertyName;
-            }
-
-            if (! $this->nodeComparator->areNodesEqual($subNode->class, $parentAssign->var)) {
-                return false;
-            }
-
-            return $propertyFetchName === $propertyName;
-        });
-    }
-}

rules/Removing/NodeManipulator/ComplexNodeRemover.php

@@ -9,34 +9,25 @@ use PhpParser\Node\Expr;
 use PhpParser\Node\Expr\ArrayDimFetch;
 use PhpParser\Node\Expr\Assign;
 use PhpParser\Node\Expr\PropertyFetch;
-use PhpParser\Node\Expr\StaticPropertyFetch;
 use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Param;
 use PhpParser\Node\Stmt\Class_;
-use PhpParser\Node\Stmt\ClassLike;
 use PhpParser\Node\Stmt\ClassMethod;
 use PhpParser\Node\Stmt\Expression;
 use PhpParser\Node\Stmt\Property;
-use Rector\Core\PhpParser\Comparing\NodeComparator;
 use Rector\Core\PhpParser\Node\BetterNodeFinder;
-use Rector\Core\PhpParser\NodeFinder\PropertyFetchFinder;
 use Rector\Core\ValueObject\MethodName;
 use Rector\DeadCode\SideEffect\SideEffectNodeDetector;
 use Rector\NodeNameResolver\NodeNameResolver;
 use Rector\NodeRemoval\NodeRemover;
-use Rector\NodeTypeResolver\Node\AttributeKey;
-use Rector\Removing\NodeAnalyzer\ForbiddenPropertyRemovalAnalyzer;
 use Symplify\Astral\NodeTraverser\SimpleCallableNodeTraverser;
 
 final class ComplexNodeRemover
 {
     public function __construct(
-        private readonly PropertyFetchFinder $propertyFetchFinder,
         private readonly NodeNameResolver $nodeNameResolver,
         private readonly BetterNodeFinder $betterNodeFinder,
         private readonly NodeRemover $nodeRemover,
-        private readonly NodeComparator $nodeComparator,
-        private readonly ForbiddenPropertyRemovalAnalyzer $forbiddenPropertyRemovalAnalyzer,
         private readonly SideEffectNodeDetector $sideEffectNodeDetector,
         private readonly SimpleCallableNodeTraverser $simpleCallableNodeTraverser,
     ) {
@@ -50,11 +41,13 @@ final class ComplexNodeRemover
         $propertyName = $this->nodeNameResolver->getName($property);
 
         $hasSideEffect = false;
+        $isPartoFAnotherAssign = false;
 
         $this->simpleCallableNodeTraverser->traverseNodesWithCallable($class->stmts, function (Node $node) use (
             $removeAssignSideEffect,
             $propertyName,
-            &$hasSideEffect
+            &$hasSideEffect,
+            &$isPartoFAnotherAssign
         ) {
             // here should be checked all expr like stmts that can hold assign, e.f. if, foreach etc. etc.
             if ($node instanceof Expression) {
@@ -66,6 +59,7 @@ final class ComplexNodeRemover
 
                     // skip double assigns
                     if ($assign->expr instanceof Assign) {
+                        $isPartoFAnotherAssign = true;
                         return null;
                     }
 
@@ -99,23 +93,12 @@ final class ComplexNodeRemover
             return;
         }
 
-        $propertyFetches = $this->propertyFetchFinder->findPrivatePropertyFetches($property);
-        $assigns = [];
-
-        foreach ($propertyFetches as $propertyFetch) {
-            $assign = $this->resolvePropertyFetchAssign($propertyFetch);
-            if (! $assign instanceof Assign) {
-                return;
-            }
-
-            if ($assign->expr instanceof Assign) {
-                return;
-            }
-
-            $assigns[] = $assign;
+        if ($isPartoFAnotherAssign) {
+            return;
         }
 
-        $this->processRemovePropertyAssigns($assigns);
+        $this->removeConstructorDependency($class, $propertyName);
+
         $this->nodeRemover->removeNode($property);
     }
 
@@ -152,140 +135,32 @@ final class ComplexNodeRemover
         return $removedParamKeys;
     }
 
-    /**
-     * @param Assign[] $assigns
-     */
-    private function processRemovePropertyAssigns(array $assigns): void
+    private function removeConstructorDependency(Class_ $class, string $propertyName): void
     {
-        foreach ($assigns as $assign) {
-            // remove assigns
-            $this->removeConstructorDependency($assign);
-        }
-    }
-
-    private function resolvePropertyFetchAssign(PropertyFetch | StaticPropertyFetch $expr): ?Assign
-    {
-        $assign = $expr->getAttribute(AttributeKey::PARENT_NODE);
-
-        while ($assign instanceof Node && ! $assign instanceof Assign) {
-            $assign = $assign->getAttribute(AttributeKey::PARENT_NODE);
-        }
-
-        if (! $assign instanceof Assign) {
-            return null;
-        }
-
-        $isInExpr = (bool) $this->betterNodeFinder->findFirst(
-            $assign->expr,
-            fn (Node $subNode): bool => $this->nodeComparator->areNodesEqual($subNode, $expr)
-        );
-
-        if ($isInExpr) {
-            return null;
-        }
-
-        $classLike = $this->betterNodeFinder->findParentType($expr, ClassLike::class);
-        $propertyName = (string) $this->nodeNameResolver->getName($expr);
-
-        if ($this->forbiddenPropertyRemovalAnalyzer->isForbiddenInNewCurrentClassNameSelfClone(
-            $propertyName,
-            $classLike
-        )) {
-            return null;
-        }
-
-        return $assign;
-    }
-
-    private function removeConstructorDependency(Assign $assign): void
-    {
-        $classMethod = $this->betterNodeFinder->findParentType($assign, ClassMethod::class);
-        if (! $classMethod instanceof  ClassMethod) {
+        $classMethod = $class->getMethod(MethodName::CONSTRUCT);
+        if (! $classMethod instanceof ClassMethod) {
             return;
         }
 
-        if (! $this->nodeNameResolver->isName($classMethod, MethodName::CONSTRUCT)) {
-            return;
-        }
+        $stmts = (array) $classMethod->stmts;
 
-        $class = $this->betterNodeFinder->findParentType($assign, Class_::class);
-        if (! $class instanceof Class_) {
-            return;
-        }
-
-        $constructClassMethod = $class->getMethod(MethodName::CONSTRUCT);
-        if (! $constructClassMethod instanceof ClassMethod) {
-            return;
-        }
-
-        $params = $constructClassMethod->getParams();
-        $paramKeysToBeRemoved = [];
-
-        /** @var Variable[] $variables */
-        $variables = $this->resolveVariables($constructClassMethod);
-        foreach ($params as $key => $param) {
-            $variable = $this->betterNodeFinder->findFirst(
-                (array) $constructClassMethod->stmts,
-                fn (Node $node): bool => $this->nodeComparator->areNodesEqual($param->var, $node)
-            );
-
-            if (! $variable instanceof Node) {
+        foreach ($stmts as $key => $stmt) {
+            if (! $stmt instanceof Expression) {
                 continue;
             }
 
-            if ($this->isExpressionVariableNotAssign($variable)) {
-                continue;
-            }
+            $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 (! $this->nodeComparator->areNodesEqual($param->var, $assign->expr)) {
-                continue;
-            }
-
-            if ($this->isInVariables($variables, $assign)) {
-                continue;
-            }
-
-            $paramKeysToBeRemoved[] = $key;
-        }
-
-        $this->processRemoveParamWithKeys($params, $paramKeysToBeRemoved);
-    }
-
-    /**
-     * @return Variable[]
-     */
-    private function resolveVariables(ClassMethod $classMethod): array
-    {
-        return $this->betterNodeFinder->find(
-            (array) $classMethod->stmts,
-            function (Node $subNode): bool {
-                if (! $subNode instanceof Variable) {
-                    return false;
+                    if ($stmtExpr->expr instanceof Variable) {
+                        $this->clearParamFromConstructor($classMethod, $stmtExpr->expr);
+                    }
                 }
-
-                return $this->isExpressionVariableNotAssign($subNode);
-            }
-        );
-    }
-
-    /**
-     * @param Variable[] $variables
-     */
-    private function isInVariables(array $variables, Assign $assign): bool
-    {
-        foreach ($variables as $variable) {
-            if ($this->nodeComparator->areNodesEqual($assign->expr, $variable)) {
-                return true;
             }
         }
-
-        return false;
-    }
-
-    private function isExpressionVariableNotAssign(Node $node): bool
-    {
-        $expressionVariable = $node->getAttribute(AttributeKey::PARENT_NODE);
-        return ! $expressionVariable instanceof Assign;
     }
 
     /**
@@ -310,4 +185,31 @@ final class ComplexNodeRemover
 
         return $propertyFetches;
     }
+
+    private function clearParamFromConstructor(ClassMethod $classMethod, Variable $assignedVariable): void
+    {
+        // is variable used somewhere else? skip it
+        $variables = $this->betterNodeFinder->findInstanceOf($classMethod, Variable::class);
+
+        $paramNamedVariables = array_filter(
+            $variables,
+            fn (Variable $variable): bool => $this->nodeNameResolver->areNamesEqual($variable, $assignedVariable)
+        );
+
+        // there is more than 1 use, keep it in the constructor
+        if (count($paramNamedVariables) > 1) {
+            return;
+        }
+
+        $paramName = $this->nodeNameResolver->getName($assignedVariable);
+        if (! is_string($paramName)) {
+            return;
+        }
+
+        foreach ($classMethod->params as $paramKey => $param) {
+            if ($this->nodeNameResolver->isName($param->var, $paramName)) {
+                unset($classMethod->params[$paramKey]);
+            }
+        }
+    }
 }

src/PhpParser/NodeFinder/PropertyFetchFinder.php

@@ -137,8 +137,8 @@ final class PropertyFetchFinder
         /** @var StaticPropertyFetch[] $matchingStaticPropertyFetches */
         $matchingStaticPropertyFetches = array_filter(
             $staticPropertyFetches,
-            fn (StaticPropertyFetch $staticPropertyFetch): bool => $this->isLocalStaticPropertyByFetchName(
-                $staticPropertyFetch,
+            fn (StaticPropertyFetch $staticPropertyFetch): bool => $this->nodeNameResolver->isName(
+                $staticPropertyFetch->name,
                 $propertyName
             )
         );
@@ -190,20 +190,4 @@ final class PropertyFetchFinder
 
         return $this->nodeNameResolver->getName($propertyOrPromotedParam->var);
     }
-
-    private function isLocalStaticPropertyByFetchName(
-        StaticPropertyFetch $staticPropertyFetch,
-        string $propertyName
-    ): bool {
-        $class = $this->nodeNameResolver->getName($staticPropertyFetch->class);
-        if (! in_array(
-            $class,
-            [ObjectReference::SELF()->getValue(), ObjectReference::STATIC()->getValue(), self::THIS],
-            true
-        )) {
-            return false;
-        }
-
-        return $this->nodeNameResolver->isName($staticPropertyFetch->name, $propertyName);
-    }
 }

tests/Issues/RemoveUnusedParamInMiddle/Fixture/do_not_remove_parameter_in_middle.php.inc

@@ -1,53 +0,0 @@
-<?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

@@ -1,33 +0,0 @@
-<?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

@@ -1,12 +0,0 @@
-<?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);
-};