diff --git a/src/Console/Command/ProcessCommand.php b/src/Console/Command/ProcessCommand.php index c2af0b54ad0..93254c835da 100644 --- a/src/Console/Command/ProcessCommand.php +++ b/src/Console/Command/ProcessCommand.php @@ -202,6 +202,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->missConfigurationReporter->reportSkippedNonRectorClasses(); $this->missConfigurationReporter->reportUnusedSkips($processResult); + // 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 3a137588b65..7fd09c02cd3 100644 --- a/src/Reporting/MissConfigurationReporter.php +++ b/src/Reporting/MissConfigurationReporter.php @@ -21,6 +21,7 @@ public function __construct( private SymfonyStyle $symfonyStyle, private VendorMissAnalyseGuard $vendorMissAnalyseGuard, private UnusedSkipResolver $unusedSkipResolver, + private NeededSkipVerifier $neededSkipVerifier, ) { } @@ -47,6 +48,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()". The re-run is forced to a dry run, + * so it is safe on an apply run too. + */ + public function reportUnnecessarySkips(): void + { + $unnecessarySkips = $this->neededSkipVerifier->verify(); + 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..0234c291faf --- /dev/null +++ b/src/Reporting/NeededSkipVerifier.php @@ -0,0 +1,114 @@ +listing()". + * + * @return string[] + */ + public function verify(): array + { + $skippedFilePathsByRule = $this->skipper->provideSkippedFilePathsByRule(); + if ($skippedFilePathsByRule === []) { + return []; + } + + $changedRulesByFilePath = $this->resolveChangedRulesByFilePath($skippedFilePathsByRule); + + $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): array + { + $uniqueFilePaths = []; + foreach ($skippedFilePathsByRule as $filePaths) { + foreach ($filePaths as $filePath) { + $uniqueFilePaths[$filePath] = true; + } + } + + // 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 = []; + try { + foreach (array_keys($uniqueFilePaths) as $filePath) { + $file = new File($filePath, FileSystem::read($filePath)); + $this->currentFileProvider->setFile($file); + + $fileProcessResult = $this->fileProcessor->processFile($file, $dryRunConfiguration); + + // 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..6649800135b --- /dev/null +++ b/tests/Reporting/NeededSkipVerifierTest.php @@ -0,0 +1,88 @@ +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(); + + // 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()); + } + + 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; + } +}