From fcb40b6fddf863030941172d2fcb3d5f3840ae66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Barto=C5=A1?= Date: Sun, 26 Apr 2026 23:39:35 +0200 Subject: [PATCH 1/5] Per-file batch fixing for --fix --- src/Analyser/Error.php | 21 ++ src/Analyser/FileAnalyserCallback.php | 50 +++-- src/Analyser/PendingFix.php | 26 +++ src/Analyser/RuleErrorTransformer.php | 198 ++++++++++++++---- src/Fixable/BatchReplacingNodeVisitor.php | 70 +++++++ src/Fixable/Patcher.php | 8 +- .../Fixable/BatchReplacingNodeVisitorTest.php | 162 ++++++++++++++ .../PHPStan/Fixable/PatcherBenchmarkTest.php | 142 +++++++++++++ .../PerFileBatchFixerBenchmarkTest.php | 102 +++++++++ .../PHPStan/Fixable/PerFileBatchFixerTest.php | 26 +++ .../PHPStan/Fixable/RenameVariableFixRule.php | 47 +++++ tests/PHPStan/Fixable/data/same-line.php | 6 + .../PHPStan/Fixable/data/same-line.php.fixed | 6 + 13 files changed, 805 insertions(+), 59 deletions(-) create mode 100644 src/Analyser/PendingFix.php create mode 100644 src/Fixable/BatchReplacingNodeVisitor.php create mode 100644 tests/PHPStan/Fixable/BatchReplacingNodeVisitorTest.php create mode 100644 tests/PHPStan/Fixable/PatcherBenchmarkTest.php create mode 100644 tests/PHPStan/Fixable/PerFileBatchFixerBenchmarkTest.php create mode 100644 tests/PHPStan/Fixable/PerFileBatchFixerTest.php create mode 100644 tests/PHPStan/Fixable/RenameVariableFixRule.php create mode 100644 tests/PHPStan/Fixable/data/same-line.php create mode 100644 tests/PHPStan/Fixable/data/same-line.php.fixed 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 @@ + Date: Sat, 2 May 2026 22:14:34 +0200 Subject: [PATCH 2/5] Fix traits --- src/Analyser/Error.php | 40 ++++++ src/Analyser/FileAnalyserCallback.php | 16 ++- src/Analyser/FinalizedPendingFixes.php | 22 +++ src/Analyser/PendingFix.php | 2 + src/Analyser/RuleErrorTransformer.php | 91 +++++++++++-- src/Command/AnalyseCommand.php | 50 +++++-- src/Fixable/BatchReplacingNodeVisitor.php | 50 ++++++- src/Fixable/PhpPrinter.php | 6 + .../ErrorFormatter/JsonErrorFormatterTest.php | 28 ++++ .../TableErrorFormatterTest.php | 49 +++++++ .../Fixable/BatchReplacingNodeVisitorTest.php | 94 +++++++++++++ .../ClassAwareRenameVariableFixRule.php | 50 +++++++ .../Fixable/PerFileBatchFixerTraitTest.php | 128 ++++++++++++++++++ .../Fixable/data/trait-agreeing-consumers.php | 15 ++ .../data/trait-agreeing-consumers.php.fixed | 15 ++ .../data/trait-disagreeing-consumers.php | 15 ++ .../trait-disagreeing-consumers.php.fixed | 15 ++ .../Fixable/data/trait-single-consumer.php | 11 ++ .../data/trait-single-consumer.php.fixed | 11 ++ .../Fixable/data/trait-without-consumer.php | 7 + .../data/trait-without-consumer.php.fixed | 7 + 21 files changed, 694 insertions(+), 28 deletions(-) create mode 100644 src/Analyser/FinalizedPendingFixes.php create mode 100644 tests/PHPStan/Fixable/ClassAwareRenameVariableFixRule.php create mode 100644 tests/PHPStan/Fixable/PerFileBatchFixerTraitTest.php create mode 100644 tests/PHPStan/Fixable/data/trait-agreeing-consumers.php create mode 100644 tests/PHPStan/Fixable/data/trait-agreeing-consumers.php.fixed create mode 100644 tests/PHPStan/Fixable/data/trait-disagreeing-consumers.php create mode 100644 tests/PHPStan/Fixable/data/trait-disagreeing-consumers.php.fixed create mode 100644 tests/PHPStan/Fixable/data/trait-single-consumer.php create mode 100644 tests/PHPStan/Fixable/data/trait-single-consumer.php.fixed create mode 100644 tests/PHPStan/Fixable/data/trait-without-consumer.php create mode 100644 tests/PHPStan/Fixable/data/trait-without-consumer.php.fixed diff --git a/src/Analyser/Error.php b/src/Analyser/Error.php index 1f99bc293d3..cd82485679f 100644 --- a/src/Analyser/Error.php +++ b/src/Analyser/Error.php @@ -40,6 +40,7 @@ public function __construct( private ?string $identifier = null, private array $metadata = [], private ?FixedErrorDiff $fixedErrorDiff = null, + private bool $wasFixable = false, ) { if ($this->identifier !== null && !self::validateIdentifier($this->identifier)) { @@ -85,6 +86,7 @@ public function changeFilePath(string $newFilePath): self $this->identifier, $this->metadata, $this->fixedErrorDiff, + $this->wasFixable, ); } @@ -103,6 +105,7 @@ public function changeTraitFilePath(string $newFilePath): self $this->identifier, $this->metadata, $this->fixedErrorDiff, + $this->wasFixable, ); } @@ -150,6 +153,7 @@ public function withoutTip(): self $this->identifier, $this->metadata, $this->fixedErrorDiff, + $this->wasFixable, ); } @@ -172,6 +176,7 @@ public function doNotIgnore(): self $this->identifier, $this->metadata, $this->fixedErrorDiff, + $this->wasFixable, ); } @@ -194,6 +199,7 @@ public function withIdentifier(string $identifier): self $identifier, $this->metadata, $this->fixedErrorDiff, + $this->wasFixable, ); } @@ -279,6 +285,37 @@ public function withFixedErrorDiff(FixedErrorDiff $fixedErrorDiff): self $this->identifier, $this->metadata, $fixedErrorDiff, + $this->wasFixable, + ); + } + + /** + * @internal Experimental + */ + public function wasFixable(): bool + { + return $this->wasFixable; + } + + /** + * @internal Experimental + */ + public function withAppendedTip(string $extraTip): self + { + return new self( + $this->message, + $this->file, + $this->line, + $this->canBeIgnored, + $this->filePath, + $this->traitFilePath, + $this->tip === null ? $extraTip : $this->tip . "\n" . $extraTip, + $this->nodeLine, + $this->nodeType, + $this->identifier, + $this->metadata, + $this->fixedErrorDiff, + $this->wasFixable, ); } @@ -310,6 +347,7 @@ public function jsonSerialize() 'metadata' => $this->metadata, 'fixedErrorDiffHash' => $fixedErrorDiffHash, 'fixedErrorDiffDiff' => $fixedErrorDiffDiff, + 'wasFixable' => $this->wasFixable, ]; } @@ -336,6 +374,7 @@ public static function decode(array $json): self $json['identifier'] ?? null, $json['metadata'] ?? [], $fixedErrorDiff, + $json['wasFixable'] ?? false, ); } @@ -357,6 +396,7 @@ public static function __set_state(array $properties): self $properties['identifier'] ?? null, $properties['metadata'] ?? [], $properties['fixedErrorDiff'] ?? null, + $properties['wasFixable'] ?? false, ); } diff --git a/src/Analyser/FileAnalyserCallback.php b/src/Analyser/FileAnalyserCallback.php index 90957ba1332..6240db25a3f 100644 --- a/src/Analyser/FileAnalyserCallback.php +++ b/src/Analyser/FileAnalyserCallback.php @@ -316,19 +316,27 @@ public function getTemporaryFileErrors(): array $parserNodesByFile[$fixingFilePath] = $this->parser->parseFile($fixingFilePath); } - $diffsByErrorId = $this->ruleErrorTransformer->finalizePendingFixes( + $result = $this->ruleErrorTransformer->finalizePendingFixes( $this->pendingFixesByFile, $parserNodesByFile, ); - if ($diffsByErrorId === []) { + if ($result->diffsByErrorId === [] && $result->skipReasonByErrorId === []) { return $this->temporaryFileErrors; } $finalErrors = []; foreach ($this->temporaryFileErrors as $error) { - $diff = $diffsByErrorId[spl_object_id($error)] ?? null; - $finalErrors[] = $diff !== null ? $error->withFixedErrorDiff($diff) : $error; + $id = spl_object_id($error); + $diff = $result->diffsByErrorId[$id] ?? null; + if ($diff !== null) { + $error = $error->withFixedErrorDiff($diff); + } + $skipReason = $result->skipReasonByErrorId[$id] ?? null; + if ($skipReason !== null) { + $error = $error->withAppendedTip($skipReason); + } + $finalErrors[] = $error; } return $finalErrors; diff --git a/src/Analyser/FinalizedPendingFixes.php b/src/Analyser/FinalizedPendingFixes.php new file mode 100644 index 00000000000..230fb397bbd --- /dev/null +++ b/src/Analyser/FinalizedPendingFixes.php @@ -0,0 +1,22 @@ + $diffsByErrorId + * @param array $skipReasonByErrorId + */ + public function __construct( + public readonly array $diffsByErrorId, + public readonly array $skipReasonByErrorId, + ) + { + } + +} diff --git a/src/Analyser/PendingFix.php b/src/Analyser/PendingFix.php index 58a574f2656..0be38d9bfac 100644 --- a/src/Analyser/PendingFix.php +++ b/src/Analyser/PendingFix.php @@ -13,12 +13,14 @@ final class PendingFix /** * @param Closure(Node): Node $newNodeCallable + * @param class-string|null $consumerClass */ public function __construct( public readonly Error $error, public readonly Node $originalNode, public readonly Closure $newNodeCallable, public readonly string $fixingFilePath, + public readonly ?string $consumerClass = null, ) { } diff --git a/src/Analyser/RuleErrorTransformer.php b/src/Analyser/RuleErrorTransformer.php index 90dcc06c41f..682b70e7f5b 100644 --- a/src/Analyser/RuleErrorTransformer.php +++ b/src/Analyser/RuleErrorTransformer.php @@ -28,8 +28,11 @@ use PHPStan\ShouldNotHappenException; use SebastianBergmann\Diff\Differ; use SebastianBergmann\Diff\Output\UnifiedDiffOutputBuilder; +use function array_slice; +use function count; use function get_class; use function hash; +use function implode; use function spl_object_id; use function str_contains; use function str_repeat; @@ -127,6 +130,7 @@ public function transformPreserveFixable( $canBeIgnored = false; } + $wasFixable = $ruleError instanceof FixableNodeRuleError; $error = new Error( $ruleError->getMessage(), $fileName, @@ -139,14 +143,25 @@ public function transformPreserveFixable( get_class($node), $identifier, $metadata, - null, + wasFixable: $wasFixable, ); $pendingFix = null; - if ($ruleError instanceof FixableNodeRuleError) { + if ($wasFixable) { if ($ruleError->getOriginalNode() instanceof VirtualNode) { throw new ShouldNotHappenException('Cannot fix virtual node'); } + + if ($scope->isInTrait() && !$scope->isInClass()) { + $error = $error->withAppendedTip( + 'Auto-fix skipped: this trait has no consumer in the analysed code, ' + . 'so the fix cannot be validated against the contexts it would run in. ' + . 'Resolve manually.', + ); + + return [$error, null]; + } + $fixingFile = $traitFilePath ?? $filePath; $originalNode = $ruleError->getOriginalNode(); @@ -156,11 +171,17 @@ public function transformPreserveFixable( throw new ShouldNotHappenException('Cannot print VirtualNode.'); } + $consumerClass = null; + if ($scope->isInTrait() && $scope->isInClass()) { + $consumerClass = $scope->getClassReflection()->getName(); + } + $pendingFix = new PendingFix( $error, $originalNode, static fn (Node $clonedNode): Node => $newNode, $fixingFile, + $consumerClass, ); } @@ -170,11 +191,11 @@ public function transformPreserveFixable( /** * @param array> $pendingFixesByFile * @param array $parserNodesByFile - * @return array */ - public function finalizePendingFixes(array $pendingFixesByFile, array $parserNodesByFile): array + public function finalizePendingFixes(array $pendingFixesByFile, array $parserNodesByFile): FinalizedPendingFixes { $diffsByErrorId = []; + $skipReasonByErrorId = []; foreach ($pendingFixesByFile as $filePath => $pendingFixes) { if ($pendingFixes === []) { @@ -191,29 +212,79 @@ public function finalizePendingFixes(array $pendingFixesByFile, array $parserNod $batchFixes[] = [ 'node' => $pendingFix->originalNode, 'callable' => $pendingFix->newNodeCallable, + 'consumerClass' => $pendingFix->consumerClass, ]; } $replacing = new BatchReplacingNodeVisitor($batchFixes); $diff = $this->buildDiffFromVisitor($filePath, $fileNodes, $replacing); - if ($diff === null) { - continue; - } $appliedSet = []; foreach ($replacing->getAppliedOriginalNodes() as $applied) { $appliedSet[spl_object_id($applied)] = true; } + $conflictClusters = $replacing->getConflictClustersByOriginalNodeId(); + foreach ($pendingFixes as $pendingFix) { - if (!isset($appliedSet[spl_object_id($pendingFix->originalNode)])) { + $nodeId = spl_object_id($pendingFix->originalNode); + if (isset($appliedSet[$nodeId])) { + if ($diff !== null) { + $diffsByErrorId[spl_object_id($pendingFix->error)] = $diff; + } continue; } - $diffsByErrorId[spl_object_id($pendingFix->error)] = $diff; + + if (isset($conflictClusters[$nodeId])) { + $skipReasonByErrorId[spl_object_id($pendingFix->error)] = + self::formatConflictReport($conflictClusters[$nodeId]); + } } } - return $diffsByErrorId; + return new FinalizedPendingFixes($diffsByErrorId, $skipReasonByErrorId); + } + + /** + * @param array> $clusters + */ + private static function formatConflictReport(array $clusters): string + { + $groups = []; + foreach ($clusters as $consumers) { + $names = []; + foreach ($consumers as $consumer) { + $names[] = $consumer ?? ''; + } + $groups[] = self::joinClassList($names); + } + + $differs = $groups[0]; + for ($i = 1; $i < count($groups); $i++) { + $differs .= ' differs from fix in ' . $groups[$i]; + } + + return 'Auto-fix skipped: trait consumers proposed conflicting rewrites. Fix in context of ' + . $differs . '.'; + } + + /** + * @param list $names + */ + private static function joinClassList(array $names): string + { + $count = count($names); + $noun = $count === 1 ? 'class' : 'classes'; + if ($count <= 1) { + return $noun . ' ' . ($names[0] ?? ''); + } + if ($count === 2) { + return $noun . ' ' . $names[0] . ' and ' . $names[1]; + } + $last = $names[$count - 1]; + $head = array_slice($names, 0, $count - 1); + + return $noun . ' ' . implode(', ', $head) . ', and ' . $last; } /** diff --git a/src/Command/AnalyseCommand.php b/src/Command/AnalyseCommand.php index f7865ad7508..d5a89410edc 100644 --- a/src/Command/AnalyseCommand.php +++ b/src/Command/AnalyseCommand.php @@ -514,16 +514,20 @@ protected function execute(InputInterface $input, OutputInterface $output): int if ($fix) { $fixableErrors = []; + $preSkippedErrors = []; foreach ($analysisResult->getFileSpecificErrors() as $fileSpecificError) { - if ($fileSpecificError->getFixedErrorDiff() === null) { + if ($fileSpecificError->getFixedErrorDiff() !== null) { + $fixableErrors[] = $fileSpecificError; continue; } - - $fixableErrors[] = $fileSpecificError; + if ($fileSpecificError->wasFixable()) { + $preSkippedErrors[] = $fileSpecificError; + } } $fixableErrorsCount = count($fixableErrors); - if ($fixableErrorsCount === 0) { + $preSkippedCount = count($preSkippedErrors); + if ($fixableErrorsCount === 0 && $preSkippedCount === 0) { $inceptionResult->getStdOutput()->getStyle()->error('No fixable errors found'); $exitCode = 1; } else { @@ -542,8 +546,10 @@ protected function execute(InputInterface $input, OutputInterface $output): int $diffsByFile[$fixFile][] = $fixableError->getFixedErrorDiff(); } - $inceptionResult->getErrorOutput()->writeLineFormatted('Fixing errors...'); - $errorOutput->getStyle()->progressStart($fixableErrorsCount); + if ($fixableErrorsCount > 0) { + $inceptionResult->getErrorOutput()->writeLineFormatted('Fixing errors...'); + $errorOutput->getStyle()->progressStart($fixableErrorsCount); + } $patcher = $container->getByType(Patcher::class); foreach ($diffsByFile as $file => $diffs) { @@ -560,24 +566,46 @@ protected function execute(InputInterface $input, OutputInterface $output): int FileWriter::write($file, $finalFileContents); } - $errorOutput->getStyle()->progressFinish(); + if ($fixableErrorsCount > 0) { + $errorOutput->getStyle()->progressFinish(); + } + + if ($preSkippedCount > 0) { + $skippedAnalysisResult = new AnalysisResult( + $preSkippedErrors, + [], + [], + [], + [], + $analysisResult->isDefaultLevelUsed(), + $analysisResult->getProjectConfigFile(), + false, + $analysisResult->getPeakMemoryUsageBytes(), + false, + [], + [], + ); + $errorFormatter->formatErrors($skippedAnalysisResult, $inceptionResult->getStdOutput()); + } - if ($skippedCount > 0) { + $totalSkipped = $skippedCount + $preSkippedCount; + if ($totalSkipped > 0) { $inceptionResult->getStdOutput()->getStyle()->warning(sprintf( '%d %s fixed, %d %s skipped', $fixableErrorsCount, $fixableErrorsCount === 1 ? 'error' : 'errors', - $skippedCount, - $skippedCount === 1 ? 'error' : 'errors', + $totalSkipped, + $totalSkipped === 1 ? 'error' : 'errors', )); + $exitCode = 1; } else { $inceptionResult->getStdOutput()->getStyle()->success(sprintf( '%d %s fixed', $fixableErrorsCount, $fixableErrorsCount === 1 ? 'error' : 'errors', )); + $exitCode = 0; } - $exitCode = 0; } } else { if (AgentDetector::isRunningInAgent() && count($analysisResult->getFileSpecificErrors()) > 0) { diff --git a/src/Fixable/BatchReplacingNodeVisitor.php b/src/Fixable/BatchReplacingNodeVisitor.php index 6de0cd48e9b..a4d47a466dd 100644 --- a/src/Fixable/BatchReplacingNodeVisitor.php +++ b/src/Fixable/BatchReplacingNodeVisitor.php @@ -8,6 +8,7 @@ use PHPStan\Node\VirtualNode; use PHPStan\ShouldNotHappenException; use function array_values; +use function count; use function spl_object_id; final class BatchReplacingNodeVisitor extends NodeVisitorAbstract @@ -16,20 +17,36 @@ final class BatchReplacingNodeVisitor extends NodeVisitorAbstract /** @var array */ private array $fixesByOriginalNodeId = []; + /** @var array>> */ + private array $clustersByOriginalNodeId = []; + /** @var array */ private array $appliedOriginalNodes = []; /** - * @param iterable $fixes + * @param iterable $fixes */ public function __construct(iterable $fixes) { + $printer = new PhpPrinter(); foreach ($fixes as $fix) { $id = spl_object_id($fix['node']); - if (isset($this->fixesByOriginalNodeId[$id])) { + $consumerClass = $fix['consumerClass'] ?? null; + + $newNode = ($fix['callable'])($fix['node']); + if ($newNode instanceof VirtualNode) { + throw new ShouldNotHappenException('Cannot print VirtualNode.'); + } + $printed = self::printNode($printer, $newNode); + + $this->clustersByOriginalNodeId[$id][$printed][] = $consumerClass; + + if (count($this->clustersByOriginalNodeId[$id]) === 1) { + $this->fixesByOriginalNodeId[$id] = $fix['callable']; continue; } - $this->fixesByOriginalNodeId[$id] = $fix['callable']; + + unset($this->fixesByOriginalNodeId[$id]); } } @@ -67,4 +84,31 @@ public function getAppliedOriginalNodes(): array return array_values($this->appliedOriginalNodes); } + /** + * @return array>> + */ + public function getConflictClustersByOriginalNodeId(): array + { + $conflicts = []; + foreach ($this->clustersByOriginalNodeId as $id => $clusters) { + if (count($clusters) < 2) { + continue; + } + $conflicts[$id] = $clusters; + } + + return $conflicts; + } + + private static function printNode(PhpPrinter $printer, Node $node): string + { + if ($node instanceof Node\Stmt) { + return $printer->prettyPrint([$node]); + } + if ($node instanceof Node\Expr) { + return $printer->prettyPrintExpr($node); + } + return $printer->printSingleNode($node); + } + } diff --git a/src/Fixable/PhpPrinter.php b/src/Fixable/PhpPrinter.php index 6a3a9999aaf..d3f03d39aff 100644 --- a/src/Fixable/PhpPrinter.php +++ b/src/Fixable/PhpPrinter.php @@ -15,6 +15,12 @@ final class PhpPrinter extends Standard public const TAB_WIDTH = 4; public const FUNC_ARGS_TRAILING_COMMA_ATTRIBUTE = 'trailing_comma'; + public function printSingleNode(Node $node): string + { + $this->resetState(); + return $this->p($node); + } + /** * @param Node[] $nodes */ diff --git a/tests/PHPStan/Command/ErrorFormatter/JsonErrorFormatterTest.php b/tests/PHPStan/Command/ErrorFormatter/JsonErrorFormatterTest.php index 19d05899f28..62764ed5a7a 100644 --- a/tests/PHPStan/Command/ErrorFormatter/JsonErrorFormatterTest.php +++ b/tests/PHPStan/Command/ErrorFormatter/JsonErrorFormatterTest.php @@ -252,4 +252,32 @@ public function testFormatTip(string $tip, string $expectedTip): void $this->assertSame($expectedTip, $json['files']['/foo/bar.php']['messages'][0]['tip']); } + public function testFormatSkippedFixErrorRendersTipAndIdentifier(): void + { + $skipReason = 'Auto-fix skipped: trait consumers proposed conflicting rewrites. ' + . 'Fix in context of class A differs from fix in class B.'; + $error = new Error( + 'is_string($x) is equivalent to !== null here. Use the latter.', + '/path/to/Trait.php', + 4, + tip: $skipReason, + identifier: 'app.type.forbidUselessIsTypeFunction', + wasFixable: true, + ); + + $formatter = new JsonErrorFormatter(false); + $formatter->formatErrors( + new AnalysisResult([$error], [], [], [], [], false, null, true, 0, false, []), + $this->getOutput(), + ); + + $json = Json::decode($this->getOutputContent(), Json::FORCE_ARRAY); + $message = $json['files']['/path/to/Trait.php']['messages'][0]; + + self::assertSame('is_string($x) is equivalent to !== null here. Use the latter.', $message['message']); + self::assertSame(4, $message['line']); + self::assertSame('app.type.forbidUselessIsTypeFunction', $message['identifier']); + self::assertSame($skipReason, $message['tip']); + } + } diff --git a/tests/PHPStan/Command/ErrorFormatter/TableErrorFormatterTest.php b/tests/PHPStan/Command/ErrorFormatter/TableErrorFormatterTest.php index 2ec0679fabd..e800e1e000e 100644 --- a/tests/PHPStan/Command/ErrorFormatter/TableErrorFormatterTest.php +++ b/tests/PHPStan/Command/ErrorFormatter/TableErrorFormatterTest.php @@ -613,6 +613,55 @@ public function testBug13317(): void [ERROR] Found 1 error +TABLE, + $this->getOutputContent(), + ); + } + + public function testFormatSkippedFixErrorRendersIdentifierAndTip(): void + { + putenv('COLUMNS=170'); + $formatter = $this->createErrorFormatter(null); + $formatter->formatErrors( + new AnalysisResult( + [ + new Error( + 'is_string($x) is equivalent to !== null here. Use the latter.', + 'Trait.php', + 4, + tip: 'Auto-fix skipped: trait consumers proposed conflicting rewrites. ' + . 'Fix in context of class A differs from fix in class B.', + identifier: 'app.type.forbidUselessIsTypeFunction', + wasFixable: true, + ), + ], + [], + [], + [], + [], + false, + null, + true, + 0, + false, + [], + ), + $this->getOutput(), + ); + $this->assertSame( + <<<'TABLE' + ------ ----------------------------------------------------------------------------------------------------------------------------- + Line Trait.php + ------ ----------------------------------------------------------------------------------------------------------------------------- + 4 is_string($x) is equivalent to !== null here. Use the latter. + 🪪 app.type.forbidUselessIsTypeFunction + 💡 Auto-fix skipped: trait consumers proposed conflicting rewrites. Fix in context of class A differs from fix in class B. + ------ ----------------------------------------------------------------------------------------------------------------------------- + + + [ERROR] Found 1 error + + TABLE, $this->getOutputContent(), ); diff --git a/tests/PHPStan/Fixable/BatchReplacingNodeVisitorTest.php b/tests/PHPStan/Fixable/BatchReplacingNodeVisitorTest.php index aa5bba65ea6..4923753400f 100644 --- a/tests/PHPStan/Fixable/BatchReplacingNodeVisitorTest.php +++ b/tests/PHPStan/Fixable/BatchReplacingNodeVisitorTest.php @@ -119,6 +119,100 @@ public function testInnerFixIsSilentlySkippedWhenOuterFixDiscardsIt(): void self::assertSame($origOuter, $applied[0]); } + public function testIdempotentDuplicateFixesOnSameNodeApplyOnce(): void + { + $code = <<<'PHP' +parseAndClone($code); + + $finder = new NodeFinder(); + $origVar = $finder->findFirstInstanceOf($origStmts, Node\Expr\Variable::class); + self::assertInstanceOf(Node\Expr\Variable::class, $origVar); + + $visitor = new BatchReplacingNodeVisitor([ + [ + 'node' => $origVar, + 'callable' => static fn (Node $n): Node => self::wrap('safe', $n), + ], + [ + 'node' => $origVar, + 'callable' => static fn (Node $n): Node => self::wrap('safe', $n), + ], + ]); + + $result = $this->runVisitor($cloneStmts, $visitor); + + self::assertSame('safe($a);', $result); + self::assertCount(1, $visitor->getAppliedOriginalNodes()); + } + + public function testConflictingFixesOnSameNodeAreSkipped(): void + { + $code = <<<'PHP' +parseAndClone($code); + + $finder = new NodeFinder(); + $origVar = $finder->findFirstInstanceOf($origStmts, Node\Expr\Variable::class); + self::assertInstanceOf(Node\Expr\Variable::class, $origVar); + + $visitor = new BatchReplacingNodeVisitor([ + [ + 'node' => $origVar, + 'callable' => static fn (Node $n): Node => self::wrap('A', $n), + ], + [ + 'node' => $origVar, + 'callable' => static fn (Node $n): Node => self::wrap('B', $n), + ], + ]); + + $result = $this->runVisitor($cloneStmts, $visitor); + + self::assertSame('$a;', $result); + self::assertSame([], $visitor->getAppliedOriginalNodes()); + } + + public function testConflictedNodeStaysSkippedEvenWhenLaterFixAgrees(): void + { + $code = <<<'PHP' +parseAndClone($code); + + $finder = new NodeFinder(); + $origVar = $finder->findFirstInstanceOf($origStmts, Node\Expr\Variable::class); + self::assertInstanceOf(Node\Expr\Variable::class, $origVar); + + $visitor = new BatchReplacingNodeVisitor([ + [ + 'node' => $origVar, + 'callable' => static fn (Node $n): Node => self::wrap('A', $n), + ], + [ + 'node' => $origVar, + 'callable' => static fn (Node $n): Node => self::wrap('B', $n), + ], + [ + 'node' => $origVar, + 'callable' => static fn (Node $n): Node => self::wrap('A', $n), + ], + ]); + + $result = $this->runVisitor($cloneStmts, $visitor); + + self::assertSame('$a;', $result); + self::assertSame([], $visitor->getAppliedOriginalNodes()); + } + /** * @param non-empty-string $name */ diff --git a/tests/PHPStan/Fixable/ClassAwareRenameVariableFixRule.php b/tests/PHPStan/Fixable/ClassAwareRenameVariableFixRule.php new file mode 100644 index 00000000000..83badfd3b2d --- /dev/null +++ b/tests/PHPStan/Fixable/ClassAwareRenameVariableFixRule.php @@ -0,0 +1,50 @@ + + */ +final class ClassAwareRenameVariableFixRule 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 []; + } + if (!$scope->isInClass()) { + return []; + } + + $shortName = $scope->getClassReflection()->getNativeReflection()->getShortName(); + $newName = $node->name . $shortName; + + return [ + RuleErrorBuilder::message(sprintf('Renaming $%s to $%s.', $node->name, $newName)) + ->identifier('tests.batchFixerBench.classAwareRenameVar') + ->fixNode($node, static function (Variable $v) use ($newName): Variable { + $v->name = $newName; + return $v; + }) + ->build(), + ]; + } + +} diff --git a/tests/PHPStan/Fixable/PerFileBatchFixerTraitTest.php b/tests/PHPStan/Fixable/PerFileBatchFixerTraitTest.php new file mode 100644 index 00000000000..daa3f60b9d7 --- /dev/null +++ b/tests/PHPStan/Fixable/PerFileBatchFixerTraitTest.php @@ -0,0 +1,128 @@ +> + */ +final class PerFileBatchFixerTraitTest extends RuleTestCase +{ + + /** @var Rule<\PhpParser\Node\Expr\Variable> */ + private Rule $rule; + + #[Override] + protected function getRule(): Rule + { + return $this->rule ?? new RenameVariableFixRule(); + } + + public function testSingleConsumerTraitFixIsApplied(): void + { + $this->rule = new RenameVariableFixRule(); + + $this->fix( + __DIR__ . '/data/trait-single-consumer.php', + __DIR__ . '/data/trait-single-consumer.php.fixed', + ); + } + + public function testAgreeingMultipleConsumersTraitFixIsApplied(): void + { + $this->rule = new RenameVariableFixRule(); + + $this->fix( + __DIR__ . '/data/trait-agreeing-consumers.php', + __DIR__ . '/data/trait-agreeing-consumers.php.fixed', + ); + } + + public function testDisagreeingMultipleConsumersTraitFixIsSkipped(): void + { + $this->rule = new ClassAwareRenameVariableFixRule(); + + $this->fix( + __DIR__ . '/data/trait-disagreeing-consumers.php', + __DIR__ . '/data/trait-disagreeing-consumers.php.fixed', + ); + } + + public function testTraitWithoutConsumerIsLeftAlone(): void + { + $this->rule = new RenameVariableFixRule(); + + $this->fix( + __DIR__ . '/data/trait-without-consumer.php', + __DIR__ . '/data/trait-without-consumer.php.fixed', + ); + } + + public function testDisagreeingConsumersErrorIsMarkedFixableWithConflictTip(): void + { + $this->rule = new ClassAwareRenameVariableFixRule(); + + $errors = $this->gatherAnalyserErrors([__DIR__ . '/data/trait-disagreeing-consumers.php']); + $skipped = self::filterFixableSkipped($errors); + + self::assertNotSame([], $skipped, 'expected at least one wasFixable && !applied error'); + foreach ($skipped as $error) { + self::assertTrue($error->wasFixable()); + self::assertNull($error->getFixedErrorDiff()); + $tip = $error->getTip(); + self::assertNotNull($tip); + self::assertStringContainsString('Auto-fix skipped: trait consumers proposed conflicting rewrites.', $tip); + self::assertStringContainsString('TraitDisagreeingConsumerOne', $tip); + self::assertStringContainsString('TraitDisagreeingConsumerTwo', $tip); + } + } + + public function testSingleConsumerErrorIsAppliedAndHasNoSkipTip(): void + { + $this->rule = new RenameVariableFixRule(); + + $errors = $this->gatherAnalyserErrors([__DIR__ . '/data/trait-single-consumer.php']); + + $applied = []; + foreach ($errors as $error) { + if ($error->getFixedErrorDiff() === null) { + continue; + } + $applied[] = $error; + } + + self::assertNotSame([], $applied, 'expected at least one applied fix on a single-consumer trait'); + foreach ($applied as $error) { + self::assertTrue($error->wasFixable()); + $tip = $error->getTip(); + if ($tip === null) { + continue; + } + self::assertStringNotContainsString('Auto-fix skipped', $tip); + } + } + + /** + * @param list<\PHPStan\Analyser\Error> $errors + * @return list<\PHPStan\Analyser\Error> + */ + private static function filterFixableSkipped(array $errors): array + { + $skipped = []; + foreach ($errors as $error) { + if (!$error->wasFixable()) { + continue; + } + if ($error->getFixedErrorDiff() !== null) { + continue; + } + $skipped[] = $error; + } + + return $skipped; + } + +} diff --git a/tests/PHPStan/Fixable/data/trait-agreeing-consumers.php b/tests/PHPStan/Fixable/data/trait-agreeing-consumers.php new file mode 100644 index 00000000000..e7bccc5f748 --- /dev/null +++ b/tests/PHPStan/Fixable/data/trait-agreeing-consumers.php @@ -0,0 +1,15 @@ + Date: Sat, 2 May 2026 22:17:22 +0200 Subject: [PATCH 3/5] Ignored errors awareness --- src/Analyser/FileAnalyser.php | 3 + src/Analyser/FileAnalyserCallback.php | 15 ++ .../Ignore/IgnoredErrorHelperResult.php | 8 + src/Analyser/RuleErrorTransformer.php | 19 ++- src/Fixable/FixIgnorePolicy.php | 53 ++++++ src/Fixable/FixIgnorePolicyFactory.php | 91 ++++++++++ src/Testing/RuleTestCase.php | 2 + tests/PHPStan/Analyser/AnalyserTest.php | 2 + .../Fixable/FixIgnorePolicyFactoryTest.php | 155 ++++++++++++++++++ tests/PHPStan/Fixable/FixIgnorePolicyTest.php | 128 +++++++++++++++ ...ixerIgnoreAwarenessBaselineNoMatchTest.php | 34 ++++ ...eBatchFixerIgnoreAwarenessBaselineTest.php | 34 ++++ ...ileBatchFixerIgnoreAwarenessInlineTest.php | 29 ++++ ...PerFileBatchFixerTraitBaselineVetoTest.php | 34 ++++ .../ignore-aware-baseline-identifier.neon | 4 + .../data/ignore-aware-baseline-identifier.php | 6 + ...ignore-aware-baseline-identifier.php.fixed | 6 + .../data/ignore-aware-baseline-no-match.neon | 4 + .../data/ignore-aware-baseline-no-match.php | 6 + .../ignore-aware-baseline-no-match.php.fixed | 6 + .../data/ignore-aware-inline-annotation.php | 6 + .../ignore-aware-inline-annotation.php.fixed | 6 + .../data/ignore-aware-trait-baseline.neon | 4 + .../data/ignore-aware-trait-baseline.php | 15 ++ .../ignore-aware-trait-baseline.php.fixed | 15 ++ .../PHPStan/Fixable/data/policy-factory/F.php | 2 + 26 files changed, 686 insertions(+), 1 deletion(-) create mode 100644 src/Fixable/FixIgnorePolicy.php create mode 100644 src/Fixable/FixIgnorePolicyFactory.php create mode 100644 tests/PHPStan/Fixable/FixIgnorePolicyFactoryTest.php create mode 100644 tests/PHPStan/Fixable/FixIgnorePolicyTest.php create mode 100644 tests/PHPStan/Fixable/PerFileBatchFixerIgnoreAwarenessBaselineNoMatchTest.php create mode 100644 tests/PHPStan/Fixable/PerFileBatchFixerIgnoreAwarenessBaselineTest.php create mode 100644 tests/PHPStan/Fixable/PerFileBatchFixerIgnoreAwarenessInlineTest.php create mode 100644 tests/PHPStan/Fixable/PerFileBatchFixerTraitBaselineVetoTest.php create mode 100644 tests/PHPStan/Fixable/data/ignore-aware-baseline-identifier.neon create mode 100644 tests/PHPStan/Fixable/data/ignore-aware-baseline-identifier.php create mode 100644 tests/PHPStan/Fixable/data/ignore-aware-baseline-identifier.php.fixed create mode 100644 tests/PHPStan/Fixable/data/ignore-aware-baseline-no-match.neon create mode 100644 tests/PHPStan/Fixable/data/ignore-aware-baseline-no-match.php create mode 100644 tests/PHPStan/Fixable/data/ignore-aware-baseline-no-match.php.fixed create mode 100644 tests/PHPStan/Fixable/data/ignore-aware-inline-annotation.php create mode 100644 tests/PHPStan/Fixable/data/ignore-aware-inline-annotation.php.fixed create mode 100644 tests/PHPStan/Fixable/data/ignore-aware-trait-baseline.neon create mode 100644 tests/PHPStan/Fixable/data/ignore-aware-trait-baseline.php create mode 100644 tests/PHPStan/Fixable/data/ignore-aware-trait-baseline.php.fixed create mode 100644 tests/PHPStan/Fixable/data/policy-factory/F.php diff --git a/src/Analyser/FileAnalyser.php b/src/Analyser/FileAnalyser.php index c59e3545fb2..b34e7105cb8 100644 --- a/src/Analyser/FileAnalyser.php +++ b/src/Analyser/FileAnalyser.php @@ -12,6 +12,7 @@ use PHPStan\Dependency\DependencyResolver; use PHPStan\DependencyInjection\AutowiredParameter; use PHPStan\DependencyInjection\AutowiredService; +use PHPStan\Fixable\FixIgnorePolicyFactory; use PHPStan\Node\FileNode; use PHPStan\Parser\Parser; use PHPStan\Parser\ParserErrorsException; @@ -62,6 +63,7 @@ public function __construct( private LocalIgnoresProcessor $localIgnoresProcessor, #[AutowiredParameter] private bool $reportIgnoresWithoutComments, + private FixIgnorePolicyFactory $fixIgnorePolicyFactory, ) { } @@ -112,6 +114,7 @@ public function analyseFile( $this->parser, $this->dependencyResolver, $this->ruleErrorTransformer, + $this->fixIgnorePolicyFactory, $processedFiles, ); $scope = $this->scopeFactory->create(ScopeContext::create($file), $nodeCallback); diff --git a/src/Analyser/FileAnalyserCallback.php b/src/Analyser/FileAnalyserCallback.php index 6240db25a3f..bf0dd9137d9 100644 --- a/src/Analyser/FileAnalyserCallback.php +++ b/src/Analyser/FileAnalyserCallback.php @@ -11,6 +11,7 @@ use PHPStan\Collectors\Registry as CollectorRegistry; use PHPStan\Dependency\DependencyResolver; use PHPStan\Dependency\RootExportedNode; +use PHPStan\Fixable\FixIgnorePolicyFactory; use PHPStan\Node\InClassNode; use PHPStan\Node\InTraitNode; use PHPStan\Parser\Parser; @@ -73,6 +74,7 @@ public function __construct( private Parser $parser, private DependencyResolver $dependencyResolver, private RuleErrorTransformer $ruleErrorTransformer, + private FixIgnorePolicyFactory $fixIgnorePolicyFactory, private array $processedFiles, ) { @@ -316,9 +318,22 @@ public function getTemporaryFileErrors(): array $parserNodesByFile[$fixingFilePath] = $this->parser->parseFile($fixingFilePath); } + $errorsByFixingFile = []; + foreach ($this->pendingFixesByFile as $fixingFilePath => $pendingFixes) { + $errorsByFixingFile[$fixingFilePath] = []; + foreach ($pendingFixes as $pendingFix) { + $errorsByFixingFile[$fixingFilePath][] = $pendingFix->error; + } + } + $policy = $this->fixIgnorePolicyFactory->buildForFiles( + $errorsByFixingFile, + $this->linesToIgnore, + ); + $result = $this->ruleErrorTransformer->finalizePendingFixes( $this->pendingFixesByFile, $parserNodesByFile, + $policy, ); if ($result->diffsByErrorId === [] && $result->skipReasonByErrorId === []) { diff --git a/src/Analyser/Ignore/IgnoredErrorHelperResult.php b/src/Analyser/Ignore/IgnoredErrorHelperResult.php index ea4c1295309..783a96326f4 100644 --- a/src/Analyser/Ignore/IgnoredErrorHelperResult.php +++ b/src/Analyser/Ignore/IgnoredErrorHelperResult.php @@ -41,6 +41,14 @@ public function getErrors(): array return $this->errors; } + /** + * @return (string|mixed[])[] + */ + public function getIgnoreErrors(): array + { + return $this->ignoreErrors; + } + /** * @param list $errors * @param string[] $analysedFiles diff --git a/src/Analyser/RuleErrorTransformer.php b/src/Analyser/RuleErrorTransformer.php index 682b70e7f5b..b4166460419 100644 --- a/src/Analyser/RuleErrorTransformer.php +++ b/src/Analyser/RuleErrorTransformer.php @@ -12,6 +12,7 @@ use PHPStan\DependencyInjection\AutowiredService; use PHPStan\File\FileReader; use PHPStan\Fixable\BatchReplacingNodeVisitor; +use PHPStan\Fixable\FixIgnorePolicy; use PHPStan\Fixable\PhpPrinter; use PHPStan\Fixable\PhpPrinterIndentationDetectorVisitor; use PHPStan\Fixable\ReplacingNodeVisitor; @@ -28,7 +29,9 @@ use PHPStan\ShouldNotHappenException; use SebastianBergmann\Diff\Differ; use SebastianBergmann\Diff\Output\UnifiedDiffOutputBuilder; +use function array_filter; use function array_slice; +use function array_values; use function count; use function get_class; use function hash; @@ -192,7 +195,11 @@ public function transformPreserveFixable( * @param array> $pendingFixesByFile * @param array $parserNodesByFile */ - public function finalizePendingFixes(array $pendingFixesByFile, array $parserNodesByFile): FinalizedPendingFixes + public function finalizePendingFixes( + array $pendingFixesByFile, + array $parserNodesByFile, + ?FixIgnorePolicy $policy = null, + ): FinalizedPendingFixes { $diffsByErrorId = []; $skipReasonByErrorId = []; @@ -207,6 +214,16 @@ public function finalizePendingFixes(array $pendingFixesByFile, array $parserNod continue; } + if ($policy !== null) { + $pendingFixes = array_values(array_filter( + $pendingFixes, + static fn (PendingFix $fix): bool => !$policy->shouldDrop($fix->error), + )); + if ($pendingFixes === []) { + continue; + } + } + $batchFixes = []; foreach ($pendingFixes as $pendingFix) { $batchFixes[] = [ diff --git a/src/Fixable/FixIgnorePolicy.php b/src/Fixable/FixIgnorePolicy.php new file mode 100644 index 00000000000..7bf3ebded36 --- /dev/null +++ b/src/Fixable/FixIgnorePolicy.php @@ -0,0 +1,53 @@ +|null>> $linesToIgnore + * @param array> $witnessedIdentifiersByFixingFile + */ + public function __construct( + private array $linesToIgnore, + private array $witnessedIdentifiersByFixingFile, + ) + { + } + + public function shouldDrop(Error $error): bool + { + $line = $error->getLine(); + if ($line !== null) { + $fileKey = $error->getFile(); + if (array_key_exists($line, $this->linesToIgnore[$fileKey] ?? [])) { + $lineIgnores = $this->linesToIgnore[$fileKey][$line]; + if ($lineIgnores === null) { + return true; + } + $errorIdentifier = $error->getIdentifier(); + foreach ($lineIgnores as $entry) { + if ($entry['name'] === $errorIdentifier) { + return true; + } + } + } + } + + $errorIdentifier = $error->getIdentifier(); + if ($errorIdentifier === null) { + return false; + } + $fixingFile = $error->getTraitFilePath() ?? $error->getFilePath(); + + return isset($this->witnessedIdentifiersByFixingFile[$fixingFile][$errorIdentifier]); + } + +} diff --git a/src/Fixable/FixIgnorePolicyFactory.php b/src/Fixable/FixIgnorePolicyFactory.php new file mode 100644 index 00000000000..7134815ba80 --- /dev/null +++ b/src/Fixable/FixIgnorePolicyFactory.php @@ -0,0 +1,91 @@ +> $errorsByFixingFile + * @param array|null>> $linesToIgnore + */ + public function buildForFiles(array $errorsByFixingFile, array $linesToIgnore): FixIgnorePolicy + { + $witnessed = []; + foreach ($errorsByFixingFile as $fixingFile => $errors) { + $witnessed[$fixingFile] = $this->collectWitnesses($errors); + } + + return new FixIgnorePolicy($linesToIgnore, $witnessed); + } + + /** + * @param list $errors + * @return array + */ + private function collectWitnesses(array $errors): array + { + $entries = $this->getExpandedEntries(); + $witnessed = []; + foreach ($errors as $error) { + $identifier = $error->getIdentifier(); + if ($identifier === null) { + continue; + } + if (isset($witnessed[$identifier])) { + continue; + } + foreach ($entries as $entry) { + if (!is_array($entry)) { + continue; + } + if (!IgnoredError::shouldIgnore( + $this->fileHelper, + $error, + $entry['message'] ?? null, + $entry['rawMessage'] ?? null, + $entry['identifier'] ?? null, + $entry['path'] ?? null, + )) { + continue; + } + $witnessed[$identifier] = true; + break; + } + } + + return $witnessed; + } + + /** + * @return (string|mixed[])[] + */ + private function getExpandedEntries(): array + { + $this->cachedResult ??= $this->ignoredErrorHelper->initialize(); + + return $this->cachedResult->getIgnoreErrors(); + } + +} diff --git a/src/Testing/RuleTestCase.php b/src/Testing/RuleTestCase.php index d8d0ba20e1c..7000fe94fc0 100644 --- a/src/Testing/RuleTestCase.php +++ b/src/Testing/RuleTestCase.php @@ -23,6 +23,7 @@ use PHPStan\DependencyInjection\Type\ParameterOutTypeExtensionProvider; use PHPStan\File\FileHelper; use PHPStan\File\FileReader; +use PHPStan\Fixable\FixIgnorePolicyFactory; use PHPStan\Fixable\Patcher; use PHPStan\Node\DeepNodeCloner; use PHPStan\PhpDoc\PhpDocInheritanceResolver; @@ -140,6 +141,7 @@ private function getAnalyser(DirectRuleRegistry $ruleRegistry): Analyser self::getContainer()->getByType(RuleErrorTransformer::class), new LocalIgnoresProcessor(), false, + self::getContainer()->getByType(FixIgnorePolicyFactory::class), ); $this->analyser = new Analyser( $fileAnalyser, diff --git a/tests/PHPStan/Analyser/AnalyserTest.php b/tests/PHPStan/Analyser/AnalyserTest.php index 39da6a5be9e..87a6b0f0ef7 100644 --- a/tests/PHPStan/Analyser/AnalyserTest.php +++ b/tests/PHPStan/Analyser/AnalyserTest.php @@ -16,6 +16,7 @@ use PHPStan\DependencyInjection\Type\ParameterClosureThisExtensionProvider; use PHPStan\DependencyInjection\Type\ParameterClosureTypeExtensionProvider; use PHPStan\DependencyInjection\Type\ParameterOutTypeExtensionProvider; +use PHPStan\Fixable\FixIgnorePolicyFactory; use PHPStan\Node\DeepNodeCloner; use PHPStan\Node\Printer\ExprPrinter; use PHPStan\Node\Printer\Printer; @@ -850,6 +851,7 @@ private function createAnalyser(): Analyser $container->getByType(RuleErrorTransformer::class), new LocalIgnoresProcessor(), false, + $container->getByType(FixIgnorePolicyFactory::class), ); return new Analyser( diff --git a/tests/PHPStan/Fixable/FixIgnorePolicyFactoryTest.php b/tests/PHPStan/Fixable/FixIgnorePolicyFactoryTest.php new file mode 100644 index 00000000000..0359b9986e0 --- /dev/null +++ b/tests/PHPStan/Fixable/FixIgnorePolicyFactoryTest.php @@ -0,0 +1,155 @@ +buildFactory([ + ['identifier' => 'rule.id'], + ]); + + $policy = $factory->buildForFiles( + [ + 'F.php' => [$this->error('F.php', 'rule.id')], + 'G.php' => [$this->error('G.php', 'rule.id')], + ], + [], + ); + + self::assertTrue($policy->shouldDrop($this->error('F.php', 'rule.id'))); + self::assertTrue($policy->shouldDrop($this->error('G.php', 'rule.id'))); + } + + public function testPathOnlyEntryWitnessesEveryIdentifierInFile(): void + { + $factory = $this->buildFactory([ + ['message' => '#.*#', 'path' => __DIR__ . '/data/policy-factory/F.php'], + ]); + + $file = realpath(__DIR__ . '/data/policy-factory/F.php'); + self::assertNotFalse($file); + + $policy = $factory->buildForFiles( + [$file => [$this->error($file, 'rule.id'), $this->error($file, 'rule.other')]], + [], + ); + + self::assertTrue($policy->shouldDrop($this->error($file, 'rule.id'))); + self::assertTrue($policy->shouldDrop($this->error($file, 'rule.other'))); + } + + public function testIdentifierAndPathEntryRequiresBoth(): void + { + $file = realpath(__DIR__ . '/data/policy-factory/F.php'); + self::assertNotFalse($file); + $factory = $this->buildFactory([ + ['identifier' => 'rule.id', 'path' => $file], + ]); + + $policy = $factory->buildForFiles( + [ + $file => [$this->error($file, 'rule.id')], + 'G.php' => [$this->error('G.php', 'rule.id')], + ], + [], + ); + + self::assertTrue($policy->shouldDrop($this->error($file, 'rule.id'))); + self::assertFalse($policy->shouldDrop($this->error('G.php', 'rule.id'))); + } + + public function testMessageRegexEntryWitnessesByIdentifierOfMatchingErrors(): void + { + $factory = $this->buildFactory([ + ['message' => '#deprecated#'], + ]); + + $policy = $factory->buildForFiles( + [ + 'F.php' => [ + $this->errorWithMessage('F.php', 'rule.id', 'this is deprecated'), + $this->errorWithMessage('F.php', 'rule.other', 'fine'), + ], + ], + [], + ); + + self::assertTrue($policy->shouldDrop($this->errorWithMessage('F.php', 'rule.id', 'this is deprecated'))); + self::assertFalse($policy->shouldDrop($this->errorWithMessage('F.php', 'rule.other', 'fine'))); + } + + public function testCountFieldIsIgnoredByPolicyFactory(): void + { + $factory = $this->buildFactory([ + ['identifier' => 'rule.id', 'count' => 1], + ]); + + $policy = $factory->buildForFiles( + [ + 'F.php' => [ + $this->error('F.php', 'rule.id'), + $this->error('F.php', 'rule.id'), + $this->error('F.php', 'rule.id'), + ], + ], + [], + ); + + self::assertTrue($policy->shouldDrop($this->error('F.php', 'rule.id'))); + } + + public function testEntryWithoutMatchingErrorsProducesNoWitness(): void + { + $factory = $this->buildFactory([ + ['identifier' => 'rule.unrelated'], + ]); + + $policy = $factory->buildForFiles( + ['F.php' => [$this->error('F.php', 'rule.id')]], + [], + ); + + self::assertFalse($policy->shouldDrop($this->error('F.php', 'rule.id'))); + } + + public function testLinesToIgnoreFlowsThroughToPolicy(): void + { + $factory = $this->buildFactory([]); + + $linesToIgnore = ['F.php' => [10 => [['name' => 'rule.id', 'comment' => null]]]]; + $policy = $factory->buildForFiles([], $linesToIgnore); + + $error = new Error('msg', 'F.php', 10, true, 'F.php', null, null, 10, null, 'rule.id'); + self::assertTrue($policy->shouldDrop($error)); + } + + /** + * @param (string|mixed[])[] $ignoreErrors + */ + private function buildFactory(array $ignoreErrors): FixIgnorePolicyFactory + { + $fileHelper = self::getContainer()->getByType(FileHelper::class); + $helper = new IgnoredErrorHelper($fileHelper, $ignoreErrors, false); + return new FixIgnorePolicyFactory($helper, $fileHelper); + } + + private function error(string $filePath, ?string $identifier): Error + { + return new Error('msg', $filePath, 10, true, $filePath, null, null, 10, null, $identifier); + } + + private function errorWithMessage(string $filePath, ?string $identifier, string $message): Error + { + return new Error($message, $filePath, 10, true, $filePath, null, null, 10, null, $identifier); + } + +} diff --git a/tests/PHPStan/Fixable/FixIgnorePolicyTest.php b/tests/PHPStan/Fixable/FixIgnorePolicyTest.php new file mode 100644 index 00000000000..5537985df81 --- /dev/null +++ b/tests/PHPStan/Fixable/FixIgnorePolicyTest.php @@ -0,0 +1,128 @@ + [ + 'linesToIgnore' => [], + 'witnessedIdentifiers' => [], + 'error' => self::error('F.php', 10, 'rule.id'), + 'expected' => false, + ]; + + yield 'wildcard line ignore drops any error on that line' => [ + 'linesToIgnore' => ['F.php' => [10 => null]], + 'witnessedIdentifiers' => [], + 'error' => self::error('F.php', 10, 'rule.id'), + 'expected' => true, + ]; + + yield 'wildcard line ignore on different line keeps error' => [ + 'linesToIgnore' => ['F.php' => [11 => null]], + 'witnessedIdentifiers' => [], + 'error' => self::error('F.php', 10, 'rule.id'), + 'expected' => false, + ]; + + yield 'identifier line ignore drops matching identifier' => [ + 'linesToIgnore' => ['F.php' => [10 => [['name' => 'rule.id', 'comment' => null]]]], + 'witnessedIdentifiers' => [], + 'error' => self::error('F.php', 10, 'rule.id'), + 'expected' => true, + ]; + + yield 'identifier line ignore keeps non-matching identifier' => [ + 'linesToIgnore' => ['F.php' => [10 => [['name' => 'other.id', 'comment' => null]]]], + 'witnessedIdentifiers' => [], + 'error' => self::error('F.php', 10, 'rule.id'), + 'expected' => false, + ]; + + yield 'baseline witness drops error of witnessed identifier' => [ + 'linesToIgnore' => [], + 'witnessedIdentifiers' => ['F.php' => ['rule.id' => true]], + 'error' => self::error('F.php', 10, 'rule.id'), + 'expected' => true, + ]; + + yield 'baseline witness for different file keeps error' => [ + 'linesToIgnore' => [], + 'witnessedIdentifiers' => ['G.php' => ['rule.id' => true]], + 'error' => self::error('F.php', 10, 'rule.id'), + 'expected' => false, + ]; + + yield 'baseline witness for different identifier keeps error' => [ + 'linesToIgnore' => [], + 'witnessedIdentifiers' => ['F.php' => ['other.id' => true]], + 'error' => self::error('F.php', 10, 'rule.id'), + 'expected' => false, + ]; + + yield 'error without identifier is never baseline-dropped' => [ + 'linesToIgnore' => [], + 'witnessedIdentifiers' => ['F.php' => ['rule.id' => true]], + 'error' => self::error('F.php', 10, null), + 'expected' => false, + ]; + + yield 'trait error uses traitFilePath for witness lookup' => [ + 'linesToIgnore' => [], + 'witnessedIdentifiers' => ['T.php' => ['rule.id' => true]], + 'error' => self::traitError('T.php', 'A.php', 4, 'rule.id'), + 'expected' => true, + ]; + } + + /** + * @param array|null>> $linesToIgnore + * @param array> $witnessedIdentifiers + */ + #[DataProvider('dataShouldDrop')] + public function testShouldDrop(array $linesToIgnore, array $witnessedIdentifiers, Error $error, bool $expected): void + { + $policy = new FixIgnorePolicy($linesToIgnore, $witnessedIdentifiers); + self::assertSame($expected, $policy->shouldDrop($error)); + } + + private static function error(string $filePath, int $line, ?string $identifier): Error + { + return new Error( + 'msg', + $filePath, + $line, + true, + $filePath, + null, + null, + $line, + null, + $identifier, + ); + } + + private static function traitError(string $traitFilePath, string $consumerFilePath, int $line, ?string $identifier): Error + { + return new Error( + 'msg', + $traitFilePath, + $line, + true, + $consumerFilePath, + $traitFilePath, + null, + $line, + null, + $identifier, + ); + } + +} diff --git a/tests/PHPStan/Fixable/PerFileBatchFixerIgnoreAwarenessBaselineNoMatchTest.php b/tests/PHPStan/Fixable/PerFileBatchFixerIgnoreAwarenessBaselineNoMatchTest.php new file mode 100644 index 00000000000..73c1cab557a --- /dev/null +++ b/tests/PHPStan/Fixable/PerFileBatchFixerIgnoreAwarenessBaselineNoMatchTest.php @@ -0,0 +1,34 @@ + + */ +final class PerFileBatchFixerIgnoreAwarenessBaselineNoMatchTest extends RuleTestCase +{ + + #[Override] + protected function getRule(): Rule + { + return new RenameVariableFixRule(); + } + + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/data/ignore-aware-baseline-no-match.neon']; + } + + public function testBaselineDifferentIdentifierLeavesUnrelatedFixesApplied(): void + { + $this->fix( + __DIR__ . '/data/ignore-aware-baseline-no-match.php', + __DIR__ . '/data/ignore-aware-baseline-no-match.php.fixed', + ); + } + +} diff --git a/tests/PHPStan/Fixable/PerFileBatchFixerIgnoreAwarenessBaselineTest.php b/tests/PHPStan/Fixable/PerFileBatchFixerIgnoreAwarenessBaselineTest.php new file mode 100644 index 00000000000..baeb2ac3f44 --- /dev/null +++ b/tests/PHPStan/Fixable/PerFileBatchFixerIgnoreAwarenessBaselineTest.php @@ -0,0 +1,34 @@ + + */ +final class PerFileBatchFixerIgnoreAwarenessBaselineTest extends RuleTestCase +{ + + #[Override] + protected function getRule(): Rule + { + return new RenameVariableFixRule(); + } + + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/data/ignore-aware-baseline-identifier.neon']; + } + + public function testBaselineIdentifierEntryDropsAllFixesOfThatIdentifier(): void + { + $this->fix( + __DIR__ . '/data/ignore-aware-baseline-identifier.php', + __DIR__ . '/data/ignore-aware-baseline-identifier.php.fixed', + ); + } + +} diff --git a/tests/PHPStan/Fixable/PerFileBatchFixerIgnoreAwarenessInlineTest.php b/tests/PHPStan/Fixable/PerFileBatchFixerIgnoreAwarenessInlineTest.php new file mode 100644 index 00000000000..2aaebc4078b --- /dev/null +++ b/tests/PHPStan/Fixable/PerFileBatchFixerIgnoreAwarenessInlineTest.php @@ -0,0 +1,29 @@ + + */ +final class PerFileBatchFixerIgnoreAwarenessInlineTest extends RuleTestCase +{ + + #[Override] + protected function getRule(): Rule + { + return new RenameVariableFixRule(); + } + + public function testInlineAnnotationDropsOnlyAnnotatedNode(): void + { + $this->fix( + __DIR__ . '/data/ignore-aware-inline-annotation.php', + __DIR__ . '/data/ignore-aware-inline-annotation.php.fixed', + ); + } + +} diff --git a/tests/PHPStan/Fixable/PerFileBatchFixerTraitBaselineVetoTest.php b/tests/PHPStan/Fixable/PerFileBatchFixerTraitBaselineVetoTest.php new file mode 100644 index 00000000000..ae81110a52c --- /dev/null +++ b/tests/PHPStan/Fixable/PerFileBatchFixerTraitBaselineVetoTest.php @@ -0,0 +1,34 @@ + + */ +final class PerFileBatchFixerTraitBaselineVetoTest extends RuleTestCase +{ + + #[Override] + protected function getRule(): Rule + { + return new RenameVariableFixRule(); + } + + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/data/ignore-aware-trait-baseline.neon']; + } + + public function testTraitWithBaselinedConsumerErrorIsNotFixed(): void + { + $this->fix( + __DIR__ . '/data/ignore-aware-trait-baseline.php', + __DIR__ . '/data/ignore-aware-trait-baseline.php.fixed', + ); + } + +} diff --git a/tests/PHPStan/Fixable/data/ignore-aware-baseline-identifier.neon b/tests/PHPStan/Fixable/data/ignore-aware-baseline-identifier.neon new file mode 100644 index 00000000000..f0a4b8495fc --- /dev/null +++ b/tests/PHPStan/Fixable/data/ignore-aware-baseline-identifier.neon @@ -0,0 +1,4 @@ +parameters: + ignoreErrors: + - + identifier: tests.batchFixerBench.renameVar diff --git a/tests/PHPStan/Fixable/data/ignore-aware-baseline-identifier.php b/tests/PHPStan/Fixable/data/ignore-aware-baseline-identifier.php new file mode 100644 index 00000000000..06e8c4ee809 --- /dev/null +++ b/tests/PHPStan/Fixable/data/ignore-aware-baseline-identifier.php @@ -0,0 +1,6 @@ + Date: Sun, 3 May 2026 02:35:51 +0200 Subject: [PATCH 4/5] File-level fixes --- src/Analyser/Analyser.php | 10 ++ src/Analyser/AnalyserResult.php | 20 ++++ src/Analyser/AnalyserResultFinalizer.php | 8 +- src/Analyser/Error.php | 55 +--------- src/Analyser/FileAnalyser.php | 3 + src/Analyser/FileAnalyserCallback.php | 18 +++- src/Analyser/FileAnalyserResult.php | 10 ++ src/Analyser/FileFix.php | 29 +++++ src/Analyser/FinalizedPendingFixes.php | 4 +- .../ResultCache/FileFixAggregator.php | 69 ++++++++++++ src/Analyser/ResultCache/ResultCache.php | 11 ++ .../ResultCache/ResultCacheManager.php | 48 +++++++-- src/Analyser/RuleErrorTransformer.php | 62 ++++------- src/Command/AnalyseApplication.php | 7 ++ src/Command/AnalyseCommand.php | 102 +++++++++--------- src/Command/AnalysisResult.php | 12 +++ src/Fixable/ReplacingNodeVisitor.php | 47 -------- src/Testing/RuleTestCase.php | 28 +++-- tests/PHPStan/Analyser/FileFixTest.php | 30 ++++++ .../ResultCache/FileFixAggregatorTest.php | 93 ++++++++++++++++ .../Fixable/PerFileBatchFixerTraitTest.php | 45 ++++++-- 21 files changed, 486 insertions(+), 225 deletions(-) create mode 100644 src/Analyser/FileFix.php create mode 100644 src/Analyser/ResultCache/FileFixAggregator.php delete mode 100644 src/Fixable/ReplacingNodeVisitor.php create mode 100644 tests/PHPStan/Analyser/FileFixTest.php create mode 100644 tests/PHPStan/Analyser/ResultCache/FileFixAggregatorTest.php diff --git a/src/Analyser/Analyser.php b/src/Analyser/Analyser.php index 475781e17b6..a2b40de2220 100644 --- a/src/Analyser/Analyser.php +++ b/src/Analyser/Analyser.php @@ -3,6 +3,7 @@ namespace PHPStan\Analyser; use Closure; +use PHPStan\Analyser\ResultCache\FileFixAggregator; use PHPStan\Collectors\CollectedData; use PHPStan\Collectors\Registry as CollectorRegistry; use PHPStan\DependencyInjection\AutowiredParameter; @@ -76,6 +77,8 @@ public function analyse( $usedTraitDependencies = []; $exportedNodes = []; $allProcessedFiles = []; + /** @var array> $perAnalysedFileFixes */ + $perAnalysedFileFixes = []; foreach ($files as $file) { if ($preFileCallback !== null) { $preFileCallback($file); @@ -106,6 +109,11 @@ public function analyse( if (count($fileExportedNodes) > 0) { $exportedNodes[$file] = $fileExportedNodes; } + + $fileFixes = $fileAnalyserResult->getFixesByFixingFile(); + if ($fileFixes !== []) { + $perAnalysedFileFixes[$file] = $fileFixes; + } } catch (Throwable $t) { if ($debug) { throw $t; @@ -146,6 +154,8 @@ public function analyse( reachedInternalErrorsCountLimit: $reachedInternalErrorsCountLimit, peakMemoryUsageBytes: memory_get_peak_usage(true), processedFiles: $allProcessedFiles, + fixesByFixingFile: FileFixAggregator::aggregate($perAnalysedFileFixes), + perAnalysedFileFixes: $perAnalysedFileFixes, ); } diff --git a/src/Analyser/AnalyserResult.php b/src/Analyser/AnalyserResult.php index 31b88e27300..9bcef25697e 100644 --- a/src/Analyser/AnalyserResult.php +++ b/src/Analyser/AnalyserResult.php @@ -29,6 +29,8 @@ final class AnalyserResult * @param array>|null $usedTraitDependencies * @param array> $exportedNodes * @param list $processedFiles + * @param array $fixesByFixingFile + * @param array> $perAnalysedFileFixes */ public function __construct( private array $unorderedErrors, @@ -45,6 +47,8 @@ public function __construct( private bool $reachedInternalErrorsCountLimit, private int $peakMemoryUsageBytes, private array $processedFiles, + private array $fixesByFixingFile = [], + private array $perAnalysedFileFixes = [], ) { } @@ -179,4 +183,20 @@ public function getProcessedFiles(): array return $this->processedFiles; } + /** + * @return array + */ + public function getFixesByFixingFile(): array + { + return $this->fixesByFixingFile; + } + + /** + * @return array> + */ + public function getPerAnalysedFileFixes(): array + { + return $this->perAnalysedFileFixes; + } + } diff --git a/src/Analyser/AnalyserResultFinalizer.php b/src/Analyser/AnalyserResultFinalizer.php index 627c946e691..b31daf62a8a 100644 --- a/src/Analyser/AnalyserResultFinalizer.php +++ b/src/Analyser/AnalyserResultFinalizer.php @@ -93,7 +93,7 @@ public function finalize(AnalyserResult $analyserResult, bool $onlyFiles, bool $ } foreach ($ruleErrors as $ruleError) { - $error = $this->ruleErrorTransformer->transform($ruleError, $scope, [], $node); + [$error] = $this->ruleErrorTransformer->transformPreserveFixable($ruleError, $scope, $node); if ($error->canBeIgnored()) { foreach ($this->ignoreErrorExtensionProvider->getExtensions() as $ignoreErrorExtension) { @@ -149,6 +149,8 @@ public function finalize(AnalyserResult $analyserResult, bool $onlyFiles, bool $ reachedInternalErrorsCountLimit: $analyserResult->hasReachedInternalErrorsCountLimit(), peakMemoryUsageBytes: $analyserResult->getPeakMemoryUsageBytes(), processedFiles: $analyserResult->getProcessedFiles(), + fixesByFixingFile: $analyserResult->getFixesByFixingFile(), + perAnalysedFileFixes: $analyserResult->getPerAnalysedFileFixes(), ), $collectorErrors, $locallyIgnoredCollectorErrors); } @@ -169,6 +171,8 @@ private function mergeFilteredPhpErrors(AnalyserResult $analyserResult): Analyse reachedInternalErrorsCountLimit: $analyserResult->hasReachedInternalErrorsCountLimit(), peakMemoryUsageBytes: $analyserResult->getPeakMemoryUsageBytes(), processedFiles: $analyserResult->getProcessedFiles(), + fixesByFixingFile: $analyserResult->getFixesByFixingFile(), + perAnalysedFileFixes: $analyserResult->getPerAnalysedFileFixes(), ); } @@ -234,6 +238,8 @@ private function addUnmatchedIgnoredErrors( reachedInternalErrorsCountLimit: $analyserResult->hasReachedInternalErrorsCountLimit(), peakMemoryUsageBytes: $analyserResult->getPeakMemoryUsageBytes(), processedFiles: $analyserResult->getProcessedFiles(), + fixesByFixingFile: $analyserResult->getFixesByFixingFile(), + perAnalysedFileFixes: $analyserResult->getPerAnalysedFileFixes(), ), $collectorErrors, $locallyIgnoredCollectorErrors, diff --git a/src/Analyser/Error.php b/src/Analyser/Error.php index cd82485679f..be838a8c11d 100644 --- a/src/Analyser/Error.php +++ b/src/Analyser/Error.php @@ -39,7 +39,6 @@ public function __construct( private ?string $nodeType = null, private ?string $identifier = null, private array $metadata = [], - private ?FixedErrorDiff $fixedErrorDiff = null, private bool $wasFixable = false, ) { @@ -85,7 +84,6 @@ public function changeFilePath(string $newFilePath): self $this->nodeType, $this->identifier, $this->metadata, - $this->fixedErrorDiff, $this->wasFixable, ); } @@ -104,7 +102,6 @@ public function changeTraitFilePath(string $newFilePath): self $this->nodeType, $this->identifier, $this->metadata, - $this->fixedErrorDiff, $this->wasFixable, ); } @@ -152,7 +149,6 @@ public function withoutTip(): self $this->nodeType, $this->identifier, $this->metadata, - $this->fixedErrorDiff, $this->wasFixable, ); } @@ -175,7 +171,6 @@ public function doNotIgnore(): self $this->nodeType, $this->identifier, $this->metadata, - $this->fixedErrorDiff, $this->wasFixable, ); } @@ -198,7 +193,6 @@ public function withIdentifier(string $identifier): self $this->nodeType, $identifier, $this->metadata, - $this->fixedErrorDiff, $this->wasFixable, ); } @@ -224,7 +218,7 @@ public function withMetadata(array $metadata): self $this->nodeType, $this->identifier, $metadata, - $this->fixedErrorDiff, + $this->wasFixable, ); } @@ -259,36 +253,6 @@ public function getMetadata(): array return $this->metadata; } - /** - * @internal Experimental - */ - 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, - $this->wasFixable, - ); - } - /** * @internal Experimental */ @@ -314,7 +278,6 @@ public function withAppendedTip(string $extraTip): self $this->nodeType, $this->identifier, $this->metadata, - $this->fixedErrorDiff, $this->wasFixable, ); } @@ -326,13 +289,6 @@ public function withAppendedTip(string $extraTip): self #[Override] public function jsonSerialize() { - $fixedErrorDiffHash = null; - $fixedErrorDiffDiff = null; - if ($this->fixedErrorDiff !== null) { - $fixedErrorDiffHash = $this->fixedErrorDiff->originalHash; - $fixedErrorDiffDiff = $this->fixedErrorDiff->diff; - } - return [ 'message' => $this->message, 'file' => $this->file, @@ -345,8 +301,6 @@ public function jsonSerialize() 'nodeType' => $this->nodeType, 'identifier' => $this->identifier, 'metadata' => $this->metadata, - 'fixedErrorDiffHash' => $fixedErrorDiffHash, - 'fixedErrorDiffDiff' => $fixedErrorDiffDiff, 'wasFixable' => $this->wasFixable, ]; } @@ -356,11 +310,6 @@ public function jsonSerialize() */ public static function decode(array $json): self { - $fixedErrorDiff = null; - if ($json['fixedErrorDiffHash'] !== null && $json['fixedErrorDiffDiff'] !== null) { - $fixedErrorDiff = new FixedErrorDiff($json['fixedErrorDiffHash'], $json['fixedErrorDiffDiff']); - } - return new self( $json['message'], $json['file'], @@ -373,7 +322,6 @@ public static function decode(array $json): self $json['nodeType'] ?? null, $json['identifier'] ?? null, $json['metadata'] ?? [], - $fixedErrorDiff, $json['wasFixable'] ?? false, ); } @@ -395,7 +343,6 @@ public static function __set_state(array $properties): self $properties['nodeType'] ?? null, $properties['identifier'] ?? null, $properties['metadata'] ?? [], - $properties['fixedErrorDiff'] ?? null, $properties['wasFixable'] ?? false, ); } diff --git a/src/Analyser/FileAnalyser.php b/src/Analyser/FileAnalyser.php index b34e7105cb8..ddc67444fe2 100644 --- a/src/Analyser/FileAnalyser.php +++ b/src/Analyser/FileAnalyser.php @@ -97,6 +97,7 @@ public function analyseFile( $exportedNodes = []; $linesToIgnore = []; $unmatchedLineIgnores = []; + $fixesByFixingFile = []; if (is_file($file)) { try { $this->collectErrors($analysedFiles); @@ -132,6 +133,7 @@ public function analyseFile( $linesToIgnore = $nodeCallback->getLinesToIgnore(); $unmatchedLineIgnores = $nodeCallback->getUnmatchedLineIgnores(); $temporaryFileErrors = $nodeCallback->getTemporaryFileErrors(); + $fixesByFixingFile = $nodeCallback->getFixesByFixingFile(); $processedFiles = $nodeCallback->getProcessedFiles(); if ($this->reportIgnoresWithoutComments) { @@ -250,6 +252,7 @@ public function analyseFile( $linesToIgnore, $unmatchedLineIgnores, $processedFiles, + $fixesByFixingFile, ); } diff --git a/src/Analyser/FileAnalyserCallback.php b/src/Analyser/FileAnalyserCallback.php index bf0dd9137d9..c818823e3a1 100644 --- a/src/Analyser/FileAnalyserCallback.php +++ b/src/Analyser/FileAnalyserCallback.php @@ -50,6 +50,9 @@ final class FileAnalyserCallback /** @var array> */ private array $pendingFixesByFile = []; + /** @var array */ + private array $fixesByFixingFile = []; + /** @var LinesToIgnore */ private array $linesToIgnore; @@ -335,18 +338,15 @@ public function getTemporaryFileErrors(): array $parserNodesByFile, $policy, ); + $this->fixesByFixingFile = $result->fixesByFixingFile; - if ($result->diffsByErrorId === [] && $result->skipReasonByErrorId === []) { + if ($result->skipReasonByErrorId === []) { return $this->temporaryFileErrors; } $finalErrors = []; foreach ($this->temporaryFileErrors as $error) { $id = spl_object_id($error); - $diff = $result->diffsByErrorId[$id] ?? null; - if ($diff !== null) { - $error = $error->withFixedErrorDiff($diff); - } $skipReason = $result->skipReasonByErrorId[$id] ?? null; if ($skipReason !== null) { $error = $error->withAppendedTip($skipReason); @@ -365,4 +365,12 @@ public function getProcessedFiles(): array return $this->processedFiles; } + /** + * @return array + */ + public function getFixesByFixingFile(): array + { + return $this->fixesByFixingFile; + } + } diff --git a/src/Analyser/FileAnalyserResult.php b/src/Analyser/FileAnalyserResult.php index 320b736c6b7..30fc2fbb4a7 100644 --- a/src/Analyser/FileAnalyserResult.php +++ b/src/Analyser/FileAnalyserResult.php @@ -25,6 +25,7 @@ final class FileAnalyserResult * @param LinesToIgnore $linesToIgnore * @param LinesToIgnore $unmatchedLineIgnores * @param list $processedFiles + * @param array $fixesByFixingFile */ public function __construct( private array $errors, @@ -38,6 +39,7 @@ public function __construct( private array $linesToIgnore, private array $unmatchedLineIgnores, private array $processedFiles, + private array $fixesByFixingFile = [], ) { } @@ -130,4 +132,12 @@ public function getProcessedFiles(): array return $this->processedFiles; } + /** + * @return array + */ + public function getFixesByFixingFile(): array + { + return $this->fixesByFixingFile; + } + } diff --git a/src/Analyser/FileFix.php b/src/Analyser/FileFix.php new file mode 100644 index 00000000000..35bc223de16 --- /dev/null +++ b/src/Analyser/FileFix.php @@ -0,0 +1,29 @@ + $errorRefs + */ + public function __construct( + public readonly FixedErrorDiff $diff, + public readonly array $errorRefs, + ) + { + } + + /** + * @param array{diff: FixedErrorDiff, errorRefs: list} $properties + */ + public static function __set_state(array $properties): self + { + return new self($properties['diff'], $properties['errorRefs']); + } + +} diff --git a/src/Analyser/FinalizedPendingFixes.php b/src/Analyser/FinalizedPendingFixes.php index 230fb397bbd..892a65571dc 100644 --- a/src/Analyser/FinalizedPendingFixes.php +++ b/src/Analyser/FinalizedPendingFixes.php @@ -9,11 +9,11 @@ final class FinalizedPendingFixes { /** - * @param array $diffsByErrorId + * @param array $fixesByFixingFile * @param array $skipReasonByErrorId */ public function __construct( - public readonly array $diffsByErrorId, + public readonly array $fixesByFixingFile, public readonly array $skipReasonByErrorId, ) { diff --git a/src/Analyser/ResultCache/FileFixAggregator.php b/src/Analyser/ResultCache/FileFixAggregator.php new file mode 100644 index 00000000000..3cac43e2055 --- /dev/null +++ b/src/Analyser/ResultCache/FileFixAggregator.php @@ -0,0 +1,69 @@ +> $perAnalysedFileFixes + * @return array + */ + public static function aggregate(array $perAnalysedFileFixes): array + { + $grouped = []; + foreach ($perAnalysedFileFixes as $perFileFixes) { + foreach ($perFileFixes as $fixingFile => $fileFix) { + $grouped[$fixingFile][] = $fileFix; + } + } + + $result = []; + foreach ($grouped as $fixingFile => $candidates) { + $first = $candidates[0]; + + if (count($candidates) === 1) { + $result[$fixingFile] = $first; + continue; + } + + $expectedHash = $first->diff->originalHash; + $conflict = false; + + foreach ($candidates as $candidate) { + if ($candidate->diff->originalHash !== $expectedHash) { + $conflict = true; + break; + } + } + + if ($conflict) { + continue; + } + + $mergedRefs = []; + $seen = []; + foreach ($candidates as $candidate) { + foreach ($candidate->errorRefs as $ref) { + $key = $ref['line'] . ':' . ($ref['identifier'] ?? ''); + if (isset($seen[$key])) { + continue; + } + $seen[$key] = true; + $mergedRefs[] = $ref; + } + } + + $result[$fixingFile] = new FileFix($first->diff, $mergedRefs); + } + + return $result; + } + +} diff --git a/src/Analyser/ResultCache/ResultCache.php b/src/Analyser/ResultCache/ResultCache.php index 5cfefc6f46a..a59f3acf130 100644 --- a/src/Analyser/ResultCache/ResultCache.php +++ b/src/Analyser/ResultCache/ResultCache.php @@ -4,6 +4,7 @@ use PHPStan\Analyser\Error; use PHPStan\Analyser\FileAnalyserResult; +use PHPStan\Analyser\FileFix; use PHPStan\Collectors\CollectedData; use PHPStan\Dependency\RootExportedNode; @@ -27,6 +28,7 @@ final class ResultCache * @param array> $exportedNodes * @param array $projectExtensionFiles * @param array $currentFileHashes + * @param array> $perAnalysedFileFixes */ public function __construct( private array $filesToAnalyse, @@ -43,6 +45,7 @@ public function __construct( private array $exportedNodes, private array $projectExtensionFiles, private array $currentFileHashes, + private array $perAnalysedFileFixes = [], ) { } @@ -153,4 +156,12 @@ public function getCurrentFileHashes(): array return $this->currentFileHashes; } + /** + * @return array> + */ + public function getPerAnalysedFileFixes(): array + { + return $this->perAnalysedFileFixes; + } + } diff --git a/src/Analyser/ResultCache/ResultCacheManager.php b/src/Analyser/ResultCache/ResultCacheManager.php index 0bc1e756e5b..226f7ba3a2a 100644 --- a/src/Analyser/ResultCache/ResultCacheManager.php +++ b/src/Analyser/ResultCache/ResultCacheManager.php @@ -6,6 +6,7 @@ use PHPStan\Analyser\AnalyserResult; use PHPStan\Analyser\Error; use PHPStan\Analyser\FileAnalyserResult; +use PHPStan\Analyser\FileFix; use PHPStan\Collectors\CollectedData; use PHPStan\Command\Output; use PHPStan\Dependency\ExportedNode\ExportedTraitNode; @@ -61,7 +62,7 @@ final class ResultCacheManager { - private const CACHE_VERSION = 'v12-linesToIgnore'; + private const CACHE_VERSION = 'v13-fileFixes'; /** @var array */ private array $fileHashes = []; @@ -362,12 +363,14 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ? $unmatchedLineIgnores = $data['unmatchedLineIgnores']; $collectedData = $data['collectedDataCallback'](); $exportedNodes = $data['exportedNodesCallback'](); + $perAnalysedFileFixes = $data['perAnalysedFileFixes'] ?? []; $filteredErrors = []; $filteredLocallyIgnoredErrors = []; $filteredLinesToIgnore = []; $filteredUnmatchedLineIgnores = []; $filteredCollectedData = []; $filteredExportedNodes = []; + $filteredPerAnalysedFileFixes = []; $newFileAppeared = false; foreach (array_keys($this->getStubFiles()) as $stubFile) { @@ -397,6 +400,9 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ? if (array_key_exists($analysedFile, $exportedNodes)) { $filteredExportedNodes[$analysedFile] = $exportedNodes[$analysedFile]; } + if (array_key_exists($analysedFile, $perAnalysedFileFixes)) { + $filteredPerAnalysedFileFixes[$analysedFile] = $perAnalysedFileFixes[$analysedFile]; + } if (!array_key_exists($analysedFile, $invertedDependencies)) { // new file $filesToAnalyse[] = $analysedFile; @@ -515,6 +521,7 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ? exportedNodes: $filteredExportedNodes, projectExtensionFiles: $data['projectExtensionFiles'], currentFileHashes: $currentFileHashes, + perAnalysedFileFixes: $filteredPerAnalysedFileFixes, ); } @@ -624,7 +631,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache if ($projectConfigArray !== null) { $meta['projectConfig'] = Neon::encode($projectConfigArray); } - $doSave = function (array $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, ?array $dependencies, ?array $usedTraitDependencies, array $exportedNodes, array $projectExtensionFiles) use ($internalErrors, $resultCache, $output, $onlyFiles, $meta): bool { + $doSave = function (array $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, ?array $dependencies, ?array $usedTraitDependencies, array $exportedNodes, array $projectExtensionFiles, array $perAnalysedFileFixes) use ($internalErrors, $resultCache, $output, $onlyFiles, $meta): bool { if ($onlyFiles) { if ($output->isVeryVerbose()) { $output->writeLineFormatted('Result cache was not saved because only files were passed as analysed paths.'); @@ -672,7 +679,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache } } - $this->save($resultCache->getLastFullAnalysisTime(), $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $exportedNodes, $projectExtensionFiles, $resultCache->getCurrentFileHashes(), $meta); + $this->save($resultCache->getLastFullAnalysisTime(), $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $exportedNodes, $projectExtensionFiles, $resultCache->getCurrentFileHashes(), $meta, $perAnalysedFileFixes); if ($output->isVeryVerbose()) { $output->writeLineFormatted('Result cache is saved.'); @@ -688,7 +695,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache if ($analyserResult->getDependencies() !== null) { $projectExtensionFiles = $this->getProjectExtensionFiles($projectConfigArray, $analyserResult->getDependencies()); } - $saved = $doSave($freshErrorsByFile, $freshLocallyIgnoredErrorsByFile, $analyserResult->getLinesToIgnore(), $analyserResult->getUnmatchedLineIgnores(), $freshCollectedDataByFile, $analyserResult->getDependencies(), $analyserResult->getUsedTraitDependencies(), $analyserResult->getExportedNodes(), $projectExtensionFiles); + $saved = $doSave($freshErrorsByFile, $freshLocallyIgnoredErrorsByFile, $analyserResult->getLinesToIgnore(), $analyserResult->getUnmatchedLineIgnores(), $freshCollectedDataByFile, $analyserResult->getDependencies(), $analyserResult->getUsedTraitDependencies(), $analyserResult->getExportedNodes(), $projectExtensionFiles, $analyserResult->getPerAnalysedFileFixes()); } else { if ($output->isVeryVerbose()) { $output->writeLineFormatted('Result cache was not saved because it was not requested.'); @@ -706,6 +713,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache $exportedNodes = $this->mergeExportedNodes($resultCache, $analyserResult->getExportedNodes()); $linesToIgnore = $this->mergeLinesToIgnore($resultCache, $analyserResult->getLinesToIgnore()); $unmatchedLineIgnores = $this->mergeUnmatchedLineIgnores($resultCache, $analyserResult->getUnmatchedLineIgnores()); + $mergedPerAnalysedFileFixes = $this->mergePerAnalysedFileFixes($resultCache, $analyserResult->getPerAnalysedFileFixes()); $saved = false; if ($save !== false) { @@ -729,7 +737,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache $projectExtensionFiles[$file] = [$hash, true, $className]; } } - $saved = $doSave($errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $exportedNodes, $projectExtensionFiles); + $saved = $doSave($errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $exportedNodes, $projectExtensionFiles, $mergedPerAnalysedFileFixes); } $flatErrors = []; @@ -761,6 +769,8 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache reachedInternalErrorsCountLimit: $analyserResult->hasReachedInternalErrorsCountLimit(), peakMemoryUsageBytes: $analyserResult->getPeakMemoryUsageBytes(), processedFiles: $analyserResult->getProcessedFiles(), + fixesByFixingFile: FileFixAggregator::aggregate($mergedPerAnalysedFileFixes), + perAnalysedFileFixes: $mergedPerAnalysedFileFixes, ), $saved); } @@ -945,6 +955,28 @@ private function mergeUnmatchedLineIgnores(ResultCache $resultCache, array $fres return $newUnmatchedLineIgnores; } + /** + * @param array> $freshPerAnalysedFile + * @return array> + */ + private function mergePerAnalysedFileFixes(ResultCache $resultCache, array $freshPerAnalysedFile): array + { + $merged = $resultCache->getPerAnalysedFileFixes(); + foreach ($resultCache->getFilesToAnalyse() as $file) { + if (array_key_exists($file, $this->fileReplacements)) { + unset($merged[$file]); + $file = $this->fileReplacements[$file]; + } + if (!array_key_exists($file, $freshPerAnalysedFile)) { + unset($merged[$file]); + continue; + } + $merged[$file] = $freshPerAnalysedFile[$file]; + } + + return $merged; + } + /** * @param array> $errors * @param array> $locallyIgnoredErrors @@ -957,6 +989,7 @@ private function mergeUnmatchedLineIgnores(ResultCache $resultCache, array $fres * @param array $projectExtensionFiles * @param array $currentFileHashes * @param mixed[] $meta + * @param array> $perAnalysedFileFixes */ private function save( int $lastFullAnalysisTime, @@ -971,6 +1004,7 @@ private function save( array $projectExtensionFiles, array $currentFileHashes, array $meta, + array $perAnalysedFileFixes, ): void { $invertedDependencies = []; @@ -1023,6 +1057,7 @@ private function save( ksort($unmatchedLineIgnores); ksort($collectedData); ksort($invertedDependencies); + ksort($perAnalysedFileFixes); foreach ($collectedData as & $collectedDataPerFile) { ksort($collectedDataPerFile); @@ -1060,7 +1095,8 @@ private function save( 'unmatchedLineIgnores' => " . var_export($unmatchedLineIgnores, true) . ", 'collectedDataCallback' => static function (): array { return " . var_export($collectedData, true) . "; }, 'dependencies' => " . var_export($invertedDependencies, true) . ", - 'exportedNodesCallback' => static function (): array { return " . var_export($exportedNodes, true) . '; }, + 'exportedNodesCallback' => static function (): array { return " . var_export($exportedNodes, true) . "; }, + 'perAnalysedFileFixes' => " . var_export($perAnalysedFileFixes, true) . ', ]; ', ); diff --git a/src/Analyser/RuleErrorTransformer.php b/src/Analyser/RuleErrorTransformer.php index b4166460419..f1d6e5a201d 100644 --- a/src/Analyser/RuleErrorTransformer.php +++ b/src/Analyser/RuleErrorTransformer.php @@ -15,7 +15,6 @@ use PHPStan\Fixable\FixIgnorePolicy; use PHPStan\Fixable\PhpPrinter; use PHPStan\Fixable\PhpPrinterIndentationDetectorVisitor; -use PHPStan\Fixable\ReplacingNodeVisitor; use PHPStan\Node\DeepNodeCloner; use PHPStan\Node\VirtualNode; use PHPStan\Rules\FileRuleError; @@ -55,29 +54,6 @@ public function __construct( $this->differ = new Differ(new UnifiedDiffOutputBuilder('', addLineNumbers: true)); } - /** - * @param Node\Stmt[] $fileNodes - */ - public function transform( - RuleError $ruleError, - Scope $scope, - 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} */ @@ -201,7 +177,7 @@ public function finalizePendingFixes( ?FixIgnorePolicy $policy = null, ): FinalizedPendingFixes { - $diffsByErrorId = []; + $fixesByFixingFile = []; $skipReasonByErrorId = []; foreach ($pendingFixesByFile as $filePath => $pendingFixes) { @@ -243,23 +219,35 @@ public function finalizePendingFixes( $conflictClusters = $replacing->getConflictClustersByOriginalNodeId(); + $errorRefs = []; foreach ($pendingFixes as $pendingFix) { $nodeId = spl_object_id($pendingFix->originalNode); if (isset($appliedSet[$nodeId])) { if ($diff !== null) { - $diffsByErrorId[spl_object_id($pendingFix->error)] = $diff; + $errorRefs[] = [ + 'line' => $pendingFix->error->getLine() ?? 0, + 'identifier' => $pendingFix->error->getIdentifier(), + ]; } continue; } - if (isset($conflictClusters[$nodeId])) { - $skipReasonByErrorId[spl_object_id($pendingFix->error)] = - self::formatConflictReport($conflictClusters[$nodeId]); + if (!isset($conflictClusters[$nodeId])) { + continue; } + + $skipReasonByErrorId[spl_object_id($pendingFix->error)] + = self::formatConflictReport($conflictClusters[$nodeId]); + } + + if ($diff === null || $errorRefs === []) { + continue; } + + $fixesByFixingFile[$filePath] = new FileFix($diff, $errorRefs); } - return new FinalizedPendingFixes($diffsByErrorId, $skipReasonByErrorId); + return new FinalizedPendingFixes($fixesByFixingFile, $skipReasonByErrorId); } /** @@ -304,20 +292,6 @@ private static function joinClassList(array $names): string return $noun . ' ' . implode(', ', $head) . ', and ' . $last; } - /** - * @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 */ diff --git a/src/Command/AnalyseApplication.php b/src/Command/AnalyseApplication.php index e78212f492c..77f3caf732a 100644 --- a/src/Command/AnalyseApplication.php +++ b/src/Command/AnalyseApplication.php @@ -74,6 +74,7 @@ public function analyse( $ignoredErrorHelperResult = $this->ignoredErrorHelper->initialize(); $fileSpecificErrors = []; + $fixesByFixingFile = []; if (count($ignoredErrorHelperResult->getErrors()) > 0) { $notFileSpecificErrors = $ignoredErrorHelperResult->getErrors(); $internalErrors = []; @@ -123,6 +124,8 @@ public function analyse( reachedInternalErrorsCountLimit: $intermediateAnalyserResult->hasReachedInternalErrorsCountLimit(), peakMemoryUsageBytes: $intermediateAnalyserResult->getPeakMemoryUsageBytes(), processedFiles: $intermediateAnalyserResult->getProcessedFiles(), + fixesByFixingFile: $intermediateAnalyserResult->getFixesByFixingFile(), + perAnalysedFileFixes: $intermediateAnalyserResult->getPerAnalysedFileFixes(), ); } @@ -174,6 +177,7 @@ public function analyse( $notFileSpecificErrors = $ignoredErrorHelperProcessedResult->getOtherIgnoreMessages(); $collectedData = $analyserResult->getCollectedData(); $savedResultCache = $resultCacheResult->isSaved(); + $fixesByFixingFile = $analyserResult->getFixesByFixingFile(); } return new AnalysisResult( @@ -189,6 +193,7 @@ public function analyse( $isResultCacheUsed, $changedProjectExtensionFilesOutsideOfAnalysedPaths, $processedFiles, + $fixesByFixingFile, ); } @@ -353,6 +358,8 @@ private function switchTmpFileInAnalyserResult( reachedInternalErrorsCountLimit: $analyserResult->hasReachedInternalErrorsCountLimit(), peakMemoryUsageBytes: $analyserResult->getPeakMemoryUsageBytes(), processedFiles: $analyserResult->getProcessedFiles(), + fixesByFixingFile: $analyserResult->getFixesByFixingFile(), + perAnalysedFileFixes: $analyserResult->getPerAnalysedFileFixes(), ); } diff --git a/src/Command/AnalyseCommand.php b/src/Command/AnalyseCommand.php index d5a89410edc..030c2437962 100644 --- a/src/Command/AnalyseCommand.php +++ b/src/Command/AnalyseCommand.php @@ -4,6 +4,8 @@ use OndraM\CiDetector\CiDetector; use Override; +use PHPStan\Analyser\Error; +use PHPStan\Analyser\FileFix; use PHPStan\Analyser\InternalError; use PHPStan\Command\ErrorFormatter\BaselineNeonErrorFormatter; use PHPStan\Command\ErrorFormatter\BaselinePhpErrorFormatter; @@ -494,6 +496,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $analysisResult->isResultCacheUsed(), $analysisResult->getChangedProjectExtensionFilesOutsideOfAnalysedPaths(), $analysisResult->getProcessedFiles(), + $analysisResult->getFixesByFixingFile(), ); $exitCode = $errorFormatter->formatErrors($analysisResult, $inceptionResult->getStdOutput()); @@ -513,60 +516,41 @@ protected function execute(InputInterface $input, OutputInterface $output): int } if ($fix) { - $fixableErrors = []; + $fixesByFixingFile = $analysisResult->getFixesByFixingFile(); + $fixedErrorCount = 0; + $patcherSkippedErrorCount = 0; + + $patcher = $container->getByType(Patcher::class); + foreach ($fixesByFixingFile as $fixingFile => $fileFix) { + try { + $finalFileContents = $patcher->applyDiffs($fixingFile, [$fileFix->diff]); + FileWriter::write($fixingFile, $finalFileContents); + $fixedErrorCount += count($fileFix->errorRefs); + } catch (FileChangedException | MergeConflictException) { + $patcherSkippedErrorCount += count($fileFix->errorRefs); + } + } + $preSkippedErrors = []; - foreach ($analysisResult->getFileSpecificErrors() as $fileSpecificError) { - if ($fileSpecificError->getFixedErrorDiff() !== null) { - $fixableErrors[] = $fileSpecificError; + foreach ($analysisResult->getFileSpecificErrors() as $error) { + if (!$error->wasFixable()) { continue; } - if ($fileSpecificError->wasFixable()) { - $preSkippedErrors[] = $fileSpecificError; + if (self::errorIsCoveredByAnyFix($error, $fixesByFixingFile)) { + continue; } + $preSkippedErrors[] = $error; } - - $fixableErrorsCount = count($fixableErrors); $preSkippedCount = count($preSkippedErrors); - if ($fixableErrorsCount === 0 && $preSkippedCount === 0) { + + if ($fixedErrorCount === 0 && $patcherSkippedErrorCount === 0 && $preSkippedCount === 0) { $inceptionResult->getStdOutput()->getStyle()->error('No fixable errors found'); $exitCode = 1; } else { - $skippedCount = 0; - $diffsByFile = []; - foreach ($fixableErrors as $fixableError) { - $fixFile = $fixableError->getFilePath(); - if ($fixableError->getTraitFilePath() !== null) { - $fixFile = $fixableError->getTraitFilePath(); - } - - if ($fixableError->getFixedErrorDiff() === null) { - throw new ShouldNotHappenException(); - } - - $diffsByFile[$fixFile][] = $fixableError->getFixedErrorDiff(); - } - - if ($fixableErrorsCount > 0) { + if ($fixedErrorCount > 0) { $inceptionResult->getErrorOutput()->writeLineFormatted('Fixing errors...'); - $errorOutput->getStyle()->progressStart($fixableErrorsCount); - } - - $patcher = $container->getByType(Patcher::class); - foreach ($diffsByFile as $file => $diffs) { - $diffsCount = count($diffs); - try { - $finalFileContents = $patcher->applyDiffs($file, $diffs); - $errorOutput->getStyle()->progressAdvance($diffsCount); - } catch (FileChangedException | MergeConflictException) { - $skippedCount += $diffsCount; - $errorOutput->getStyle()->progressAdvance($diffsCount); - continue; - } - - FileWriter::write($file, $finalFileContents); - } - - if ($fixableErrorsCount > 0) { + $errorOutput->getStyle()->progressStart($fixedErrorCount); + $errorOutput->getStyle()->progressAdvance($fixedErrorCount); $errorOutput->getStyle()->progressFinish(); } @@ -584,16 +568,17 @@ protected function execute(InputInterface $input, OutputInterface $output): int false, [], [], + [], ); $errorFormatter->formatErrors($skippedAnalysisResult, $inceptionResult->getStdOutput()); } - $totalSkipped = $skippedCount + $preSkippedCount; + $totalSkipped = $patcherSkippedErrorCount + $preSkippedCount; if ($totalSkipped > 0) { $inceptionResult->getStdOutput()->getStyle()->warning(sprintf( '%d %s fixed, %d %s skipped', - $fixableErrorsCount, - $fixableErrorsCount === 1 ? 'error' : 'errors', + $fixedErrorCount, + $fixedErrorCount === 1 ? 'error' : 'errors', $totalSkipped, $totalSkipped === 1 ? 'error' : 'errors', )); @@ -601,8 +586,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int } else { $inceptionResult->getStdOutput()->getStyle()->success(sprintf( '%d %s fixed', - $fixableErrorsCount, - $fixableErrorsCount === 1 ? 'error' : 'errors', + $fixedErrorCount, + $fixedErrorCount === 1 ? 'error' : 'errors', )); $exitCode = 0; } @@ -696,6 +681,25 @@ private function createStreamOutput(): StreamOutput return new StreamOutput($resource); } + /** + * @param array $fixesByFixingFile + */ + private static function errorIsCoveredByAnyFix(Error $error, array $fixesByFixingFile): bool + { + $fixingFile = $error->getTraitFilePath() ?? $error->getFilePath(); + if (!isset($fixesByFixingFile[$fixingFile])) { + return false; + } + $identifier = $error->getIdentifier(); + $line = $error->getLine(); + foreach ($fixesByFixingFile[$fixingFile]->errorRefs as $ref) { + if ($ref['line'] === $line && $ref['identifier'] === $identifier) { + return true; + } + } + return false; + } + private function getMessageFromInternalError(FileHelper $fileHelper, InternalError $internalError, int $verbosity): string { $message = sprintf('%s while %s', $internalError->getMessage(), $internalError->getContextDescription()); diff --git a/src/Command/AnalysisResult.php b/src/Command/AnalysisResult.php index bdcb5fd1961..fe87771c68d 100644 --- a/src/Command/AnalysisResult.php +++ b/src/Command/AnalysisResult.php @@ -3,6 +3,7 @@ namespace PHPStan\Command; use PHPStan\Analyser\Error; +use PHPStan\Analyser\FileFix; use PHPStan\Analyser\InternalError; use PHPStan\Collectors\CollectedData; use function count; @@ -25,6 +26,7 @@ final class AnalysisResult * @param list $collectedData * @param array $changedProjectExtensionFilesOutsideOfAnalysedPaths * @param list $processedFiles + * @param array $fixesByFixingFile */ public function __construct( array $fileSpecificErrors, @@ -39,6 +41,7 @@ public function __construct( private bool $isResultCacheUsed, private array $changedProjectExtensionFilesOutsideOfAnalysedPaths, private array $processedFiles = [], + private array $fixesByFixingFile = [], ) { usort( @@ -158,6 +161,14 @@ public function getProcessedFiles(): array return $this->processedFiles; } + /** + * @return array + */ + public function getFixesByFixingFile(): array + { + return $this->fixesByFixingFile; + } + /** * @api * @param list $fileSpecificErrors @@ -177,6 +188,7 @@ public function withFileSpecificErrors(array $fileSpecificErrors): self $this->isResultCacheUsed, $this->changedProjectExtensionFilesOutsideOfAnalysedPaths, $this->processedFiles, + $this->fixesByFixingFile, ); } diff --git a/src/Fixable/ReplacingNodeVisitor.php b/src/Fixable/ReplacingNodeVisitor.php deleted file mode 100644 index 33be2e7fa0e..00000000000 --- a/src/Fixable/ReplacingNodeVisitor.php +++ /dev/null @@ -1,47 +0,0 @@ -getAttribute('origNode'); - if ($origNode !== $this->originalNode) { - return null; - } - - $this->found = true; - - $callable = $this->newNodeCallable; - $newNode = $callable($node); - if ($newNode instanceof VirtualNode) { - throw new ShouldNotHappenException('Cannot print VirtualNode.'); - } - - return $newNode; - } - - public function isFound(): bool - { - return $this->found; - } - -} diff --git a/src/Testing/RuleTestCase.php b/src/Testing/RuleTestCase.php index 7000fe94fc0..19d78acd7e5 100644 --- a/src/Testing/RuleTestCase.php +++ b/src/Testing/RuleTestCase.php @@ -4,11 +4,13 @@ use PhpParser\Node; use PHPStan\Analyser\Analyser; +use PHPStan\Analyser\AnalyserResult; use PHPStan\Analyser\AnalyserResultFinalizer; use PHPStan\Analyser\Error; use PHPStan\Analyser\ExprHandler\Helper\ImplicitToStringCallHelper; use PHPStan\Analyser\Fiber\FiberNodeScopeResolver; use PHPStan\Analyser\FileAnalyser; +use PHPStan\Analyser\FileFix; use PHPStan\Analyser\IgnoreErrorExtensionProvider; use PHPStan\Analyser\InternalError; use PHPStan\Analyser\LocalIgnoresProcessor; @@ -259,13 +261,14 @@ static function (Error $error) use ($strictlyTypedSprintf): string { public function fix(string $file, string $expectedFile): void { - [$errors] = $this->gatherAnalyserErrorsWithDelayedErrors([$file]); + [, , $analyserResult] = $this->gatherAnalyserErrorsWithDelayedErrors([$file]); + $normalizedFile = $this->getFileHelper()->normalizePath($file); $diffs = []; - foreach ($errors as $error) { - if ($error->getFixedErrorDiff() === null) { + foreach ($analyserResult->getFixesByFixingFile() as $fixingFile => $fileFix) { + if ($fixingFile !== $normalizedFile) { continue; } - $diffs[] = $error->getFixedErrorDiff(); + $diffs[] = $fileFix->diff; } $patcher = self::getContainer()->getByType(Patcher::class); @@ -292,7 +295,17 @@ public function gatherAnalyserErrors(array $files): array /** * @param string[] $files - * @return array{list, list} + * @return array{list, array} + */ + public function gatherAnalyserErrorsAndFixes(array $files): array + { + [$errors, , $analyserResult] = $this->gatherAnalyserErrorsWithDelayedErrors($files); + return [$errors, $analyserResult->getFixesByFixingFile()]; + } + + /** + * @param string[] $files + * @return array{list, list, AnalyserResult} */ private function gatherAnalyserErrorsWithDelayedErrors(array $files): array { @@ -331,9 +344,12 @@ private function gatherAnalyserErrorsWithDelayedErrors(array $files): array true, ); + $finalizedResult = $finalizer->finalize($analyserResult, false, true)->getAnalyserResult(); + return [ - $finalizer->finalize($analyserResult, false, true)->getAnalyserResult()->getUnorderedErrors(), + $finalizedResult->getUnorderedErrors(), array_merge($classRule->getDelayedErrors(), $traitRule->getDelayedErrors()), + $finalizedResult, ]; } diff --git a/tests/PHPStan/Analyser/FileFixTest.php b/tests/PHPStan/Analyser/FileFixTest.php new file mode 100644 index 00000000000..ca3d7fc4524 --- /dev/null +++ b/tests/PHPStan/Analyser/FileFixTest.php @@ -0,0 +1,30 @@ + 12, 'identifier' => 'rule.id'], + ['line' => 24, 'identifier' => null], + ]; + + $original = new FileFix($diff, $errorRefs); + + $exported = var_export($original, true); + $reconstructed = eval('return ' . $exported . ';'); + + self::assertInstanceOf(FileFix::class, $reconstructed); + self::assertSame($diff->originalHash, $reconstructed->diff->originalHash); + self::assertSame($diff->diff, $reconstructed->diff->diff); + self::assertSame($errorRefs, $reconstructed->errorRefs); + } + +} diff --git a/tests/PHPStan/Analyser/ResultCache/FileFixAggregatorTest.php b/tests/PHPStan/Analyser/ResultCache/FileFixAggregatorTest.php new file mode 100644 index 00000000000..8b37983ec04 --- /dev/null +++ b/tests/PHPStan/Analyser/ResultCache/FileFixAggregatorTest.php @@ -0,0 +1,93 @@ + 10, 'identifier' => 'rule.id']]); + + $result = FileFixAggregator::aggregate([ + 'A.php' => ['T.php' => $fileFix], + ]); + + self::assertArrayHasKey('T.php', $result); + self::assertCount(1, $result); + self::assertSame($fileFix, $result['T.php']); + } + + public function testTwoAnalysedFilesAgreeingDiffMergesErrorRefsDeduped(): void + { + $diff = new FixedErrorDiff('hash-A', 'diff-body-A'); + $fileFixA = new FileFix($diff, [['line' => 10, 'identifier' => 'rule.id']]); + $fileFixB = new FileFix($diff, [['line' => 10, 'identifier' => 'rule.id']]); + + $result = FileFixAggregator::aggregate([ + 'A.php' => ['T.php' => $fileFixA], + 'B.php' => ['T.php' => $fileFixB], + ]); + + self::assertArrayHasKey('T.php', $result); + self::assertSame($diff, $result['T.php']->diff); + self::assertCount(1, $result['T.php']->errorRefs); + self::assertSame(['line' => 10, 'identifier' => 'rule.id'], $result['T.php']->errorRefs[0]); + } + + public function testTwoAnalysedFilesAgreeingDiffMergesDistinctErrorRefs(): void + { + $diff = new FixedErrorDiff('hash-A', 'diff-body-A'); + $fileFixA = new FileFix($diff, [['line' => 10, 'identifier' => 'rule.id']]); + $fileFixB = new FileFix($diff, [['line' => 20, 'identifier' => 'rule.other']]); + + $result = FileFixAggregator::aggregate([ + 'A.php' => ['T.php' => $fileFixA], + 'B.php' => ['T.php' => $fileFixB], + ]); + + self::assertArrayHasKey('T.php', $result); + self::assertCount(2, $result['T.php']->errorRefs); + } + + public function testTwoAnalysedFilesDisagreeingDiffDropsTheFixingFile(): void + { + $diffA = new FixedErrorDiff('hash-A', 'diff-body-A'); + $diffB = new FixedErrorDiff('hash-B', 'diff-body-B'); + $fileFixA = new FileFix($diffA, [['line' => 10, 'identifier' => 'rule.id']]); + $fileFixB = new FileFix($diffB, [['line' => 10, 'identifier' => 'rule.id']]); + + $result = FileFixAggregator::aggregate([ + 'A.php' => ['T.php' => $fileFixA], + 'B.php' => ['T.php' => $fileFixB], + ]); + + self::assertArrayNotHasKey('T.php', $result); + } + + public function testDifferentFixingFilesPassedThrough(): void + { + $diff1 = new FixedErrorDiff('hash-1', 'diff-1'); + $diff2 = new FixedErrorDiff('hash-2', 'diff-2'); + + $result = FileFixAggregator::aggregate([ + 'A.php' => ['T1.php' => new FileFix($diff1, [['line' => 1, 'identifier' => 'r.a']])], + 'B.php' => ['T2.php' => new FileFix($diff2, [['line' => 2, 'identifier' => 'r.b']])], + ]); + + self::assertArrayHasKey('T1.php', $result); + self::assertArrayHasKey('T2.php', $result); + self::assertCount(2, $result); + } + +} diff --git a/tests/PHPStan/Fixable/PerFileBatchFixerTraitTest.php b/tests/PHPStan/Fixable/PerFileBatchFixerTraitTest.php index daa3f60b9d7..bd378e8d0c7 100644 --- a/tests/PHPStan/Fixable/PerFileBatchFixerTraitTest.php +++ b/tests/PHPStan/Fixable/PerFileBatchFixerTraitTest.php @@ -3,16 +3,19 @@ namespace PHPStan\Fixable; use Override; +use PhpParser\Node\Expr\Variable; +use PHPStan\Analyser\Error; +use PHPStan\Analyser\FileFix; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; /** - * @extends RuleTestCase> + * @extends RuleTestCase> */ final class PerFileBatchFixerTraitTest extends RuleTestCase { - /** @var Rule<\PhpParser\Node\Expr\Variable> */ + /** @var Rule */ private Rule $rule; #[Override] @@ -65,13 +68,13 @@ public function testDisagreeingConsumersErrorIsMarkedFixableWithConflictTip(): v { $this->rule = new ClassAwareRenameVariableFixRule(); - $errors = $this->gatherAnalyserErrors([__DIR__ . '/data/trait-disagreeing-consumers.php']); - $skipped = self::filterFixableSkipped($errors); + [$errors, $fixesByFixingFile] = $this->gatherAnalyserErrorsAndFixes([__DIR__ . '/data/trait-disagreeing-consumers.php']); + $skipped = self::filterFixableSkipped($errors, $fixesByFixingFile); self::assertNotSame([], $skipped, 'expected at least one wasFixable && !applied error'); foreach ($skipped as $error) { self::assertTrue($error->wasFixable()); - self::assertNull($error->getFixedErrorDiff()); + self::assertFalse(self::errorIsCoveredByAnyFix($error, $fixesByFixingFile)); $tip = $error->getTip(); self::assertNotNull($tip); self::assertStringContainsString('Auto-fix skipped: trait consumers proposed conflicting rewrites.', $tip); @@ -84,11 +87,11 @@ public function testSingleConsumerErrorIsAppliedAndHasNoSkipTip(): void { $this->rule = new RenameVariableFixRule(); - $errors = $this->gatherAnalyserErrors([__DIR__ . '/data/trait-single-consumer.php']); + [$errors, $fixesByFixingFile] = $this->gatherAnalyserErrorsAndFixes([__DIR__ . '/data/trait-single-consumer.php']); $applied = []; foreach ($errors as $error) { - if ($error->getFixedErrorDiff() === null) { + if (!self::errorIsCoveredByAnyFix($error, $fixesByFixingFile)) { continue; } $applied[] = $error; @@ -106,17 +109,18 @@ public function testSingleConsumerErrorIsAppliedAndHasNoSkipTip(): void } /** - * @param list<\PHPStan\Analyser\Error> $errors - * @return list<\PHPStan\Analyser\Error> + * @param list $errors + * @param array $fixesByFixingFile + * @return list */ - private static function filterFixableSkipped(array $errors): array + private static function filterFixableSkipped(array $errors, array $fixesByFixingFile): array { $skipped = []; foreach ($errors as $error) { if (!$error->wasFixable()) { continue; } - if ($error->getFixedErrorDiff() !== null) { + if (self::errorIsCoveredByAnyFix($error, $fixesByFixingFile)) { continue; } $skipped[] = $error; @@ -125,4 +129,23 @@ private static function filterFixableSkipped(array $errors): array return $skipped; } + /** + * @param array $fixesByFixingFile + */ + private static function errorIsCoveredByAnyFix(Error $error, array $fixesByFixingFile): bool + { + $fixingFile = $error->getTraitFilePath() ?? $error->getFilePath(); + if (!isset($fixesByFixingFile[$fixingFile])) { + return false; + } + $identifier = $error->getIdentifier(); + $line = $error->getLine(); + foreach ($fixesByFixingFile[$fixingFile]->errorRefs as $ref) { + if ($ref['line'] === $line && $ref['identifier'] === $identifier) { + return true; + } + } + return false; + } + } From c78f1d744af5ee98ba94e3ef68edf43efbd9fb00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Barto=C5=A1?= Date: Sun, 3 May 2026 01:59:48 +0200 Subject: [PATCH 5/5] Parallel-mode FileFix transport --- src/Analyser/FileFix.php | 27 ++++++++++++++++++++++++++- src/Analyser/FixedErrorDiff.php | 27 ++++++++++++++++++++++++++- src/Command/WorkerCommand.php | 7 +++++++ src/Parallel/ParallelAnalyser.php | 20 +++++++++++++++++--- 4 files changed, 76 insertions(+), 5 deletions(-) diff --git a/src/Analyser/FileFix.php b/src/Analyser/FileFix.php index 35bc223de16..f1086177eaa 100644 --- a/src/Analyser/FileFix.php +++ b/src/Analyser/FileFix.php @@ -2,10 +2,14 @@ namespace PHPStan\Analyser; +use JsonSerializable; +use Override; +use ReturnTypeWillChange; + /** * @internal */ -final class FileFix +final class FileFix implements JsonSerializable { /** @@ -26,4 +30,25 @@ public static function __set_state(array $properties): self return new self($properties['diff'], $properties['errorRefs']); } + /** + * @return array{diff: FixedErrorDiff, errorRefs: list} + */ + #[ReturnTypeWillChange] + #[Override] + public function jsonSerialize() + { + return [ + 'diff' => $this->diff, + 'errorRefs' => $this->errorRefs, + ]; + } + + /** + * @param array{diff: array{originalHash: string, diff: string}, errorRefs: list} $json + */ + public static function decode(array $json): self + { + return new self(FixedErrorDiff::decode($json['diff']), $json['errorRefs']); + } + } diff --git a/src/Analyser/FixedErrorDiff.php b/src/Analyser/FixedErrorDiff.php index af8755b2f88..616bc853132 100644 --- a/src/Analyser/FixedErrorDiff.php +++ b/src/Analyser/FixedErrorDiff.php @@ -2,7 +2,11 @@ namespace PHPStan\Analyser; -final class FixedErrorDiff +use JsonSerializable; +use Override; +use ReturnTypeWillChange; + +final class FixedErrorDiff implements JsonSerializable { public function __construct( @@ -20,4 +24,25 @@ public static function __set_state(array $properties): self return new self($properties['originalHash'], $properties['diff']); } + /** + * @return array{originalHash: string, diff: string} + */ + #[ReturnTypeWillChange] + #[Override] + public function jsonSerialize() + { + return [ + 'originalHash' => $this->originalHash, + 'diff' => $this->diff, + ]; + } + + /** + * @param array{originalHash: string, diff: string} $json + */ + public static function decode(array $json): self + { + return new self($json['originalHash'], $json['diff']); + } + } diff --git a/src/Command/WorkerCommand.php b/src/Command/WorkerCommand.php index d5f42b87956..c60fbd486db 100644 --- a/src/Command/WorkerCommand.php +++ b/src/Command/WorkerCommand.php @@ -200,6 +200,7 @@ private function runWorker( 'dependencies' => [], 'exportedNodes' => [], 'files' => [], + 'perAnalysedFileFixes' => [], 'internalErrorsCount' => 1, ], ]); @@ -229,6 +230,7 @@ private function runWorker( $usedTraitDependencies = []; $exportedNodes = []; $processedFiles = []; + $perAnalysedFileFixes = []; foreach ($files as $file) { try { if ($file === $insteadOfFile) { @@ -244,6 +246,10 @@ private function runWorker( $usedTraitDependencies[$file] = $fileAnalyserResult->getUsedTraitDependencies(); $exportedNodes[$file] = $fileAnalyserResult->getExportedNodes(); $processedFiles = array_merge($processedFiles, $fileAnalyserResult->getProcessedFiles()); + $fileFixes = $fileAnalyserResult->getFixesByFixingFile(); + if ($fileFixes !== []) { + $perAnalysedFileFixes[$file] = $fileFixes; + } foreach ($fileErrors as $fileError) { $errors[] = $fileError; } @@ -286,6 +292,7 @@ private function runWorker( 'exportedNodes' => $exportedNodes, 'files' => $files, 'processedFiles' => $processedFiles, + 'perAnalysedFileFixes' => $perAnalysedFileFixes, 'internalErrorsCount' => $internalErrorsCount, ]]); }); diff --git a/src/Parallel/ParallelAnalyser.php b/src/Parallel/ParallelAnalyser.php index 248b52a4a34..67545ab968f 100644 --- a/src/Parallel/ParallelAnalyser.php +++ b/src/Parallel/ParallelAnalyser.php @@ -8,7 +8,9 @@ use Nette\Utils\Random; use PHPStan\Analyser\AnalyserResult; use PHPStan\Analyser\Error; +use PHPStan\Analyser\FileFix; use PHPStan\Analyser\InternalError; +use PHPStan\Analyser\ResultCache\FileFixAggregator; use PHPStan\Dependency\RootExportedNode; use PHPStan\DependencyInjection\AutowiredParameter; use PHPStan\DependencyInjection\AutowiredService; @@ -94,12 +96,14 @@ public function analyse( $exportedNodes = []; /** @var list $allProcessedFiles */ $allProcessedFiles = []; + /** @var array> $perAnalysedFileFixes */ + $perAnalysedFileFixes = []; /** @var Deferred $deferred */ $deferred = new Deferred(); $server = new TcpServer('127.0.0.1:0', $loop); - $this->processPool = new ProcessPool($server, static function () use ($deferred, &$jobs, &$internalErrors, &$internalErrorsCount, &$reachedInternalErrorsCountLimit, &$errors, &$filteredPhpErrors, &$allPhpErrors, &$locallyIgnoredErrors, &$linesToIgnore, &$unmatchedLineIgnores, &$collectedData, &$dependencies, &$usedTraitDependencies, &$exportedNodes, &$peakMemoryUsages, &$allProcessedFiles): void { + $this->processPool = new ProcessPool($server, static function () use ($deferred, &$jobs, &$internalErrors, &$internalErrorsCount, &$reachedInternalErrorsCountLimit, &$errors, &$filteredPhpErrors, &$allPhpErrors, &$locallyIgnoredErrors, &$linesToIgnore, &$unmatchedLineIgnores, &$collectedData, &$dependencies, &$usedTraitDependencies, &$exportedNodes, &$peakMemoryUsages, &$allProcessedFiles, &$perAnalysedFileFixes): void { if (count($jobs) > 0 && $internalErrorsCount === 0) { $internalErrors[] = new InternalError( 'Some parallel worker jobs have not finished.', @@ -124,8 +128,10 @@ public function analyse( usedTraitDependencies: $internalErrorsCount === 0 ? $usedTraitDependencies : null, exportedNodes: $exportedNodes, reachedInternalErrorsCountLimit: $reachedInternalErrorsCountLimit, - peakMemoryUsageBytes: array_sum($peakMemoryUsages), // not 100% correct as the peak usages of workers might not have met + peakMemoryUsageBytes: array_sum($peakMemoryUsages), processedFiles: $allProcessedFiles, + fixesByFixingFile: FileFixAggregator::aggregate($perAnalysedFileFixes), + perAnalysedFileFixes: $perAnalysedFileFixes, )); }); $server->on('connection', function (ConnectionInterface $connection) use (&$jobs): void { @@ -197,7 +203,7 @@ public function analyse( $commandOptions, $input, ), $loop, $this->processTimeout); - $process->start(function (array $json) use ($process, &$internalErrors, &$errors, &$filteredPhpErrors, &$allPhpErrors, &$locallyIgnoredErrors, &$linesToIgnore, &$unmatchedLineIgnores, &$collectedData, &$dependencies, &$usedTraitDependencies, &$exportedNodes, &$peakMemoryUsages, &$jobs, $postFileCallback, &$internalErrorsCount, &$reachedInternalErrorsCountLimit, $processIdentifier, $onFileAnalysisHandler, &$allProcessedFiles): void { + $process->start(function (array $json) use ($process, &$internalErrors, &$errors, &$filteredPhpErrors, &$allPhpErrors, &$locallyIgnoredErrors, &$linesToIgnore, &$unmatchedLineIgnores, &$collectedData, &$dependencies, &$usedTraitDependencies, &$exportedNodes, &$peakMemoryUsages, &$jobs, $postFileCallback, &$internalErrorsCount, &$reachedInternalErrorsCountLimit, $processIdentifier, $onFileAnalysisHandler, &$allProcessedFiles, &$perAnalysedFileFixes): void { $fileErrors = []; foreach ($json['errors'] as $jsonError) { $fileErrors[] = Error::decode($jsonError); @@ -288,6 +294,14 @@ public function analyse( $allProcessedFiles[] = $processedFile; } + foreach ($json['perAnalysedFileFixes'] as $analysedFile => $fixesByFixingFile) { + $decoded = []; + foreach ($fixesByFixingFile as $fixingFile => $jsonFileFix) { + $decoded[$fixingFile] = FileFix::decode($jsonFileFix); + } + $perAnalysedFileFixes[$analysedFile] = $decoded; + } + if ($postFileCallback !== null) { $postFileCallback(count($json['files']), $json['processedFiles']); }