diff --git a/src/Analyser/Error.php b/src/Analyser/Error.php index 9af5f2b297d..1f99bc293d3 100644 --- a/src/Analyser/Error.php +++ b/src/Analyser/Error.php @@ -261,6 +261,27 @@ public function getFixedErrorDiff(): ?FixedErrorDiff return $this->fixedErrorDiff; } + /** + * @internal Experimental + */ + public function withFixedErrorDiff(FixedErrorDiff $fixedErrorDiff): self + { + return new self( + $this->message, + $this->file, + $this->line, + $this->canBeIgnored, + $this->filePath, + $this->traitFilePath, + $this->tip, + $this->nodeLine, + $this->nodeType, + $this->identifier, + $this->metadata, + $fixedErrorDiff, + ); + } + /** * @return mixed */ diff --git a/src/Analyser/FileAnalyserCallback.php b/src/Analyser/FileAnalyserCallback.php index 304513f7f4e..90957ba1332 100644 --- a/src/Analyser/FileAnalyserCallback.php +++ b/src/Analyser/FileAnalyserCallback.php @@ -17,6 +17,7 @@ use PHPStan\Rules\Registry as RuleRegistry; use function array_keys; use function get_class; +use function spl_object_id; use function sprintf; /** @@ -45,6 +46,9 @@ final class FileAnalyserCallback /** @var list */ private array $temporaryFileErrors = []; + /** @var array> */ + private array $pendingFixesByFile = []; + /** @var LinesToIgnore */ private array $linesToIgnore; @@ -77,8 +81,6 @@ public function __construct( public function __invoke(Node $node, Scope $scope): void { - $parserNodes = $this->parserNodes; - /** @var Scope&NodeCallbackInvoker $scope */ if ($node instanceof Node\Stmt\Trait_) { foreach (array_keys($this->linesToIgnore[$this->file] ?? []) as $lineToIgnore) { @@ -101,14 +103,6 @@ public function __invoke(Node $node, Scope $scope): void } } - if ($scope->isInTrait()) { - $traitReflection = $scope->getTraitReflection(); - if ($traitReflection->getFileName() !== null) { - $traitFilePath = $traitReflection->getFileName(); - $parserNodes = $this->parser->parseFile($traitFilePath); - } - } - if ($this->outerNodeCallback !== null) { ($this->outerNodeCallback)($node, $scope); } @@ -149,7 +143,7 @@ public function __invoke(Node $node, Scope $scope): void } foreach ($ruleErrors as $ruleError) { - $error = $this->ruleErrorTransformer->transform($ruleError, $scope, $parserNodes, $node); + [$error, $pendingFix] = $this->ruleErrorTransformer->transformPreserveFixable($ruleError, $scope, $node); if ($error->canBeIgnored()) { foreach ($this->ignoreErrorExtensions as $ignoreErrorExtension) { @@ -160,6 +154,11 @@ public function __invoke(Node $node, Scope $scope): void } $this->temporaryFileErrors[] = $error; + if ($pendingFix === null) { + continue; + } + + $this->pendingFixesByFile[$pendingFix->fixingFilePath][] = $pendingFix; } } @@ -305,7 +304,34 @@ public function getUnmatchedLineIgnores(): array */ public function getTemporaryFileErrors(): array { - return $this->temporaryFileErrors; + if ($this->pendingFixesByFile === []) { + return $this->temporaryFileErrors; + } + + $parserNodesByFile = [$this->file => $this->parserNodes]; + foreach (array_keys($this->pendingFixesByFile) as $fixingFilePath) { + if (isset($parserNodesByFile[$fixingFilePath])) { + continue; + } + $parserNodesByFile[$fixingFilePath] = $this->parser->parseFile($fixingFilePath); + } + + $diffsByErrorId = $this->ruleErrorTransformer->finalizePendingFixes( + $this->pendingFixesByFile, + $parserNodesByFile, + ); + + if ($diffsByErrorId === []) { + return $this->temporaryFileErrors; + } + + $finalErrors = []; + foreach ($this->temporaryFileErrors as $error) { + $diff = $diffsByErrorId[spl_object_id($error)] ?? null; + $finalErrors[] = $diff !== null ? $error->withFixedErrorDiff($diff) : $error; + } + + return $finalErrors; } /** diff --git a/src/Analyser/PendingFix.php b/src/Analyser/PendingFix.php new file mode 100644 index 00000000000..58a574f2656 --- /dev/null +++ b/src/Analyser/PendingFix.php @@ -0,0 +1,26 @@ +differ = new Differ(new UnifiedDiffOutputBuilder('', addLineNumbers: true)); @@ -54,6 +58,28 @@ public function transform( array $fileNodes, Node $node, ): Error + { + [$error, $pendingFix] = $this->transformPreserveFixable($ruleError, $scope, $node); + if ($pendingFix === null) { + return $error; + } + + $diff = $this->buildSinglePendingFixDiff($pendingFix, $fileNodes); + if ($diff === null) { + return $error; + } + + return $error->withFixedErrorDiff($diff); + } + + /** + * @return array{Error, PendingFix|null} + */ + public function transformPreserveFixable( + RuleError $ruleError, + Scope $scope, + Node $node, + ): array { $line = $node->getStartLine(); $canBeIgnored = true; @@ -101,69 +127,149 @@ public function transform( $canBeIgnored = false; } - $fixedErrorDiff = null; + $error = new Error( + $ruleError->getMessage(), + $fileName, + $line, + $canBeIgnored, + $filePath, + $traitFilePath, + $tip, + $node->getStartLine(), + get_class($node), + $identifier, + $metadata, + null, + ); + + $pendingFix = null; if ($ruleError instanceof FixableNodeRuleError) { if ($ruleError->getOriginalNode() instanceof VirtualNode) { throw new ShouldNotHappenException('Cannot fix virtual node'); } - $fixingFile = $filePath; - if ($traitFilePath !== null) { - $fixingFile = $traitFilePath; + $fixingFile = $traitFilePath ?? $filePath; + + $originalNode = $ruleError->getOriginalNode(); + $clone = $this->deepNodeCloner->cloneNode($originalNode); + $newNode = ($ruleError->getNewNodeCallable())($clone); + if ($newNode instanceof VirtualNode) { + throw new ShouldNotHappenException('Cannot print VirtualNode.'); } - $oldCode = FileReader::read($fixingFile); + $pendingFix = new PendingFix( + $error, + $originalNode, + static fn (Node $clonedNode): Node => $newNode, + $fixingFile, + ); + } - $this->parser->parse($oldCode); - $hash = hash('sha256', $oldCode); - $oldTokens = $this->parser->getTokens(); + return [$error, $pendingFix]; + } - $indentTraverser = new NodeTraverser(); - $indentDetector = new PhpPrinterIndentationDetectorVisitor(new TokenStream($oldTokens, PhpPrinter::TAB_WIDTH)); - $indentTraverser->addVisitor($indentDetector); - $indentTraverser->traverse($fileNodes); + /** + * @param array> $pendingFixesByFile + * @param array $parserNodesByFile + * @return array + */ + public function finalizePendingFixes(array $pendingFixesByFile, array $parserNodesByFile): array + { + $diffsByErrorId = []; - $cloningTraverser = new NodeTraverser(); - $cloningTraverser->addVisitor(new CloningVisitor()); + foreach ($pendingFixesByFile as $filePath => $pendingFixes) { + if ($pendingFixes === []) { + continue; + } - /** @var Stmt[] $newStmts */ - $newStmts = $cloningTraverser->traverse($fileNodes); + $fileNodes = $parserNodesByFile[$filePath] ?? null; + if ($fileNodes === null) { + continue; + } - $traverser = new NodeTraverser(); - $visitor = new ReplacingNodeVisitor($ruleError->getOriginalNode(), $ruleError->getNewNodeCallable()); - $traverser->addVisitor($visitor); + $batchFixes = []; + foreach ($pendingFixes as $pendingFix) { + $batchFixes[] = [ + 'node' => $pendingFix->originalNode, + 'callable' => $pendingFix->newNodeCallable, + ]; + } + $replacing = new BatchReplacingNodeVisitor($batchFixes); - /** @var Stmt[] $newStmts */ - $newStmts = $traverser->traverse($newStmts); + $diff = $this->buildDiffFromVisitor($filePath, $fileNodes, $replacing); + if ($diff === null) { + continue; + } - if ($visitor->isFound()) { - if (str_contains($indentDetector->indentCharacter, "\t")) { - $indent = "\t"; - } else { - $indent = str_repeat($indentDetector->indentCharacter, $indentDetector->indentSize); - } - $printer = new PhpPrinter(['indent' => $indent]); - $newCode = $printer->printFormatPreserving($newStmts, $fileNodes, $oldTokens); + $appliedSet = []; + foreach ($replacing->getAppliedOriginalNodes() as $applied) { + $appliedSet[spl_object_id($applied)] = true; + } - if ($oldCode !== $newCode) { - $fixedErrorDiff = new FixedErrorDiff($hash, $this->differ->diff($oldCode, $newCode)); + foreach ($pendingFixes as $pendingFix) { + if (!isset($appliedSet[spl_object_id($pendingFix->originalNode)])) { + continue; } + $diffsByErrorId[spl_object_id($pendingFix->error)] = $diff; } } - return new Error( - $ruleError->getMessage(), - $fileName, - $line, - $canBeIgnored, - $filePath, - $traitFilePath, - $tip, - $node->getStartLine(), - get_class($node), - $identifier, - $metadata, - $fixedErrorDiff, - ); + return $diffsByErrorId; + } + + /** + * @param Node\Stmt[] $fileNodes + */ + private function buildSinglePendingFixDiff(PendingFix $pendingFix, array $fileNodes): ?FixedErrorDiff + { + $visitor = new ReplacingNodeVisitor($pendingFix->originalNode, $pendingFix->newNodeCallable); + $diff = $this->buildDiffFromVisitor($pendingFix->fixingFilePath, $fileNodes, $visitor); + if ($diff === null || !$visitor->isFound()) { + return null; + } + + return $diff; + } + + /** + * @param Node\Stmt[] $fileNodes + */ + private function buildDiffFromVisitor( + string $filePath, + array $fileNodes, + NodeVisitorAbstract $replacingVisitor, + ): ?FixedErrorDiff + { + $oldCode = FileReader::read($filePath); + $this->parser->parse($oldCode); + $hash = hash('sha256', $oldCode); + $oldTokens = $this->parser->getTokens(); + + $indentTraverser = new NodeTraverser(); + $indentDetector = new PhpPrinterIndentationDetectorVisitor(new TokenStream($oldTokens, PhpPrinter::TAB_WIDTH)); + $indentTraverser->addVisitor($indentDetector); + $indentTraverser->traverse($fileNodes); + + $cloningTraverser = new NodeTraverser(); + $cloningTraverser->addVisitor(new CloningVisitor()); + $newStmts = $cloningTraverser->traverse($fileNodes); + + $traverser = new NodeTraverser(); + $traverser->addVisitor($replacingVisitor); + $newStmts = $traverser->traverse($newStmts); + + if (str_contains($indentDetector->indentCharacter, "\t")) { + $indent = "\t"; + } else { + $indent = str_repeat($indentDetector->indentCharacter, $indentDetector->indentSize); + } + $printer = new PhpPrinter(['indent' => $indent]); + $newCode = $printer->printFormatPreserving($newStmts, $fileNodes, $oldTokens); + + if ($oldCode === $newCode) { + return null; + } + + return new FixedErrorDiff($hash, $this->differ->diff($oldCode, $newCode)); } } diff --git a/src/Fixable/BatchReplacingNodeVisitor.php b/src/Fixable/BatchReplacingNodeVisitor.php new file mode 100644 index 00000000000..6de0cd48e9b --- /dev/null +++ b/src/Fixable/BatchReplacingNodeVisitor.php @@ -0,0 +1,70 @@ + */ + private array $fixesByOriginalNodeId = []; + + /** @var array */ + private array $appliedOriginalNodes = []; + + /** + * @param iterable $fixes + */ + public function __construct(iterable $fixes) + { + foreach ($fixes as $fix) { + $id = spl_object_id($fix['node']); + if (isset($this->fixesByOriginalNodeId[$id])) { + continue; + } + $this->fixesByOriginalNodeId[$id] = $fix['callable']; + } + } + + #[Override] + public function enterNode(Node $node): ?Node + { + $origNode = $node->getAttribute('origNode'); + if ($origNode === null) { + return null; + } + $id = spl_object_id($origNode); + if (!isset($this->fixesByOriginalNodeId[$id])) { + return null; + } + + if (isset($this->appliedOriginalNodes[$id])) { + return null; + } + $this->appliedOriginalNodes[$id] = $origNode; + + $callable = $this->fixesByOriginalNodeId[$id]; + $newNode = $callable($node); + if ($newNode instanceof VirtualNode) { + throw new ShouldNotHappenException('Cannot print VirtualNode.'); + } + + return $newNode; + } + + /** + * @return list + */ + public function getAppliedOriginalNodes(): array + { + return array_values($this->appliedOriginalNodes); + } + +} diff --git a/src/Fixable/Patcher.php b/src/Fixable/Patcher.php index 46fdb8fcee4..cde7e786486 100644 --- a/src/Fixable/Patcher.php +++ b/src/Fixable/Patcher.php @@ -43,8 +43,14 @@ public function applyDiffs(string $fileName, array $diffs): string { $fileContents = FileReader::read($fileName); $fileHash = hash('sha256', $fileContents); - $diffHunks = []; + + $uniqueDiffs = []; foreach ($diffs as $diff) { + $uniqueDiffs[$diff->diff] = $diff; + } + + $diffHunks = []; + foreach ($uniqueDiffs as $diff) { if ($diff->originalHash !== $fileHash) { throw new FileChangedException(); } diff --git a/tests/PHPStan/Fixable/BatchReplacingNodeVisitorTest.php b/tests/PHPStan/Fixable/BatchReplacingNodeVisitorTest.php new file mode 100644 index 00000000000..aa5bba65ea6 --- /dev/null +++ b/tests/PHPStan/Fixable/BatchReplacingNodeVisitorTest.php @@ -0,0 +1,162 @@ +parseAndClone($code); + + $finder = new NodeFinder(); + $vars = $finder->findInstanceOf($origStmts, Node\Expr\Variable::class); + self::assertCount(2, $vars); + [$origA, $origB] = $vars; + self::assertSame('a', $origA->name); + self::assertSame('b', $origB->name); + + $visitor = new BatchReplacingNodeVisitor([ + [ + 'node' => $origA, + 'callable' => static fn (Node $n): Node => self::wrap('A', $n), + ], + [ + 'node' => $origB, + 'callable' => static fn (Node $n): Node => self::wrap('B', $n), + ], + ]); + + $result = $this->runVisitor($cloneStmts, $visitor); + + self::assertSame('f(A($a), B($b));', $result); + self::assertCount(2, $visitor->getAppliedOriginalNodes()); + } + + public function testNestedFixesAreAppliedRecursively(): void + { + $code = <<<'PHP' +parseAndClone($code); + + $finder = new NodeFinder(); + $origOuter = $finder->findFirstInstanceOf($origStmts, Node\Expr\FuncCall::class); + self::assertInstanceOf(Node\Expr\FuncCall::class, $origOuter); + $origInner = $finder->findFirstInstanceOf($origStmts, Node\Expr\Variable::class); + self::assertNotNull($origInner); + self::assertSame('inner', $origInner->name); + + $visitor = new BatchReplacingNodeVisitor([ + [ + 'node' => $origOuter, + 'callable' => static function (Node $n): Node { + self::assertInstanceOf(Node\Expr\FuncCall::class, $n); + return new Node\Expr\FuncCall(new Node\Name('wrap'), $n->getArgs()); + }, + ], + [ + 'node' => $origInner, + 'callable' => static fn (Node $n): Node => self::wrap('mod', $n), + ], + ]); + + $result = $this->runVisitor($cloneStmts, $visitor); + + self::assertSame('wrap(mod($inner));', $result); + self::assertCount(2, $visitor->getAppliedOriginalNodes()); + } + + public function testInnerFixIsSilentlySkippedWhenOuterFixDiscardsIt(): void + { + $code = <<<'PHP' +parseAndClone($code); + + $finder = new NodeFinder(); + $origOuter = $finder->findFirstInstanceOf($origStmts, Node\Expr\FuncCall::class); + self::assertInstanceOf(Node\Expr\FuncCall::class, $origOuter); + $origInner = $finder->findFirstInstanceOf($origStmts, Node\Expr\Variable::class); + self::assertInstanceOf(Node\Expr\Variable::class, $origInner); + + $visitor = new BatchReplacingNodeVisitor([ + [ + 'node' => $origOuter, + 'callable' => static fn (Node $n): Node => new Node\Scalar\String_('replaced'), + ], + [ + 'node' => $origInner, + 'callable' => static fn (Node $n): Node => self::wrap('mod', $n), + ], + ]); + + $result = $this->runVisitor($cloneStmts, $visitor); + + self::assertSame("'replaced';", $result); + $applied = $visitor->getAppliedOriginalNodes(); + self::assertCount(1, $applied); + self::assertSame($origOuter, $applied[0]); + } + + /** + * @param non-empty-string $name + */ + private static function wrap(string $name, Node $expr): Node\Expr\FuncCall + { + if (!$expr instanceof Node\Expr) { + throw new LogicException('wrap() expects an expression'); + } + return new Node\Expr\FuncCall(new Node\Name($name), [new Node\Arg($expr)]); + } + + /** + * @return array{0: Node\Stmt[], 1: Node[]} + */ + private function parseAndClone(string $code): array + { + $parser = new Php8(new Emulative(PhpParserPhpVersion::fromComponents(8, 5))); + $origStmts = $parser->parse($code); + self::assertNotNull($origStmts); + + $cloningTraverser = new NodeTraverser(); + $cloningTraverser->addVisitor(new CloningVisitor()); + $cloneStmts = $cloningTraverser->traverse($origStmts); + + return [$origStmts, $cloneStmts]; + } + + /** + * @param Node[] $stmts + */ + private function runVisitor(array $stmts, BatchReplacingNodeVisitor $visitor): string + { + $traverser = new NodeTraverser(); + $traverser->addVisitor($visitor); + $result = $traverser->traverse($stmts); + + $printer = new StandardPrettyPrinter(); + return $printer->prettyPrint($result); + } + +} diff --git a/tests/PHPStan/Fixable/PatcherBenchmarkTest.php b/tests/PHPStan/Fixable/PatcherBenchmarkTest.php new file mode 100644 index 00000000000..d4fbba9d307 --- /dev/null +++ b/tests/PHPStan/Fixable/PatcherBenchmarkTest.php @@ -0,0 +1,142 @@ +runBenchmark(100, 5); + } + + /** + * @throws FileChangedException + * @throws MergeConflictException + */ + public function testTwentyHunksOverFiveHundredLines(): void + { + $this->runBenchmark(500, 20); + } + + /** + * @throws FileChangedException + * @throws MergeConflictException + */ + public function testFiftyHunksOverThousandLines(): void + { + $this->runBenchmark(1000, 50); + } + + /** + * @throws FileChangedException + * @throws MergeConflictException + */ + public function testHundredHunksOverThousandLines(): void + { + $this->runBenchmark(1000, 100); + } + + /** + * @throws FileChangedException + * @throws MergeConflictException + */ + private function runBenchmark(int $fileLines, int $hunkCount): void + { + [$originalContent, $expectedContent, $diffs] = $this->buildSyntheticInput($fileLines, $hunkCount); + + $patcher = self::getContainer()->getByType(Patcher::class); + + $startedAt = microtime(true); + $result = $patcher->applyDiffs($this->writeTempFile($originalContent), $diffs); + $elapsed = microtime(true) - $startedAt; + + fwrite( + STDERR, + sprintf( + "\n[Patcher bench] hunks=%d lines=%d t=%.3fs\n", + $hunkCount, + $fileLines, + $elapsed, + ), + ); + + self::assertSame( + $this->normalizeLineEndings($expectedContent), + $this->normalizeLineEndings($result), + ); + } + + /** + * @return array{0: string, 1: string, 2: list} + */ + private function buildSyntheticInput(int $fileLines, int $hunkCount): array + { + $originalLines = []; + for ($i = 0; $i < $fileLines; $i++) { + $originalLines[] = sprintf("echo %d;\n", $i); + } + $originalContent = "diff($originalContent, $modifiedContent), + ); + + $expectedLines[$lineIndex] = $newLine; + } + + $expectedContent = " + */ +final class PerFileBatchFixerBenchmarkTest extends RuleTestCase +{ + + #[Override] + protected function getRule(): Rule + { + return new RenameVariableFixRule(); + } + + public function testFiveHunksOverHundredLines(): void + { + $this->runBatchedBenchmark(100, 5); + } + + public function testTwentyHunksOverFiveHundredLines(): void + { + $this->runBatchedBenchmark(500, 20); + } + + public function testFiftyHunksOverThousandLines(): void + { + $this->runBatchedBenchmark(1000, 50); + } + + public function testHundredHunksOverThousandLines(): void + { + $this->runBatchedBenchmark(1000, 100); + } + + private function runBatchedBenchmark(int $fileLines, int $hunkCount): void + { + [$file, $expectedFile] = $this->writeSyntheticInput($fileLines, $hunkCount); + + try { + $startedAt = microtime(true); + $this->fix($file, $expectedFile); + $elapsed = microtime(true) - $startedAt; + } finally { + @unlink($file); + @unlink($expectedFile); + } + + fwrite( + STDERR, + sprintf( + "\n[BatchFixer bench] hunks=%d lines=%d t=%.3fs\n", + $hunkCount, + $fileLines, + $elapsed, + ), + ); + } + + /** + * @return array{0: string, 1: string} + */ + private function writeSyntheticInput(int $fileLines, int $hunkCount): array + { + $lines = [" + */ +final class PerFileBatchFixerTest extends RuleTestCase +{ + + #[Override] + protected function getRule(): Rule + { + return new RenameVariableFixRule(); + } + + public function testMultipleSameLineFixesAllApply(): void + { + $this->fix(__DIR__ . '/data/same-line.php', __DIR__ . '/data/same-line.php.fixed'); + } + +} diff --git a/tests/PHPStan/Fixable/RenameVariableFixRule.php b/tests/PHPStan/Fixable/RenameVariableFixRule.php new file mode 100644 index 00000000000..2f342b04182 --- /dev/null +++ b/tests/PHPStan/Fixable/RenameVariableFixRule.php @@ -0,0 +1,47 @@ + + */ +final class RenameVariableFixRule implements Rule +{ + + #[Override] + public function getNodeType(): string + { + return Variable::class; + } + + #[Override] + public function processNode(Node $node, Scope $scope): array + { + if (!is_string($node->name)) { + return []; + } + + $last = $node->name[-1]; + $newName = $node->name . $last; + + return [ + RuleErrorBuilder::message(sprintf('Renaming $%s to $%s.', $node->name, $newName)) + ->identifier('tests.batchFixerBench.renameVar') + ->fixNode($node, static function (Variable $v) use ($newName): Variable { + $v->name = $newName; + return $v; + }) + ->build(), + ]; + } + +} diff --git a/tests/PHPStan/Fixable/data/same-line.php b/tests/PHPStan/Fixable/data/same-line.php new file mode 100644 index 00000000000..a0f5a4079a0 --- /dev/null +++ b/tests/PHPStan/Fixable/data/same-line.php @@ -0,0 +1,6 @@ +