[TypeDeclaration] Various AddArrayReturnDocTypeRector improvements

ecs.yaml

@@ -106,6 +106,7 @@ parameters:
 
         Symplify\CodingStandard\Sniffs\CleanCode\CognitiveComplexitySniff:
             # tough logic
+            - 'packages/NodeTypeResolver/src/PhpDoc/NodeAnalyzer/FqnNamePhpDocNodeDecorator.php'
             - 'packages/NodeTypeResolver/src/PHPStan/Type/StaticTypeAnalyzer.php'
             - 'src/NodeContainer/ParsedNodesByType.php'
             - 'packages/PHPStan/src/Rector/Node/RemoveNonExistingVarAnnotationRector.php'

packages/DeadCode/src/Rector/Property/RemoveUnusedPrivatePropertyRector.php

@@ -5,6 +5,7 @@
 use PhpParser\Node;
 use PhpParser\Node\Expr\Assign;
 use PhpParser\Node\Expr\PropertyFetch;
+use PhpParser\Node\Expr\StaticPropertyFetch;
 use PhpParser\Node\Stmt\Class_;
 use PhpParser\Node\Stmt\Interface_;
 use PhpParser\Node\Stmt\Property;
@@ -100,7 +101,7 @@ public function refactor(Node $node): ?Node
      * Matches all-only: "$this->property = x"
      * If these is ANY OTHER use of property, e.g. process($this->property), it returns []
      *
-     * @param PropertyFetch[] $propertyFetches
+     * @param PropertyFetch[]|StaticPropertyFetch[] $propertyFetches
      * @return Assign[]
      */
     private function resolveUselessAssignNode(array $propertyFetches): array

packages/NodeTypeResolver/src/NodeTypeResolver.php

@@ -333,6 +333,7 @@ public function resolveSingleTypeToStrings(Node $node): array
             $arrayType = $this->getNodeStaticType($node);
             if ($arrayType instanceof ArrayType) {
                 $itemTypes = $this->staticTypeToStringResolver->resolveObjectType($arrayType->getItemType());
+
                 foreach ($itemTypes as $key => $itemType) {
                     $itemTypes[$key] = $itemType . '[]';
                 }

packages/NodeTypeResolver/src/Php/AbstractTypeInfo.php

@@ -142,18 +142,41 @@ public function isTypehintAble(): bool
         return $typeCount === 1;
     }
 
+    /**
+     * @return string[]
+     */
+    public function getFqnTypes(): array
+    {
+        return $this->fqnTypes;
+    }
+
     /**
      * @return string[]
      */
     public function getDocTypes(): array
     {
         $allTypes = array_merge($this->types, $this->removedTypes);
+        $types = array_filter(array_unique($allTypes));
 
         if ($this->isNullable) {
-            $allTypes[] = 'null';
+            $types[] = 'null';
+        }
+
+        $types = $this->removeIterableTypeIfTraversableType($types);
+
+        // use mixed[] over array, that is more explicit about implicitnes
+        if ($types === ['array']) {
+            return ['mixed[]'];
         }
 
-        return array_filter(array_unique($allTypes));
+        // remove void types, as its useless in annotation
+        foreach ($types as $key => $value) {
+            if ($value === 'void') {
+                unset($types[$key]);
+            }
+        }
+
+        return $types;
     }
 
     protected function normalizeName(string $name): string
@@ -345,6 +368,10 @@ private function resolveTypeForTypehint(bool $forceFqn): ?string
             return $this->resolveMutualObjectSubtype($this->types);
         }
 
+        if (in_array('iterable', $this->types, true)) {
+            return 'iterable';
+        }
+
         $types = $forceFqn ? $this->fqnTypes : $this->types;
 
         return $types[0];
@@ -354,4 +381,33 @@ private function classLikeExists(string $type): bool
     {
         return ! class_exists($type) && ! interface_exists($type) && ! trait_exists($type);
     }
+
+    /**
+     * @param string[] $types
+     * @return string[]
+     */
+    private function removeIterableTypeIfTraversableType(array $types): array
+    {
+        $hasTraversableType = false;
+
+        foreach ($types as $type) {
+            if (Strings::endsWith($type, '[]')) {
+                $hasTraversableType = true;
+                break;
+            }
+        }
+
+        if (! $hasTraversableType) {
+            return $types;
+        }
+
+        foreach ($types as $key => $uniqeueType) {
+            // remove iterable if other types are provided
+            if (in_array($uniqeueType, ['iterable', 'array'], true)) {
+                unset($types[$key]);
+            }
+        }
+
+        return $types;
+    }
 }

packages/NodeTypeResolver/src/PhpDoc/NodeAnalyzer/DocBlockManipulator.php

@@ -269,6 +269,16 @@ public function changeVarTag(Node $node, $type): void
 
     public function addReturnTag(Node $node, string $type): void
     {
+        // make sure the tags are not identical, e.g imported class vs FQN class
+        $returnTypeInfo = $this->getReturnTypeInfo($node);
+        if ($returnTypeInfo) {
+            // already added
+            if ([ltrim($type, '\\')] === $returnTypeInfo->getFqnTypes()) {
+                return;
+            }
+        }
+
+        $this->removeTagFromNode($node, 'return');
         $this->addTypeSpecificTag($node, 'return', $type);
     }
 

