[DeadCode] Handle edge cases on RemoveJustVariableAssignRector (#2621)

* [DeadCode] Handle edge cases on RemoveJustVariableAssignRector * indirect next used * skip with var doc * merge comment * handle indirect used * skip @var usage * merge comment * clean up * final touch: fixture for next used outside parent if
rectorphp · Jul 3, 2022 · dfc6e1d · dfc6e1d
1 parent 3120994
commit dfc6e1d
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 30 deletions.
diff --git a/config/set/dead-code.php b/config/set/dead-code.php
@@ -44,6 +44,7 @@
 use Rector\DeadCode\Rector\StaticCall\RemoveParentCallWithoutParentRector;
 use Rector\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector;
 use Rector\DeadCode\Rector\StmtsAwareInterface\RemoveJustPropertyFetchForAssignRector;
+use Rector\DeadCode\Rector\StmtsAwareInterface\RemoveJustVariableAssignRector;
 use Rector\DeadCode\Rector\Switch_\RemoveDuplicatedCaseInSwitchRector;
 use Rector\DeadCode\Rector\Ternary\TernaryToBooleanOrFalseToBooleanAndRector;
 use Rector\DeadCode\Rector\TryCatch\RemoveDeadTryCatchRector;
@@ -97,5 +98,7 @@
         RemoveUnusedPromotedPropertyRector::class,
         RemoveLastReturnRector::class,
         RemoveJustPropertyFetchForAssignRector::class,
+
+        RemoveJustVariableAssignRector::class,
     ]);
 };
diff --git a/config/set/early-return.php b/config/set/early-return.php
@@ -3,7 +3,6 @@
 declare(strict_types=1);
 
 use Rector\Config\RectorConfig;
-use Rector\DeadCode\Rector\StmtsAwareInterface\RemoveJustVariableAssignRector;
 use Rector\EarlyReturn\Rector\Foreach_\ChangeNestedForeachIfsToEarlyContinueRector;
 use Rector\EarlyReturn\Rector\Foreach_\ReturnAfterToEarlyOnBreakRector;
 use Rector\EarlyReturn\Rector\If_\ChangeAndIfToEarlyReturnRector;
@@ -31,6 +30,5 @@
         PreparedValueToEarlyReturnRector::class,
         ReturnBinaryOrToEarlyReturnRector::class,
         ReturnEarlyIfVariableRector::class,
-        RemoveJustVariableAssignRector::class,
     ]);
 };
