Merge pull request #52 from magento-tsg/MC-25111

[Arrows] MC-25111: SVC false-positive: method return typing changed

Author: zakdma

composer.lock

@@ -9,6 +9,7 @@
 
 namespace Magento\SemanticVersionChecker\Analyzer;
 
+use Magento\SemanticVersionChecker\ClassHierarchy\DependencyGraph;
 use PhpParser\Node\Stmt\Class_;
 use PHPSemVerChecker\Operation\ClassAdded;
 use PHPSemVerChecker\Operation\ClassRemoved;

src/Analyzer/ClassAnalyzer.php

@@ -24,7 +24,8 @@
 use Magento\SemanticVersionChecker\Operation\Visibility\MethodDecreased as VisibilityMethodDecreased;
 use Magento\SemanticVersionChecker\Operation\Visibility\MethodIncreased as VisibilityMethodIncreased;
 use PhpParser\Node\NullableType;
-use PhpParser\Node\Stmt;
+use PhpParser\Node\Name;
+use PhpParser\Node\Stmt\Class_;
 use PhpParser\Node\Stmt\ClassLike;
 use PhpParser\Node\Stmt\ClassMethod;
 use PHPSemVerChecker\Comparator\Implementation;
@@ -38,12 +39,6 @@
 use PHPSemVerChecker\Operation\ClassMethodParameterTypingRemoved;
 use PHPSemVerChecker\Operation\ClassMethodRemoved;
 use PHPSemVerChecker\Report\Report;
