From 233a0b09422248d523845dab602d4156f6633d00 Mon Sep 17 00:00:00 2001 From: Sander Muller Date: Wed, 4 Nov 2026 13:35:08 +0100 Subject: [PATCH] Clone and print only the nodes that a fix changes For each fixable error, fix() cloned every node of the file and printed the whole file again. PathCloningVisitor now clones only the fixed nodes and their parents. Each other node stays the original object, and PhpPrinter copies its original code without printing its children. The original nodes get themselves as origNode while the fix is printed, and lose the attribute again afterwards. When a fix adds a class name, SharedNodeCloningVisitor clones the whole tree first, because NameResolver and NewClassNamesVisitor change nodes in place. Co-Authored-By: Claude Opus 5.5 --- phpstan-baseline.neon | 12 ++ src/Analyser/RuleErrorTransformer.php | 75 ++++++---- src/Fixable/PathCloningVisitor.php | 141 ++++++++++++++++++ src/Fixable/PhpPrinter.php | 30 ++++ src/Fixable/SharedNodeCloningVisitor.php | 54 +++++++ tests/PHPStan/Fixable/RenameFunctionRule.php | 54 +++++++ .../Fixable/RenameFunctionRuleTest.php | 33 ++++ .../PHPStan/Fixable/data/rename-function.php | 18 +++ .../Fixable/data/rename-function.php.fixed | 18 +++ 9 files changed, 407 insertions(+), 28 deletions(-) create mode 100644 src/Fixable/PathCloningVisitor.php create mode 100644 src/Fixable/SharedNodeCloningVisitor.php create mode 100644 tests/PHPStan/Fixable/RenameFunctionRule.php create mode 100644 tests/PHPStan/Fixable/RenameFunctionRuleTest.php create mode 100644 tests/PHPStan/Fixable/data/rename-function.php create mode 100644 tests/PHPStan/Fixable/data/rename-function.php.fixed diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 14f91cdd161..5405c84fb3b 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -295,6 +295,18 @@ parameters: count: 1 path: src/DependencyInjection/NeonAdapter.php + - + rawMessage: 'Call to method getIndentationBefore() of internal class PhpParser\Internal\TokenStream from outside its root namespace PhpParser.' + identifier: method.internalClass + count: 1 + path: src/Fixable/PhpPrinter.php + + - + rawMessage: 'Call to method getTokenCode() of internal class PhpParser\Internal\TokenStream from outside its root namespace PhpParser.' + identifier: method.internalClass + count: 1 + path: src/Fixable/PhpPrinter.php + - rawMessage: 'Call to method getTokenCode() of internal class PhpParser\Internal\TokenStream from outside its root namespace PhpParser.' identifier: method.internalClass diff --git a/src/Analyser/RuleErrorTransformer.php b/src/Analyser/RuleErrorTransformer.php index 6f043096dc0..43ea60731d7 100644 --- a/src/Analyser/RuleErrorTransformer.php +++ b/src/Analyser/RuleErrorTransformer.php @@ -15,9 +15,11 @@ use PHPStan\DependencyInjection\AutowiredService; use PHPStan\File\FileReader; use PHPStan\Fixable\NewClassNamesVisitor; +use PHPStan\Fixable\PathCloningVisitor; use PHPStan\Fixable\PhpPrinter; use PHPStan\Fixable\PhpPrinterIndentationDetectorVisitor; use PHPStan\Fixable\ReplacingNodeVisitor; +use PHPStan\Fixable\SharedNodeCloningVisitor; use PHPStan\Node\VirtualNode; use PHPStan\Parser\Parser as AnalysisParser; use PHPStan\Parser\ParserErrorsException; @@ -35,6 +37,7 @@ use SebastianBergmann\Diff\Differ; use SebastianBergmann\Diff\Output\UnifiedDiffOutputBuilder; use function array_key_exists; +use function array_map; use function array_shift; use function array_slice; use function count; @@ -217,50 +220,66 @@ private function fix(string $fixingFile, array $fileNodes, array $fixes): ?Fixed $parsedFile = $this->getParsedFile($fixingFile); $indentDetector = null; + $pathCloningVisitor = null; $visitors = []; + $fixedNodes = array_map(static fn (NodeFix $fix): Node => $fix->node, $fixes); if ($parsedFile['indent'] === null) { $indentDetector = new PhpPrinterIndentationDetectorVisitor(new TokenStream($parsedFile['tokens'], PhpPrinter::TAB_WIDTH)); $visitors[] = $indentDetector; + $visitors[] = new CloningVisitor(); + } elseif (PathCloningVisitor::canFind($fixedNodes)) { + $pathCloningVisitor = new PathCloningVisitor($fixedNodes); + $visitors[] = $pathCloningVisitor; + } else { + $visitors[] = new CloningVisitor(); } - $visitors[] = new CloningVisitor(); $replacingVisitor = new ReplacingNodeVisitor($fixes); $visitors[] = $replacingVisitor; - /** @var Stmt[] $newStmts */ - $newStmts = (new NodeTraverser(...$visitors))->traverse($fileNodes); + try { + /** @var Stmt[] $newStmts */ + $newStmts = (new NodeTraverser(...$visitors))->traverse($fileNodes); + + $indent = $parsedFile['indent']; + if ($indentDetector !== null) { + if (str_contains($indentDetector->indentCharacter, "\t")) { + $indent = "\t"; + } else { + $indent = str_repeat($indentDetector->indentCharacter, $indentDetector->indentSize); + } - $indent = $parsedFile['indent']; - if ($indentDetector !== null) { - if (str_contains($indentDetector->indentCharacter, "\t")) { - $indent = "\t"; - } else { - $indent = str_repeat($indentDetector->indentCharacter, $indentDetector->indentSize); + if ($indentDetector->found) { + $this->parsedFiles[$fixingFile]['indent'] = $indent; + } } - if ($indentDetector->found) { - $this->parsedFiles[$fixingFile]['indent'] = $indent; + if (!$replacingVisitor->isComplete()) { + return null; } - } - if (!$replacingVisitor->isComplete()) { - return null; - } + if (NewClassNamesVisitor::hasNewNames($replacingVisitor->getNewNodes())) { + if ($pathCloningVisitor !== null) { + // the visitors below change nodes in place, so the original nodes must not be shared + /** @var Stmt[] $newStmts */ + $newStmts = (new NodeTraverser(new SharedNodeCloningVisitor()))->traverse($newStmts); + } + $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::hasNewNames($replacingVisitor->getNewNodes())) { - $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) { + return null; + } - $newCode = $this->getPrinter($indent)->printFormatPreserving($newStmts, $fileNodes, $parsedFile['tokens']); - if ($parsedFile['code'] === $newCode) { - return null; + return new FixedErrorDiff($fixingFile, $parsedFile['hash'], $this->diff($parsedFile['code'], $newCode)); + } finally { + $pathCloningVisitor?->restoreOriginalNodes(); } - - return new FixedErrorDiff($fixingFile, $parsedFile['hash'], $this->diff($parsedFile['code'], $newCode)); } /** diff --git a/src/Fixable/PathCloningVisitor.php b/src/Fixable/PathCloningVisitor.php new file mode 100644 index 00000000000..901c4770550 --- /dev/null +++ b/src/Fixable/PathCloningVisitor.php @@ -0,0 +1,141 @@ + */ + private array $targetRanges; + + /** @var array */ + private array $targetKeys = []; + + private int $insideTarget = 0; + + /** @var list */ + private array $enteredTargets = []; + + /** @var list */ + private array $sharedNodes = []; + + /** + * @param non-empty-list $targets + */ + public function __construct(array $targets) + { + $ranges = []; + foreach ($targets as $target) { + $ranges[] = [$target->getStartFilePos(), $target->getEndFilePos()]; + $this->targetKeys[self::getKey($target)] = true; + } + $this->targetRanges = $ranges; + } + + /** + * @param non-empty-list $targets + */ + public static function canFind(array $targets): bool + { + foreach ($targets as $target) { + if ($target->getStartFilePos() < 0 || $target->getEndFilePos() < 0) { + return false; + } + } + + return true; + } + + #[Override] + public function enterNode(Node $node): Node|int + { + if ($this->insideTarget > 0) { + $this->enteredTargets[] = false; + return $this->cloneNode($node); + } + + if (!$this->containsTarget($node)) { + $this->enteredTargets[] = false; + $node->setAttribute('origNode', $node); + $this->sharedNodes[] = $node; + return NodeVisitor::DONT_TRAVERSE_CHILDREN; + } + + $isTarget = isset($this->targetKeys[self::getKey($node)]); + $this->enteredTargets[] = $isTarget; + if ($isTarget) { + $this->insideTarget++; + } + + return $this->cloneNode($node); + } + + #[Override] + public function leaveNode(Node $node): ?Node + { + if (array_pop($this->enteredTargets) === true) { + $this->insideTarget--; + } + + return null; + } + + /** + * Removes the origNode attribute from the original nodes again. + */ + public function restoreOriginalNodes(): void + { + foreach ($this->sharedNodes as $node) { + $attributes = $node->getAttributes(); + unset($attributes['origNode']); + $node->setAttributes($attributes); + } + $this->sharedNodes = []; + } + + private function containsTarget(Node $node): bool + { + $start = $node->getStartFilePos(); + $end = $node->getEndFilePos(); + if ($start < 0 || $end < 0) { + return true; + } + + foreach ($this->targetRanges as [$targetStart, $targetEnd]) { + if ($start <= $targetStart && $targetEnd <= $end) { + return true; + } + } + + return false; + } + + private function cloneNode(Node $node): Node + { + $clone = clone $node; + $clone->setAttribute('origNode', $node); + + return $clone; + } + + private static function getKey(Node $node): string + { + return $node::class . ':' . $node->getStartFilePos() . ':' . $node->getEndFilePos(); + } + +} diff --git a/src/Fixable/PhpPrinter.php b/src/Fixable/PhpPrinter.php index 6a3a9999aaf..7dca6b2d7f2 100644 --- a/src/Fixable/PhpPrinter.php +++ b/src/Fixable/PhpPrinter.php @@ -35,4 +35,34 @@ protected function pCommaSeparated(array $nodes): string return $result; } + /** + * A node that is its own origNode was not cloned, so a fix did not change it + * (see PathCloningVisitor). Its original code is copied as it is, without + * printing its children one by one. + */ + #[Override] + protected function p( + Node $node, + int $precedence = self::MAX_PRECEDENCE, + int $lhsPrecedence = self::MAX_PRECEDENCE, + bool $parentFormatPreserved = false, + ): string + { + if ( + $parentFormatPreserved + && $this->origTokens !== null + && $node->getAttribute('origNode') === $node + ) { + $startPos = $node->getStartTokenPos(); + $endPos = $node->getEndTokenPos(); + if ($startPos >= 0 && $endPos >= 0) { + $indentAdjustment = $this->indentLevel - $this->origTokens->getIndentationBefore($startPos); + + return $this->origTokens->getTokenCode($startPos, $endPos + 1, $indentAdjustment); + } + } + + return parent::p($node, $precedence, $lhsPrecedence, $parentFormatPreserved); + } + } diff --git a/src/Fixable/SharedNodeCloningVisitor.php b/src/Fixable/SharedNodeCloningVisitor.php new file mode 100644 index 00000000000..4c3a929e766 --- /dev/null +++ b/src/Fixable/SharedNodeCloningVisitor.php @@ -0,0 +1,54 @@ + */ + private array $enteredShared = []; + + #[Override] + public function enterNode(Node $node): Node + { + $origNode = $node->getAttribute('origNode'); + $isShared = $origNode === $node; + $this->enteredShared[] = $isShared; + if ($isShared) { + $this->insideShared++; + } + + $clone = clone $node; + if ($this->insideShared > 0) { + $clone->setAttribute('origNode', $node); + } + + return $clone; + } + + #[Override] + public function leaveNode(Node $node): ?Node + { + if (array_pop($this->enteredShared) === true) { + $this->insideShared--; + } + + return null; + } + +} diff --git a/tests/PHPStan/Fixable/RenameFunctionRule.php b/tests/PHPStan/Fixable/RenameFunctionRule.php new file mode 100644 index 00000000000..1cfa0513a3e --- /dev/null +++ b/tests/PHPStan/Fixable/RenameFunctionRule.php @@ -0,0 +1,54 @@ + + */ +final class RenameFunctionRule implements Rule +{ + + public function getNodeType(): string + { + return Function_::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if ($node->getAttribute('origNode') !== null) { + return [ + RuleErrorBuilder::message('Function has an origNode attribute.') + ->identifier('tests.origNode') + ->build(), + ]; + } + + if (str_ends_with($node->name->toString(), 'Renamed')) { + return []; + } + + return [ + RuleErrorBuilder::message('Renaming function.') + ->identifier('tests.renameFunction') + ->fixNode($node, static function (Function_ $function): Function_ { + $function->name = new Identifier($function->name->toString() . 'Renamed'); + + return $function; + }) + ->build(), + ]; + } + +} diff --git a/tests/PHPStan/Fixable/RenameFunctionRuleTest.php b/tests/PHPStan/Fixable/RenameFunctionRuleTest.php new file mode 100644 index 00000000000..27950f2fed4 --- /dev/null +++ b/tests/PHPStan/Fixable/RenameFunctionRuleTest.php @@ -0,0 +1,33 @@ + + */ +class RenameFunctionRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new RenameFunctionRule(); + } + + public function testFixesDoNotChangeTheAnalysedNodes(): void + { + $this->analyse([__DIR__ . '/data/rename-function.php'], [ + ['Renaming function.', 5], + ['Renaming function.', 10], + ['Renaming function.', 15], + ]); + } + + public function testFix(): void + { + $this->fix(__DIR__ . '/data/rename-function.php', __DIR__ . '/data/rename-function.php.fixed'); + } + +} diff --git a/tests/PHPStan/Fixable/data/rename-function.php b/tests/PHPStan/Fixable/data/rename-function.php new file mode 100644 index 00000000000..21127c79f3b --- /dev/null +++ b/tests/PHPStan/Fixable/data/rename-function.php @@ -0,0 +1,18 @@ +