diff --git a/...e/Rector/StmtsAwareInterface/RemoveJustVariableAssignRector/Fixture/merge_comment.php.inc b/...e/Rector/StmtsAwareInterface/RemoveJustVariableAssignRector/Fixture/merge_comment.php.inc
@@ -0,0 +1,37 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\StmtsAwareInterface\RemoveJustVariableAssignRector\Fixture;
+
+final class SomeClass
+{
+    private int $temporaryValue;
+
+    public function run()
+    {
+        // some comment
+        $result = execute();
+
+        // another comment
+        $this->temporaryValue = $result;
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\StmtsAwareInterface\RemoveJustVariableAssignRector\Fixture;
+
+final class SomeClass
+{
+    private int $temporaryValue;
+
+    public function run()
+    {
+        // some comment
+        // another comment
+        $this->temporaryValue = execute();
+    }
+}
+
+?>
diff --git a/...tmtsAwareInterface/RemoveJustVariableAssignRector/Fixture/skip_indirect_next_used.php.inc b/...tmtsAwareInterface/RemoveJustVariableAssignRector/Fixture/skip_indirect_next_used.php.inc
@@ -0,0 +1,21 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\StmtsAwareInterface\RemoveJustVariableAssignRector\Fixture;
+
+final class SkipIndirectNextUsed
+{
+    public function run()
+    {
+        $result = execute();
+        $this->temporaryValue = $result;
+
+        otherCode();
+        otherCode();
+
+        function () use ($result) {
+
+        };
+
+        echo $result;
+    }
+}
diff --git a/...Interface/RemoveJustVariableAssignRector/Fixture/skip_indirect_next_used_after_if.php.inc b/...Interface/RemoveJustVariableAssignRector/Fixture/skip_indirect_next_used_after_if.php.inc
@@ -0,0 +1,16 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\StmtsAwareInterface\RemoveJustVariableAssignRector\Fixture;
+
+final class SkipIndirectNextUsedAfterIf
+{
+    public function run()
+    {
+        if (rand(0, 1)) {
+            $result = execute();
+            $this->temporaryValue = $result;
+        }
+
+        echo $result ?? 'test';
+    }
+}
diff --git a/...ctor/StmtsAwareInterface/RemoveJustVariableAssignRector/Fixture/skip_with_var_doc.php.inc b/...ctor/StmtsAwareInterface/RemoveJustVariableAssignRector/Fixture/skip_with_var_doc.php.inc
@@ -0,0 +1,13 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\StmtsAwareInterface\RemoveJustVariableAssignRector\Fixture;
+
+final class SkipWithVarDoc
+{
+    public function run()
+    {
+        /** @var string $result */
+        $result = execute();
+        $this->temporaryValue = $result;
+    }
+}
diff --git a/rules/DeadCode/Rector/StmtsAwareInterface/RemoveJustVariableAssignRector.php b/rules/DeadCode/Rector/StmtsAwareInterface/RemoveJustVariableAssignRector.php
@@ -12,9 +12,12 @@
 use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Stmt;
 use PhpParser\Node\Stmt\Expression;
+use PHPStan\PhpDocParser\Ast\PhpDoc\VarTagValueNode;
+use Rector\BetterPhpDocParser\Comment\CommentsMerger;
 use Rector\Core\Contract\PhpParser\Node\StmtsAwareInterface;
 use Rector\Core\NodeAnalyzer\VariableAnalyzer;
 use Rector\Core\Rector\AbstractRector;
+use Rector\DeadCode\NodeAnalyzer\ExprUsedInNextNodeAnalyzer;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
 use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
 
@@ -24,7 +27,9 @@
 final class RemoveJustVariableAssignRector extends AbstractRector
 {
     public function __construct(
-        private readonly VariableAnalyzer $variableAnalyzer
+        private readonly VariableAnalyzer $variableAnalyzer,
+        private readonly ExprUsedInNextNodeAnalyzer $exprUsedInNextNodeAnalyzer,
+        private readonly CommentsMerger $commentsMerger
     ) {
     }
 
@@ -89,6 +94,11 @@ public function refactor(Node $node): ?Node
                 continue;
             }
 
+            $phpDocInfo = $this->phpDocInfoFactory->createFromNodeOrEmpty($stmt);
+            if ($phpDocInfo->getVarTagValueNode() instanceof VarTagValueNode) {
+                continue;
+            }
+
             $nextAssign = $this->matchExpressionAssign($nextStmt);
             if (! $nextAssign instanceof Assign) {
                 continue;
@@ -98,12 +108,14 @@ public function refactor(Node $node): ?Node
                 continue;
             }
 
-            if (! $this->areTwoVariablesCrossAssign($currentAssign, $nextAssign, $node)) {
+            if (! $this->areTwoVariablesCrossAssign($currentAssign, $nextAssign)) {
                 continue;
             }
 
             // ...
             $currentAssign->var = $nextAssign->var;
+            $this->commentsMerger->keepComments($stmt, [$stmts[$key + 1]]);
+
             unset($stmts[$key + 1]);
         }
 
@@ -124,11 +136,8 @@ public function refactor(Node $node): ?Node
      *
      * + not used $<some> bellow, so removal will not break it
      */
-    private function areTwoVariablesCrossAssign(
-        Assign $currentAssign,
-        Assign $nextAssign,
-        StmtsAwareInterface $stmtsAware
-    ): bool {
+    private function areTwoVariablesCrossAssign(Assign $currentAssign, Assign $nextAssign): bool
+    {
         // is just re-assign to variable
         if (! $currentAssign->var instanceof Variable) {
             return false;
@@ -150,27 +159,7 @@ private function areTwoVariablesCrossAssign(
             return false;
         }
 
-        $currentVariable = $currentAssign->var;
-        $nextVariable = $nextAssign->expr;
-
-        // is variable used later?
-        $nextUsedVariable = $this->betterNodeFinder->findFirst($stmtsAware, function (Node $node) use (
-            $currentVariable,
-            $nextVariable
-        ): bool {
-            if (in_array($node, [$currentVariable, $nextVariable], true)) {
-                return false;
-            }
-
-            if (! $node instanceof Variable) {
-                return false;
-            }
-
-            // is variable name?
-            return $this->nodeNameResolver->areNamesEqual($node, $currentVariable);
-        });
-
-        return ! $nextUsedVariable instanceof Variable;
+        return ! $this->exprUsedInNextNodeAnalyzer->isUsed($nextAssign->expr);
     }
 
     /**