-use PHPStan\PhpDocParser\Ast\PhpDoc\PhpDocTagNode;
-use PHPStan\PhpDocParser\Lexer\Lexer;
-use PHPStan\PhpDocParser\Parser\ConstExprParser;
-use PHPStan\PhpDocParser\Parser\PhpDocParser;
-use PHPStan\PhpDocParser\Parser\TokenIterator;
-use PHPStan\PhpDocParser\Parser\TypeParser;
 
 /**
  * Class method analyzer.
@@ -429,9 +424,55 @@ private function getDocReturnDeclaration(ClassMethod $method)
             $result = implode('|', $parsedComment['return']);
 
             return $result;
-        } else {
-            return ' ';
+        } elseif ($this->dependencyGraph !== null) {
+            /** @var Class_ $methodClass */
+            $methodClass = $method->getAttribute('parent');
+            if ($methodClass) {
+                $ancestors = [];
+                if (!empty($methodClass->extends)) {
+                    $ancestors = $this->addAncestorsToArray($ancestors, $methodClass->extends);
+                }
+                if (!empty($methodClass->implements)) {
+                    $ancestors = $this->addAncestorsToArray($ancestors, $methodClass->implements);
+                }
+                /** @var Name $ancestor */
+                foreach ($ancestors as $ancestor) {
+                    $ancestorClass = $this->dependencyGraph->findEntityByName($ancestor->toString());
+                    if ($ancestorClass) {
+                        foreach ($ancestorClass->getMethodList() as $methodItem) {
+                            if ($method->name->toString() == $methodItem->name->toString()) {
+                                $result = $this->getDocReturnDeclaration($methodItem);
+                                if (!empty(trim($result))) {
+                                    return $result;
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        return ' ';
+    }
+
+    /**
+     * Add ancestors to array
+     *
+     * @param array $ancestors
+     * @param array|Name $toAdd
+     * @return array
+     */
+    private function addAncestorsToArray(array $ancestors, $toAdd)
+    {
+        if (!empty($toAdd)) {
+            if (is_array($toAdd)) {
+                $ancestors = array_merge($ancestors, $toAdd);
+            } else {
+                $ancestors[] = $toAdd;
+            }
         }
+
+        return $ancestors;
     }
 
     /**

src/Analyzer/ClassMethodAnalyzer.php

@@ -30,7 +30,7 @@ public function create(DependencyGraph $dependencyGraph = null): AnalyzerInterfa
     {
         $analyzers = [
             new ClassAnalyzer(null, null, null, $dependencyGraph),
-            new InterfaceAnalyzer(),
+            new InterfaceAnalyzer(null, null, null, $dependencyGraph),
             new TraitAnalyzer(),
         ];
 

src/Analyzer/Factory/AnalyzerFactory.php

@@ -28,8 +28,8 @@ class NonApiAnalyzerFactory implements AnalyzerFactoryInterface
     public function create(DependencyGraph $dependencyGraph = null): AnalyzerInterface
     {
         $analyzers = [
-            new ClassAnalyzer(),
-            new InterfaceAnalyzer(),
+            new ClassAnalyzer(null, null, null, $dependencyGraph),
+            new InterfaceAnalyzer(null, null, null, $dependencyGraph),
             new TraitAnalyzer(),
         ];
 

src/Analyzer/Factory/NonApiAnalyzerFactory.php

@@ -139,7 +139,7 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe
     protected function getContentAnalyzers($context, $fileBefore, $fileAfter)
     {
         return [
-            new ClassMethodAnalyzer($context, $fileBefore, $fileAfter),
+            new ClassMethodAnalyzer($context, $fileBefore, $fileAfter, $this->dependencyGraph),
             new ClassConstantAnalyzer($context, $fileBefore, $fileAfter),
             new ClassMethodExceptionAnalyzer($context, $fileBefore, $fileAfter),
             new InterfaceExtendsAnalyzer($context, $fileBefore, $fileAfter)

src/Analyzer/InterfaceAnalyzer.php

@@ -35,6 +35,11 @@
  * - method param typehint moved from in-line to doc block
  * - method return typehint moved from doc block to in-line
  * - method return typehint moved from in-line to doc block
+ *
+ * TODO: this class should be rewritten using new possibility added by
+ * Magento\SemanticVersionChecker\Visitor\NameResolver
+ * Now all information (and resolved typed) about DocBlock params and return type exists in
+ * method node 'docCommentParsed' attribute
  */
 class MethodDocBlockAnalyzer
 {

src/Analyzer/MethodDocBlockAnalyzer.php

@@ -11,7 +11,8 @@
 
 use Magento\SemanticVersionChecker\Helper\Node as NodeHelper;
 use PhpParser\NodeTraverser;
-use PhpParser\NodeVisitor\NameResolver;
+use Magento\SemanticVersionChecker\Visitor\NameResolver;
+use PhpParser\NodeVisitor\ParentConnectingVisitor;
 use PhpParser\ParserFactory;
 
 /**
@@ -31,6 +32,7 @@ public function create(): StaticAnalyzer
         );
         $nodeTraverser               = new NodeTraverser();
 
+        $nodeTraverser->addVisitor(new ParentConnectingVisitor());
         $nodeTraverser->addVisitor(new NameResolver());
 
         return new StaticAnalyzer($parser, $dependencyInspectionVisitor, $nodeTraverser);

src/ClassHierarchy/StaticAnalyzerFactory.php

@@ -19,6 +19,7 @@
 use PhpParser\Lexer\Emulative;
 use PhpParser\NodeTraverser;
 use Magento\SemanticVersionChecker\Visitor\NameResolver;
+use PhpParser\NodeVisitor\ParentConnectingVisitor;
 use PhpParser\Parser\Php7 as Parser;
 use PHPSemVerChecker\Registry\Registry;
 use PHPSemVerChecker\Visitor\ClassVisitor;
@@ -38,6 +39,7 @@ private function buildFullScanner()
         $traverser   = new NodeTraverser();
         $apiVisitors = [
             new NameResolver(),
+            new ParentConnectingVisitor(),
             new ClassVisitor($registry),
             new InterfaceVisitor($registry),
             new FunctionVisitor($registry),
@@ -59,6 +61,7 @@ private function buildApiScanner(DependencyGraph $dependencyGraph = null)
         $nodeHelper  = new NodeHelper();
         $apiVisitors = [
             new NameResolver(),
+            new ParentConnectingVisitor(),
             new ApiClassVisitor($registry, $nodeHelper, $dependencyGraph),
             new ApiInterfaceVisitor($registry, $nodeHelper, $dependencyGraph),
             new ApiTraitVisitor($registry, $nodeHelper, $dependencyGraph),

src/Scanner/ScannerRegistryFactory.php

@@ -10,6 +10,7 @@
 use PhpParser\Node;
 use PhpParser\Node\Name;
 use PhpParser\Node\Stmt\ClassMethod;
+use PhpParser\NodeAbstract;
 use PhpParser\NodeVisitor\NameResolver as ParserNameResolver;
 use PhpParser\BuilderHelpers;
 use PHPStan\PhpDocParser\Ast\PhpDoc\ParamTagValueNode;
@@ -24,10 +25,29 @@
 use PHPStan\PhpDocParser\Parser\TypeParser;
 
 /**
- * Extended Name Resolver that parse and resolve also docblock hintings
+ * Extended Name Resolver that parse and resolve also docblock params and return type hinting.
  */
 class NameResolver extends ParserNameResolver
 {
+    /**
+     * Internal types that should not be resolved for docblock
+     *
+     * @var string[]
+     */
+    private $internalTypes = [
+        'string',
+        'integer',
+        'float',
+        'double',
+        'boolean',
+        'bool',
+        'array',
+        'object',
+        'null',
+        'resource',
+        '$this',
+    ];
+
     /**
      * @inheritDoc
      */
@@ -85,14 +105,10 @@ private function parseType($type)
         $result = [];
         if ($type instanceof UnionTypeNode) {
             foreach ($type->types as $typeNode) {
-                $normalizedType = BuilderHelpers::normalizeType((string)$typeNode);
-                $resolvedType = $this->resolveType($normalizedType);
-                $result[] = $resolvedType;
+                $result[] = $this->normalizeAndResolve($typeNode);
             }
         } else {
-            $normalizedType = BuilderHelpers::normalizeType((string)$type);
-            $resolvedType = $this->resolveType($normalizedType);
-            $result[] = $resolvedType;
+            $result[] = $this->normalizeAndResolve($type);
         }
 
         uasort(
@@ -105,11 +121,28 @@ function ($elementOne, $elementTwo) {
         return $result;
     }
 
+    /**
+     * @param TypeNode $type
+     * @return NodeAbstract
+     */
+    private function normalizeAndResolve($type)
+    {
+        $normalizedType = BuilderHelpers::normalizeType((string)$type);
+
+        if (in_array(strtolower((string)$type), $this->internalTypes)) {
+            $resolvedType = $normalizedType;
+        } else {
+            $resolvedType = $this->resolveType($normalizedType);
+        }
+
+        return $resolvedType;
+    }
+
     /**
      * Resolve type from Relative to FQCN
      *
      * @param $node
-     * @return Name|Node\NullableType|Node\UnionType
+     * @return NodeAbstract
      */
     private function resolveType($node)
     {