[DeadCode] Add RemoveUselessReturnExprInConstructRector (#5158)

* [DeadCode] Add RemoveUselessReturnExprInConstruct * [ci-review] Rector Rectify * skip inner class test * [ci-review] Rector Rectify * fix phpstan * add Rector suffix * register * add fixture for return dynamic * update doc sample * fix --------- Co-authored-by: GitHub Action <actions@github.com>
rectorphp · Oct 12, 2023 · b7a5fef · b7a5fef
1 parent 444bc59
commit b7a5fef
Show file tree

Hide file tree

Showing 14 changed files with 358 additions and 12 deletions.
diff --git a/build/target-repository/docs/rector_rules_overview.md b/build/target-repository/docs/rector_rules_overview.md
@@ -1,4 +1,4 @@
-# 354 Rules Overview
+# 355 Rules Overview
 
 <br>
 
@@ -10,7 +10,7 @@
 
 - [CodingStyle](#codingstyle) (29)
 
-- [DeadCode](#deadcode) (41)
+- [DeadCode](#deadcode) (42)
 
 - [EarlyReturn](#earlyreturn) (9)
 
@@ -2839,6 +2839,36 @@ Remove `@param` docblock with same type as parameter type
 
 <br>
 
+### RemoveUselessReturnExprInConstructRector
+
+Remove useless return Expr in `__construct()`
+
+- class: [`Rector\DeadCode\Rector\ClassMethod\RemoveUselessReturnExprInConstructRector`](../rules/DeadCode/Rector/ClassMethod/RemoveUselessReturnExprInConstructRector.php)
+
+```diff
+ class SomeClass
+ {
+     public function __construct()
+     {
+         if (rand(0, 1)) {
+             $this->init();
+-            return true;
++            return;
+         }
+
+         if (rand(2, 3)) {
+-            return parent::construct();
++            parent::construct();
++            return;
+         }
+
+         $this->execute();
+     }
+ }
+```
+
+<br>
+
 ### RemoveUselessReturnTagRector
 
 Remove `@return` docblock with same type as defined in PHP

diff --git a/config/set/dead-code.php b/config/set/dead-code.php
@@ -16,6 +16,7 @@
 use Rector\DeadCode\Rector\ClassMethod\RemoveUnusedPrivateMethodRector;
 use Rector\DeadCode\Rector\ClassMethod\RemoveUnusedPromotedPropertyRector;
 use Rector\DeadCode\Rector\ClassMethod\RemoveUselessParamTagRector;
+use Rector\DeadCode\Rector\ClassMethod\RemoveUselessReturnExprInConstructRector;
 use Rector\DeadCode\Rector\ClassMethod\RemoveUselessReturnTagRector;
 use Rector\DeadCode\Rector\Concat\RemoveConcatAutocastRector;
 use Rector\DeadCode\Rector\ConstFetch\RemovePhpVersionIdCheckRector;
@@ -91,5 +92,6 @@
         RemoveAlwaysTrueIfConditionRector::class,
         RemoveDeadZeroAndOneOperationRector::class,
         RemovePhpVersionIdCheckRector::class,
+        RemoveUselessReturnExprInConstructRector::class,
     ]);
 };
diff --git a/packages/PhpDocParser/NodeTraverser/SimpleCallableNodeTraverser.php b/packages/PhpDocParser/NodeTraverser/SimpleCallableNodeTraverser.php
@@ -14,7 +14,7 @@
 final class SimpleCallableNodeTraverser
 {
     /**
-     * @param callable(Node $node): (int|Node|null) $callable
+     * @param callable(Node): (int|Node|null|Node[]) $callable
      * @param Node|Node[]|null $node
      */
     public function traverseNodesWithCallable(Node | array | null $node, callable $callable): void

diff --git a/packages/PhpDocParser/NodeVisitor/CallableNodeVisitor.php b/packages/PhpDocParser/NodeVisitor/CallableNodeVisitor.php
@@ -14,14 +14,19 @@
 final class CallableNodeVisitor extends NodeVisitorAbstract
 {
     /**
-     * @var callable(Node): (int|Node|null)
+     * @var callable(Node): (int|Node|null|Node[])
      */
     private $callable;
 
     private ?int $nodeIdToRemove = null;
 
     /**
-     * @param callable(Node $node): (int|Node|null) $callable
+     * @var array<int, Node[]>
+     */
+    private array $nodesToReturn = [];
+
+    /**
+     * @param callable(Node $node): (int|Node|null|Node[]) $callable
      */
     public function __construct(callable $callable)
     {
@@ -34,30 +39,42 @@ public function enterNode(Node $node): int|Node|null
 
         $callable = $this->callable;
 
-        /** @var int|Node|null $newNode */
+        /** @var int|Node|null|Node[] $newNode */
         $newNode = $callable($node);
 
         if ($newNode === NodeTraverser::REMOVE_NODE) {
             $this->nodeIdToRemove = spl_object_id($originalNode);
             return $originalNode;
         }
 
+        if (is_array($newNode)) {
+            $nodeId = spl_object_id($node);
+            $this->nodesToReturn[$nodeId] = $newNode;
+
+            return $node;
+        }
+
         if ($originalNode instanceof Stmt && $newNode instanceof Expr) {
             return new Expression($newNode);
         }
 
         return $newNode;
     }
 
-    public function leaveNode(Node $node): int|Node
+    /**
+     * @return int|Node|Node[]
+     */
+    public function leaveNode(Node $node): int|Node|array
     {
-        if ($this->nodeIdToRemove !== null
-            && $this->nodeIdToRemove === spl_object_id($node)
-        ) {
+        if ($this->nodeIdToRemove !== null && $this->nodeIdToRemove === spl_object_id($node)) {
             $this->nodeIdToRemove = null;
             return NodeTraverser::REMOVE_NODE;
         }
 
-        return $node;
+        if ($this->nodesToReturn === []) {
+            return $node;
+        }
+
+        return $this->nodesToReturn[spl_object_id($node)] ?? $node;
     }
 }
diff --git a/...RemoveUselessReturnExprInConstructRector/Fixture/return_dynamic_expr_in_construct.php.inc b/...RemoveUselessReturnExprInConstructRector/Fixture/return_dynamic_expr_in_construct.php.inc
@@ -0,0 +1,38 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\ClassMethod\RemoveUselessReturnExprInConstructRector\Fixture;
+
+final class ReturnDynamicExprInConstruct
+{
+    public function __construct()
+    {
+        if (rand(0, 1)) {
+            return parent::__construct();
+            echo 'dead code, ensure stmt inserted between';
+        }
+
+        $this->execute();
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\ClassMethod\RemoveUselessReturnExprInConstructRector\Fixture;
+
+final class ReturnDynamicExprInConstruct
+{
+    public function __construct()
+    {
+        if (rand(0, 1)) {
+            parent::__construct();
+            return;
+            echo 'dead code, ensure stmt inserted between';
+        }
+
+        $this->execute();
+    }
+}
+
+?>
diff --git a/...sMethod/RemoveUselessReturnExprInConstructRector/Fixture/return_expr_in_construct.php.inc b/...sMethod/RemoveUselessReturnExprInConstructRector/Fixture/return_expr_in_construct.php.inc
@@ -0,0 +1,37 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\ClassMethod\RemoveUselessReturnExprInConstructRector\Fixture;
+
+final class ReturnExprInConstruct
+{
+    public function __construct()
+    {
+        if (rand(0, 1)) {
+            $this->init();
+            return true;
+        }
+
+        $this->execute();
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\ClassMethod\RemoveUselessReturnExprInConstructRector\Fixture;
+
+final class ReturnExprInConstruct
+{
+    public function __construct()
+    {
+        if (rand(0, 1)) {
+            $this->init();
+            return;
+        }
+
+        $this->execute();
+    }
+}
+
+?>
diff --git a/...ClassMethod/RemoveUselessReturnExprInConstructRector/Fixture/skip_abstract_method.php.inc b/...ClassMethod/RemoveUselessReturnExprInConstructRector/Fixture/skip_abstract_method.php.inc
@@ -0,0 +1,8 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\ClassMethod\RemoveUselessReturnExprInConstructRector\Fixture;
+
+abstract class SkipAbstractMethod
+{
+    abstract public function run();
+}
diff --git a/...tor/ClassMethod/RemoveUselessReturnExprInConstructRector/Fixture/skip_inner_class.php.inc b/...tor/ClassMethod/RemoveUselessReturnExprInConstructRector/Fixture/skip_inner_class.php.inc
@@ -0,0 +1,16 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\ClassMethod\RemoveUselessReturnExprInConstructRector\Fixture;
+
+final class SkipInnerClass
+{
+    public function __construct()
+    {
+        $obj = new class {
+            public function run()
+            {
+                return true;
+            }
+        };
+    }
+}
diff --git a/...r/ClassMethod/RemoveUselessReturnExprInConstructRector/Fixture/skip_not_construct.php.inc b/...r/ClassMethod/RemoveUselessReturnExprInConstructRector/Fixture/skip_not_construct.php.inc
@@ -0,0 +1,11 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\ClassMethod\RemoveUselessReturnExprInConstructRector\Fixture;
+
+final class SkipNotConstruct
+{
+    public function run()
+    {
+        return true;
+    }
+}
diff --git a/.../ClassMethod/RemoveUselessReturnExprInConstructRector/Fixture/skip_return_no_expr.php.inc b/.../ClassMethod/RemoveUselessReturnExprInConstructRector/Fixture/skip_return_no_expr.php.inc
@@ -0,0 +1,16 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\ClassMethod\RemoveUselessReturnExprInConstructRector\Fixture;
+
+final class SkipReturnNoExpr
+{
+    public function __construct()
+    {
+        if (rand(0, 1)) {
+            $this->init();
+            return;
+        }
+
+        $this->execute();
+    }
+}
diff --git a/...RemoveUselessReturnExprInConstructRector/RemoveUselessReturnExprInConstructRectorTest.php b/...RemoveUselessReturnExprInConstructRector/RemoveUselessReturnExprInConstructRectorTest.php
@@ -0,0 +1,28 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\DeadCode\Rector\ClassMethod\RemoveUselessReturnExprInConstructRector;
+
+use Iterator;
+use PHPUnit\Framework\Attributes\DataProvider;
+use Rector\Testing\PHPUnit\AbstractRectorTestCase;
+
+final class RemoveUselessReturnExprInConstructRectorTest extends AbstractRectorTestCase
+{
+    #[DataProvider('provideData')]
+    public function test(string $filePath): void
+    {
+        $this->doTestFile($filePath);
+    }
+
+    public static function provideData(): Iterator
+    {
+        return self::yieldFilesFromDirectory(__DIR__ . '/Fixture');
+    }
+
+    public function provideConfigFilePath(): string
+    {
+        return __DIR__ . '/config/configured_rule.php';
+    }
+}
diff --git a/...de/Rector/ClassMethod/RemoveUselessReturnExprInConstructRector/config/configured_rule.php b/...de/Rector/ClassMethod/RemoveUselessReturnExprInConstructRector/config/configured_rule.php
@@ -0,0 +1,10 @@
+<?php
+
+declare(strict_types=1);
+
+use Rector\Config\RectorConfig;
+use Rector\DeadCode\Rector\ClassMethod\RemoveUselessReturnExprInConstructRector;
+
+return static function (RectorConfig $rectorConfig): void {
+    $rectorConfig->rule(RemoveUselessReturnExprInConstructRector::class);
+};