[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>

rules-tests/CodeQuality/Rector/ClassMethod/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;
+    }
+}
+
+?>

rules-tests/CodeQuality/Rector/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;
+    }
+}
+
+?>

rules-tests/CodeQuality/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;
+    }
+}
+
+?>

rules-tests/CodeQuality/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;
+        }
+    }
+}

rules-tests/CodeQuality/Rector/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;
+         }
+    }
+}

rules-tests/CodeQuality/Rector/ClassMethod/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;
+         }
+    }
+}

rules-tests/CodeQuality/Rector/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;
+         }
+    }
+}

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(

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