[CodingStyle] Improve unspread array (#5120)

* decouple VendorFileDetector * decouple SpreadVariablesCollector * [CodingStyle] Improve UnSpreadOperatorRector to join argument to array
rectorphp · Jan 9, 2021 · 4756535 · 4756535
1 parent f1d4a4a
commit 4756535
Show file tree

Hide file tree

Showing 7 changed files with 155 additions and 135 deletions.
diff --git a/rules/coding-style/src/Node.php b/rules/coding-style/src/Node.php
diff --git a/rules/coding-style/src/NodeAnalyzer/SpreadVariablesCollector.php b/rules/coding-style/src/NodeAnalyzer/SpreadVariablesCollector.php
@@ -0,0 +1,36 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\CodingStyle\NodeAnalyzer;
+
+use PhpParser\Node\Param;
+use PhpParser\Node\Stmt\ClassMethod;
+use Rector\NodeTypeResolver\Node\AttributeKey;
+
+final class SpreadVariablesCollector
+{
+    /**
+     * @return array<int, Param>
+     */
+    public function resolveFromClassMethod(ClassMethod $classMethod): array
+    {
+        $spreadParams = [];
+
+        foreach ($classMethod->params as $key => $param) {
+            // prevent race-condition removal on class method
+            $originalParam = $param->getAttribute(AttributeKey::ORIGINAL_NODE);
+            if (! $originalParam instanceof Param) {
+                continue;
+            }
+
+            if (! $originalParam->variadic) {
+                continue;
+            }
+
+            $spreadParams[$key] = $param;
+        }
+
+        return $spreadParams;
+    }
+}
diff --git a/rules/coding-style/src/Rector/ClassMethod/UnSpreadOperatorRector.php b/rules/coding-style/src/Rector/ClassMethod/UnSpreadOperatorRector.php
@@ -4,20 +4,13 @@
 
 namespace Rector\CodingStyle\Rector\ClassMethod;
 
-use Nette\Utils\Strings;
 use PhpParser\Node;
 use PhpParser\Node\Arg;
 use PhpParser\Node\Expr\MethodCall;
-use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Identifier;
-use PhpParser\Node\Param;
-use PhpParser\Node\Stmt\Class_;
 use PhpParser\Node\Stmt\ClassMethod;
-use PHPStan\Type\ObjectType;
-use PHPStan\Type\ThisType;
+use Rector\CodingStyle\NodeAnalyzer\SpreadVariablesCollector;
 use Rector\Core\Rector\AbstractRector;
-use Rector\NodeTypeResolver\Node\AttributeKey;
-use ReflectionClass;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
 use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
 
@@ -27,10 +20,14 @@
 final class UnSpreadOperatorRector extends AbstractRector
 {
     /**
-     * @see https://regex101.com/r/ChpDsj/1
-     * @var string
+     * @var SpreadVariablesCollector
      */
-    private const ANONYMOUS_CLASS_REGEX = '#^AnonymousClass[\w+]#';
+    private $spreadVariablesCollector;
+
+    public function __construct(SpreadVariablesCollector $spreadVariablesCollector)
+    {
+        $this->spreadVariablesCollector = $spreadVariablesCollector;
+    }
 
     public function getRuleDefinition(): RuleDefinition
     {
@@ -87,135 +84,70 @@ public function refactor(Node $node): ?Node
         return $this->processUnspreadOperatorMethodCallArgs($node);
     }
 
-    private function getClassFileNameByClassMethod(ClassMethod $classMethod): ?string
-    {
-        $parent = $classMethod->getAttribute(AttributeKey::PARENT_NODE);
-        if (! $parent instanceof Class_) {
-            return null;
-        }
-
-        $shortClassName = (string) $parent->name;
-        if (Strings::match($shortClassName, self::ANONYMOUS_CLASS_REGEX)) {
-            return null;
-        }
-
-        $reflectionClass = new ReflectionClass((string) $parent->namespacedName);
-        return (string) $reflectionClass->getFileName();
-    }
-
-    private function getClassFileNameByMethodCall(MethodCall $methodCall): ?string
-    {
-        $scope = $methodCall->getAttribute(AttributeKey::SCOPE);
-        if ($scope === null) {
-            return null;
-        }
-
-        $type = $scope->getType($methodCall->var);
-        if ($type instanceof ObjectType) {
-            $classReflection = $type->getClassReflection();
-            if ($classReflection === null) {
-                return null;
-            }
-
-            return (string) $classReflection->getFileName();
-        }
-
-        if ($type instanceof ThisType) {
-            $staticObjectType = $type->getStaticObjectType();
-            $classReflection = $staticObjectType->getClassReflection();
-            if ($classReflection === null) {
-                return null;
-            }
-
-            return (string) $classReflection->getFileName();
-        }
-
-        return null;
-    }
-
     private function processUnspreadOperatorClassMethodParams(ClassMethod $classMethod): ?ClassMethod
     {
-        if ($this->isInVendor($classMethod)) {
-            return null;
-        }
-
-        $params = $classMethod->params;
-        if ($params === []) {
-            return null;
-        }
-
-        $spreadVariables = $this->getSpreadVariables($params);
-        if ($spreadVariables === []) {
+        $spreadParams = $this->spreadVariablesCollector->resolveFromClassMethod($classMethod);
+        if ($spreadParams === []) {
             return null;
         }
 
-        foreach (array_keys($spreadVariables) as $key) {
-            $classMethod->params[$key]->variadic = false;
-            $classMethod->params[$key]->type = new Identifier('array');
+        foreach ($spreadParams as $spreadParam) {
+            $spreadParam->variadic = false;
+            $spreadParam->type = new Identifier('array');
         }
 
         return $classMethod;
     }
 
-    /**
-     * @param ClassMethod|MethodCall $node
-     */
-    private function isInVendor(Node $node): bool
-    {
-        $scope = $node->getAttribute(AttributeKey::SCOPE);
-        $fileName = $node instanceof ClassMethod
-            ? $this->getClassFileNameByClassMethod($node)
-            : $this->getClassFileNameByMethodCall($node);
-
-        if ($fileName === null) {
-            return false;
-        }
-
-        return Strings::contains($fileName, 'vendor');
-    }
-
     private function processUnspreadOperatorMethodCallArgs(MethodCall $methodCall): ?MethodCall
     {
-        if ($this->isInVendor($methodCall)) {
+        $classMethod = $this->nodeRepository->findClassMethodByMethodCall($methodCall);
+        if ($classMethod === null) {
             return null;
         }
 
-        $args = $methodCall->args;
-        if ($args === []) {
+        $spreadParams = $this->spreadVariablesCollector->resolveFromClassMethod($classMethod);
+        if ($spreadParams === []) {
             return null;
         }
 
-        $spreadVariables = $this->getSpreadVariables($args);
-        if ($spreadVariables === []) {
-            return null;
+        $firstSpreadParamPosition = array_key_first($spreadParams);
+        $variadicArgs = $this->resolveVariadicArgsByVariadicParams($methodCall, $firstSpreadParamPosition);
+
+        $hasUnpacked = false;
+
+        foreach ($variadicArgs as $position => $variadicArg) {
+            if ($variadicArg->unpack) {
+                $variadicArg->unpack = false;
+                $hasUnpacked = true;
+                $methodCall->args[$position] = $variadicArg;
+            }
         }
 
-        foreach (array_keys($spreadVariables) as $key) {
-            $methodCall->args[$key]->unpack = false;
+        if ($hasUnpacked) {
+            return $methodCall;
         }
 
+        $methodCall->args[$firstSpreadParamPosition] = new Arg($this->createArray($variadicArgs));
         return $methodCall;
     }
 
     /**
-     * @param Param[]|Arg[] $array
-     * @return Param[]|Arg[]
+     * @return Arg[]
      */
-    private function getSpreadVariables(array $array): array
+    private function resolveVariadicArgsByVariadicParams(MethodCall $methodCall, int $firstSpreadParamPosition): array
     {
-        $spreadVariables = [];
-        foreach ($array as $key => $paramOrArg) {
-            if ($paramOrArg instanceof Param && (! $paramOrArg->variadic || $paramOrArg->type !== null)) {
-                continue;
-            }
+        $variadicArgs = [];
 
-            if ($paramOrArg instanceof Arg && (! $paramOrArg->unpack || ! $paramOrArg->value instanceof Variable)) {
+        foreach ($methodCall->args as $position => $arg) {
+            if ($position < $firstSpreadParamPosition) {
                 continue;
             }
 
-            $spreadVariables[$key] = $paramOrArg;
+            $variadicArgs[] = $arg;
+            unset($methodCall->args[$position]);
         }
 
-        return $spreadVariables;
+        return $variadicArgs;
     }
 }
diff --git a/.../Rector/ClassMethod/UnSpreadOperatorRector/Fixture/handle_multiple_items_to_array.php.inc b/.../Rector/ClassMethod/UnSpreadOperatorRector/Fixture/handle_multiple_items_to_array.php.inc
@@ -0,0 +1,35 @@
+<?php
+
+namespace Rector\CodingStyle\Tests\Rector\ClassMethod\UnSpreadOperatorRector\Fixture;
+
+final class HandleMultipleItemsToArray
+{
+    public function go()
+    {
+        $this->run('John', 1, 2, 3, 4, 5);
+    }
+
+    public function run(string $name, ...$items)
+    {
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\CodingStyle\Tests\Rector\ClassMethod\UnSpreadOperatorRector\Fixture;
+
+final class HandleMultipleItemsToArray
+{
+    public function go()
+    {
+        $this->run('John', [1, 2, 3, 4, 5]);
+    }
+
+    public function run(string $name, array $items)
+    {
+    }
+}
+
+?>
diff --git a/...tests/Rector/ClassMethod/UnSpreadOperatorRector/Fixture/skip_typed_param_variadic.php.inc b/...tests/Rector/ClassMethod/UnSpreadOperatorRector/Fixture/skip_typed_param_variadic.php.inc
diff --git a/...tyle/tests/Rector/ClassMethod/UnSpreadOperatorRector/Fixture/typed_param_variadic.php.inc b/...tyle/tests/Rector/ClassMethod/UnSpreadOperatorRector/Fixture/typed_param_variadic.php.inc
@@ -0,0 +1,25 @@
+<?php
+
+namespace Rector\CodingStyle\Tests\Rector\ClassMethod\UnSpreadOperatorRector\Fixture;
+
+final class TypedParamVariadic
+{
+    public function run(int ...$var)
+    {
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\CodingStyle\Tests\Rector\ClassMethod\UnSpreadOperatorRector\Fixture;
+
+final class TypedParamVariadic
+{
+    public function run(array $var)
+    {
+    }
+}
+
+?>
diff --git a/src/PhpParser/Node/NodeFactory.php b/src/PhpParser/Node/NodeFactory.php
@@ -516,19 +516,17 @@ private function createArrayItem($item, $key = null): ArrayItem
             $arrayItem = new ArrayItem($this->createArray($item));
         }
 
-        if ($arrayItem !== null) {
-            if ($key === null) {
-                return $arrayItem;
-            }
-
-            $arrayItem->key = BuilderHelpers::normalizeValue($key);
+        if ($item instanceof ClassConstFetch) {
+            $itemValue = BuilderHelpers::normalizeValue($item);
+            $arrayItem = new ArrayItem($itemValue);
+        }
 
-            return $arrayItem;
+        if ($item instanceof Arg) {
+            $arrayItem = new ArrayItem($item->value);
         }
 
-        if ($item instanceof ClassConstFetch) {
-            $itemValue = BuilderHelpers::normalizeValue($item);
-            return new ArrayItem($itemValue);
+        if ($arrayItem !== null) {
+            return $this->createArrayItemWithKey($key, $arrayItem);
         }
 
         throw new NotImplementedException(sprintf(
@@ -618,4 +616,16 @@ private function createClassConstant(string $name, $value, int $modifier): Class
 
         return $classConst;
     }
+
+    /**
+     * @param int|string|null $key
+     */
+    private function createArrayItemWithKey($key, ArrayItem $arrayItem): ArrayItem
+    {
+        if ($key !== null) {
+            $arrayItem->key = BuilderHelpers::normalizeValue($key);
+        }
+
+        return $arrayItem;
+    }
 }