[CodeQuality] Handle possible crash on ParametersAcceptorSelector::se…

…lectSingle() on OptionalParametersAfterRequiredRector (#3192) Co-authored-by: GitHub Action <action@github.com>
rectorphp · Dec 13, 2022 · d80ddd1 · d80ddd1
1 parent de00876
commit d80ddd1
Show file tree

Hide file tree

Showing 12 changed files with 67 additions and 65 deletions.
diff --git a/build/target-repository/composer.json b/build/target-repository/composer.json
@@ -7,7 +7,7 @@
     ],
     "require": {
         "php": "^7.2|^8.0",
-        "phpstan/phpstan": "^1.9.2"
+        "phpstan/phpstan": "^1.9.3"
     },
     "autoload": {
         "files": [

diff --git a/composer.json b/composer.json
@@ -17,7 +17,7 @@
         "nikic/php-parser": "^4.15.2",
         "ondram/ci-detector": "^4.1",
         "phpstan/phpdoc-parser": "1.13.1",
-        "phpstan/phpstan": "^1.9.2",
+        "phpstan/phpstan": "^1.9.3",
         "phpstan/phpstan-phpunit": "^1.2.2",
         "react/event-loop": "^1.3",
         "react/socket": "^1.12",

diff --git a/packages/NodeTypeResolver/PHPStan/ParametersAcceptorSelectorVariantsWrapper.php b/packages/NodeTypeResolver/PHPStan/ParametersAcceptorSelectorVariantsWrapper.php
@@ -5,6 +5,7 @@
 namespace Rector\NodeTypeResolver\PHPStan;
 
 use PhpParser\Node\Expr\CallLike;
+use PhpParser\Node\FunctionLike;
 use PHPStan\Analyser\Scope;
 use PHPStan\Reflection\FunctionReflection;
 use PHPStan\Reflection\MethodReflection;
@@ -15,17 +16,20 @@ final class ParametersAcceptorSelectorVariantsWrapper
 {
     public static function select(
         FunctionReflection|MethodReflection $reflection,
-        CallLike $callLike,
+        CallLike|FunctionLike $node,
         Scope $scope
     ): ParametersAcceptor {
         $variants = $reflection->getVariants();
+        if ($node instanceof FunctionLike) {
+            return ParametersAcceptorSelector::selectSingle($variants);
+        }
 
-        if ($callLike->isFirstClassCallable()) {
+        if ($node->isFirstClassCallable()) {
             return ParametersAcceptorSelector::selectSingle($variants);
         }
 
         return count($variants) > 1
-            ? ParametersAcceptorSelector::selectFromArgs($scope, $callLike->getArgs(), $variants)
+            ? ParametersAcceptorSelector::selectFromArgs($scope, $node->getArgs(), $variants)
             : ParametersAcceptorSelector::selectSingle($variants);
     }
 }
diff --git a/packages/NodeTypeResolver/PHPStan/TypeHasher.php b/packages/NodeTypeResolver/PHPStan/TypeHasher.php
@@ -119,7 +119,7 @@ private function normalizeObjectType(Type $type): Type
                 return new FullyQualifiedObjectType($currentType->getFullyQualifiedName());
             }
 
-            if ($currentType instanceof ObjectType && ! $currentType instanceof GenericObjectType && ! $currentType instanceof AliasedObjectType && $currentType->getClassName() !== 'Iterator' && $currentType->getClassName() !== 'iterable') {
+            if ($currentType instanceof ObjectType && ! $currentType instanceof GenericObjectType && $currentType->getClassName() !== 'Iterator' && $currentType->getClassName() !== 'iterable') {
                 return new FullyQualifiedObjectType($currentType->getClassName());
             }
 

diff --git a/phpstan.neon b/phpstan.neon
@@ -393,14 +393,20 @@ parameters:
             path: src/NodeManipulator/BinaryOpManipulator.php
 
         # 3rd party code without types
-        - '#Add explicit array type to assigned "(.*?)->(args|params|items|parts|stmts)" expression#'
+        - '#Add explicit array type to assigned "(.*?)(args|params|items|parts|stmts)" expression#'
         # false positive on string type
         - '#Add explicit array type to assigned "\$unusedParameters" expression#'
 
         - '#Add explicit array type to assigned "\$innerPattern" expression#'
         # known doc
         - '#Add explicit array type to assigned "\$spreadParams" expression#'
 
+        - '#Add explicit array type to assigned "\$items" expression#'
+
+        - '#Add explicit array type to assigned "\$chunks" expression#'
+
+        - '#Add explicit array type to assigned "\$resolvedValue" expression#'
+
         - '#foreach\(\), while\(\), for\(\) or if\(\) cannot contain a complex expression\. Extract it to a new variable on a line before#'
 
         # double protection on purpose to ensure the type
@@ -758,7 +764,7 @@ parameters:
             path: packages/BetterPhpDocParser/PhpDocParser/BetterPhpDocParser.php
 
         -
-            message: '#Offset \(int\|string\) on non\-empty\-array<PHPStan\\PhpDocParser\\Ast\\PhpDoc\\PhpDocChildNode> in isset\(\) always exists and is not nullable#'
+            message: '#Offset \(int\|string\) on array<PHPStan\\PhpDocParser\\Ast\\PhpDoc\\PhpDocChildNode> in isset\(\) always exists and is not nullable#'
             path: packages/BetterPhpDocParser/PhpDocParser/DoctrineAnnotationDecorator.php
 
         -

diff --git a/...Quality/Rector/ClassConstFetch/ConvertStaticPrivateConstantToSelfRector/config/config.php b/...Quality/Rector/ClassConstFetch/ConvertStaticPrivateConstantToSelfRector/config/config.php
@@ -2,9 +2,10 @@
 
 declare(strict_types=1);
 
+use Rector\CodeQuality\Rector\ClassConstFetch\ConvertStaticPrivateConstantToSelfRector;
 use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
 
 return static function (ContainerConfigurator $containerConfigurator): void {
     $services = $containerConfigurator->services();
-    $services->set(Rector\CodeQuality\Rector\ClassConstFetch\ConvertStaticPrivateConstantToSelfRector::class);
+    $services->set(ConvertStaticPrivateConstantToSelfRector::class);
 };
diff --git a/rules/CodeQuality/Rector/ClassConstFetch/ConvertStaticPrivateConstantToSelfRector.php b/rules/CodeQuality/Rector/ClassConstFetch/ConvertStaticPrivateConstantToSelfRector.php
@@ -6,7 +6,9 @@
 
 use PhpParser\Node;
 use PhpParser\Node\Expr\ClassConstFetch;
+use PhpParser\Node\Identifier;
 use PhpParser\Node\Name;
+use PhpParser\Node\Stmt\Class_;
 use Rector\Core\Rector\AbstractRector;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
 use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
@@ -55,7 +57,7 @@ public function getNodeTypes(): array
     }
 
     /**
-     * @param \PhpParser\Node\Expr\ClassConstFetch $node
+     * @param ClassConstFetch $node
      */
     public function refactor(Node $node): ?ClassConstFetch
     {
@@ -72,28 +74,31 @@ public function refactor(Node $node): ?ClassConstFetch
         return $node;
     }
 
-    private function isUsingStatic(ClassConstFetch $node): bool
+    private function isUsingStatic(ClassConstFetch $classConstFetch): bool
     {
-        if (! $node->class instanceof Name) {
+        if (! $classConstFetch->class instanceof Name) {
             return false;
         }
 
-        return $node->class->toString() === 'static';
+        return $classConstFetch->class->toString() === 'static';
     }
 
-    private function isPrivateConstant(ClassConstFetch $node): bool
+    private function isPrivateConstant(ClassConstFetch $classConstFetch): bool
     {
-        $class = $this->betterNodeFinder->findParentType($node, Node\Stmt\Class_::class);
-        if (! $class instanceof Node\Stmt\Class_) {
+        $class = $this->betterNodeFinder->findParentType($classConstFetch, Class_::class);
+        if (! $class instanceof Class_) {
             return false;
         }
+
         if (! $class->isFinal()) {
             return false;
         }
-        $constantName = $node->name;
-        if (! $constantName instanceof Node\Identifier) {
+
+        $constantName = $classConstFetch->name;
+        if (! $constantName instanceof Identifier) {
             return false;
         }
+
         foreach ($class->getConstants() as $classConst) {
             if (! $this->nodeNameResolver->isName($classConst, $constantName->toString())) {
                 continue;

diff --git a/rules/CodeQuality/Rector/ClassMethod/OptionalParametersAfterRequiredRector.php b/rules/CodeQuality/Rector/ClassMethod/OptionalParametersAfterRequiredRector.php
@@ -11,11 +11,9 @@
 use PhpParser\Node\Stmt\ClassMethod;
 use PHPStan\Analyser\Scope;
 use PHPStan\Reflection\MethodReflection;
-use PHPStan\Reflection\ParametersAcceptorSelector;
 use Rector\CodingStyle\Reflection\VendorLocationDetector;
-use Rector\Core\Rector\AbstractRector;
+use Rector\Core\Rector\AbstractScopeAwareRector;
 use Rector\Core\Reflection\ReflectionResolver;
-use Rector\NodeTypeResolver\Node\AttributeKey;
 use Rector\NodeTypeResolver\PHPStan\ParametersAcceptorSelectorVariantsWrapper;
 use Rector\Php80\NodeResolver\ArgumentSorter;
 use Rector\Php80\NodeResolver\RequireOptionalParamResolver;
@@ -27,7 +25,7 @@
  *
  * @see \Rector\Tests\CodeQuality\Rector\ClassMethod\OptionalParametersAfterRequiredRector\OptionalParametersAfterRequiredRectorTest
  */
-final class OptionalParametersAfterRequiredRector extends AbstractRector
+final class OptionalParametersAfterRequiredRector extends AbstractScopeAwareRector
 {
     public function __construct(
         private readonly RequireOptionalParamResolver $requireOptionalParamResolver,
@@ -74,20 +72,20 @@ public function getNodeTypes(): array
     /**
      * @param ClassMethod|New_|MethodCall|StaticCall $node
      */
-    public function refactor(Node $node): ?Node
+    public function refactorWithScope(Node $node, Scope $scope)
     {
         if ($node instanceof ClassMethod) {
-            return $this->refactorClassMethod($node);
+            return $this->refactorClassMethod($node, $scope);
         }
 
         if ($node instanceof New_) {
-            return $this->refactorNew($node);
+            return $this->refactorNew($node, $scope);
         }
 
-        return $this->refactorMethodCall($node);
+        return $this->refactorMethodCall($node, $scope);
     }
 
-    private function refactorClassMethod(ClassMethod $classMethod): ?ClassMethod
+    private function refactorClassMethod(ClassMethod $classMethod, Scope $scope): ?ClassMethod
     {
         if ($classMethod->params === []) {
             return null;
@@ -98,7 +96,11 @@ private function refactorClassMethod(ClassMethod $classMethod): ?ClassMethod
             return null;
         }
 
-        $expectedArgOrParamOrder = $this->resolveExpectedArgParamOrderIfDifferent($classMethodReflection, $classMethod);
+        $expectedArgOrParamOrder = $this->resolveExpectedArgParamOrderIfDifferent(
+            $classMethodReflection,
+            $classMethod,
+            $scope
+        );
         if ($expectedArgOrParamOrder === null) {
             return null;
         }
@@ -111,7 +113,7 @@ private function refactorClassMethod(ClassMethod $classMethod): ?ClassMethod
         return $classMethod;
     }
 
-    private function refactorNew(New_ $new): ?New_
+    private function refactorNew(New_ $new, Scope $scope): ?New_
     {
         if ($new->args === []) {
             return null;
@@ -122,7 +124,7 @@ private function refactorNew(New_ $new): ?New_
             return null;
         }
 
-        $expectedArgOrParamOrder = $this->resolveExpectedArgParamOrderIfDifferent($methodReflection, $new);
+        $expectedArgOrParamOrder = $this->resolveExpectedArgParamOrderIfDifferent($methodReflection, $new, $scope);
         if ($expectedArgOrParamOrder === null) {
             return null;
         }
@@ -132,14 +134,18 @@ private function refactorNew(New_ $new): ?New_
         return $new;
     }
 
-    private function refactorMethodCall(MethodCall|StaticCall $methodCall): MethodCall|StaticCall|null
+    private function refactorMethodCall(MethodCall|StaticCall $methodCall, Scope $scope): MethodCall|StaticCall|null
     {
         $methodReflection = $this->reflectionResolver->resolveFunctionLikeReflectionFromCall($methodCall);
         if (! $methodReflection instanceof MethodReflection) {
             return null;
         }
 
-        $expectedArgOrParamOrder = $this->resolveExpectedArgParamOrderIfDifferent($methodReflection, $methodCall);
+        $expectedArgOrParamOrder = $this->resolveExpectedArgParamOrderIfDifferent(
+            $methodReflection,
+            $methodCall,
+            $scope
+        );
         if ($expectedArgOrParamOrder === null) {
             return null;
         }
@@ -162,26 +168,16 @@ private function refactorMethodCall(MethodCall|StaticCall $methodCall): MethodCa
      */
     private function resolveExpectedArgParamOrderIfDifferent(
         MethodReflection $methodReflection,
-        New_|MethodCall|ClassMethod|StaticCall $node
+        New_|MethodCall|ClassMethod|StaticCall $node,
+        Scope $scope
     ): ?array {
         if ($this->vendorLocationDetector->detectMethodReflection($methodReflection)) {
             return null;
         }
 
-        if ($node instanceof ClassMethod) {
-            $parametersAcceptor = ParametersAcceptorSelector::selectSingle($methodReflection->getVariants());
-        } else {
-            $scope = $node->getAttribute(AttributeKey::SCOPE);
-
-            if (! $scope instanceof Scope) {
-                return null;
-            }
-
-            $parametersAcceptor = ParametersAcceptorSelectorVariantsWrapper::select($methodReflection, $node, $scope);
-        }
-
-        $expectedParameterReflections = $this->requireOptionalParamResolver->resolveFromReflection(
-            $methodReflection
+        $parametersAcceptor = ParametersAcceptorSelectorVariantsWrapper::select($methodReflection, $node, $scope);
+        $expectedParameterReflections = $this->requireOptionalParamResolver->resolveFromParametersAcceptor(
+            $parametersAcceptor
         );
 
         if ($expectedParameterReflections === $parametersAcceptor->getParameters()) {

diff --git a/rules/Php80/NodeResolver/RequireOptionalParamResolver.php b/rules/Php80/NodeResolver/RequireOptionalParamResolver.php
@@ -4,19 +4,16 @@
 
 namespace Rector\Php80\NodeResolver;
 
-use PHPStan\Reflection\MethodReflection;
 use PHPStan\Reflection\ParameterReflection;
-use PHPStan\Reflection\ParametersAcceptorSelector;
+use PHPStan\Reflection\ParametersAcceptor;
 
 final class RequireOptionalParamResolver
 {
     /**
      * @return ParameterReflection[]
      */
-    public function resolveFromReflection(MethodReflection $methodReflection): array
+    public function resolveFromParametersAcceptor(ParametersAcceptor $parametersAcceptor): array
     {
-        $parametersAcceptor = ParametersAcceptorSelector::selectSingle($methodReflection->getVariants());
-
         $optionalParams = [];
         $requireParams = [];
 

diff --git a/src/Rector/AbstractRector.php b/src/Rector/AbstractRector.php
@@ -231,17 +231,19 @@ final public function enterNode(Node $node)
         $currentScope = $originalNode->getAttribute(AttributeKey::SCOPE);
         $filePath = $this->file->getFilePath();
 
-        if (is_array($refactoredNode)) {
-            $originalNodeHash = spl_object_hash($originalNode);
-            $this->nodesToReturn[$originalNodeHash] = $refactoredNode;
+        // search "infinite recursion" in https://github.com/nikic/PHP-Parser/blob/master/doc/component/Walking_the_AST.markdown
+        $originalNodeHash = spl_object_hash($originalNode);
 
+        if (is_array($refactoredNode)) {
             $firstNode = current($refactoredNode);
             $this->mirrorComments($firstNode, $originalNode);
 
             $this->updateAndconnectParentNodes($refactoredNode, $parentNode);
             $this->connectNodes($refactoredNode, $node);
             $this->refreshScopeNodes($refactoredNode, $filePath, $currentScope);
 
+            $this->nodesToReturn[$originalNodeHash] = $refactoredNode;
+
             // will be replaced in leaveNode() the original node must be passed
             return $originalNode;
         }
@@ -254,15 +256,7 @@ final public function enterNode(Node $node)
         $this->connectNodes([$refactoredNode], $node);
         $this->refreshScopeNodes($refactoredNode, $filePath, $currentScope);
 
-        // is equals node type? return node early
-        if ($originalNode::class === $refactoredNode::class) {
-            return $refactoredNode;
-        }
-
-        // search "infinite recursion" in https://github.com/nikic/PHP-Parser/blob/master/doc/component/Walking_the_AST.markdown
-        $originalNodeHash = spl_object_hash($originalNode);
         $this->nodesToReturn[$originalNodeHash] = $refactoredNode;
-
         return $refactoredNode;
     }
 

diff --git a/tests/Issues/IssueVarConstantNewline/Fixture/fixture.php.inc b/tests/Issues/IssueVarConstantNewline/Fixture/fixture.php.inc
@@ -32,7 +32,6 @@ class Fixture
      * @var string
      */
     public const B = 'b';
-
     /**
      * @var string
      */
@@ -42,7 +41,6 @@ class Fixture
      * @var string
      */
     public const D = 'd';
-
     /**
      * @var string
      */

diff --git a/utils/Command/MissingInSetCommand.php b/utils/Command/MissingInSetCommand.php
@@ -77,6 +77,7 @@ static function (string $rectorClass): bool {
                             return false;
                         }
                     }
+
                     return true;
                 }
             );