[CodeQuality] Handle return throw and return null in void mock callbacks (#713)

Author: TomasVotruba

rules-tests/CodeQuality/Rector/Class_/RemoveReturnFromVoidMethodMockCallbackRector/Fixture/return_throw_and_null.php.inc

@@ -0,0 +1,53 @@
+<?php
+
+namespace Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\RemoveReturnFromVoidMethodMockCallbackRector\Fixture;
+
+use PHPUnit\Framework\TestCase;
+use Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\RemoveReturnFromVoidMethodMockCallbackRector\Source\SomeEntityManager;
+
+final class ReturnThrowAndNull extends TestCase
+{
+    public function test($matcher): void
+    {
+        $this->createMock(SomeEntityManager::class)
+            ->method('persist')
+            ->willReturnCallback(function ($entity) use ($matcher) {
+                if (1 === $matcher->numberOfInvocations()) {
+                    return null;
+                }
+
+                if (2 === $matcher->numberOfInvocations()) {
+                    return throw new \RuntimeException('boom');
+                }
+            });
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\RemoveReturnFromVoidMethodMockCallbackRector\Fixture;
+
+use PHPUnit\Framework\TestCase;
+use Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\RemoveReturnFromVoidMethodMockCallbackRector\Source\SomeEntityManager;
+
+final class ReturnThrowAndNull extends TestCase
+{
+    public function test($matcher): void
+    {
+        $this->createMock(SomeEntityManager::class)
+            ->method('persist')
+            ->willReturnCallback(function ($entity) use ($matcher): void {
+                if (1 === $matcher->numberOfInvocations()) {
+                    return;
+                }
+
+                if (2 === $matcher->numberOfInvocations()) {
+                    throw new \RuntimeException('boom');
+                }
+            });
+    }
+}
+
+?>

rules/CodeQuality/Rector/Class_/RemoveReturnFromVoidMethodMockCallbackRector.php

@@ -11,11 +11,13 @@
 use PhpParser\Node\Expr\ConstFetch;
 use PhpParser\Node\Expr\MethodCall;
 use PhpParser\Node\Expr\PropertyFetch;
+use PhpParser\Node\Expr\Throw_;
 use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Identifier;
 use PhpParser\Node\Scalar;
 use PhpParser\Node\Scalar\String_;
 use PhpParser\Node\Stmt\Class_;
+use PhpParser\Node\Stmt\Expression;
 use PhpParser\Node\Stmt\Function_;
 use PhpParser\Node\Stmt\Return_;
 use PhpParser\NodeFinder;
@@ -275,21 +277,39 @@ private function refactorClosureToVoid(Closure $closure): bool
             return false;
         }
 
-        // only strip side-effect-free values, to not lose behavior
+        // only strip side-effect-free values or a "return throw", to not lose behavior
         foreach ($valueReturns as $valueReturn) {
             $returnedExpr = $valueReturn->expr;
             if (! $returnedExpr instanceof Expr) {
                 continue;
             }
 
+            // "return throw new X" becomes a bare "throw new X" statement below
+            if ($returnedExpr instanceof Throw_) {
+                continue;
+            }
+
             if (! $this->isPureValue($returnedExpr)) {
                 return false;
             }
         }
 
-        foreach ($valueReturns as $valueReturn) {
-            $valueReturn->expr = null;
-        }
+        $this->traverseNodesWithCallable($closure->stmts, static function (Node $subNode): ?Expression {
+            if (! $subNode instanceof Return_) {
+                return null;
+            }
+
+            // "return throw new X;" is itself invalid in a void function, unwrap to "throw new X;"
+            if ($subNode->expr instanceof Throw_) {
+                return new Expression($subNode->expr);
+            }
+
+            if ($subNode->expr instanceof Expr) {
+                $subNode->expr = null;
+            }
+
+            return null;
+        });
 
         // drop a now-empty trailing "return;"
         $lastStmt = $closure->stmts[array_key_last($closure->stmts)] ?? null;