From 85797b520d0e63ba3f1635fccd34cb6ff1b70d90 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sat, 11 Jul 2026 18:30:20 +0200 Subject: [PATCH] [CodeQuality] Add WillReturnCallbackFallbackToThrowRector to throw on unexpected extra consecutive call --- config/sets/phpunit-code-quality.php | 3 + config/sets/phpunit-mock-to-stub.php | 2 +- config/sets/phpunit100.php | 2 +- ecs.php | 2 +- .../Fixture/skip_fallback_return.php.inc | 28 +++ .../Fixture/skip_no_matcher_branch.php.inc | 20 ++ .../Fixture/void_fallback.php.inc | 50 +++++ ...eturnCallbackFallbackToThrowRectorTest.php | 28 +++ .../config/configured_rule.php | 10 + .../Class_/TestWithToDataProviderRector.php | 2 +- ...illReturnCallbackFallbackToThrowRector.php | 191 ++++++++++++++++++ ...ficAssertContainsWithoutIdentityRector.php | 4 +- src/NodeAnalyzer/AssertCallAnalyzer.php | 2 +- src/NodeAnalyzer/IdentifierManipulator.php | 4 +- 14 files changed, 339 insertions(+), 9 deletions(-) create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/Fixture/skip_fallback_return.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/Fixture/skip_no_matcher_branch.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/Fixture/void_fallback.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/WillReturnCallbackFallbackToThrowRectorTest.php create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/config/configured_rule.php create mode 100644 rules/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector.php diff --git a/config/sets/phpunit-code-quality.php b/config/sets/phpunit-code-quality.php index 449d3fe6..78cd18ce 100644 --- a/config/sets/phpunit-code-quality.php +++ b/config/sets/phpunit-code-quality.php @@ -87,6 +87,9 @@ NarrowSingleWillReturnCallbackRector::class, SingleWithConsecutiveToWithRector::class, + // enable once better tested + // WillReturnCallbackFallbackToThrowRector::class, + // type declarations TypeWillReturnCallableArrowFunctionRector::class, StringCastAssertStringContainsStringRector::class, diff --git a/config/sets/phpunit-mock-to-stub.php b/config/sets/phpunit-mock-to-stub.php index 47b26e3e..a2fee2ea 100644 --- a/config/sets/phpunit-mock-to-stub.php +++ b/config/sets/phpunit-mock-to-stub.php @@ -4,8 +4,8 @@ use Rector\Config\RectorConfig; use Rector\PHPUnit\CodeQuality\Rector\Class_\AddIntersectionVarToMockObjectPropertyRector; -use Rector\PHPUnit\CodeQuality\Rector\ClassMethod\BareCreateMockAssignToDirectUseRector; use Rector\PHPUnit\CodeQuality\Rector\Class_\AddStubIntersectionVarToStubPropertyRector; +use Rector\PHPUnit\CodeQuality\Rector\ClassMethod\BareCreateMockAssignToDirectUseRector; use Rector\PHPUnit\PHPUnit120\Rector\CallLike\CreateStubInCoalesceArgRector; use Rector\PHPUnit\PHPUnit120\Rector\CallLike\CreateStubOverCreateMockArgRector; use Rector\PHPUnit\PHPUnit120\Rector\Class_\PropertyCreateMockToCreateStubRector; diff --git a/config/sets/phpunit100.php b/config/sets/phpunit100.php index eb9045c4..d456d305 100644 --- a/config/sets/phpunit100.php +++ b/config/sets/phpunit100.php @@ -76,7 +76,7 @@ // https://github.com/sebastianbergmann/phpunit/pull/3687 new MethodCallRename('PHPUnit\Framework\MockObject\MockBuilder', 'setMethods', 'onlyMethods'), - //https://github.com/sebastianbergmann/phpunit/issues/5062 + // https://github.com/sebastianbergmann/phpunit/issues/5062 new MethodCallRename('PHPUnit\Framework\TestCase', 'expectDeprecationMessage', 'expectExceptionMessage'), new MethodCallRename( 'PHPUnit\Framework\TestCase', diff --git a/ecs.php b/ecs.php index 083b69b3..32457f38 100644 --- a/ecs.php +++ b/ecs.php @@ -5,7 +5,7 @@ use Symplify\EasyCodingStandard\Config\ECSConfig; return ECSConfig::configure() - ->withPreparedSets(psr12: true, common: true, symplify: true) + ->withPreparedSets(psr12: true, common: true) ->withPaths([ __DIR__ . '/src', __DIR__ . '/rules', diff --git a/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/Fixture/skip_fallback_return.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/Fixture/skip_fallback_return.php.inc new file mode 100644 index 00000000..2427fb71 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/Fixture/skip_fallback_return.php.inc @@ -0,0 +1,28 @@ +exactly(2); + + $someServiceMock = $this->createMock(SomeMockedClass::class); + $someServiceMock->expects($matcher) + ->method('run') + ->willReturnCallback(function () use ($matcher) { + if ($matcher->numberOfInvocations() === 1) { + return 'first'; + } + + if ($matcher->numberOfInvocations() === 2) { + return 'second'; + } + + return 'third'; + }); + } +} diff --git a/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/Fixture/skip_no_matcher_branch.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/Fixture/skip_no_matcher_branch.php.inc new file mode 100644 index 00000000..b0f9a312 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/Fixture/skip_no_matcher_branch.php.inc @@ -0,0 +1,20 @@ +exactly(1); + + $someServiceMock = $this->createMock(SomeMockedClass::class); + $someServiceMock->expects($matcher) + ->method('run') + ->willReturnCallback(function () use ($matcher) { + return 'value'; + }); + } +} diff --git a/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/Fixture/void_fallback.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/Fixture/void_fallback.php.inc new file mode 100644 index 00000000..e3093500 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/Fixture/void_fallback.php.inc @@ -0,0 +1,50 @@ +exactly(1); + + $someServiceMock = $this->createMock(SomeMockedClass::class); + $someServiceMock->expects($matcher) + ->method('run') + ->willReturnCallback(function () use ($matcher) { + if ($matcher->numberOfInvocations() === 1) { + return 'first'; + } + }); + } +} + +?> +----- +exactly(1); + + $someServiceMock = $this->createMock(SomeMockedClass::class); + $someServiceMock->expects($matcher) + ->method('run') + ->willReturnCallback(function () use ($matcher) { + if ($matcher->numberOfInvocations() === 1) { + return 'first'; + } + throw new \PHPUnit\Framework\Exception(sprintf('Method should not be called for the %dth time', $matcher->numberOfInvocations())); + }); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/WillReturnCallbackFallbackToThrowRectorTest.php b/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/WillReturnCallbackFallbackToThrowRectorTest.php new file mode 100644 index 00000000..57724913 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/WillReturnCallbackFallbackToThrowRectorTest.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/WillReturnCallbackFallbackToThrowRector/config/configured_rule.php b/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/config/configured_rule.php new file mode 100644 index 00000000..e34176a5 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector/config/configured_rule.php @@ -0,0 +1,10 @@ +rule(WillReturnCallbackFallbackToThrowRector::class); +}; diff --git a/rules/CodeQuality/Rector/Class_/TestWithToDataProviderRector.php b/rules/CodeQuality/Rector/Class_/TestWithToDataProviderRector.php index fa462f67..c292063a 100644 --- a/rules/CodeQuality/Rector/Class_/TestWithToDataProviderRector.php +++ b/rules/CodeQuality/Rector/Class_/TestWithToDataProviderRector.php @@ -152,7 +152,7 @@ private function refactorClassMethod(Class_ $class, ClassMethod $classMethod): v $arrayItemsSingleLine[] = new ArrayItem($this->createArrayItem($values[0])); } - //cleanup + // cleanup if ($this->phpDocTagRemover->removeTagValueFromNode($phpDocInfo, $testWithPhpDocTagNode)) { $this->hasChanged = true; } diff --git a/rules/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector.php b/rules/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector.php new file mode 100644 index 00000000..2987f8f3 --- /dev/null +++ b/rules/CodeQuality/Rector/MethodCall/WillReturnCallbackFallbackToThrowRector.php @@ -0,0 +1,191 @@ +exactly(1); + + $this->someServiceMock->expects($matcher) + ->method('run') + ->willReturnCallback(function () use ($matcher) { + if ($matcher->numberOfInvocations() === 1) { + return 1; + } + }); + } +} +CODE_SAMPLE + , + <<<'CODE_SAMPLE' +use PHPUnit\Framework\TestCase; + +final class SomeTest extends TestCase +{ + public function test() + { + $matcher = $this->exactly(1); + + $this->someServiceMock->expects($matcher) + ->method('run') + ->willReturnCallback(function () use ($matcher) { + if ($matcher->numberOfInvocations() === 1) { + return 1; + } + + throw new \PHPUnit\Framework\Exception(sprintf('Method should not be called for the %dth time', $matcher->numberOfInvocations())); + }); + } +} +CODE_SAMPLE + ), + ] + ); + } + + /** + * @return array> + */ + public function getNodeTypes(): array + { + return [MethodCall::class]; + } + + /** + * @param MethodCall $node + */ + public function refactor(Node $node): ?MethodCall + { + if (! $this->testsNodeAnalyzer->isInTestClass($node)) { + return null; + } + + if (! $this->isName($node->name, 'willReturnCallback')) { + return null; + } + + if ($node->isFirstClassCallable()) { + return null; + } + + if (count($node->getArgs()) !== 1) { + return null; + } + + $closure = $node->getArgs()[0] + ->value; + if (! $closure instanceof Closure) { + return null; + } + + if (! $this->usesMatcher($closure)) { + return null; + } + + // the closure must branch on the matcher invocation count first + if (! $this->hasConsecutiveIf($closure)) { + return null; + } + + $lastStmt = $closure->stmts[array_key_last($closure->stmts)] ?? null; + + // the last statement must be an if branch; an explicit fallback "return ;" is left untouched + if (! $lastStmt instanceof If_) { + return null; + } + + $closure->stmts[] = $this->createThrow(); + + return $node; + } + + private function usesMatcher(Closure $closure): bool + { + return array_any( + $closure->uses, + fn (ClosureUse $use): bool => $this->isName($use->var, ConsecutiveVariable::MATCHER) + ); + } + + private function hasConsecutiveIf(Closure $closure): bool + { + foreach ($closure->stmts as $stmt) { + if (! $stmt instanceof If_) { + continue; + } + + $hasMatcherCall = false; + $this->traverseNodesWithCallable($stmt->cond, function (Node $node) use (&$hasMatcherCall): null { + if ($node instanceof MethodCall + && $node->var instanceof Variable + && $this->isName($node->var, ConsecutiveVariable::MATCHER) + ) { + $hasMatcherCall = true; + } + + return null; + }); + + if ($hasMatcherCall) { + return true; + } + } + + return false; + } + + private function createThrow(): Expression + { + $sprintfFuncCall = $this->nodeFactory->createFuncCall('sprintf', [ + new String_('Method should not be called for the %dth time'), + $this->matcherInvocationCountMethodCallNodeFactory->create(), + ]); + + $new = new New_(new FullyQualified('PHPUnit\Framework\Exception'), [new Arg($sprintfFuncCall)]); + + return new Expression(new Throw_($new)); + } +} diff --git a/rules/PHPUnit90/Rector/MethodCall/SpecificAssertContainsWithoutIdentityRector.php b/rules/PHPUnit90/Rector/MethodCall/SpecificAssertContainsWithoutIdentityRector.php index e9a51710..e466386b 100644 --- a/rules/PHPUnit90/Rector/MethodCall/SpecificAssertContainsWithoutIdentityRector.php +++ b/rules/PHPUnit90/Rector/MethodCall/SpecificAssertContainsWithoutIdentityRector.php @@ -102,14 +102,14 @@ public function refactor(Node $node): ?Node return null; } - //when less then 5 arguments given: do nothing + // when less then 5 arguments given: do nothing if (! isset($node->getArgs()[4])) { return null; } $fourthArg = $node->getArgs()[4]; - //when 5th argument check identity is true: do nothing + // when 5th argument check identity is true: do nothing if ($this->valueResolver->isValue($fourthArg->value, true)) { return null; } diff --git a/src/NodeAnalyzer/AssertCallAnalyzer.php b/src/NodeAnalyzer/AssertCallAnalyzer.php index dcdc9f30..9b756f4c 100644 --- a/src/NodeAnalyzer/AssertCallAnalyzer.php +++ b/src/NodeAnalyzer/AssertCallAnalyzer.php @@ -170,7 +170,7 @@ private function hasNestedAssertCall(ClassMethod $classMethod): bool }); } - private function resolveClassMethodFromCall(StaticCall | MethodCall $call): ?ClassMethod + private function resolveClassMethodFromCall(StaticCall|MethodCall $call): ?ClassMethod { if ($call instanceof MethodCall) { $objectType = $this->nodeTypeResolver->getType($call->var); diff --git a/src/NodeAnalyzer/IdentifierManipulator.php b/src/NodeAnalyzer/IdentifierManipulator.php index 79bae387..636e872e 100644 --- a/src/NodeAnalyzer/IdentifierManipulator.php +++ b/src/NodeAnalyzer/IdentifierManipulator.php @@ -29,7 +29,7 @@ public function __construct( * @param array $renameMethodMap */ public function renameNodeWithMap( - ClassConstFetch | MethodCall | PropertyFetch | StaticCall | ClassMethod $node, + ClassConstFetch|MethodCall|PropertyFetch|StaticCall|ClassMethod $node, array $renameMethodMap ): bool { $oldNodeMethodName = $this->resolveOldMethodName($node); @@ -42,7 +42,7 @@ public function renameNodeWithMap( } private function resolveOldMethodName( - ClassConstFetch | MethodCall | PropertyFetch | StaticCall | ClassMethod $node + ClassConstFetch|MethodCall|PropertyFetch|StaticCall|ClassMethod $node ): ?string { if ($node instanceof StaticCall || $node instanceof MethodCall) { return $this->nodeNameResolver->getName($node->name);