From faeba9a766763b3bd8d089c9d93fd6602d15b3ee Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Fri, 13 Nov 2026 11:57:24 +0100 Subject: [PATCH 1/3] ShouldCallParentMethodsRule.php is auto-fixable --- .../PHPUnit/ShouldCallParentMethodsRule.php | 114 +++++++--- .../ShouldCallParentMethodsRuleTest.php | 8 + .../missing-parent-method-calls-fixable.php | 195 ++++++++++++++++ ...sing-parent-method-calls-fixable.php.fixed | 208 ++++++++++++++++++ 4 files changed, 497 insertions(+), 28 deletions(-) create mode 100644 tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php create mode 100644 tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php.fixed diff --git a/src/Rules/PHPUnit/ShouldCallParentMethodsRule.php b/src/Rules/PHPUnit/ShouldCallParentMethodsRule.php index f272cc5..0ae0291 100644 --- a/src/Rules/PHPUnit/ShouldCallParentMethodsRule.php +++ b/src/Rules/PHPUnit/ShouldCallParentMethodsRule.php @@ -3,6 +3,7 @@ namespace PHPStan\Rules\PHPUnit; use PhpParser\Node; +use PhpParser\Node\Stmt\ClassMethod; use PhpParser\NodeTraverser; use PhpParser\NodeVisitor; use PhpParser\NodeVisitorAbstract; @@ -11,6 +12,10 @@ use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use PHPUnit\Framework\TestCase; +use function array_pop; +use function array_splice; +use function count; +use function end; use function in_array; use function sprintf; use function strtolower; @@ -54,37 +59,99 @@ public function processNode(Node $node, Scope $scope): array return []; } - $hasParentCall = $this->hasParentClassCall($node->getOriginalNode()->getStmts(), strtolower($methodName)); + $stmts = $node->getOriginalNode()->getStmts() ?? []; + if ($this->hasParentClassCall($stmts, strtolower($methodName))) { + return []; + } - if (!$hasParentCall) { - return [ - RuleErrorBuilder::message( - sprintf('Missing call to parent::%s() method.', $methodName), - )->identifier('phpunit.callParent')->build(), - ]; + $errorBuilder = RuleErrorBuilder::message( + sprintf('Missing call to parent::%s() method.', $methodName), + )->identifier('phpunit.callParent'); + + $position = $this->getParentCallPosition($stmts, strtolower($methodName) === 'setup'); + if ($position !== null && !$scope->isInTrait()) { + $parentMethodName = $parentMethod->getName(); + $errorBuilder->fixNode($node->getOriginalNode(), static function (ClassMethod $methodNode) use ($position, $parentMethodName): ClassMethod { + $stmts = $methodNode->stmts ?? []; + array_splice($stmts, $position, 0, [ + new Node\Stmt\Expression( + new Node\Expr\StaticCall(new Node\Name('parent'), new Node\Identifier($parentMethodName)), + ), + ]); + $methodNode->stmts = $stmts; + + return $methodNode; + }); } - return []; + return [$errorBuilder->build()]; } /** - * @param Node\Stmt[]|null $stmts + * @param Node\Stmt[] $stmts */ - private function hasParentClassCall(?array $stmts, string $methodName): bool + private function hasParentClassCall(array $stmts, string $methodName): bool { - if ($stmts === null) { - return false; + return $this->findInMethodScope($stmts, static fn (Node $node): bool => $node instanceof Node\Expr\StaticCall + && !$node->isFirstClassCallable() + && $node->class instanceof Node\Name + && $node->class->toLowerString() === 'parent' + && $node->name instanceof Node\Identifier + && $node->name->toLowerString() === $methodName) !== []; + } + + /** + * setUp() gets the parent call as its first statement, tearDown() as its last one + * (before a trailing return). A tearDown() with an early return is not fixable, + * because the appended call would be skipped on that path. + * + * @param Node\Stmt[] $stmts + */ + private function getParentCallPosition(array $stmts, bool $isSetUp): ?int + { + if ($isSetUp) { + return 0; } - $visitor = new class ($methodName) extends NodeVisitorAbstract { + $position = count($stmts); + $returns = $this->findInMethodScope($stmts, static fn (Node $node): bool => $node instanceof Node\Stmt\Return_); + + if (end($stmts) instanceof Node\Stmt\Return_) { + // the trailing return is the last one found + array_pop($returns); + $position--; + } + + if ($returns !== []) { + return null; + } + + return $position; + } + + /** + * Finds nodes in the method body, skipping nested functions, closures and classes. + * + * @param Node\Stmt[] $stmts + * @param callable(Node): bool $filter + * @return list + */ + private function findInMethodScope(array $stmts, callable $filter): array + { + $visitor = new class ($filter) extends NodeVisitorAbstract { - private string $methodName; + /** @var callable(Node): bool */ + private $filter; - public bool $found = false; + /** @var list */ + public array $found = []; - public function __construct(string $methodName) + /** + * @param callable(Node): bool $filter + */ + public function __construct(callable $filter) { - $this->methodName = $methodName; + $this->filter = $filter; } public function enterNode(Node $node): ?int @@ -93,17 +160,8 @@ public function enterNode(Node $node): ?int return NodeVisitor::DONT_TRAVERSE_CHILDREN; } - if ( - $node instanceof Node\Expr\StaticCall - && !$node->isFirstClassCallable() - && $node->class instanceof Node\Name - && $node->class->toLowerString() === 'parent' - && $node->name instanceof Node\Identifier - && $node->name->toLowerString() === $this->methodName - ) { - $this->found = true; - - return NodeVisitor::STOP_TRAVERSAL; + if (($this->filter)($node)) { + $this->found[] = $node; } return null; diff --git a/tests/Rules/PHPUnit/ShouldCallParentMethodsRuleTest.php b/tests/Rules/PHPUnit/ShouldCallParentMethodsRuleTest.php index 76705d1..51fff4e 100644 --- a/tests/Rules/PHPUnit/ShouldCallParentMethodsRuleTest.php +++ b/tests/Rules/PHPUnit/ShouldCallParentMethodsRuleTest.php @@ -47,6 +47,14 @@ public function testRule(): void ]); } + public function testFix(): void + { + $this->fix( + __DIR__ . '/data/missing-parent-method-calls-fixable.php', + __DIR__ . '/data/missing-parent-method-calls-fixable.php.fixed', + ); + } + public function testFirstClassCallable(): void { if (PHP_VERSION_ID < 80100) { diff --git a/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php b/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php new file mode 100644 index 0000000..5b7f3ce --- /dev/null +++ b/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php @@ -0,0 +1,195 @@ +resource = new \stdClass(); + } +} + +// tearDown(): parent call is appended +class TearDownTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function tearDown(): void + { + $this->resource = null; + } +} + +// both methods in one class are fixed +class SetUpAndTearDownTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function setUp(): void + { + $this->resource = new \stdClass(); + } + + protected function tearDown(): void + { + $this->resource = null; + } +} + +// empty bodies +class EmptyBodyTest extends BaseTestCase +{ + protected function setUp(): void + { + } + + protected function tearDown(): void + { + } +} + +// bodies containing only a comment +class CommentOnlyBodyTest extends BaseTestCase +{ + protected function setUp(): void + { + // nothing to set up + } + + protected function tearDown(): void + { + // nothing to tear down + } +} + +// the generated call uses the name of the parent method, not the lowercase declaration +class LowercaseMethodNameTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function setup(): void + { + $this->resource = new \stdClass(); + } + + protected function teardown(): void + { + $this->resource = null; + } +} + +// setUp() with an early return: prepending still runs on every path +class SetUpEarlyReturnTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function setUp(): void + { + if ($this->resource !== null) { + return; + } + + $this->resource = new \stdClass(); + } +} + +// tearDown() with a trailing return: parent call is inserted before it +class TearDownTrailingReturnTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function tearDown(): void + { + $this->resource = null; + + return; + } +} + +// tearDown() with an early return: an appended call would be skipped, not fixed +class TearDownEarlyReturnTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function tearDown(): void + { + if ($this->resource === null) { + return; + } + + $this->resource = null; + } +} + +// a return inside a closure is not an early return of tearDown() +class TearDownReturnInClosureTest extends BaseTestCase +{ + private ?\Closure $factory = null; + + protected function tearDown(): void + { + $this->factory = static function (): int { + return 1; + }; + } +} + +// parent call already present in a nested statement: nothing to fix +class NestedParentCallTest extends BaseTestCase +{ + private bool $enabled = true; + + protected function setUp(): void + { + if ($this->enabled) { + parent::setUp(); + } + } + + protected function tearDown(): void + { + try { + $this->enabled = false; + } finally { + parent::tearDown(); + } + } +} + +// methods from a trait: fixing the trait would affect every class using it, not fixed +trait LifecycleTrait +{ + protected function setUp(): void + { + $this->resource = new \stdClass(); + } + + protected function tearDown(): void + { + $this->resource = null; + } +} + +class TraitLifecycleTest extends BaseTestCase +{ + use LifecycleTrait; + + private ?object $resource = null; +} diff --git a/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php.fixed b/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php.fixed new file mode 100644 index 0000000..8e3a37d --- /dev/null +++ b/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php.fixed @@ -0,0 +1,208 @@ +resource = new \stdClass(); + } +} + +// tearDown(): parent call is appended +class TearDownTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function tearDown(): void + { + $this->resource = null; + parent::tearDown(); + } +} + +// both methods in one class are fixed +class SetUpAndTearDownTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function setUp(): void + { + parent::setUp(); + $this->resource = new \stdClass(); + } + + protected function tearDown(): void + { + $this->resource = null; + parent::tearDown(); + } +} + +// empty bodies +class EmptyBodyTest extends BaseTestCase +{ + protected function setUp(): void + { + parent::setUp(); + } + + protected function tearDown(): void + { + parent::tearDown(); + } +} + +// bodies containing only a comment +class CommentOnlyBodyTest extends BaseTestCase +{ + protected function setUp(): void + { + parent::setUp(); + // nothing to set up + } + + protected function tearDown(): void + { + // nothing to tear down + parent::tearDown(); + } +} + +// the generated call uses the name of the parent method, not the lowercase declaration +class LowercaseMethodNameTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function setup(): void + { + parent::setUp(); + $this->resource = new \stdClass(); + } + + protected function teardown(): void + { + $this->resource = null; + parent::tearDown(); + } +} + +// setUp() with an early return: prepending still runs on every path +class SetUpEarlyReturnTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function setUp(): void + { + parent::setUp(); + if ($this->resource !== null) { + return; + } + + $this->resource = new \stdClass(); + } +} + +// tearDown() with a trailing return: parent call is inserted before it +class TearDownTrailingReturnTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function tearDown(): void + { + $this->resource = null; + parent::tearDown(); + + return; + } +} + +// tearDown() with an early return: an appended call would be skipped, not fixed +class TearDownEarlyReturnTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function tearDown(): void + { + if ($this->resource === null) { + return; + } + + $this->resource = null; + } +} + +// a return inside a closure is not an early return of tearDown() +class TearDownReturnInClosureTest extends BaseTestCase +{ + private ?\Closure $factory = null; + + protected function tearDown(): void + { + $this->factory = static function (): int { + return 1; + }; + parent::tearDown(); + } +} + +// parent call already present in a nested statement: nothing to fix +class NestedParentCallTest extends BaseTestCase +{ + private bool $enabled = true; + + protected function setUp(): void + { + if ($this->enabled) { + parent::setUp(); + } + } + + protected function tearDown(): void + { + try { + $this->enabled = false; + } finally { + parent::tearDown(); + } + } +} + +// methods from a trait: fixing the trait would affect every class using it, not fixed +trait LifecycleTrait +{ + protected function setUp(): void + { + $this->resource = new \stdClass(); + } + + protected function tearDown(): void + { + $this->resource = null; + } +} + +class TraitLifecycleTest extends BaseTestCase +{ + use LifecycleTrait; + + private ?object $resource = null; +} From a442725301f0f5bd463701079abdade0cc981067 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Fri, 13 Nov 2026 12:02:02 +0100 Subject: [PATCH 2/3] cheap checks first --- .../PHPUnit/ShouldCallParentMethodsRule.php | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/src/Rules/PHPUnit/ShouldCallParentMethodsRule.php b/src/Rules/PHPUnit/ShouldCallParentMethodsRule.php index 0ae0291..6b691ad 100644 --- a/src/Rules/PHPUnit/ShouldCallParentMethodsRule.php +++ b/src/Rules/PHPUnit/ShouldCallParentMethodsRule.php @@ -68,22 +68,28 @@ public function processNode(Node $node, Scope $scope): array sprintf('Missing call to parent::%s() method.', $methodName), )->identifier('phpunit.callParent'); - $position = $this->getParentCallPosition($stmts, strtolower($methodName) === 'setup'); - if ($position !== null && !$scope->isInTrait()) { - $parentMethodName = $parentMethod->getName(); - $errorBuilder->fixNode($node->getOriginalNode(), static function (ClassMethod $methodNode) use ($position, $parentMethodName): ClassMethod { - $stmts = $methodNode->stmts ?? []; - array_splice($stmts, $position, 0, [ - new Node\Stmt\Expression( - new Node\Expr\StaticCall(new Node\Name('parent'), new Node\Identifier($parentMethodName)), - ), - ]); - $methodNode->stmts = $stmts; - - return $methodNode; - }); + if ($scope->isInTrait()) { + return [$errorBuilder->build()]; } + $position = $this->getParentCallPosition($stmts, $methodName); + if ($position === null) { + return [$errorBuilder->build()]; + } + + $parentMethodName = $parentMethod->getName(); + $errorBuilder->fixNode($node->getOriginalNode(), static function (ClassMethod $methodNode) use ($position, $parentMethodName): ClassMethod { + $stmts = $methodNode->stmts ?? []; + array_splice($stmts, $position, 0, [ + new Node\Stmt\Expression( + new Node\Expr\StaticCall(new Node\Name('parent'), new Node\Identifier($parentMethodName)), + ), + ]); + $methodNode->stmts = $stmts; + + return $methodNode; + }); + return [$errorBuilder->build()]; } @@ -107,9 +113,9 @@ private function hasParentClassCall(array $stmts, string $methodName): bool * * @param Node\Stmt[] $stmts */ - private function getParentCallPosition(array $stmts, bool $isSetUp): ?int + private function getParentCallPosition(array $stmts, string $methodName): ?int { - if ($isSetUp) { + if (strtolower($methodName) === 'setup') { return 0; } From 6036e8ef5e2a0bab1ac9dbedb778be66e56a4f36 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Fri, 13 Nov 2026 12:15:20 +0100 Subject: [PATCH 3/3] Address feedback --- .../PHPUnit/ShouldCallParentMethodsRule.php | 10 ++++---- .../missing-parent-method-calls-fixable.php | 24 +++++++++++++++++++ ...sing-parent-method-calls-fixable.php.fixed | 24 +++++++++++++++++++ 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/src/Rules/PHPUnit/ShouldCallParentMethodsRule.php b/src/Rules/PHPUnit/ShouldCallParentMethodsRule.php index 6b691ad..7f3d082 100644 --- a/src/Rules/PHPUnit/ShouldCallParentMethodsRule.php +++ b/src/Rules/PHPUnit/ShouldCallParentMethodsRule.php @@ -108,8 +108,8 @@ private function hasParentClassCall(array $stmts, string $methodName): bool /** * setUp() gets the parent call as its first statement, tearDown() as its last one - * (before a trailing return). A tearDown() with an early return is not fixable, - * because the appended call would be skipped on that path. + * (before a trailing return). A tearDown() with an early return or with a throw + * is not fixable, because the appended call would be skipped on that path. * * @param Node\Stmt[] $stmts */ @@ -120,15 +120,15 @@ private function getParentCallPosition(array $stmts, string $methodName): ?int } $position = count($stmts); - $returns = $this->findInMethodScope($stmts, static fn (Node $node): bool => $node instanceof Node\Stmt\Return_); + $exits = $this->findInMethodScope($stmts, static fn (Node $node): bool => $node instanceof Node\Stmt\Return_ || $node instanceof Node\Expr\Throw_); if (end($stmts) instanceof Node\Stmt\Return_) { // the trailing return is the last one found - array_pop($returns); + array_pop($exits); $position--; } - if ($returns !== []) { + if ($exits !== []) { return null; } diff --git a/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php b/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php index 5b7f3ce..7e64282 100644 --- a/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php +++ b/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php @@ -193,3 +193,27 @@ class TraitLifecycleTest extends BaseTestCase private ?object $resource = null; } + +// tearDown() ending with a throw: an appended call would be unreachable, not fixed +class TearDownTrailingThrowTest extends BaseTestCase +{ + protected function tearDown(): void + { + throw new \RuntimeException('tearDown failed'); + } +} + +// tearDown() with a conditional throw: an appended call would be skipped, not fixed +class TearDownEarlyThrowTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function tearDown(): void + { + if ($this->resource === null) { + throw new \RuntimeException('tearDown failed'); + } + + $this->resource = null; + } +} diff --git a/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php.fixed b/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php.fixed index 8e3a37d..9beb0ce 100644 --- a/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php.fixed +++ b/tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php.fixed @@ -206,3 +206,27 @@ class TraitLifecycleTest extends BaseTestCase private ?object $resource = null; } + +// tearDown() ending with a throw: an appended call would be unreachable, not fixed +class TearDownTrailingThrowTest extends BaseTestCase +{ + protected function tearDown(): void + { + throw new \RuntimeException('tearDown failed'); + } +} + +// tearDown() with a conditional throw: an appended call would be skipped, not fixed +class TearDownEarlyThrowTest extends BaseTestCase +{ + private ?object $resource = null; + + protected function tearDown(): void + { + if ($this->resource === null) { + throw new \RuntimeException('tearDown failed'); + } + + $this->resource = null; + } +}