Merge pull request #2787 from rectorphp/remove-setter-only

packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php

@@ -5,6 +5,8 @@
 namespace Rector\DeadCode\Rector\Class_;
 
 use PhpParser\Node;
+use PhpParser\Node\Expr\PropertyFetch;
+use PhpParser\Node\Expr\StaticPropertyFetch;
 use PhpParser\Node\Stmt\Class_;
 use PhpParser\Node\Stmt\ClassMethod;
 use PhpParser\Node\Stmt\Interface_;
@@ -15,6 +17,7 @@
 use Rector\Rector\AbstractRector;
 use Rector\RectorDefinition\CodeSample;
 use Rector\RectorDefinition\RectorDefinition;
+use Rector\TypeDeclaration\VendorLock\VendorLockResolver;
 
 /**
  * @sponsor Thanks https://spaceflow.io/ for sponsoring this rule - visit them on https://github.com/SpaceFlow-app
@@ -28,9 +31,15 @@ final class RemoveSetterOnlyPropertyAndMethodCallRector extends AbstractRector
      */
     private $propertyManipulator;
 
-    public function __construct(PropertyManipulator $propertyManipulator)
+    /**
+     * @var VendorLockResolver
+     */
+    private $vendorLockResolver;
+
+    public function __construct(PropertyManipulator $propertyManipulator, VendorLockResolver $vendorLockResolver)
     {
         $this->propertyManipulator = $propertyManipulator;
+        $this->vendorLockResolver = $vendorLockResolver;
     }
 
     public function getDefinition(): RectorDefinition
@@ -92,29 +101,25 @@ public function refactor(Node $node): ?Node
             return null;
         }
 
-        if ($this->propertyManipulator->isPropertyUsedInReadContext($node)) {
-            return null;
-        }
-
         $propertyFetches = $this->propertyManipulator->getAllPropertyFetch($node);
+        $classMethodsToCheck = $this->collectClassMethodsToCheck($propertyFetches);
 
-        $methodsToCheck = [];
-        foreach ($propertyFetches as $propertyFetch) {
-            $methodName = $propertyFetch->getAttribute(AttributeKey::METHOD_NAME);
-            if ($methodName !== '__construct') {
-                //this rector does not remove empty constructors
-                $methodsToCheck[$methodName] =
-                    $propertyFetch->getAttribute(AttributeKey::METHOD_NODE);
-            }
-        }
+        $vendorLockedClassMethodNames = $this->getVendorLockedClassMethodNames($classMethodsToCheck);
 
-        $this->removePropertyAndUsages($node);
+        $this->removePropertyAndUsages($node, $vendorLockedClassMethodNames);
 
         /** @var ClassMethod $method */
-        foreach ($methodsToCheck as $method) {
-            if ($this->methodHasNoStmtsLeft($method)) {
-                $this->removeClassMethodAndUsages($method);
+        foreach ($classMethodsToCheck as $method) {
+            if (! $this->methodHasNoStmtsLeft($method)) {
+                continue;
+            }
+
+            $classMethodName = $this->getName($method->name);
+            if (in_array($classMethodName, $vendorLockedClassMethodNames, true)) {
+                continue;
             }
+
+            $this->removeClassMethodAndUsages($method);
         }
 
         return $node;
@@ -138,6 +143,63 @@ private function shouldSkipProperty(PropertyProperty $propertyProperty): bool
 
         /** @var Class_|Interface_|Trait_|null $classNode */
         $classNode = $propertyProperty->getAttribute(AttributeKey::CLASS_NODE);
-        return $classNode === null || $classNode instanceof Trait_ || $classNode instanceof Interface_;
+        if ($classNode === null) {
+            return true;
+        }
+
+        if ($classNode instanceof Trait_) {
+            return true;
+        }
+
+        if ($classNode instanceof Interface_) {
+            return true;
+        }
+
+        return $this->propertyManipulator->isPropertyUsedInReadContext($propertyProperty);
+    }
+
+    /**
+     * @param PropertyFetch[]|StaticPropertyFetch[] $propertyFetches
+     * @return ClassMethod[]
+     */
+    private function collectClassMethodsToCheck(array $propertyFetches): array
+    {
+        $classMethodsToCheck = [];
+
+        foreach ($propertyFetches as $propertyFetch) {
+            $methodName = $propertyFetch->getAttribute(AttributeKey::METHOD_NAME);
+            // this rector does not remove empty constructors
+            if ($methodName === '__construct') {
+                continue;
+            }
+
+            /** @var ClassMethod|null $classMethod */
+            $classMethod = $propertyFetch->getAttribute(AttributeKey::METHOD_NODE);
+            if ($classMethod === null) {
+                continue;
+            }
+
+            $classMethodsToCheck[$methodName] = $classMethod;
+        }
+
+        return $classMethodsToCheck;
+    }
+
+    /**
+     * @param ClassMethod[] $methodsToCheck
+     * @return string[]
+     */
+    private function getVendorLockedClassMethodNames(array $methodsToCheck): array
+    {
+        $vendorLockedClassMethodsNames = [];
+        foreach ($methodsToCheck as $method) {
+            if (! $this->vendorLockResolver->isClassMethodRemovalVendorLocked($method)) {
+                continue;
+            }
+
+            $vendorLockedClassMethodsNames[] = $this->getName($method);
+        }
+
+        return $vendorLockedClassMethodsNames;
     }
 }

packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/skip_abstract_class_required.php.inc

