[DeadCode] Fix removal of variable-used promoted property (#593)

rectorphp · Aug 4, 2021 · 37b0602 · 37b0602
1 parent 1f538c6
commit 37b0602
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 177 deletions.
diff --git a/.github/workflows/code_analysis.yaml b/.github/workflows/code_analysis.yaml
@@ -29,6 +29,11 @@ jobs:
                         name: 'PHPStan for config'
                         run: composer phpstan-config
 
+                    -
+                        name: Commented Code
+                        run: vendor/bin/easy-ci check-commented-code src packages rules tests packages-tests rules-tests --line-limit 5 --ansi
+
+
                     # see https://github.com/rectorphp/rector-generator
                     -
                         name: 'Rector Generate From Recipe'

diff --git a/...dCode/Rector/ClassMethod/RemoveUnusedPromotedPropertyRector/Fixture/user_variable.php.inc b/...dCode/Rector/ClassMethod/RemoveUnusedPromotedPropertyRector/Fixture/user_variable.php.inc
@@ -0,0 +1,29 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\ClassMethod\RemoveUnusedPromotedPropertyRector\Fixture;
+
+final class UserVariable
+{
+    public function __construct(
+        private string $usedDependency
+    ) {
+        echo $usedDependency;
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\ClassMethod\RemoveUnusedPromotedPropertyRector\Fixture;
+
+final class UserVariable
+{
+    public function __construct(
+        string $usedDependency
+    ) {
+        echo $usedDependency;
+    }
+}
+
+?>
diff --git a/rules/DeadCode/Rector/ClassMethod/RemoveUnusedPromotedPropertyRector.php b/rules/DeadCode/Rector/ClassMethod/RemoveUnusedPromotedPropertyRector.php
@@ -5,6 +5,7 @@
 namespace Rector\DeadCode\Rector\ClassMethod;
 
 use PhpParser\Node;
+use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Stmt\Class_;
 use PhpParser\Node\Stmt\ClassMethod;
 use Rector\Core\NodeManipulator\PropertyManipulator;
@@ -90,26 +91,29 @@ public function refactor(Node $node): ?Node
         }
 
         foreach ($node->getParams() as $param) {
-            if ($param->flags === 0) {
+            // only private local scope; removing public property might be dangerous
+            if ($param->flags !== Class_::MODIFIER_PRIVATE) {
                 continue;
             }
 
             if ($this->propertyManipulator->isPropertyUsedInReadContext($param)) {
                 continue;
             }
 
-            // only private local scope; removing public property might be dangerous
-            if ($param->flags !== Class_::MODIFIER_PRIVATE) {
-                continue;
-            }
-
             $paramName = $this->getName($param);
 
             $propertyFetches = $this->propertyFetchFinder->findLocalPropertyFetchesByName($class, $paramName);
             if ($propertyFetches !== []) {
                 continue;
             }
 
+            // is variable used? only remove property, keep param
+            $variable = $this->betterNodeFinder->findVariableOfName((array) $node->stmts, $paramName);
+            if ($variable instanceof Variable) {
+                $param->flags = 0;
+                continue;
+            }
+
             // remove param
             $this->removeNode($param);
         }

diff --git a/rules/Naming/Naming/VariableNaming.php b/rules/Naming/Naming/VariableNaming.php
@@ -38,12 +38,6 @@ public function __construct(
     ) {
     }
 
-//    public function resolveFromNode(Node $node): ?string
-//    {
-//        $nodeType = $this->nodeTypeResolver->getStaticType($node);
-//        return $this->resolveFromNodeAndType($node, $nodeType);
-//    }
-
     public function resolveFromNodeAndType(Node $node, Type $type): ?string
     {
         $variableName = $this->resolveBareFromNode($node);
@@ -111,23 +105,6 @@ public function resolveFromFuncCallFirstArgumentWithSuffix(
         return $this->createCountedValueName($bareName, $scope);
     }
 
-//    private function resolveFromNodeAndType(Node $node, Type $type): ?string
-//    {
-//        $variableName = $this->resolveBareFromNode($node);
-//        if ($variableName === null) {
-//            return null;
-//        }
-//
-//        // adjust static to specific class
-//        if ($variableName === 'this' && $type instanceof ThisType) {
-//            $shortClassName = $this->nodeNameResolver->getShortName($type->getClassName());
-//            $variableName = lcfirst($shortClassName);
-//        }
-//
-//        $stringy = new Stringy($variableName);
-//        return (string) $stringy->camelize();
-//    }
-
     public function resolveFromNode(Node $node): ?string
     {
         $nodeType = $this->nodeTypeResolver->getStaticType($node);
@@ -174,23 +151,6 @@ private function resolveBareFromNode(Node $node): ?string
         return null;
     }
 
-//    private function unwrapNode(Node $node): ?Node
-//    {
-//        if ($node instanceof Arg) {
-//            return $node->value;
-//        }
-//
-//        if ($node instanceof Cast) {
-//            return $node->expr;
-//        }
-//
-//        if ($node instanceof Ternary) {
-//            return $node->if;
-//        }
-//
-//        return $node;
-//    }
-
     private function resolveParamNameFromArrayDimFetch(ArrayDimFetch $arrayDimFetch): ?string
     {
         while ($arrayDimFetch instanceof ArrayDimFetch) {
@@ -270,111 +230,6 @@ private function unwrapNode(Node $node): ?Node
         return $node;
     }
 
-//    private function resolveParamNameFromArrayDimFetch(ArrayDimFetch $arrayDimFetch): ?string
-//    {
-//        while ($arrayDimFetch instanceof ArrayDimFetch) {
-//            if ($arrayDimFetch->dim instanceof Scalar) {
-//                $valueName = $this->nodeNameResolver->getName($arrayDimFetch->var);
-//                $dimName = $this->valueResolver->getValue($arrayDimFetch->dim);
-//
-//                $stringy = new Stringy($dimName);
-//                $dimName = (string) $stringy->upperCamelize();
-//
-//                return $valueName . $dimName;
-//            }
-//
-//            $arrayDimFetch = $arrayDimFetch->var;
-//        }
-//
-//        return $this->resolveBareFromNode($arrayDimFetch);
-//    }
-
-//    private function resolveBareFromNode(Node $node): ?string
-//    {
-//        $node = $this->unwrapNode($node);
-//
-//        if ($node instanceof ArrayDimFetch) {
-//            return $this->resolveParamNameFromArrayDimFetch($node);
-//        }
-//
-//        if ($node instanceof PropertyFetch) {
-//            return $this->resolveFromPropertyFetch($node);
-//        }
-//
-//        if ($node instanceof MethodCall || $node instanceof NullsafeMethodCall || $node instanceof StaticCall) {
-//            return $this->resolveFromMethodCall($node);
-//        }
-//
-//        if ($node instanceof New_) {
-//            return $this->resolveFromNew($node);
-//        }
-//
-//        if ($node instanceof FuncCall) {
-//            return $this->resolveFromNode($node->name);
-//        }
-//
-//        if (! $node instanceof Node) {
-//            throw new NotImplementedYetException();
-//        }
-//
-//        $paramName = $this->nodeNameResolver->getName($node);
-//        if ($paramName !== null) {
-//            return $paramName;
-//        }
-//
-//        if ($node instanceof String_) {
-//            return $node->value;
-//        }
-//
-//        return null;
-//    }
-
-//    private function resolveFromNew(New_ $new): string
-//    {
-//        if ($new->class instanceof Name) {
-//            $className = $this->nodeNameResolver->getName($new->class);
-//            return $this->nodeNameResolver->getShortName($className);
-//        }
-//
-//        throw new NotImplementedYetException();
-//    }
-//
-//    /**
-//     * @param MethodCall|NullsafeMethodCall|StaticCall $node
-//     */
-//    private function resolveFromMethodCall(Node $node): ?string
-//    {
-//        if ($node->name instanceof MethodCall) {
-//            return $this->resolveFromMethodCall($node->name);
-//        }
-//
-//        $methodName = $this->nodeNameResolver->getName($node->name);
-//        if (! is_string($methodName)) {
-//            return null;
-//        }
-//
-//        return $methodName;
-//    }
-//
-//    private function resolveFromPropertyFetch(PropertyFetch $propertyFetch): string
-//    {
-//        $varName = $this->nodeNameResolver->getName($propertyFetch->var);
-//        if (! is_string($varName)) {
-//            throw new NotImplementedYetException();
-//        }
-//
-//        $propertyName = $this->nodeNameResolver->getName($propertyFetch->name);
-//        if (! is_string($propertyName)) {
-//            throw new NotImplementedYetException();
-//        }
-//
-//        if ($varName === 'this') {
-//            return $propertyName;
-//        }
-//
-//        return $varName . ucfirst($propertyName);
-//    }
-
     private function resolveBareFuncCallArgumentName(
         FuncCall $funcCall,
         string $fallbackName,

diff --git a/rules/Php72/Rector/Assign/ListEachRector.php b/rules/Php72/Rector/Assign/ListEachRector.php
@@ -90,10 +90,6 @@ public function refactor(Node $node): ?Node
         }
 
         // both: list($key, $value) = each($values);
-        // ↓
-        // $key = key($values);
-        // $value = current($values);
-        // next($values);
         $currentFuncCall = $this->nodeFactory->createFuncCall('current', $eachFuncCall->args);
 
         $secondArrayItem = $listNode->items[1];

diff --git a/src/PhpParser/Node/BetterNodeFinder.php b/src/PhpParser/Node/BetterNodeFinder.php
@@ -125,28 +125,6 @@ public function hasVariableOfName(Node | array $nodes, string $name): bool
         return (bool) $this->findVariableOfName($nodes, $name);
     }
 
-//    /**
-//     * @param Node|Stmt[] $nodes
-//     * @return Variable[]
-//     */
-//    public function findVariablesOfName(Node | array $nodes, string $variableName): array
-//    {
-//        /** @var Variable[] $variables */
-//        $variables = $this->findInstanceOf($nodes, Variable::class);
-//
-//        $variablesOfName = [];
-//
-//        foreach ($variables as $variable) {
-//            if (! $this->nodeNameResolver->isName($variable, $variableName)) {
-//                continue;
-//            }
-//
-//            $variablesOfName[] = $variable;
-//        }
-//
-//        return $variablesOfName;
-//    }
-
     /**
      * @param Node|Node[] $nodes
      * @return Variable|null