[DeadCode] Improve unused private property detection (#4430)

Co-authored-by: rector-bot <tomas@getrector.org>

phpstan.neon

@@ -990,3 +990,5 @@ parameters:
         - '#Method Rector\\Utils\\PHPStanAttributeTypeSyncer\\NodeFactory\\AttributeAwareClassFactoryFactory\:\:createFromPhpDocParserNodeClass\(\) should return PhpParser\\Node\\Stmt\\Namespace_ but returns PhpParser\\Node#'
         - '#Method Rector\\Utils\\PHPStanAttributeTypeSyncer\\NodeFactory\\AttributeAwareClassFactory\:\:createFromPhpDocParserNodeClass\(\) should return PhpParser\\Node\\Stmt\\Namespace_ but returns PhpParser\\Node#'
         - '#Method Rector\\NetteKdyby\\NodeFactory\\EventValueObjectClassFactory\:\:wrapClassToNamespace\(\) should return PhpParser\\Node\\Stmt\\Namespace_ but returns PhpParser\\Node#'
+
+        - '#Cannot cast \(array<string\>&nonEmpty\)\|string\|true to string#'

rules/dead-code/src/Rector/Property/RemoveSetterOnlyPropertyAndMethodCallRector.php

@@ -102,7 +102,7 @@ CODE_SAMPLE
             return null;
         }
 
-        $propertyFetches = $this->propertyManipulator->getAllPropertyFetch($node);
+        $propertyFetches = $this->propertyManipulator->getPrivatePropertyFetches($node);
         $classMethodsToCheck = $this->collectClassMethodsToCheck($propertyFetches);
 
         $vendorLockedClassMethodNames = $this->getVendorLockedClassMethodNames($classMethodsToCheck);

rules/dead-code/tests/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/skip_read_dim_fetch_use.php.inc

@@ -0,0 +1,23 @@
+<?php
+
+namespace Rector\DeadCode\Tests\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;
+
+final class SkipReadDimFetchUse
+{
+    /**
+     * @var string
+     */
+    private $key;
+
+    public function __construct(string $key)
+    {
+        $this->key = $key;
+    }
+
+    public function buildData(): array
+    {
+        $data[$this->key] = 100000000;
+
+        return $data;
+    }
+}

rules/dead-code/tests/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/write_only_dim_fetch_use.php.inc

