[Traverser] Add ByRefReturnNodeVisitor (#3826)

Co-authored-by: Tomas Votruba <tomas.vot@gmail.com> Co-authored-by: GitHub Action <actions@github.com>
rectorphp · May 13, 2023 · 2406dc8 · 2406dc8
1 parent a74c640
commit 2406dc8
Show file tree

Hide file tree

Showing 7 changed files with 84 additions and 48 deletions.
diff --git a/packages/NodeTypeResolver/Node/AttributeKey.php b/packages/NodeTypeResolver/Node/AttributeKey.php
@@ -216,4 +216,9 @@ final class AttributeKey
      * @var string
      */
     public const IS_BYREF_VAR = 'is_byref_var';
+
+    /**
+     * @var string
+     */
+    public const IS_BYREF_RETURN = 'is_byref_return';
 }
diff --git a/packages/NodeTypeResolver/PHPStan/Scope/NodeVisitor/ByRefReturnNodeVisitor.php b/packages/NodeTypeResolver/PHPStan/Scope/NodeVisitor/ByRefReturnNodeVisitor.php
@@ -0,0 +1,55 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\NodeTypeResolver\PHPStan\Scope\NodeVisitor;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr\CallLike;
+use PhpParser\Node\FunctionLike;
+use PhpParser\Node\Stmt\Return_;
+use PhpParser\NodeTraverser;
+use PhpParser\NodeVisitorAbstract;
+use Rector\NodeTypeResolver\Node\AttributeKey;
+use Rector\PhpDocParser\NodeTraverser\SimpleCallableNodeTraverser;
+
+final class ByRefReturnNodeVisitor extends NodeVisitorAbstract
+{
+    public function __construct(private readonly SimpleCallableNodeTraverser $simpleCallableNodeTraverser)
+    {
+    }
+
+    public function enterNode(Node $node): ?Node
+    {
+        if (! $node instanceof FunctionLike) {
+            return null;
+        }
+
+        if (! $node->returnsByRef()) {
+            return null;
+        }
+
+        $stmts = $node->getStmts();
+        if ($stmts === null) {
+            return null;
+        }
+
+        $this->simpleCallableNodeTraverser->traverseNodesWithCallable(
+            $stmts,
+            static function (Node $subNode) : int|null|Return_ {
+                if ($subNode instanceof CallLike || $subNode instanceof FunctionLike) {
+                    return NodeTraverser::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
+                }
+
+                if (! $subNode instanceof Return_) {
+                    return null;
+                }
+
+                $subNode->setAttribute(AttributeKey::IS_BYREF_RETURN, true);
+                return $subNode;
+            }
+        );
+
+        return null;
+    }
+}
diff --git a/packages/NodeTypeResolver/PHPStan/Scope/PHPStanNodeScopeResolver.php b/packages/NodeTypeResolver/PHPStan/Scope/PHPStanNodeScopeResolver.php
@@ -51,6 +51,7 @@
 use Rector\NodeNameResolver\NodeNameResolver;
 use Rector\NodeTypeResolver\Node\AttributeKey;
 use Rector\NodeTypeResolver\PHPStan\Scope\NodeVisitor\AssignedToNodeVisitor;
+use Rector\NodeTypeResolver\PHPStan\Scope\NodeVisitor\ByRefReturnNodeVisitor;
 use Rector\NodeTypeResolver\PHPStan\Scope\NodeVisitor\ByRefVariableNodeVisitor;
 use Rector\NodeTypeResolver\PHPStan\Scope\NodeVisitor\GlobalVariableNodeVisitor;
 use Rector\NodeTypeResolver\PHPStan\Scope\NodeVisitor\RemoveDeepChainMethodCallNodeVisitor;
