ShouldCallParentMethodsRule is auto-fixable

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,105 @@ 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 [];
+		}
+
+		$errorBuilder = RuleErrorBuilder::message(
+			sprintf('Missing call to parent::%s() method.', $methodName),
+		)->identifier('phpunit.callParent');
 
-		if (!$hasParentCall) {
-			return [
-				RuleErrorBuilder::message(
-					sprintf('Missing call to parent::%s() method.', $methodName),
-				)->identifier('phpunit.callParent')->build(),
-			];
+		if ($scope->isInTrait()) {
+			return [$errorBuilder->build()];
 		}
 
-		return [];
+		$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()];
+	}
+
+	/**
+	 * @param Node\Stmt[] $stmts
+	 */
+	private function hasParentClassCall(array $stmts, string $methodName): bool
+	{
+		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) !== [];
 	}
 
 	/**
-	 * @param Node\Stmt[]|null $stmts
+	 * setUp() gets the parent call as its first statement, tearDown() as its last one
+	 * (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
 	 */
-	private function hasParentClassCall(?array $stmts, string $methodName): bool
+	private function getParentCallPosition(array $stmts, string $methodName): ?int
 	{
-		if ($stmts === null) {
-			return false;
+		if (strtolower($methodName) === 'setup') {
+			return 0;
+		}
+
+		$position = count($stmts);
+		$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($exits);
+			$position--;
+		}
+
+		if ($exits !== []) {
+			return null;
 		}
 
-		$visitor = new class ($methodName) extends NodeVisitorAbstract {
+		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<Node>
+	 */
+	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<Node> */
+			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 +166,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;

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) {

tests/Rules/PHPUnit/data/missing-parent-method-calls-fixable.php

@@ -0,0 +1,219 @@
+<?php
+
+namespace MissingParentMethodCallsFixable;
+
+use PHPUnit\Framework\TestCase;
+
+abstract class BaseTestCase extends TestCase
+{
+	protected function setUp(): void
+	{
+	}
+
+	protected function tearDown(): void
+	{
+	}
+}
+
+// setUp(): parent call is prepended
+class SetUpTest extends BaseTestCase
+{
+	private ?object $resource = null;
+
+	protected function setUp(): void
+	{
+		$this->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;
+}
+
+// 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;
+	}
+}