packages/NodeTypeResolver/src/PhpDoc/NodeAnalyzer/FqnNamePhpDocNodeDecorator.php

@@ -110,16 +110,39 @@ private function collectResolvedNames(TypeNode $typeNode): array
             foreach ($typeNode->types as $subtype) {
                 $resolvedNames = array_merge($resolvedNames, $this->collectResolvedNames($subtype));
             }
-        } elseif ($typeNode instanceof AttributeAwareArrayTypeNode || $typeNode instanceof AttributeAwareThisTypeNode) {
+        } elseif ($typeNode instanceof AttributeAwareThisTypeNode) {
             $resolvedNames[] = $typeNode->getAttribute(Attribute::TYPE_AS_STRING);
-        } elseif ($typeNode instanceof AttributeAwareIdentifierTypeNode) {
-            if ($typeNode->getAttribute(Attribute::RESOLVED_NAME)) {
-                $resolvedNames[] = $typeNode->getAttribute(Attribute::RESOLVED_NAME);
-            } elseif ($typeNode->getAttribute(Attribute::TYPE_AS_STRING)) {
+        } elseif ($typeNode instanceof AttributeAwareArrayTypeNode) {
+            $resolved = false;
+            if ($typeNode->type instanceof AttributeAwareIdentifierTypeNode) {
+                $resolvedType = $this->resolveIdentifierType($typeNode->type);
+                if ($resolvedType) {
+                    $resolvedNames[] = $resolvedType . '[]';
+                    $resolved = true;
+                }
+            }
+
+            if ($resolved === false) {
                 $resolvedNames[] = $typeNode->getAttribute(Attribute::TYPE_AS_STRING);
             }
+        } elseif ($typeNode instanceof AttributeAwareIdentifierTypeNode) {
+            $resolvedType = $this->resolveIdentifierType($typeNode);
+            if ($resolvedType) {
+                $resolvedNames[] = $resolvedType;
+            }
         }
 
         return array_filter($resolvedNames);
     }
+
+    private function resolveIdentifierType(AttributeAwareIdentifierTypeNode $attributeAwareIdentifierTypeNode): ?string
+    {
+        if ($attributeAwareIdentifierTypeNode->getAttribute(Attribute::RESOLVED_NAME)) {
+            return $attributeAwareIdentifierTypeNode->getAttribute(Attribute::RESOLVED_NAME);
+        } elseif ($attributeAwareIdentifierTypeNode->getAttribute(Attribute::TYPE_AS_STRING)) {
+            return $attributeAwareIdentifierTypeNode->getAttribute(Attribute::TYPE_AS_STRING);
+        }
+
+        return null;
+    }
 }

packages/NodeTypeResolver/src/StaticTypeToStringResolver.php

@@ -88,7 +88,9 @@ public function resolveObjectType(?Type $staticType): array
 
         foreach ($this->resolversByArgumentType as $type => $resolverCallable) {
             if (is_a($staticType, $type, true)) {
-                return $resolverCallable($staticType);
+                $types = $resolverCallable($staticType);
+
+                return array_unique($types);
             }
         }
 

packages/TypeDeclaration/src/Rector/ClassMethod/AddArrayReturnDocTypeRector.php