@@ -0,0 +1,40 @@
+<?php
+
+namespace Rector\DeadCode\Tests\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;
+
+final class WriteOnlyDimFetchUse
+{
+    /**
+     * @var string
+     */
+    private $key;
+
+    public function __construct(string $key)
+    {
+        $this->key = $key;
+    }
+
+    public function buildData(): array
+    {
+        $data[$this->key] = 10000;
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\DeadCode\Tests\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;
+
+final class WriteOnlyDimFetchUse
+{
+    public function __construct()
+    {
+    }
+
+    public function buildData(): array
+    {
+    }
+}
+
+?>

rules/privatization/config/config.php

@@ -9,7 +9,8 @@ return static function (ContainerConfigurator $containerConfigurator): void {
 
     $services->defaults()
         ->public()
-        ->autowire();
+        ->autowire()
+        ->autoconfigure();
 
     $services->load('Rector\Privatization\\', __DIR__ . '/../src')
         ->exclude([__DIR__ . '/../src/Rector']);

rules/privatization/tests/Rector/Property/PrivatizeLocalPropertyToPrivatePropertyRector/Fixture/skip_dim_fetch_dispatcher.php.inc

@@ -0,0 +1,28 @@
+<?php
+
+namespace Rector\Privatization\Tests\Rector\Property\PrivatizeLocalPropertyToPrivatePropertyRector\Fixture;
+
+use Rector\Privatization\Tests\Rector\Property\PrivatizeLocalPropertyToPrivatePropertyRector\Source\EventInterface;
+
+final class SkipDimFetchDispatcher
+{
+    /**
+     * @var EventInterface[]
+     */
+    private $dispatched = [];
+
+    public function dispatch(EventInterface $event)
+    {
+        $this->dispatched[] = $event;
+
+        return $event;
+    }
+
+    /**
+     * @return EventInterface[]
+     */
+    public function getDispatchedEvents(): array
+    {
+        return $this->dispatched;
+    }
+}

rules/privatization/tests/Rector/Property/PrivatizeLocalPropertyToPrivatePropertyRector/Source/EventInterface.php

@@ -0,0 +1,10 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Privatization\Tests\Rector\Property\PrivatizeLocalPropertyToPrivatePropertyRector\Source;
+
+interface EventInterface
+{
+
+}

src/Exception/Node/MissingParentNodeException.php

@@ -0,0 +1,11 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\Exception\Node;
+
+use Exception;
+
+final class MissingParentNodeException extends Exception
+{
+}

src/NodeAnalyzer/ReadExprAnalyzer.php

@@ -0,0 +1,66 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\NodeAnalyzer;
+
+use PhpParser\Node\Expr;
+use PhpParser\Node\Expr\Variable;
+use PhpParser\Node\Stmt\Return_;
+use Rector\Core\Exception\NotImplementedYetException;
+use Rector\Core\NodeFinder\NodeUsageFinder;
+use Rector\NodeNestingScope\ParentScopeFinder;
+use Rector\NodeTypeResolver\Node\AttributeKey;
+
+final class ReadExprAnalyzer
+{
+    /**
+     * @var ParentScopeFinder
+     */
+    private $parentScopeFinder;
+
+    /**
+     * @var NodeUsageFinder
+     */
+    private $nodeUsageFinder;
+
+    public function __construct(ParentScopeFinder $parentScopeFinder, NodeUsageFinder $nodeUsageFinder)
+    {
+        $this->parentScopeFinder = $parentScopeFinder;
+        $this->nodeUsageFinder = $nodeUsageFinder;
+    }
+
+    /**
+     * Is the value read or used for read purpose (at least, not only)
+     */
+    public function isExprRead(Expr $expr): bool
+    {
+        if ($expr instanceof Variable) {
+            $parentScope = $this->parentScopeFinder->find($expr);
+            if ($parentScope === null) {
+                return false;
+            }
+
+            $variableUsages = $this->nodeUsageFinder->findVariableUsages((array) $parentScope->stmts, $expr);
+            foreach ($variableUsages as $variableUsage) {
+                if ($this->isCurrentContextRead($variableUsage)) {
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
+        throw new NotImplementedYetException(get_class($expr));
+    }
+
+    private function isCurrentContextRead(Expr $expr): bool
+    {
+        $parent = $expr->getAttribute(AttributeKey::PARENT_NODE);
+        if ($parent instanceof Return_) {
+            return true;
+        }
+
+        throw new NotImplementedYetException();
+    }
+}

src/NodeAnalyzer/ReadWritePropertyAnalyzer.php

@@ -0,0 +1,107 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\NodeAnalyzer;
+
+use PhpParser\Node;
+use PhpParser\Node\Arg;
+use PhpParser\Node\Expr\ArrayDimFetch;
+use PhpParser\Node\Expr\PostDec;
+use PhpParser\Node\Expr\PostInc;
+use PhpParser\Node\Expr\PreDec;
+use PhpParser\Node\Expr\PreInc;
+use PhpParser\Node\Expr\PropertyFetch;
+use PhpParser\Node\Expr\StaticPropertyFetch;
+use Rector\Core\Exception\Node\MissingParentNodeException;
+use Rector\Core\PhpParser\Node\Manipulator\AssignManipulator;
+use Rector\NodeTypeResolver\Node\AttributeKey;
+use Rector\SOLID\Guard\VariableToConstantGuard;
+use Webmozart\Assert\Assert;
+
+final class ReadWritePropertyAnalyzer
+{
+    /**
+     * @var VariableToConstantGuard
+     */
+    private $variableToConstantGuard;
+
+    /**
+     * @var AssignManipulator
+     */
+    private $assignManipulator;
+
+    /**
+     * @var ReadExprAnalyzer
+     */
+    private $readExprAnalyzer;
+
+    public function __construct(
+        VariableToConstantGuard $variableToConstantGuard,
+        AssignManipulator $assignManipulator,
+        ReadExprAnalyzer $readExprAnalyzer
+    ) {
+        $this->variableToConstantGuard = $variableToConstantGuard;
+        $this->assignManipulator = $assignManipulator;
+        $this->readExprAnalyzer = $readExprAnalyzer;
+    }
+
+    /**
+     * @param PropertyFetch|StaticPropertyFetch $node
+     */
+    public function isRead(Node $node): bool
+    {
+        Assert::isAnyOf($node, [PropertyFetch::class, StaticPropertyFetch::class]);
+
+        $parent = $node->getAttribute(AttributeKey::PARENT_NODE);
+        if ($parent === null) {
+            throw new MissingParentNodeException();
+        }
+
+        $parent = $this->unwrapPostPreIncDec($parent);
+
+        if ($parent instanceof Arg) {
+            $readArg = $this->variableToConstantGuard->isReadArg($parent);
+            if ($readArg) {
+                return true;
+            }
+        }
+
+        if ($parent instanceof ArrayDimFetch && $parent->dim === $node) {
+            return $this->isArrayDimFetchRead($parent);
+        }
+
+        return ! $this->assignManipulator->isNodeLeftPartOfAssign($node);
+    }
+
+    private function unwrapPostPreIncDec(Node $node): Node
+    {
+        if ($node instanceof PreInc || $node instanceof PreDec || $node instanceof PostInc || $node instanceof PostDec) {
+            $node = $node->getAttribute(AttributeKey::PARENT_NODE);
+            if ($node === null) {
+                throw new MissingParentNodeException();
+            }
+        }
+
+        return $node;
+    }
+
+    private function isArrayDimFetchRead(ArrayDimFetch $arrayDimFetch): bool
+    {
+        $parentParent = $arrayDimFetch->getAttribute(AttributeKey::PARENT_NODE);
+        if ($parentParent === null) {
+            throw new MissingParentNodeException();
+        }
+
+        if (! $this->assignManipulator->isNodeLeftPartOfAssign($arrayDimFetch)) {
+            return false;
+        }
+
+        // the array dim fetch is assing here only; but the variable might be used later
+        if ($this->readExprAnalyzer->isExprRead($arrayDimFetch->var)) {
+            return true;
+        }
+
+        return ! $this->assignManipulator->isNodeLeftPartOfAssign($arrayDimFetch);
+    }
+}

src/NodeFinder/NodeUsageFinder.php

@@ -0,0 +1,50 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\NodeFinder;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr\Variable;
+use Rector\Core\PhpParser\Node\BetterNodeFinder;
+use Rector\NodeNameResolver\NodeNameResolver;
+
+final class NodeUsageFinder
+{
+    /**
+     * @var NodeNameResolver
+     */
+    private $nodeNameResolver;
+
+    /**
+     * @var BetterNodeFinder
+     */
+    private $betterNodeFinder;
+
+    public function __construct(NodeNameResolver $nodeNameResolver, BetterNodeFinder $betterNodeFinder)
+    {
+        $this->nodeNameResolver = $nodeNameResolver;
+        $this->betterNodeFinder = $betterNodeFinder;
+    }
+
+    /**
+     * @param Node[] $nodes
+     * @return Variable[]
+     */
+    public function findVariableUsages(array $nodes, Variable $variable): array
+    {
+        $variableName = $this->nodeNameResolver->getName($variable);
+
+        return $this->betterNodeFinder->find($nodes, function (Node $node) use ($variable, $variableName): bool {
+            if (! $node instanceof Variable) {
+                return false;
+            }
+
+            if ($node === $variable) {
+                return false;
+            }
+
+            return $this->nodeNameResolver->isName($node, $variableName);
+        });
+    }
+}

src/PhpParser/Node/Manipulator/PropertyManipulator.php

@@ -18,6 +18,7 @@ use PhpParser\Node\Stmt\ClassLike;
 use PhpParser\Node\Stmt\Property;
 use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfo;
 use Rector\BetterPhpDocParser\PhpDocNode\JMS\SerializerTypeTagValueNode;
+use Rector\Core\NodeAnalyzer\ReadWritePropertyAnalyzer;
 use Rector\Core\PhpParser\Node\BetterNodeFinder;
 use Rector\Core\PhpParser\Printer\BetterStandardPrinter;
 use Rector\Doctrine\AbstractRector\DoctrineTrait;
@@ -63,13 +64,19 @@ final class PropertyManipulator
      */
     private $nodeRepository;
 
+    /**
+     * @var ReadWritePropertyAnalyzer
+     */
+    private $readWritePropertyAnalyzer;
+
     public function __construct(
         AssignManipulator $assignManipulator,
         BetterNodeFinder $betterNodeFinder,
         BetterStandardPrinter $betterStandardPrinter,
         NodeNameResolver $nodeNameResolver,
         VariableToConstantGuard $variableToConstantGuard,
-        NodeRepository $nodeRepository
+        NodeRepository $nodeRepository,
+        ReadWritePropertyAnalyzer $readWritePropertyAnalyzer
     ) {
         $this->betterNodeFinder = $betterNodeFinder;
         $this->betterStandardPrinter = $betterStandardPrinter;
@@ -77,12 +84,13 @@ final class PropertyManipulator
         $this->assignManipulator = $assignManipulator;
         $this->variableToConstantGuard = $variableToConstantGuard;
         $this->nodeRepository = $nodeRepository;
+        $this->readWritePropertyAnalyzer = $readWritePropertyAnalyzer;
     }
 
     /**
-     * @return PropertyFetch[]
+     * @return PropertyFetch[]|StaticPropertyFetch[]
      */
-    public function getAllPropertyFetch(Property $property): array
+    public function getPrivatePropertyFetches(Property $property): array
     {
         /** @var Class_|null $classLike */
         $classLike = $property->getAttribute(AttributeKey::CLASS_NODE);
@@ -95,7 +103,7 @@ final class PropertyManipulator
 
         $singleProperty = $property->props[0];
 
-        /** @var PropertyFetch[] $propertyFetches */
+        /** @var PropertyFetch[]|StaticPropertyFetch[] $propertyFetches */
         $propertyFetches = $this->betterNodeFinder->find($nodesToSearch, function (Node $node) use (
             $singleProperty,
             $nodesToSearch
@@ -133,8 +141,9 @@ final class PropertyManipulator
             return true;
         }
 
-        foreach ($this->getAllPropertyFetch($property) as $propertyFetch) {
-            if ($this->isReadContext($propertyFetch)) {
+        $privatePropertyFetches = $this->getPrivatePropertyFetches($property);
+        foreach ($privatePropertyFetches as $propertyFetch) {
+            if ($this->readWritePropertyAnalyzer->isRead($propertyFetch)) {
                 return true;
             }
         }
@@ -148,7 +157,7 @@ final class PropertyManipulator
                 return false;
             }
 
-            if (! $this->isReadContext($node)) {
+            if (! $this->readWritePropertyAnalyzer->isRead($node)) {
                 return false;
             }
 
@@ -158,7 +167,7 @@ final class PropertyManipulator
 
     public function isPropertyChangeable(Property $property): bool
     {
-        foreach ($this->getAllPropertyFetch($property) as $propertyFetch) {
+        foreach ($this->getPrivatePropertyFetches($property) as $propertyFetch) {
             if ($this->isChangeableContext($propertyFetch)) {
                 return true;
             }
@@ -167,26 +176,6 @@ final class PropertyManipulator
         return false;
     }
 
-    /**
-     * @param PropertyFetch|StaticPropertyFetch $node
-     */
-    private function isReadContext(Node $node): bool
-    {
-        $parent = $node->getAttribute(AttributeKey::PARENT_NODE);
-        if ($parent instanceof PreInc || $parent instanceof PreDec || $parent instanceof PostInc || $parent instanceof PostDec) {
-            $parent = $parent->getAttribute(AttributeKey::PARENT_NODE);
-        }
-
-        if ($parent instanceof Arg) {
-            $readArg = $this->variableToConstantGuard->isReadArg($parent);
-            if ($readArg) {
-                return true;
-            }
-        }
-
-        return ! $this->assignManipulator->isNodeLeftPartOfAssign($node);
-    }
-
     /**
      * @param PropertyFetch|StaticPropertyFetch $node
      */

src/Rector/AbstractRector/ComplexRemovalTrait.php

@@ -91,7 +91,7 @@ trait ComplexRemovalTrait
     {
         $shouldKeepProperty = false;
 
-        $propertyFetches = $this->propertyManipulator->getAllPropertyFetch($property);
+        $propertyFetches = $this->propertyManipulator->getPrivatePropertyFetches($property);
         foreach ($propertyFetches as $propertyFetch) {
             if ($this->shouldSkipPropertyForClassMethod($propertyFetch, $classMethodNamesToSkip)) {
                 $shouldKeepProperty = true;