[DeadCode] Skip using coealesce assign operator on return on RemoveUnusedPrivatePropertyRector (#2476)

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

packages/ReadWrite/NodeAnalyzer/ReadWritePropertyAnalyzer.php

@@ -55,14 +55,14 @@ final class ReadWritePropertyAnalyzer
             return true;
         }
 
-        if ($parent instanceof ArrayDimFetch && $parent->dim === $node && $this->isNotInsideIssetUnset($parent)) {
-            return $this->isArrayDimFetchRead($parent);
-        }
-
         if (! $parent instanceof ArrayDimFetch) {
             return ! $this->assignManipulator->isLeftPartOfAssign($node);
         }
 
+        if ($parent->dim === $node && $this->isNotInsideIssetUnset($parent)) {
+            return $this->isArrayDimFetchRead($parent);
+        }
+
         if ($this->assignManipulator->isLeftPartOfAssign($parent)) {
             return false;
         }

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

@@ -0,0 +1,20 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;
+
+final class SkipUsingCoalesceAssignOperator
+{
+    private SomeService $someService;
+
+    private array $types = [];
+
+    public function __construct(SomeService $someService)
+    {
+        $this->someService = $someService;
+    }
+
+    public function get(string $key)
+    {
+        return $this->types[$key] ??= $this->someService->resolve();
+    }
+}

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

@@ -0,0 +1,20 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;
+
+final class SkipUsingCoalesceAssignOperator2
+{
+    private SomeService $someService;
+
+    private array $types = [];
+
+    public function __construct(SomeService $someService)
+    {
+        $this->someService = $someService;
+    }
+
+    public function get(string $key, string $key2)
+    {
+        return $this->types[$key][$key2] ??= $this->someService->resolve();
+    }
+}

rules/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector.php

@@ -83,7 +83,7 @@ CODE_SAMPLE
      */
     public function refactor(Node $node): ?Node
     {
-        $hasChanged = false;
+        $hasRemoved = false;
 
         foreach ($node->getProperties() as $property) {
             if ($this->shouldSkipProperty($property)) {
@@ -94,12 +94,20 @@ CODE_SAMPLE
                 continue;
             }
 
-            $this->complexNodeRemover->removePropertyAndUsages($node, $property, $this->removeAssignSideEffect);
+            // use different variable to avoid re-assign back $hasRemoved to false
+            // when already asssigned to true
+            $isRemoved = $this->complexNodeRemover->removePropertyAndUsages(
+                $node,
+                $property,
+                $this->removeAssignSideEffect
+            );
 
-            $hasChanged = true;
+            if ($isRemoved) {
+                $hasRemoved = true;
+            }
         }
 
-        return $hasChanged ? $node : null;
+        return $hasRemoved ? $node : null;
     }
 
     private function shouldSkipProperty(Property $property): bool

rules/Removing/NodeManipulator/ComplexNodeRemover.php

@@ -17,11 +17,13 @@ use PhpParser\Node\Stmt\ClassMethod;
 use PhpParser\Node\Stmt\Expression;
 use PhpParser\Node\Stmt\Property;
 use Rector\Core\NodeAnalyzer\PropertyFetchAnalyzer;
+use Rector\Core\PhpParser\Comparing\NodeComparator;
 use Rector\Core\PhpParser\Node\BetterNodeFinder;
 use Rector\Core\ValueObject\MethodName;
 use Rector\DeadCode\SideEffect\SideEffectNodeDetector;
 use Rector\NodeNameResolver\NodeNameResolver;
 use Rector\NodeRemoval\NodeRemover;
+use Rector\NodeTypeResolver\Node\AttributeKey;
 use Symplify\Astral\NodeTraverser\SimpleCallableNodeTraverser;
 
 final class ComplexNodeRemover
@@ -32,7 +34,8 @@ final class ComplexNodeRemover
         private readonly NodeRemover $nodeRemover,
         private readonly SideEffectNodeDetector $sideEffectNodeDetector,
         private readonly SimpleCallableNodeTraverser $simpleCallableNodeTraverser,
-        private readonly PropertyFetchAnalyzer $propertyFetchAnalyzer
+        private readonly PropertyFetchAnalyzer $propertyFetchAnalyzer,
+        private readonly NodeComparator $nodeComparator
     ) {
     }
 
@@ -40,18 +43,15 @@ final class ComplexNodeRemover
         Class_ $class,
         Property $property,
         bool $removeAssignSideEffect
