[CodeQuality] Skip if else return on ExplicitReturnNullRector (#5755)

* [CodeQuality] Skip if else return on ExplicitReturnNullRector * Fix * fix ci * add in try catch * Fix * more fixture * try catch check * try catch check * Fix * [ci-review] Rector Rectify * if else functionality * more fixture * more fixture --------- Co-authored-by: GitHub Action <actions@github.com>
rectorphp · Mar 22, 2024 · 32bf5c7 · 32bf5c7
1 parent b4eb883
commit 32bf5c7
Show file tree

Hide file tree

Showing 10 changed files with 262 additions and 40 deletions.
diff --git a/...od/ExplicitReturnNullRector/Fixture/if_else_return_in_try_catch_finally_no_return.php.inc b/...od/ExplicitReturnNullRector/Fixture/if_else_return_in_try_catch_finally_no_return.php.inc
@@ -0,0 +1,48 @@
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\ClassMethod\ExplicitReturnNullRector\Fixture;
+
+final class IfElseReturnInTryCatchFinallyNoReturn
+{
+    public function run(int $number)
+    {
+        if (rand(0, 1)) {
+            try {
+                return execute();
+            } catch (\Exception $e) {
+                return 2;
+            } finally {
+                echo 'here no return';
+            }
+         } else {
+             return 2;
+         }
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\ClassMethod\ExplicitReturnNullRector\Fixture;
+
+final class IfElseReturnInTryCatchFinallyNoReturn
+{
+    public function run(int $number)
+    {
+        if (rand(0, 1)) {
+            try {
+                return execute();
+            } catch (\Exception $e) {
+                return 2;
+            } finally {
+                echo 'here no return';
+            }
+         } else {
+             return 2;
+         }
+        return null;
+    }
+}
+
+?>
diff --git a/...tor/ClassMethod/ExplicitReturnNullRector/Fixture/if_elseif_else_partial_no_return.php.inc b/...tor/ClassMethod/ExplicitReturnNullRector/Fixture/if_elseif_else_partial_no_return.php.inc
@@ -0,0 +1,40 @@
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\ClassMethod\ExplicitReturnNullRector\Fixture;
+
+final class IfElseIfElsePartialNoReturn
+{
+    public function run(int $number)
+    {
+        if (rand(0, 1)) {
+            return 1;
+         } elseif (rand(0, 1)) {
+            // no return here
+         } else {
+             return 2;
+         }
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\ClassMethod\ExplicitReturnNullRector\Fixture;
+
+final class IfElseIfElsePartialNoReturn
+{
+    public function run(int $number)
+    {
+        if (rand(0, 1)) {
+            return 1;
+         } elseif (rand(0, 1)) {
+            // no return here
+         } else {
+             return 2;
+         }
+        return null;
+    }
+}
+
+?>
diff --git a/...uality/Rector/ClassMethod/ExplicitReturnNullRector/Fixture/if_elseif_without_else.php.inc b/...uality/Rector/ClassMethod/ExplicitReturnNullRector/Fixture/if_elseif_without_else.php.inc
@@ -0,0 +1,36 @@
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\ClassMethod\ExplicitReturnNullRector\Fixture;
+
+final class IfElseifWithoutElse
+{
+    public function run(int $number)
+    {
+        if (rand(0, 1)) {
+            return 1;
+         } elseif (rand(0, 1)) {
+            return 2;
+         }
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\ClassMethod\ExplicitReturnNullRector\Fixture;
+
+final class IfElseifWithoutElse
+{
+    public function run(int $number)
+    {
+        if (rand(0, 1)) {
+            return 1;
+         } elseif (rand(0, 1)) {
+            return 2;
+         }
+        return null;
+    }
+}
+
+?>
diff --git a/...deQuality/Rector/ClassMethod/ExplicitReturnNullRector/Fixture/skip_if_else_return.php.inc b/...deQuality/Rector/ClassMethod/ExplicitReturnNullRector/Fixture/skip_if_else_return.php.inc
@@ -0,0 +1,15 @@
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\ClassMethod\ExplicitReturnNullRector\Fixture;
+
+final class SkipIfElseReturn
+{
+    public function run(int $number)
+    {
+        if ($number > 50) {
+            return 'yes';
+        } else {
+            return 100;
+        }
+    }
+}
diff --git a/...tor/ClassMethod/ExplicitReturnNullRector/Fixture/skip_if_else_return_in_try_catch.php.inc b/...tor/ClassMethod/ExplicitReturnNullRector/Fixture/skip_if_else_return_in_try_catch.php.inc
@@ -0,0 +1,19 @@
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\ClassMethod\ExplicitReturnNullRector\Fixture;
+
+final class SkipIfElseReturnInTryCatch
+{
+    public function run(int $number)
+    {
+        if (rand(0, 1)) {
+            try {
+                return execute();
+            } catch (\Exception $e) {
+                return 2;
+            }
+         } else {
+             return 2;
+         }
+    }
+}
diff --git a/...sMethod/ExplicitReturnNullRector/Fixture/skip_if_else_return_in_try_catch_finally.php.inc b/...sMethod/ExplicitReturnNullRector/Fixture/skip_if_else_return_in_try_catch_finally.php.inc
@@ -0,0 +1,21 @@
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\ClassMethod\ExplicitReturnNullRector\Fixture;
+
+final class SkipIfElseReturnInTryCatchFinally
+{
+    public function run(int $number)
+    {
+        if (rand(0, 1)) {
+            try {
+                return execute();
+            } catch (\Exception $e) {
+                return 2;
+            } finally {
+                return 3;
+            }
+         } else {
+             return 2;
+         }
+    }
+}
diff --git a/...or/ClassMethod/ExplicitReturnNullRector/Fixture/skip_if_elseif_else_always_return.php.inc b/...or/ClassMethod/ExplicitReturnNullRector/Fixture/skip_if_elseif_else_always_return.php.inc
@@ -0,0 +1,17 @@
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\ClassMethod\ExplicitReturnNullRector\Fixture;
+
+final class SkipIfElseReturnInTryCatch
+{
+    public function run(int $number)
+    {
+        if (rand(0, 1)) {
+            return 1;
+         } elseif (rand(0, 2)) {
+            return 3;
+         } else {
+             return 2;
+         }
+    }
+}
diff --git a/rules/CodeQuality/Rector/ClassMethod/ExplicitReturnNullRector.php b/rules/CodeQuality/Rector/ClassMethod/ExplicitReturnNullRector.php
@@ -14,6 +14,7 @@
 use PhpParser\Node\Stmt\Throw_;
 use Rector\PhpParser\Node\BetterNodeFinder;
 use Rector\Rector\AbstractRector;
+use Rector\TypeDeclaration\TypeInferer\SilentVoidResolver;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
 use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
 
@@ -24,6 +25,7 @@ final class ExplicitReturnNullRector extends AbstractRector
 {
     public function __construct(
         private readonly BetterNodeFinder $betterNodeFinder,
+        private readonly SilentVoidResolver $silentVoidResolver
     ) {
     }
 
@@ -83,10 +85,6 @@ public function refactor(Node $node): ?Node
             return null;
         }
 
-        if ($this->hasRootLevelReturn($node)) {
-            return null;
-        }
-
         if ($this->containsYieldOrThrow($node)) {
             return null;
         }
@@ -96,22 +94,15 @@ public function refactor(Node $node): ?Node
             return null;
         }
 
+        if (! $this->silentVoidResolver->hasSilentVoid($node)) {
+            return null;
+        }
+
         $node->stmts[] = new Return_(new ConstFetch(new Name('null')));
 
         return $node;
     }
 
-    private function hasRootLevelReturn(ClassMethod $classMethod): bool
-    {
-        foreach ((array) $classMethod->stmts as $stmt) {
-            if ($stmt instanceof Return_) {
-                return true;
-            }
-        }
-
-        return false;
-    }
-
     private function containsYieldOrThrow(ClassMethod $classMethod): bool
     {
         return (bool) $this->betterNodeFinder->findInstancesOf(

diff --git a/rules/TypeDeclaration/TypeInferer/SilentVoidResolver.php b/rules/TypeDeclaration/TypeInferer/SilentVoidResolver.php
@@ -8,13 +8,17 @@
 use PhpParser\Node\Expr;
 use PhpParser\Node\Expr\ArrowFunction;
 use PhpParser\Node\Expr\Closure;
+use PhpParser\Node\Expr\Exit_;
 use PhpParser\Node\Expr\Yield_;
 use PhpParser\Node\Expr\YieldFrom;
 use PhpParser\Node\FunctionLike;
 use PhpParser\Node\Stmt;
 use PhpParser\Node\Stmt\ClassMethod;
+use PhpParser\Node\Stmt\Else_;
 use PhpParser\Node\Stmt\Expression;
+use PhpParser\Node\Stmt\Finally_;
 use PhpParser\Node\Stmt\Function_;
+use PhpParser\Node\Stmt\If_;
 use PhpParser\Node\Stmt\Return_;
 use PhpParser\Node\Stmt\Switch_;
 use PhpParser\Node\Stmt\Throw_;
@@ -58,46 +62,67 @@ public function hasSilentVoid(FunctionLike $functionLike): bool
             return false;
         }
 
-        if ($this->hasStmtsAlwaysReturn((array) $functionLike->getStmts())) {
-            return false;
-        }
+        $stmts = (array) $functionLike->getStmts();
+        return ! $this->hasStmtsAlwaysReturnOrExit($stmts);
+    }
+
+    /**
+     * @param Stmt[]|Expression[] $stmts
+     */
+    private function hasStmtsAlwaysReturnOrExit(array $stmts): bool
+    {
+        foreach ($stmts as $stmt) {
+            if ($stmt instanceof Expression) {
+                $stmt = $stmt->expr;
+            }
+
+            if ($this->isStopped($stmt)) {
+                return true;
+            }
 
-        foreach ((array) $functionLike->getStmts() as $stmt) {
             // has switch with always return
             if ($stmt instanceof Switch_ && $this->isSwitchWithAlwaysReturn($stmt)) {
-                return false;
+                return true;
             }
 
-            // is part of try/catch
             if ($stmt instanceof TryCatch && $this->isTryCatchAlwaysReturn($stmt)) {
-                return false;
+                return true;
             }
 
-            if ($stmt instanceof Throw_) {
-                return false;
+            if ($this->isIfReturn($stmt)) {
+                return true;
             }
         }
 
-        return true;
+        return false;
     }
 
-    /**
-     * @param Stmt[]|Expression[] $stmts
-     */
-    private function hasStmtsAlwaysReturn(array $stmts): bool
+    private function isIfReturn(Stmt|Expr $stmt): bool
     {
-        foreach ($stmts as $stmt) {
-            if ($stmt instanceof Expression) {
-                $stmt = $stmt->expr;
-            }
+        if (! $stmt instanceof If_) {
+            return false;
+        }
 
-            // is 1st level return
-            if ($stmt instanceof Return_) {
-                return true;
+        foreach ($stmt->elseifs as $elseIf) {
+            if (! $this->hasStmtsAlwaysReturnOrExit($elseIf->stmts)) {
+                return false;
             }
         }
 
-        return false;
+        if (! $stmt->else instanceof Else_) {
+            return false;
+        }
+
+        if (! $this->hasStmtsAlwaysReturnOrExit($stmt->stmts)) {
+            return false;
+        }
+
+        return $this->hasStmtsAlwaysReturnOrExit($stmt->else->stmts);
+    }
+
+    private function isStopped(Stmt|Expr $stmt): bool
+    {
+        return $stmt instanceof Throw_ || $stmt instanceof Exit_ || $stmt instanceof Return_;
     }
 
     private function isSwitchWithAlwaysReturn(Switch_ $switch): bool
@@ -123,15 +148,19 @@ private function isSwitchWithAlwaysReturn(Switch_ $switch): bool
 
     private function isTryCatchAlwaysReturn(TryCatch $tryCatch): bool
     {
-        if (! $this->hasStmtsAlwaysReturn($tryCatch->stmts)) {
+        if (! $this->hasStmtsAlwaysReturnOrExit($tryCatch->stmts)) {
             return false;
         }
 
         foreach ($tryCatch->catches as $catch) {
-            return $this->hasStmtsAlwaysReturn($catch->stmts);
+            if (! $this->hasStmtsAlwaysReturnOrExit($catch->stmts)) {
+                return false;
+            }
         }
 
-        return true;
+        return ! ($tryCatch->finally instanceof Finally_ && ! $this->hasStmtsAlwaysReturnOrExit(
+            $tryCatch->finally->stmts
+        ));
     }
 
     private function resolveReturnCount(Switch_ $switch): int