@@ -80,6 +81,7 @@ public function __construct(
         GlobalVariableNodeVisitor $globalVariableNodeVisitor,
         StaticVariableNodeVisitor $staticVariableNodeVisitor,
         ByRefVariableNodeVisitor $byRefVariableNodeVisitor,
+        ByRefReturnNodeVisitor $byRefReturnNodeVisitor,
         private readonly ScopeFactory $scopeFactory,
         private readonly PrivatesAccessor $privatesAccessor,
         private readonly NodeNameResolver $nodeNameResolver,
@@ -92,6 +94,7 @@ public function __construct(
         $this->nodeTraverser->addVisitor($globalVariableNodeVisitor);
         $this->nodeTraverser->addVisitor($staticVariableNodeVisitor);
         $this->nodeTraverser->addVisitor($byRefVariableNodeVisitor);
+        $this->nodeTraverser->addVisitor($byRefReturnNodeVisitor);
     }
 
     /**

diff --git a/phpstan.neon b/phpstan.neon
@@ -798,4 +798,5 @@ parameters:
         - '#Property Rector\\Core\\Contract\\PhpParser\\Node\\StmtsAwareInterface\:\:\$stmts \(array<PhpParser\\Node\\Stmt>\|null\) does not accept array<PhpParser\\Node\\Stmt\|null>#'
 
         # make autoload of php-parser in rector first
-        - '#PHPDoc tag @return contains generic type PhpParser\\Node\\Stmt\\Expression<PhpParser\\Node\\Expr\\Assign> but class PhpParser\\Node\\Stmt\\Expression is not generic#'
+        - '#but class PhpParser\\Node\\Stmt\\Expression is not generic#'
+        - '#Parameter \#3 \$assign of method Rector\\CodeQuality\\Rector\\FunctionLike\\SimplifyUselessVariableRector\:\:processSimplifyUselessVariable\(\) expects PhpParser\\Node\\Expr\\Assign\|PhpParser\\Node\\Expr\\AssignOp, PhpParser\\Node\\Expr given#'
diff --git a/...y/Rector/FunctionLike/SimplifyUselessVariableRector/SimplifyUselessVariableRectorTest.php b/...y/Rector/FunctionLike/SimplifyUselessVariableRector/SimplifyUselessVariableRectorTest.php
@@ -8,11 +8,6 @@
 use PHPUnit\Framework\Attributes\DataProvider;
 use Rector\Testing\PHPUnit\AbstractRectorTestCase;
 
-/**
- * Tests copied from:
- * - https://github.com/slevomat/coding-standard/blob/9978172758e90bc2355573e0b5d99062d87b14a3/tests/Sniffs/Variables/data/uselessVariableErrors.fixed.php
- * - https://github.com/slevomat/coding-standard/blob/9978172758e90bc2355573e0b5d99062d87b14a3/tests/Sniffs/Variables/data/uselessVariableNoErrors.php
- */
 final class SimplifyUselessVariableRectorTest extends AbstractRectorTestCase
 {
     #[DataProvider('provideData')]

diff --git a/rules/CodeQuality/NodeAnalyzer/ReturnAnalyzer.php b/rules/CodeQuality/NodeAnalyzer/ReturnAnalyzer.php
diff --git a/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector.php b/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector.php
@@ -12,27 +12,27 @@
 use PhpParser\Node\Stmt;
 use PhpParser\Node\Stmt\Expression;
 use PhpParser\Node\Stmt\Return_;
+use PHPStan\Analyser\Scope;
+use PHPStan\Reflection\FunctionReflection;
 use PHPStan\Type\MixedType;
-use Rector\CodeQuality\NodeAnalyzer\ReturnAnalyzer;
 use Rector\Core\Contract\PhpParser\Node\StmtsAwareInterface;
 use Rector\Core\NodeAnalyzer\CallAnalyzer;
 use Rector\Core\NodeAnalyzer\VariableAnalyzer;
 use Rector\Core\PhpParser\Node\AssignAndBinaryMap;
-use Rector\Core\Rector\AbstractRector;
+use Rector\Core\Rector\AbstractScopeAwareRector;
+use Rector\NodeTypeResolver\Node\AttributeKey;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
 use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
 
 /**
- * @see Based on https://github.com/slevomat/coding-standard/blob/master/SlevomatCodingStandard/Sniffs/Variables/UselessVariableSniff.php
  * @see \Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\SimplifyUselessVariableRectorTest
  */
-final class SimplifyUselessVariableRector extends AbstractRector
+final class SimplifyUselessVariableRector extends AbstractScopeAwareRector
 {
     public function __construct(
         private readonly AssignAndBinaryMap $assignAndBinaryMap,
         private readonly VariableAnalyzer $variableAnalyzer,
         private readonly CallAnalyzer $callAnalyzer,
-        private readonly ReturnAnalyzer $returnAnalyzer,
     ) {
     }
 
@@ -67,14 +67,15 @@ public function getNodeTypes(): array
     /**
      * @param StmtsAwareInterface $node
      */
-    public function refactor(Node $node): ?Node
+    public function refactorWithScope(Node $node, Scope $scope): ?Node
     {
         $stmts = $node->stmts;
         if ($stmts === null) {
             return null;
         }
 
         foreach ($stmts as $key => $stmt) {
+            // has previous node?
             if (! isset($stmts[$key - 1])) {
                 continue;
             }
@@ -84,19 +85,21 @@ public function refactor(Node $node): ?Node
             }
 
             $previousStmt = $stmts[$key - 1];
-            if ($this->shouldSkipStmt($stmt, $previousStmt)) {
+            if ($this->shouldSkipStmt($stmt, $previousStmt, $scope)) {
+                return null;
+            }
+
+            if ($this->hasSomeComment($previousStmt)) {
                 return null;
             }
 
             if ($this->isReturnWithVarAnnotation($stmt)) {
                 return null;
             }
 
-            /**
-             * @var Expression $previousStmt
-             * @var Assign|AssignOp $assign
-             */
+            /** @var Expression<Assign|AssignOp> $previousStmt */
             $assign = $previousStmt->expr;
+
             return $this->processSimplifyUselessVariable($node, $stmt, $assign, $key);
         }
 
@@ -124,17 +127,18 @@ private function processSimplifyUselessVariable(
         return $stmtsAware;
     }
 
-    private function shouldSkipStmt(Return_ $return, Stmt $previousStmt): bool
+    private function shouldSkipStmt(Return_ $return, Stmt $previousStmt, Scope $scope): bool
     {
-        if ($this->hasSomeComment($previousStmt)) {
+        if (! $return->expr instanceof Variable) {
             return true;
         }
 
-        if (! $return->expr instanceof Variable) {
+        $functionReflection = $scope->getFunction();
+        if ($functionReflection instanceof FunctionReflection && $functionReflection->returnsByReference()->yes()) {
             return true;
         }
 
-        if ($this->returnAnalyzer->hasByRefReturn($return)) {
+        if ($return->getAttribute(AttributeKey::IS_BYREF_RETURN) === true) {
             return true;
         }