@@ -0,0 +1,17 @@
+<?php
+
+namespace Rector\DeadCode\Tests\Rector\Property\RemoveSetterOnlyPropertyAndMethodCallRector\StaticProperty;
+
+class SkipAbstractClassRequired extends AbstractClassRequiring
+{
+    private $name;
+    public function setName(string $name): void
+    {
+        $this->name = $name;
+    }
+}
+
+abstract class AbstractClassRequiring
+{
+    protected abstract function setName(string $name);
+}

packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/skip_interface_required.php.inc

@@ -0,0 +1,17 @@
+<?php
+
+namespace Rector\DeadCode\Tests\Rector\Property\RemoveSetterOnlyPropertyAndMethodCallRector\StaticProperty;
+
+class SkipInterfaceRequired implements ParentInterface
+{
+    private $name;
+    public function setName(string $name): void
+    {
+        $this->name = $name;
+    }
+}
+
+interface ParentInterface
+{
+    public function setName(string $name): void;
+}

packages/TypeDeclaration/src/VendorLock/VendorLockResolver.php

@@ -15,6 +15,7 @@
 use Rector\NodeTypeResolver\Node\AttributeKey;
 use Rector\PhpParser\Node\Manipulator\ClassManipulator;
 use Rector\PhpParser\Node\Resolver\NameResolver;
+use ReflectionClass;
 
 final class VendorLockResolver
 {
@@ -212,6 +213,65 @@ public function isPropertyChangeVendorLockedIn(Property $property): bool
         return false;
     }
 
+    /**
+     * Checks for:
+     * - interface required methods
+     * - abstract classes reqired method
+     *
+     * Prevent:
+     * - removing class methods, that breaks the code
+     */
+    public function isClassMethodRemovalVendorLocked(ClassMethod $classMethod): bool
+    {
+        $classMethodName = $this->nameResolver->getName($classMethod);
+
+        /** @var Class_|null $class */
+        $class = $classMethod->getAttribute(AttributeKey::CLASS_NODE);
+        if ($class === null) {
+            return false;
+        }
+
+        // required by interface?
+        foreach ($class->implements as $implement) {
+            $implementedInterfaceName = $this->nameResolver->getName($implement);
+
+            if (interface_exists($implementedInterfaceName)) {
+                $interfaceMethods = get_class_methods($implementedInterfaceName);
+                if (in_array($classMethodName, $interfaceMethods, true)) {
+                    return true;
+                }
+            }
+        }
+
+        if ($class->extends === null) {
+            return false;
+        }
+
+        /** @var string $className */
+        $className = $classMethod->getAttribute(AttributeKey::CLASS_NAME);
+
+        $classParents = class_parents($className);
+        foreach ($classParents as $classParent) {
+            if (! class_exists($classParent)) {
+                continue;
+            }
+
+            $parentClassReflection = new ReflectionClass($classParent);
+            if (! $parentClassReflection->hasMethod($classMethodName)) {
+                continue;
+            }
+
+            $methodReflection = $parentClassReflection->getMethod($classMethodName);
+            if (! $methodReflection->isAbstract()) {
+                continue;
+            }
+
+            return true;
+        }
+
+        return false;
+    }
+
     // Until we have getProperty (https://github.com/nikic/PHP-Parser/pull/646)
     private function getProperty(ClassLike $classLike, string $name)
     {

src/Rector/AbstractRector/ComplexRemovalTrait.php

@@ -87,10 +87,22 @@ protected function removeClassMethodAndUsages(ClassMethod $classMethod): void
         }
     }
 
-    protected function removePropertyAndUsages(PropertyProperty $propertyProperty): void
-    {
+    /**
+     * @param string[] $classMethodNamesToSkip
+     */
+    protected function removePropertyAndUsages(
+        PropertyProperty $propertyProperty,
+        array $classMethodNamesToSkip = []
+    ): void {
+        $shouldKeepProperty = false;
+
         $propertyFetches = $this->propertyManipulator->getAllPropertyFetch($propertyProperty);
         foreach ($propertyFetches as $propertyFetch) {
+            if ($this->shouldSkipPropertyForClassMethod($propertyFetch, $classMethodNamesToSkip)) {
+                $shouldKeepProperty = true;
+                continue;
+            }
+
             $assign = $propertyFetch->getAttribute(AttributeKey::PARENT_NODE);
 
             while ($assign !== null && ! $assign instanceof Assign) {
@@ -104,6 +116,10 @@ protected function removePropertyAndUsages(PropertyProperty $propertyProperty):
             $this->removeAssignNode($assign);
         }
 
+        if ($shouldKeepProperty) {
+            return;
+        }
+
         /** @var Property $property */
         $property = $propertyProperty->getAttribute(AttributeKey::PARENT_NODE);
         $this->removeNode($propertyProperty);
@@ -223,4 +239,24 @@ private function removeMethodCall(Node $node): void
         }
         $this->removeNode($node);
     }
+
+    /**
+     * @param StaticPropertyFetch|PropertyFetch $expr
+     * @param string[] $classMethodNamesToSkip
+     */
+    private function shouldSkipPropertyForClassMethod(Expr $expr, array $classMethodNamesToSkip): bool
+    {
+        /** @var ClassMethod|null $classMethodNode */
+        $classMethodNode = $expr->getAttribute(AttributeKey::METHOD_NODE);
+        if ($classMethodNode === null) {
+            return false;
+        }
+
+        $classMethodName = $this->getName($classMethodNode);
+        if ($classMethodName === null) {
+            return false;
+        }
+
+        return in_array($classMethodName, $classMethodNamesToSkip, true);
+    }
 }