From 2695b653301ca36bcce7cc6bf76d77526143b632 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Mon, 26 Oct 2026 17:17:56 +0100 Subject: [PATCH 1/2] Verify matched skips by re-running the rule, report unnecessary ones reportUnusedSkips() so far only flagged a skip whose path never matched any file. A skip can also match a file while the rule would not have changed it anyway - the clone-probe used to catch this per node, removed for performance. On a dry run, re-run each rule-scoped skip on its exact file with skipping off; if the rule still does not change the file, report the skip as unnecessary. Paid once per skipped file, only when reporting is enabled. --- src/Console/Command/ProcessCommand.php | 5 + src/Reporting/MissConfigurationReporter.php | 29 +++++ src/Reporting/NeededSkipVerifier.php | 111 ++++++++++++++++++ src/Skipper/Skipper/Skipper.php | 51 ++++++++ .../Command/ValidateConfigCommandTest.php | 2 + .../MissConfigurationReporterTest.php | 2 + tests/Reporting/NeededSkipVerifierTest.php | 86 ++++++++++++++ .../Reporting/Source/MakeClassFinalRector.php | 40 +++++++ 8 files changed, 326 insertions(+) create mode 100644 src/Reporting/NeededSkipVerifier.php create mode 100644 tests/Reporting/NeededSkipVerifierTest.php create mode 100644 tests/Reporting/Source/MakeClassFinalRector.php diff --git a/src/Console/Command/ProcessCommand.php b/src/Console/Command/ProcessCommand.php index c2af0b54ad0..6c43479092d 100644 --- a/src/Console/Command/ProcessCommand.php +++ b/src/Console/Command/ProcessCommand.php @@ -202,6 +202,11 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->missConfigurationReporter->reportSkippedNonRectorClasses(); $this->missConfigurationReporter->reportUnusedSkips($processResult); + // re-runs skipped rules on their files, so only safe on a dry run that never writes + if ($configuration->isDryRun()) { + $this->missConfigurationReporter->reportUnnecessarySkips($configuration); + } + return $this->resolveReturnCode($processResult, $configuration); } diff --git a/src/Reporting/MissConfigurationReporter.php b/src/Reporting/MissConfigurationReporter.php index 3a137588b65..280ff8c298f 100644 --- a/src/Reporting/MissConfigurationReporter.php +++ b/src/Reporting/MissConfigurationReporter.php @@ -9,6 +9,7 @@ use Rector\Configuration\VendorMissAnalyseGuard; use Rector\Contract\Rector\RectorInterface; use Rector\PostRector\Contract\Rector\PostRectorInterface; +use Rector\ValueObject\Configuration; use Rector\ValueObject\ProcessResult; use Symfony\Component\Console\Style\SymfonyStyle; @@ -21,6 +22,7 @@ public function __construct( private SymfonyStyle $symfonyStyle, private VendorMissAnalyseGuard $vendorMissAnalyseGuard, private UnusedSkipResolver $unusedSkipResolver, + private NeededSkipVerifier $neededSkipVerifier, ) { } @@ -47,6 +49,33 @@ public function reportUnusedSkips(ProcessResult $processResult): void $this->symfonyStyle->listing($spacedUnusedSkips); } + /** + * Reports rule-scoped skips that matched a file but whose rule would not have changed it, so the + * skip is unnecessary. Enabled with "->reportUnusedSkips()" and only on dry runs, as it re-runs + * the rules on the skipped files. + */ + public function reportUnnecessarySkips(Configuration $configuration): void + { + $unnecessarySkips = $this->neededSkipVerifier->verify($configuration); + if ($unnecessarySkips === []) { + return; + } + + $this->symfonyStyle->warning(sprintf( + '%s matched a file, but the rule would not have changed it. You can remove %s from "->withSkip()"', + count($unnecessarySkips) > 1 ? 'These skips are unnecessary, they' : 'This skip is unnecessary, it', + count($unnecessarySkips) > 1 ? 'them' : 'it' + )); + + // add a blank line between items, so grouped rule skips stay visually separated + $spacedUnnecessarySkips = array_map( + static fn (string $unnecessarySkip): string => $unnecessarySkip . "\n", + $unnecessarySkips + ); + + $this->symfonyStyle->listing($spacedUnnecessarySkips); + } + public function reportSkippedNeverRegisteredRules(): int { $registeredRules = SimpleParameterProvider::provideArrayParameter(Option::REGISTERED_RECTOR_RULES); diff --git a/src/Reporting/NeededSkipVerifier.php b/src/Reporting/NeededSkipVerifier.php new file mode 100644 index 00000000000..4ce9bdee878 --- /dev/null +++ b/src/Reporting/NeededSkipVerifier.php @@ -0,0 +1,111 @@ +listing()". + * + * @return string[] + */ + public function verify(Configuration $configuration): array + { + $skippedFilePathsByRule = $this->skipper->provideSkippedFilePathsByRule(); + if ($skippedFilePathsByRule === []) { + return []; + } + + $changedRulesByFilePath = $this->resolveChangedRulesByFilePath($skippedFilePathsByRule, $configuration); + + $unnecessarySkips = []; + foreach ($skippedFilePathsByRule as $rectorClass => $filePaths) { + $unnecessaryRelativePaths = []; + foreach ($filePaths as $filePath) { + // file could not be re-processed (e.g. parse error), keep the skip to stay safe + if (! array_key_exists($filePath, $changedRulesByFilePath)) { + continue; + } + + // the rule changed the file, so the skip really prevents a change - keep it + if (in_array($rectorClass, $changedRulesByFilePath[$filePath], true)) { + continue; + } + + $unnecessaryRelativePaths[] = $this->filePathHelper->relativePath($filePath); + } + + if ($unnecessaryRelativePaths === []) { + continue; + } + + // rule on its own line, with each unnecessary path nested below it as a sub-item + $unnecessarySkips[] = $rectorClass . ':' . "\n * " . implode("\n * ", $unnecessaryRelativePaths); + } + + return $unnecessarySkips; + } + + /** + * @param array $skippedFilePathsByRule + * @return array real file path => rule classes that changed it + */ + private function resolveChangedRulesByFilePath(array $skippedFilePathsByRule, Configuration $configuration): array + { + $uniqueFilePaths = []; + foreach ($skippedFilePathsByRule as $filePaths) { + foreach ($filePaths as $filePath) { + $uniqueFilePaths[$filePath] = true; + } + } + + $this->skipper->startVerifyingSkips(); + + $changedRulesByFilePath = []; + try { + foreach (array_keys($uniqueFilePaths) as $filePath) { + $file = new File($filePath, FileSystem::read($filePath)); + $this->currentFileProvider->setFile($file); + + $fileProcessResult = $this->fileProcessor->processFile($file, $configuration); + + // a broken file is not verifiable, leave it out so its skips are kept + if ($fileProcessResult->getSystemErrors() !== []) { + continue; + } + + $fileDiff = $fileProcessResult->getFileDiff(); + $changedRulesByFilePath[$filePath] = $fileDiff?->getRectorClasses() ?? []; + } + } finally { + $this->skipper->stopVerifyingSkips(); + } + + return $changedRulesByFilePath; + } +} diff --git a/src/Skipper/Skipper/Skipper.php b/src/Skipper/Skipper/Skipper.php index 10ace51e36f..07d55eebadb 100644 --- a/src/Skipper/Skipper/Skipper.php +++ b/src/Skipper/Skipper/Skipper.php @@ -31,6 +31,17 @@ final class Skipper */ private array $usedSkips = []; + /** + * Real file paths skipped per rule, kept only when unused-skip reporting is on, so a matched + * skip can be re-run on the exact file to learn whether it ever prevented a change. + * + * @var array> + */ + private array $skippedFilePathsByRule = []; + + // flipped on while the needed-skip verification re-runs rules with skipping off + private bool $isVerifyingSkips = false; + public function __construct( private readonly PathSkipper $pathSkipper, private readonly FileInfoMatcher $fileInfoMatcher, @@ -51,15 +62,55 @@ public function shouldSkipFilePath(string $filePath): bool public function shouldSkipRectorAndFile(object $rector, string $filePath): bool { + // verification re-runs every rule with skipping off, to learn which skips really matter + if ($this->isVerifyingSkips) { + return false; + } + $skipMatch = $this->matchSkip($rector, $filePath); if (! $skipMatch instanceof SkipMatch) { return false; } $this->markSkipUsed($skipMatch); + + // remember the real skipped file so a rule-scoped skip can be verified later; only path + // skips are re-runnable, and only when reporting is on, so normal runs pay nothing + if ($skipMatch->getMatchedPath() !== null && SimpleParameterProvider::provideBoolParameter( + Option::REPORT_UNUSED_SKIPS, + false + )) { + $this->skippedFilePathsByRule[$rector::class][$filePath] = true; + } + return true; } + public function startVerifyingSkips(): void + { + $this->isVerifyingSkips = true; + } + + public function stopVerifyingSkips(): void + { + $this->isVerifyingSkips = false; + } + + /** + * Real file paths that were skipped per rule-scoped path skip during the run. + * + * @return array + */ + public function provideSkippedFilePathsByRule(): array + { + $skippedFilePathsByRule = []; + foreach ($this->skippedFilePathsByRule as $rectorClass => $filePaths) { + $skippedFilePathsByRule[$rectorClass] = array_keys($filePaths); + } + + return $skippedFilePathsByRule; + } + /** * Match a class/path skip without marking it used. Callers that can only tell whether the skip * actually prevented a change later on must mark it used themselves via markSkipUsed(). diff --git a/tests/Console/Command/ValidateConfigCommandTest.php b/tests/Console/Command/ValidateConfigCommandTest.php index 18b723548b1..00e534c1233 100644 --- a/tests/Console/Command/ValidateConfigCommandTest.php +++ b/tests/Console/Command/ValidateConfigCommandTest.php @@ -13,6 +13,7 @@ use Rector\Php85\Rector\FuncCall\OrdSingleByteRector; use Rector\Reporting\DeprecatedRulesReporter; use Rector\Reporting\MissConfigurationReporter; +use Rector\Reporting\NeededSkipVerifier; use Rector\Reporting\UnusedSkipResolver; use Rector\Skipper\Skipper\Skipper; use Rector\Testing\PHPUnit\AbstractLazyTestCase; @@ -53,6 +54,7 @@ protected function setUp(): void $symfonyStyle, new VendorMissAnalyseGuard(), $this->make(UnusedSkipResolver::class), + $this->make(NeededSkipVerifier::class), ), $this->make(Skipper::class), ); diff --git a/tests/Reporting/MissConfigurationReporterTest.php b/tests/Reporting/MissConfigurationReporterTest.php index 50c1a8cdcc5..a0a98664545 100644 --- a/tests/Reporting/MissConfigurationReporterTest.php +++ b/tests/Reporting/MissConfigurationReporterTest.php @@ -10,6 +10,7 @@ use Rector\Php85\Rector\FuncCall\OrdSingleByteRector; use Rector\PostRector\Rector\NameImportingPostRector; use Rector\Reporting\MissConfigurationReporter; +use Rector\Reporting\NeededSkipVerifier; use Rector\Reporting\UnusedSkipResolver; use Rector\Testing\PHPUnit\AbstractLazyTestCase; use Rector\Tests\Skipper\Skipper\Fixture\Element\FifthElement; @@ -45,6 +46,7 @@ protected function setUp(): void $symfonyStyle, new VendorMissAnalyseGuard(), $this->make(UnusedSkipResolver::class), + $this->make(NeededSkipVerifier::class), ); SimpleParameterProvider::setParameter(Option::SKIP, [ diff --git a/tests/Reporting/NeededSkipVerifierTest.php b/tests/Reporting/NeededSkipVerifierTest.php new file mode 100644 index 00000000000..0a7d138471d --- /dev/null +++ b/tests/Reporting/NeededSkipVerifierTest.php @@ -0,0 +1,86 @@ +tempDirectory = sys_get_temp_dir() . '/rector_needed_skip_' . uniqid(); + } + + protected function tearDown(): void + { + FileSystem::delete($this->tempDirectory); + + SimpleParameterProvider::setParameter(Option::SKIP, []); + SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, false); + } + + public function testReportsSkipThatRuleWouldNotHaveChanged(): void + { + // the rule would turn this into "final class", so skipping it really prevents a change + $neededFilePath = $this->tempDirectory . '/needed_skip.php'; + FileSystem::write($neededFilePath, 'tempDirectory . '/unnecessary_skip.php'; + FileSystem::write($unnecessaryFilePath, ' [$neededFilePath, $unnecessaryFilePath], + ]); + + $rectorConfig = $this->createContainerWithRule(); + + // simulate the run skipping the rule on both files, so they are captured for verification + $skipper = $rectorConfig->make(Skipper::class); + $rule = $rectorConfig->make(MakeClassFinalRector::class); + $this->assertTrue($skipper->shouldSkipRectorAndFile($rule, $neededFilePath)); + $this->assertTrue($skipper->shouldSkipRectorAndFile($rule, $unnecessaryFilePath)); + + $neededSkipVerifier = $rectorConfig->make(NeededSkipVerifier::class); + $unnecessarySkips = $neededSkipVerifier->verify(new Configuration(isDryRun: true)); + + // one rule group, listing only the unnecessary file + $this->assertCount(1, $unnecessarySkips); + $this->assertStringContainsString(MakeClassFinalRector::class, $unnecessarySkips[0]); + $this->assertStringContainsString('unnecessary_skip.php', $unnecessarySkips[0]); + $this->assertStringNotContainsString('needed_skip.php', $unnecessarySkips[0]); + } + + public function testReportsNothingWithoutCapturedSkips(): void + { + $rectorConfig = $this->createContainerWithRule(); + + $neededSkipVerifier = $rectorConfig->make(NeededSkipVerifier::class); + $this->assertSame([], $neededSkipVerifier->verify(new Configuration(isDryRun: true))); + } + + private function createContainerWithRule(): RectorConfig + { + $rectorConfig = new LazyContainerFactory()->create(); + $rectorConfig->rule(MakeClassFinalRector::class); + $rectorConfig->boot(); + + return $rectorConfig; + } +} diff --git a/tests/Reporting/Source/MakeClassFinalRector.php b/tests/Reporting/Source/MakeClassFinalRector.php new file mode 100644 index 00000000000..0e7bf67099b --- /dev/null +++ b/tests/Reporting/Source/MakeClassFinalRector.php @@ -0,0 +1,40 @@ +> + */ + public function getNodeTypes(): array + { + return [Class_::class]; + } + + public function refactor(Node $node): ?Node + { + if ($node->isFinal() || $node->isAbstract()) { + return null; + } + + $node->flags |= Modifiers::FINAL; + return $node; + } +} From 7167a79f08c1a91b96d8cb16143fe3a84e503e3a Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Mon, 26 Oct 2026 17:49:32 +0100 Subject: [PATCH 2/2] Force the verification re-run to a dry run, so it also runs on apply runs The verifier re-processes skipped files only to read which rules would change them. Building its own dry-run Configuration keeps FileProcessor from writing the skipped file, so the check is safe outside a dry run too. Drop the dry-run gate in ProcessCommand. --- src/Console/Command/ProcessCommand.php | 6 ++---- src/Reporting/MissConfigurationReporter.php | 9 ++++----- src/Reporting/NeededSkipVerifier.php | 15 +++++++++------ tests/Reporting/NeededSkipVerifierTest.php | 8 +++++--- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/Console/Command/ProcessCommand.php b/src/Console/Command/ProcessCommand.php index 6c43479092d..93254c835da 100644 --- a/src/Console/Command/ProcessCommand.php +++ b/src/Console/Command/ProcessCommand.php @@ -202,10 +202,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->missConfigurationReporter->reportSkippedNonRectorClasses(); $this->missConfigurationReporter->reportUnusedSkips($processResult); - // re-runs skipped rules on their files, so only safe on a dry run that never writes - if ($configuration->isDryRun()) { - $this->missConfigurationReporter->reportUnnecessarySkips($configuration); - } + // the verification re-run is forced to a dry run, so it is safe on an apply run too + $this->missConfigurationReporter->reportUnnecessarySkips(); return $this->resolveReturnCode($processResult, $configuration); } diff --git a/src/Reporting/MissConfigurationReporter.php b/src/Reporting/MissConfigurationReporter.php index 280ff8c298f..7fd09c02cd3 100644 --- a/src/Reporting/MissConfigurationReporter.php +++ b/src/Reporting/MissConfigurationReporter.php @@ -9,7 +9,6 @@ use Rector\Configuration\VendorMissAnalyseGuard; use Rector\Contract\Rector\RectorInterface; use Rector\PostRector\Contract\Rector\PostRectorInterface; -use Rector\ValueObject\Configuration; use Rector\ValueObject\ProcessResult; use Symfony\Component\Console\Style\SymfonyStyle; @@ -51,12 +50,12 @@ public function reportUnusedSkips(ProcessResult $processResult): void /** * Reports rule-scoped skips that matched a file but whose rule would not have changed it, so the - * skip is unnecessary. Enabled with "->reportUnusedSkips()" and only on dry runs, as it re-runs - * the rules on the skipped files. + * skip is unnecessary. Enabled with "->reportUnusedSkips()". The re-run is forced to a dry run, + * so it is safe on an apply run too. */ - public function reportUnnecessarySkips(Configuration $configuration): void + public function reportUnnecessarySkips(): void { - $unnecessarySkips = $this->neededSkipVerifier->verify($configuration); + $unnecessarySkips = $this->neededSkipVerifier->verify(); if ($unnecessarySkips === []) { return; } diff --git a/src/Reporting/NeededSkipVerifier.php b/src/Reporting/NeededSkipVerifier.php index 4ce9bdee878..0234c291faf 100644 --- a/src/Reporting/NeededSkipVerifier.php +++ b/src/Reporting/NeededSkipVerifier.php @@ -14,8 +14,8 @@ /** * Re-runs each rule-scoped skip on its exact file with skipping off. A skip whose rule still does - * not change the file never prevented anything and can be removed. Runs on dry runs only, so the - * re-processing never writes to disk. + * not change the file never prevented anything and can be removed. The re-processing is forced to a + * dry run, so it never writes, no matter the mode of the real run. * * @see \Rector\Tests\Reporting\NeededSkipVerifierTest */ @@ -34,14 +34,14 @@ public function __construct( * * @return string[] */ - public function verify(Configuration $configuration): array + public function verify(): array { $skippedFilePathsByRule = $this->skipper->provideSkippedFilePathsByRule(); if ($skippedFilePathsByRule === []) { return []; } - $changedRulesByFilePath = $this->resolveChangedRulesByFilePath($skippedFilePathsByRule, $configuration); + $changedRulesByFilePath = $this->resolveChangedRulesByFilePath($skippedFilePathsByRule); $unnecessarySkips = []; foreach ($skippedFilePathsByRule as $rectorClass => $filePaths) { @@ -75,7 +75,7 @@ public function verify(Configuration $configuration): array * @param array $skippedFilePathsByRule * @return array real file path => rule classes that changed it */ - private function resolveChangedRulesByFilePath(array $skippedFilePathsByRule, Configuration $configuration): array + private function resolveChangedRulesByFilePath(array $skippedFilePathsByRule): array { $uniqueFilePaths = []; foreach ($skippedFilePathsByRule as $filePaths) { @@ -84,6 +84,9 @@ private function resolveChangedRulesByFilePath(array $skippedFilePathsByRule, Co } } + // force a dry run, so re-processing detects the change but never writes the skipped file + $dryRunConfiguration = new Configuration(isDryRun: true); + $this->skipper->startVerifyingSkips(); $changedRulesByFilePath = []; @@ -92,7 +95,7 @@ private function resolveChangedRulesByFilePath(array $skippedFilePathsByRule, Co $file = new File($filePath, FileSystem::read($filePath)); $this->currentFileProvider->setFile($file); - $fileProcessResult = $this->fileProcessor->processFile($file, $configuration); + $fileProcessResult = $this->fileProcessor->processFile($file, $dryRunConfiguration); // a broken file is not verifiable, leave it out so its skips are kept if ($fileProcessResult->getSystemErrors() !== []) { diff --git a/tests/Reporting/NeededSkipVerifierTest.php b/tests/Reporting/NeededSkipVerifierTest.php index 0a7d138471d..6649800135b 100644 --- a/tests/Reporting/NeededSkipVerifierTest.php +++ b/tests/Reporting/NeededSkipVerifierTest.php @@ -13,7 +13,6 @@ use Rector\Skipper\Skipper\Skipper; use Rector\Testing\PHPUnit\AbstractLazyTestCase; use Rector\Tests\Reporting\Source\MakeClassFinalRector; -use Rector\ValueObject\Configuration; final class NeededSkipVerifierTest extends AbstractLazyTestCase { @@ -58,13 +57,16 @@ public function testReportsSkipThatRuleWouldNotHaveChanged(): void $this->assertTrue($skipper->shouldSkipRectorAndFile($rule, $unnecessaryFilePath)); $neededSkipVerifier = $rectorConfig->make(NeededSkipVerifier::class); - $unnecessarySkips = $neededSkipVerifier->verify(new Configuration(isDryRun: true)); + $unnecessarySkips = $neededSkipVerifier->verify(); // one rule group, listing only the unnecessary file $this->assertCount(1, $unnecessarySkips); $this->assertStringContainsString(MakeClassFinalRector::class, $unnecessarySkips[0]); $this->assertStringContainsString('unnecessary_skip.php', $unnecessarySkips[0]); $this->assertStringNotContainsString('needed_skip.php', $unnecessarySkips[0]); + + // the verification is forced to a dry run, so the skipped file on disk stays untouched + $this->assertSame('createContainerWithRule(); $neededSkipVerifier = $rectorConfig->make(NeededSkipVerifier::class); - $this->assertSame([], $neededSkipVerifier->verify(new Configuration(isDryRun: true))); + $this->assertSame([], $neededSkipVerifier->verify()); } private function createContainerWithRule(): RectorConfig