diff --git a/config/sets/phpunit-code-quality.php b/config/sets/phpunit-code-quality.php index 2c5407af..e37c210b 100644 --- a/config/sets/phpunit-code-quality.php +++ b/config/sets/phpunit-code-quality.php @@ -43,6 +43,7 @@ use Rector\PHPUnit\CodeQuality\Rector\MethodCall\AssertSameBoolNullToSpecificMethodRector; use Rector\PHPUnit\CodeQuality\Rector\MethodCall\AssertSameTrueFalseToAssertTrueFalseRector; use Rector\PHPUnit\CodeQuality\Rector\MethodCall\AssertTrueFalseToSpecificMethodRector; +use Rector\PHPUnit\CodeQuality\Rector\MethodCall\CallbackSingleAssertToSimplerRector; use Rector\PHPUnit\CodeQuality\Rector\MethodCall\FlipAssertRector; use Rector\PHPUnit\CodeQuality\Rector\MethodCall\MatchAssertSameExpectedTypeRector; use Rector\PHPUnit\CodeQuality\Rector\MethodCall\MergeWithCallableAndWillReturnRector; @@ -137,6 +138,7 @@ */ RemoveExpectAnyFromMockRector::class, SingleMockPropertyTypeRector::class, + CallbackSingleAssertToSimplerRector::class, SimplerWithIsInstanceOfRector::class, DirectInstanceOverMockArgRector::class, diff --git a/rules-tests/CodeQuality/Rector/Class_/RemoveReturnFromVoidMethodMockCallbackRector/Fixture/return_throw_and_null.php.inc b/rules-tests/CodeQuality/Rector/Class_/RemoveReturnFromVoidMethodMockCallbackRector/Fixture/return_throw_and_null.php.inc new file mode 100644 index 00000000..ceb9faad --- /dev/null +++ b/rules-tests/CodeQuality/Rector/Class_/RemoveReturnFromVoidMethodMockCallbackRector/Fixture/return_throw_and_null.php.inc @@ -0,0 +1,53 @@ +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'); + } + }); + } +} + +?> +----- +createMock(SomeEntityManager::class) + ->method('persist') + ->willReturnCallback(function ($entity) use ($matcher): void { + if (1 === $matcher->numberOfInvocations()) { + return; + } + + if (2 === $matcher->numberOfInvocations()) { + throw new \RuntimeException('boom'); + } + }); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/CallbackSingleAssertToSimplerRectorTest.php b/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/CallbackSingleAssertToSimplerRectorTest.php new file mode 100644 index 00000000..f61ce465 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/CallbackSingleAssertToSimplerRectorTest.php @@ -0,0 +1,28 @@ +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/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/Fixture/multiple_with_args.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/Fixture/multiple_with_args.php.inc new file mode 100644 index 00000000..6f8e64d8 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/Fixture/multiple_with_args.php.inc @@ -0,0 +1,47 @@ +getMockBuilder('AnyType')->getMock(); + + $builder->expects($this->exactly(2)) + ->method('add') + ->with($this->callback(function ($type): bool { + $this->assertSame(TextType::class, $type); + + return true; + }), $this->callback(function ($options): bool { + $this->assertSame([], $options); + + return true; + })); + } +} + +?> +----- +getMockBuilder('AnyType')->getMock(); + + $builder->expects($this->exactly(2)) + ->method('add') + ->with($this->equalTo(TextType::class), $this->equalTo([])); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/Fixture/skip_different_assert.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/Fixture/skip_different_assert.php.inc new file mode 100644 index 00000000..36687291 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/Fixture/skip_different_assert.php.inc @@ -0,0 +1,21 @@ +getMockBuilder('AnyType')->getMock(); + + $builder->expects($this->exactly(2)) + ->method('add') + ->with($this->callback(function ($type): bool { + $this->assertInstanceOf(TextType::class, $type); + + return true; + })); + } +} diff --git a/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/Fixture/skip_multiple_stmts.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/Fixture/skip_multiple_stmts.php.inc new file mode 100644 index 00000000..ae9a55a0 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/Fixture/skip_multiple_stmts.php.inc @@ -0,0 +1,22 @@ +getMockBuilder('AnyType')->getMock(); + + $builder->expects($this->exactly(2)) + ->method('add') + ->with($this->callback(function ($type): bool { + $this->assertSame(TextType::class, $type); + $this->assertNotNull($type); + + return true; + })); + } +} diff --git a/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/Fixture/some_file.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/Fixture/some_file.php.inc new file mode 100644 index 00000000..520e6073 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/Fixture/some_file.php.inc @@ -0,0 +1,43 @@ +getMockBuilder('AnyType')->getMock(); + + $builder->expects($this->exactly(2)) + ->method('add') + ->with($this->callback(function ($type): bool { + $this->assertSame(TextType::class, $type); + + return true; + })); + } +} + +?> +----- +getMockBuilder('AnyType')->getMock(); + + $builder->expects($this->exactly(2)) + ->method('add') + ->with($this->equalTo(TextType::class)); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/config/configured_rule.php b/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/config/configured_rule.php new file mode 100644 index 00000000..24cb411c --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector/config/configured_rule.php @@ -0,0 +1,10 @@ +rule(CallbackSingleAssertToSimplerRector::class); +}; diff --git a/rules-tests/CodeQuality/Rector/MethodCall/SimplerWithIsInstanceOfRector/Fixture/multiple_with_args.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/SimplerWithIsInstanceOfRector/Fixture/multiple_with_args.php.inc new file mode 100644 index 00000000..8299d53d --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/SimplerWithIsInstanceOfRector/Fixture/multiple_with_args.php.inc @@ -0,0 +1,45 @@ +getMockBuilder('AnyType')->getMock(); + + $someMock->expects($this->any()) + ->method('dispatch') + ->with($this->callback(function ($event): bool { + $this->assertInstanceOf(\stdClass::class, $event); + return true; + }), $this->callback(function ($name): bool { + $this->assertInstanceOf(\Stringable::class, $name); + return true; + })); + } +} + +?> +----- +getMockBuilder('AnyType')->getMock(); + + $someMock->expects($this->any()) + ->method('dispatch') + ->with($this->isInstanceOf(\stdClass::class), $this->isInstanceOf(\Stringable::class)); + } +} + +?> diff --git a/rules/CodeQuality/Rector/Class_/RemoveReturnFromVoidMethodMockCallbackRector.php b/rules/CodeQuality/Rector/Class_/RemoveReturnFromVoidMethodMockCallbackRector.php index 087929fc..ca8a4c3d 100644 --- a/rules/CodeQuality/Rector/Class_/RemoveReturnFromVoidMethodMockCallbackRector.php +++ b/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; diff --git a/rules/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector.php b/rules/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector.php new file mode 100644 index 00000000..4d4cae45 --- /dev/null +++ b/rules/CodeQuality/Rector/MethodCall/CallbackSingleAssertToSimplerRector.php @@ -0,0 +1,157 @@ +expects($this->exactly(2)) + ->method('add') + ->with($this->callback(function ($type): bool { + $this->assertSame(TextType::class, $type); + + return true; + })); + } +} +CODE_SAMPLE + , + <<<'CODE_SAMPLE' +use PHPUnit\Framework\TestCase; + +final class SomeClass extends TestCase +{ + public function test() + { + $builder->expects($this->exactly(2)) + ->method('add') + ->with($this->equalTo(TextType::class)); + } +} +CODE_SAMPLE + ), + ] + ); + } + + public function getNodeTypes(): array + { + return [MethodCall::class]; + } + + /** + * @param MethodCall $node + */ + public function refactor(Node $node): MethodCall|null + { + if (! $this->testsNodeAnalyzer->isInTestClass($node)) { + return null; + } + + if ($node->isFirstClassCallable()) { + return null; + } + + if (! $this->isName($node->name, 'with')) { + return null; + } + + $hasChanged = false; + + foreach ($node->getArgs() as $arg) { + $expectedExpr = $this->matchCallbackSoleAssertSameExpected($arg->value); + if (! $expectedExpr instanceof Expr) { + continue; + } + + $arg->value = $this->nodeFactory->createMethodCall('this', 'equalTo', [$expectedExpr]); + $hasChanged = true; + } + + if (! $hasChanged) { + return null; + } + + return $node; + } + + private function matchCallbackSoleAssertSameExpected(Expr $expr): ?Expr + { + if (! $expr instanceof MethodCall || ! $this->isName($expr->name, 'callback')) { + return null; + } + + $callbackArgs = $expr->getArgs(); + if ($callbackArgs === []) { + return null; + } + + $innerClosure = $callbackArgs[0]->value; + if (! $innerClosure instanceof Closure) { + return null; + } + + // exactly assertSame() expression + "return true;" + $closureStmts = $innerClosure->getStmts(); + if (count($closureStmts) !== 2) { + return null; + } + + [$firstStmt, $secondStmt] = $closureStmts; + + if (! $firstStmt instanceof Expression) { + return null; + } + + $firstStmtExpr = $firstStmt->expr; + if (! $firstStmtExpr instanceof MethodCall || ! $this->isName($firstStmtExpr->name, 'assertSame')) { + return null; + } + + if (! $secondStmt instanceof Return_ || ! $this->isTrueReturn($secondStmt)) { + return null; + } + + return $firstStmtExpr->getArgs()[0] + ->value; + } + + private function isTrueReturn(Return_ $return): bool + { + return $return->expr instanceof ConstFetch && $this->isName($return->expr, 'true'); + } +} diff --git a/rules/CodeQuality/Rector/MethodCall/SimplerWithIsInstanceOfRector.php b/rules/CodeQuality/Rector/MethodCall/SimplerWithIsInstanceOfRector.php index 4b8a34d8..574071ba 100644 --- a/rules/CodeQuality/Rector/MethodCall/SimplerWithIsInstanceOfRector.php +++ b/rules/CodeQuality/Rector/MethodCall/SimplerWithIsInstanceOfRector.php @@ -5,7 +5,6 @@ namespace Rector\PHPUnit\CodeQuality\Rector\MethodCall; use PhpParser\Node; -use PhpParser\Node\Arg; use PhpParser\Node\Expr; use PhpParser\Node\Expr\Closure; use PhpParser\Node\Expr\Instanceof_; @@ -90,40 +89,50 @@ public function refactor(Node $node): MethodCall|null return null; } - $withFirstArgValue = $node->getArgs()[0] - ->value; - if (! $withFirstArgValue instanceof MethodCall || ! $this->isName($withFirstArgValue->name, 'callback')) { - return null; - } + $hasChanged = false; - $callableMethodCall = $withFirstArgValue; - $callableFirstArgValue = $callableMethodCall->getArgs()[0] - ->value; + foreach ($node->getArgs() as $arg) { + $instanceCheckedClassName = $this->matchCallbackSoleInstanceofCheckClassName($arg->value); + if (! $instanceCheckedClassName instanceof Node) { + continue; + } - $innerClosure = $callableFirstArgValue; - if (! $innerClosure instanceof Closure) { + // convert name to expr + if ($instanceCheckedClassName instanceof Name) { + $instanceCheckedClassName = $this->nodeFactory->createClassConstFetch( + $instanceCheckedClassName->toString(), + 'class' + ); + } + + $arg->value = $this->nodeFactory->createMethodCall('this', 'isInstanceOf', [$instanceCheckedClassName]); + $hasChanged = true; + } + + if (! $hasChanged) { return null; } - $instanceCheckedClassName = $this->matchSoleInstanceofCheckClassName($innerClosure); + return $node; + } - if (! $instanceCheckedClassName instanceof Node) { + private function matchCallbackSoleInstanceofCheckClassName(Expr $expr): Node|null|Name + { + if (! $expr instanceof MethodCall || ! $this->isName($expr->name, 'callback')) { return null; } - // convert name to expr - if ($instanceCheckedClassName instanceof Name) { - $instanceCheckedClassName = $this->nodeFactory->createClassConstFetch( - $instanceCheckedClassName->toString(), - 'class' - ); + $callbackArgs = $expr->getArgs(); + if ($callbackArgs === []) { + return null; } - $node->args = [ - new Arg($this->nodeFactory->createMethodCall('this', 'isInstanceOf', [$instanceCheckedClassName])), - ]; + $innerClosure = $callbackArgs[0]->value; + if (! $innerClosure instanceof Closure) { + return null; + } - return $node; + return $this->matchSoleInstanceofCheckClassName($innerClosure); } private function matchSoleInstanceofCheckClassName(Closure $innerClosure): Node|null|Expr|Name