-    ): void {
+    ): bool {
         $propertyName = $this->nodeNameResolver->getName($property);
-
-        $hasSideEffect = false;
-        $isPartOfAnotherAssign = false;
+        $totalPropertyFetch = $this->propertyFetchAnalyzer->countLocalPropertyFetchName($class, $propertyName);
 
         $this->simpleCallableNodeTraverser->traverseNodesWithCallable($class->stmts, function (Node $node) use (
             $removeAssignSideEffect,
             $propertyName,
-            &$hasSideEffect,
-            &$isPartOfAnotherAssign
-        ) {
+            &$totalPropertyFetch
+        ): ?Node {
             // here should be checked all expr like stmts that can hold assign, e.f. if, foreach etc. etc.
             if (! $node instanceof Expression) {
                 return null;
@@ -68,7 +68,11 @@ final class ComplexNodeRemover
 
             // skip double assigns
             if ($assign->expr instanceof Assign) {
-                $isPartOfAnotherAssign = true;
+                return null;
+            }
+
+            $originalNode = $assign->getAttribute(AttributeKey::ORIGINAL_NODE);
+            if (! $this->nodeComparator->areNodesEqual($originalNode, $assign)) {
                 return null;
             }
 
@@ -77,32 +81,35 @@ final class ComplexNodeRemover
                 return null;
             }
 
+            $currentTotalPropertyFetch = $totalPropertyFetch;
             foreach ($propertyFetches as $propertyFetch) {
                 if ($this->nodeNameResolver->isName($propertyFetch->name, $propertyName)) {
                     if (! $removeAssignSideEffect && $this->sideEffectNodeDetector->detect($assign->expr)) {
-                        $hasSideEffect = true;
                         return null;
                     }
 
-                    $this->nodeRemover->removeNode($node);
+                    --$totalPropertyFetch;
                 }
             }
 
+            if ($totalPropertyFetch < $currentTotalPropertyFetch) {
+                $this->nodeRemover->removeNode($node);
+                return $node;
+            }
+
             return null;
         });
 
-        // do not remove anyhting in case of side-effect
-        if ($hasSideEffect) {
-            return;
-        }
-
-        if ($isPartOfAnotherAssign) {
-            return;
+        // not all property fetch with name removed
+        if ($totalPropertyFetch > 0) {
+            return false;
         }
 
         $this->removeConstructorDependency($class, $propertyName);
 
         $this->nodeRemover->removeNode($property);
+
+        return true;
     }
 
     /**

src/NodeAnalyzer/PropertyFetchAnalyzer.php

@@ -23,6 +23,7 @@ use Rector\Core\PhpParser\AstResolver;
 use Rector\Core\PhpParser\Node\BetterNodeFinder;
 use Rector\Core\ValueObject\MethodName;
 use Rector\NodeNameResolver\NodeNameResolver;
+use Symplify\Astral\NodeTraverser\SimpleCallableNodeTraverser;
 
 final class PropertyFetchAnalyzer
 {
@@ -34,7 +35,8 @@ final class PropertyFetchAnalyzer
     public function __construct(
         private readonly NodeNameResolver $nodeNameResolver,
         private readonly BetterNodeFinder $betterNodeFinder,
-        private readonly AstResolver $astResolver
+        private readonly AstResolver $astResolver,
+        private readonly SimpleCallableNodeTraverser $simpleCallableNodeTraverser
     ) {
     }
 
@@ -72,11 +74,58 @@ final class PropertyFetchAnalyzer
         return $this->nodeNameResolver->isName($node->name, $desiredPropertyName);
     }
 
+    public function countLocalPropertyFetchName(ClassLike $classLike, string $propertyName): int
+    {
+        $total = 0;
+
+        $this->simpleCallableNodeTraverser->traverseNodesWithCallable($classLike->stmts, function (Node $subNode) use (
+            $classLike,
+            $propertyName,
+            &$total
+        ): ?Node {
+            if (! $this->isLocalPropertyFetchName($subNode, $propertyName)) {
+                return null;
+            }
+
+            $parentClassLike = $this->betterNodeFinder->findParentType($subNode, ClassLike::class);
+
+            // property fetch in Trait cannot get parent ClassLike
+            if (! $parentClassLike instanceof ClassLike) {
+                ++$total;
+            }
+
+            if ($parentClassLike === $classLike) {
+                ++$total;
+            }
+
+            return $subNode;
+        });
+
+        return $total;
+    }
+
     public function containsLocalPropertyFetchName(Node $node, string $propertyName): bool
     {
+        $classLike = $node instanceof ClassLike
+            ? $node
+            : $this->betterNodeFinder->findParentType($node, ClassLike::class);
+
         return (bool) $this->betterNodeFinder->findFirst(
             $node,
-            fn (Node $node): bool => $this->isLocalPropertyFetchName($node, $propertyName)
+            function (Node $node) use ($classLike, $propertyName): bool {
+                if (! $this->isLocalPropertyFetchName($node, $propertyName)) {
+                    return false;
+                }
+
+                $parentClassLike = $this->betterNodeFinder->findParentType($node, ClassLike::class);
+
+                // property fetch in Trait cannot get parent ClassLike
+                if (! $parentClassLike instanceof ClassLike) {
+                    return true;
+                }
+
+                return $parentClassLike === $classLike;
+            }
         );
     }