From 2d5de61b0bd6cbc00f86df8b45cd54c013981324 Mon Sep 17 00:00:00 2001 From: Sander Muller Date: Wed, 4 Nov 2026 01:16:45 +0100 Subject: [PATCH] Skip writing new class names when no fix added one RuleErrorTransformer ran NameResolver and NewClassNamesVisitor over the whole file for every fixable error. NewClassNamesVisitor also collects the names the file already uses, twice in a file with a namespace, and parses every PHPDoc in the file for that. A fix that adds no class name does not need any of it. ReplacingNodeVisitor now keeps the nodes that the fixes returned. The traversal runs only when one of them contains a fully qualified name without an original node, or a PHPDoc that differs from the original. Co-Authored-By: Claude Opus 5.5 --- src/Analyser/RuleErrorTransformer.php | 14 +-- src/Fixable/NewClassNamesVisitor.php | 25 +++++ src/Fixable/ReplacingNodeVisitor.php | 15 +++ tests/PHPStan/Fixable/AddNamesRule.php | 22 +++-- tests/PHPStan/Fixable/AddNamesRuleTest.php | 5 + .../Fixable/NewClassNamesVisitorTest.php | 97 +++++++++++++++++++ .../Fixable/data/add-names-phpdoc-only.php | 18 ++++ .../data/add-names-phpdoc-only.php.fixed | 19 ++++ 8 files changed, 203 insertions(+), 12 deletions(-) create mode 100644 tests/PHPStan/Fixable/NewClassNamesVisitorTest.php create mode 100644 tests/PHPStan/Fixable/data/add-names-phpdoc-only.php create mode 100644 tests/PHPStan/Fixable/data/add-names-phpdoc-only.php.fixed diff --git a/src/Analyser/RuleErrorTransformer.php b/src/Analyser/RuleErrorTransformer.php index ef730cac9f4..c70835be6fc 100644 --- a/src/Analyser/RuleErrorTransformer.php +++ b/src/Analyser/RuleErrorTransformer.php @@ -246,12 +246,14 @@ private function fix(string $fixingFile, array $fileNodes, array $fixes): ?Fixed return null; } - $nameResolver = new NameResolver(new Collecting(), ['replaceNodes' => false]); - /** @var Stmt[] $newStmts */ - $newStmts = (new NodeTraverser( - $nameResolver, - new NewClassNamesVisitor($nameResolver->getNameContext(), $this->importNames, $this->preferSelf), - ))->traverse($newStmts); + if (NewClassNamesVisitor::hasWork($replacingVisitor->getFixedNodes())) { + $nameResolver = new NameResolver(new Collecting(), ['replaceNodes' => false]); + /** @var Stmt[] $newStmts */ + $newStmts = (new NodeTraverser( + $nameResolver, + new NewClassNamesVisitor($nameResolver->getNameContext(), $this->importNames, $this->preferSelf), + ))->traverse($newStmts); + } $newCode = $this->getPrinter($indent)->printFormatPreserving($newStmts, $fileNodes, $parsedFile['tokens']); if ($parsedFile['code'] === $newCode) { diff --git a/src/Fixable/NewClassNamesVisitor.php b/src/Fixable/NewClassNamesVisitor.php index e114f107c59..1acc9897675 100644 --- a/src/Fixable/NewClassNamesVisitor.php +++ b/src/Fixable/NewClassNamesVisitor.php @@ -95,6 +95,31 @@ public function __construct( $this->phpDocParser = new PhpDocParser($config, new TypeParser($config, $constExprParser), $constExprParser); } + /** + * Whether the nodes that the fixes returned contain a class name + * or a PHPDoc that this visitor may need to write. + * + * @param Node[] $fixedNodes + */ + public static function hasWork(array $fixedNodes): bool + { + return (new NodeFinder())->findFirst($fixedNodes, static function (Node $node): bool { + $originalNode = $node->getAttribute('origNode'); + if ($node instanceof FullyQualified && $originalNode === null) { + return true; + } + + $doc = $node->getDocComment(); + if ($doc === null) { + return false; + } + + $originalDoc = $originalNode instanceof Node ? $originalNode->getDocComment() : null; + + return $originalDoc === null || $originalDoc->getText() !== $doc->getText(); + }) !== null; + } + #[Override] public function beforeTraverse(array $nodes): ?array { diff --git a/src/Fixable/ReplacingNodeVisitor.php b/src/Fixable/ReplacingNodeVisitor.php index 99081afc3b2..3466c4df16b 100644 --- a/src/Fixable/ReplacingNodeVisitor.php +++ b/src/Fixable/ReplacingNodeVisitor.php @@ -35,6 +35,9 @@ final class ReplacingNodeVisitor extends NodeVisitorAbstract private bool $failed = false; + /** @var list */ + private array $fixedNodes = []; + /** * @param non-empty-list $fixes */ @@ -115,9 +118,21 @@ public function leaveNode(Node $node): Node|int|null } } + $this->fixedNodes[] = $newNode; + return $newNode; } + /** + * Nodes returned by the fixes. A fix changes only these nodes and their children. + * + * @return list + */ + public function getFixedNodes(): array + { + return $this->fixedNodes; + } + /** * Whether every node was found and fixed. */ diff --git a/tests/PHPStan/Fixable/AddNamesRule.php b/tests/PHPStan/Fixable/AddNamesRule.php index 2f73b4571dc..76853b09eaa 100644 --- a/tests/PHPStan/Fixable/AddNamesRule.php +++ b/tests/PHPStan/Fixable/AddNamesRule.php @@ -12,13 +12,15 @@ use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use function count; +use function in_array; use function preg_match_all; use function sprintf; /** * Turns each `@add-name \Some\Class` tag into a native parameter type * and a `@param` tag, to show new names are written the same way - * in PHP code and in PHPDocs. + * in PHP code and in PHPDocs. An `@add-doc-name \Some\Class` tag becomes + * only a `@param` tag. * * @implements Rule */ @@ -37,19 +39,27 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { $docComment = $node->getDocComment(); - if ($docComment === null || preg_match_all('/@add-name\s+\\\\?(\S+)/', $docComment->getText(), $matches) === 0) { + if ($docComment === null || preg_match_all('/@add-(doc-)?name\s+\\\\?(\S+)/', $docComment->getText(), $matches) === 0) { return []; } - $names = $matches[1]; + $names = $matches[2]; + $parameterNames = []; + foreach ($matches[1] as $j => $docOnly) { + if ($docOnly !== '') { + continue; + } + + $parameterNames[] = $names[$j]; + } return [ RuleErrorBuilder::message(sprintf('Adding %d names.', count($names))) ->identifier('tests.addNames') - ->fixNode($node, function (Node\Stmt\ClassMethod $method) use ($names) { + ->fixNode($node, function (Node\Stmt\ClassMethod $method) use ($parameterNames) { $i = 0; $this->phpDocEditor->edit($method, static function (\PHPStan\PhpDocParser\Ast\Node $phpDocNode) use (&$i) { - if (!$phpDocNode instanceof PhpDocTagNode || $phpDocNode->name !== '@add-name') { + if (!$phpDocNode instanceof PhpDocTagNode || !in_array($phpDocNode->name, ['@add-name', '@add-doc-name'], true)) { return null; } @@ -64,7 +74,7 @@ public function processNode(Node $node, Scope $scope): array )); }); - foreach ($names as $j => $name) { + foreach ($parameterNames as $j => $name) { $method->params[] = new Node\Param( new Node\Expr\Variable('p' . ($j + 1)), type: new FullyQualified($name), diff --git a/tests/PHPStan/Fixable/AddNamesRuleTest.php b/tests/PHPStan/Fixable/AddNamesRuleTest.php index 2b40879f90c..798bed33a1c 100644 --- a/tests/PHPStan/Fixable/AddNamesRuleTest.php +++ b/tests/PHPStan/Fixable/AddNamesRuleTest.php @@ -37,4 +37,9 @@ public function testFixSortedUses(): void $this->fix(__DIR__ . '/data/add-names-sorted-uses.php', __DIR__ . '/data/add-names-sorted-uses.php.fixed'); } + public function testFixPhpDocOnly(): void + { + $this->fix(__DIR__ . '/data/add-names-phpdoc-only.php', __DIR__ . '/data/add-names-phpdoc-only.php.fixed'); + } + } diff --git a/tests/PHPStan/Fixable/NewClassNamesVisitorTest.php b/tests/PHPStan/Fixable/NewClassNamesVisitorTest.php new file mode 100644 index 00000000000..42394e7433a --- /dev/null +++ b/tests/PHPStan/Fixable/NewClassNamesVisitorTest.php @@ -0,0 +1,97 @@ + + */ + public static function dataHasWork(): iterable + { + yield 'unchanged' => [ + static fn (ClassMethod $method): Node => $method, + false, + ]; + + yield 'removed attribute' => [ + static function (ClassMethod $method): Node { + $method->attrGroups = []; + return $method; + }, + false, + ]; + + yield 'added name' => [ + static function (ClassMethod $method): Node { + $method->params[] = new Node\Param(new Node\Expr\Variable('qux'), type: new FullyQualified('Qux\Quux')); + return $method; + }, + true, + ]; + + yield 'changed PHPDoc' => [ + static function (ClassMethod $method): Node { + $method->setDocComment(new Doc('/** @return positive-int */')); + return $method; + }, + true, + ]; + + yield 'new node with a PHPDoc' => [ + static function (ClassMethod $method): Node { + $newMethod = new ClassMethod('doBar'); + $newMethod->setDocComment(new Doc('/** @return void */')); + return $newMethod; + }, + true, + ]; + } + + /** + * @param callable(ClassMethod): Node $fix + */ + #[DataProvider('dataHasWork')] + public function testHasWork(callable $fix, bool $expected): void + { + $stmts = (new ParserFactory())->createForHostVersion()->parse(self::CODE); + $this->assertNotNull($stmts); + + $newStmts = (new NodeTraverser(new CloningVisitor()))->traverse($stmts); + $method = (new NodeFinder())->findFirstInstanceOf($newStmts, ClassMethod::class); + $this->assertNotNull($method); + + $this->assertSame($expected, NewClassNamesVisitor::hasWork([$fix($method)])); + } + +} diff --git a/tests/PHPStan/Fixable/data/add-names-phpdoc-only.php b/tests/PHPStan/Fixable/data/add-names-phpdoc-only.php new file mode 100644 index 00000000000..5a6c2c6d3e1 --- /dev/null +++ b/tests/PHPStan/Fixable/data/add-names-phpdoc-only.php @@ -0,0 +1,18 @@ +