@@ -111,17 +111,16 @@ public function refactor(Node $node): ?Node
 
     private function shouldSkip(ClassMethod $classMethod): bool
     {
-        if ($classMethod->returnType === null) {
+        if ($this->isName($classMethod->name, '__*')) {
             return true;
         }
 
-        if (! $this->isName($classMethod->returnType, 'array')) {
-            return true;
+        if ($classMethod->returnType) {
+            if (! $this->isNames($classMethod->returnType, ['array', 'iterable'])) {
+                return true;
+            }
         }
 
-        $returnTypeInfo = $this->docBlockManipulator->getReturnTypeInfo($classMethod);
-
-        // already has return type info, skip
-        return $returnTypeInfo !== null;
+        return false;
     }
 }

packages/TypeDeclaration/src/TypeInferer/PropertyTypeInferer/ConstructorPropertyTypeInferer.php

@@ -7,6 +7,7 @@
 use PhpParser\Node\Expr\Assign;
 use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\NullableType;
+use PhpParser\Node\Param;
 use PhpParser\Node\Stmt\ClassMethod;
 use PhpParser\NodeTraverser;
 use PHPStan\Type\NullType;
@@ -118,7 +119,7 @@ private function resolveParamStaticType(ClassMethod $classMethod, string $proper
      * ↓
      * $anotherValue param
      */
-    private function resolveParamForPropertyFetch(ClassMethod $classMethod, string $propertyName): ?Node\Param
+    private function resolveParamForPropertyFetch(ClassMethod $classMethod, string $propertyName): ?Param
     {
         $assignedParamName = null;
 
@@ -144,7 +145,7 @@ private function resolveParamForPropertyFetch(ClassMethod $classMethod, string $
             return null;
         }
 
-        /** @var Node\Param $param */
+        /** @var Param $param */
         foreach ((array) $classMethod->params as $param) {
             if (! $this->nameResolver->isName($param, $assignedParamName)) {
                 continue;
@@ -161,7 +162,7 @@ private function removePreSlash(string $content): string
         return ltrim($content, '\\');
     }
 
-    private function isParamNullable(Node\Param $param): bool
+    private function isParamNullable(Param $param): bool
     {
         if ($param->type instanceof NullableType) {
             return true;
@@ -180,7 +181,7 @@ private function isParamNullable(Node\Param $param): bool
     /**
      * @return IdentifierValueObject|string|null
      */
-    private function resolveParamTypeToString(Node\Param $param)
+    private function resolveParamTypeToString(Param $param)
     {
         if ($param->type === null) {
             return null;

packages/TypeDeclaration/src/TypeInferer/PropertyTypeInferer/DoctrineColumnPropertyTypeInferer.php

@@ -94,8 +94,7 @@ public function inferProperty(Property $property): array
         $types = [$scalarType];
 
         // is nullable?
-        $isNullable = $this->isNullable($columnTag->value->value);
-        if ($isNullable) {
+        if ($this->isNullable($columnTag->value->value)) {
             $types[] = 'null';
         }
 

packages/TypeDeclaration/tests/Rector/ClassMethod/AddArrayReturnDocTypeRector/AddArrayReturnDocTypeRectorTest.php

@@ -13,6 +13,15 @@ public function test(): void
             __DIR__ . '/Fixture/fixture.php.inc',
             __DIR__ . '/Fixture/setter_based.php.inc',
             __DIR__ . '/Fixture/fully_qualified_name.php.inc',
+            __DIR__ . '/Fixture/yield_strings.php.inc',
+            __DIR__ . '/Fixture/simple_array.php.inc',
+            __DIR__ . '/Fixture/add_without_return_type_declaration.php.inc',
+            __DIR__ . '/Fixture/fix_incorrect_array.php.inc',
+            // skip
+            __DIR__ . '/Fixture/skip_constructor.php.inc',
+            __DIR__ . '/Fixture/skip_array_after_array_type.php.inc',
+            __DIR__ . '/Fixture/skip_shorten_class_name.php.inc',
+            __DIR__ . '/Fixture/skip_inner_function_return.php.inc',
         ]);
     }
 

packages/TypeDeclaration/tests/Rector/ClassMethod/AddArrayReturnDocTypeRector/Fixture/add_without_return_type_declaration.php.inc

@@ -0,0 +1,40 @@
+<?php
+
+namespace Rector\TypeDeclaration\Tests\Rector\ClassMethod\AddArrayReturnDocTypeRector\Fixture;
+
+class AddWithoutReturnTypeDeclaration
+{
+    /**
+     * @var int[]
+     */
+    private $values;
+
+    public function getValues()
+    {
+        return $this->values;
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\TypeDeclaration\Tests\Rector\ClassMethod\AddArrayReturnDocTypeRector\Fixture;
+
+class AddWithoutReturnTypeDeclaration
+{
+    /**
+     * @var int[]
+     */
+    private $values;
+
+    /**
+     * @return int[]
+     */
+    public function getValues()
+    {
+        return $this->values;
+    }
+}
+
+?>

packages/TypeDeclaration/tests/Rector/ClassMethod/AddArrayReturnDocTypeRector/Fixture/fix_incorrect_array.php.inc

@@ -0,0 +1,33 @@
+<?php
+
+namespace Rector\TypeDeclaration\Tests\Rector\ClassMethod\AddArrayReturnDocTypeRector\Fixture;
+
+class FixIncorrectArray
+{
+    /**
+     * @return string[]
+     */
+    public function getValues()
+    {
+        return [1, 2, 3];
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\TypeDeclaration\Tests\Rector\ClassMethod\AddArrayReturnDocTypeRector\Fixture;
+
+class FixIncorrectArray
+{
+    /**
+     * @return int[]
+     */
+    public function getValues()
+    {
+        return [1, 2, 3];
+    }
+}
+
+?>

packages/TypeDeclaration/tests/Rector/ClassMethod/AddArrayReturnDocTypeRector/Fixture/simple_array.php.inc

@@ -0,0 +1,30 @@
+<?php
+
+namespace Rector\TypeDeclaration\Tests\Rector\ClassMethod\AddArrayReturnDocTypeRector\Fixture;
+
+class SimpleArray
+{
+    public function getValues(): array
+    {
+        return [];
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\TypeDeclaration\Tests\Rector\ClassMethod\AddArrayReturnDocTypeRector\Fixture;
+
+class SimpleArray
+{
+    /**
+     * @return mixed[]
+     */
+    public function getValues(): array
+    {
+        return [];
+    }
+}
+
+?>