[dx] Report rule-scoped skips that matched a file but never changed it

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);
     }
 

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);

src/Reporting/NeededSkipVerifier.php

@@ -0,0 +1,114 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Reporting;
+
+use Nette\Utils\FileSystem;
+use Rector\Application\FileProcessor;
+use Rector\Application\Provider\CurrentFileProvider;
+use Rector\FileSystem\FilePathHelper;
+use Rector\Skipper\Skipper\Skipper;
+use Rector\ValueObject\Application\File;
+use Rector\ValueObject\Configuration;
+
+/**
+ * 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. 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
+ */
+final readonly class NeededSkipVerifier
+{
+    public function __construct(
+        private Skipper $skipper,
+        private FileProcessor $fileProcessor,
+        private CurrentFileProvider $currentFileProvider,
+        private FilePathHelper $filePathHelper,
+    ) {
+    }
+
+    /**
+     * Unnecessary skips grouped under their rule, ready for "->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<class-string, string[]> $skippedFilePathsByRule
+     * @return array<string, string[]> 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;
+    }
+}

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<class-string, array<string, true>>
+     */
+    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<class-string, string[]>
+     */
+    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().

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),
         );

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, [

tests/Reporting/NeededSkipVerifierTest.php

@@ -0,0 +1,88 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\Reporting;
+
+use Nette\Utils\FileSystem;
+use Rector\Config\RectorConfig;
+use Rector\Configuration\Option;
+use Rector\Configuration\Parameter\SimpleParameterProvider;
+use Rector\DependencyInjection\LazyContainerFactory;
+use Rector\Reporting\NeededSkipVerifier;
+use Rector\Skipper\Skipper\Skipper;
+use Rector\Testing\PHPUnit\AbstractLazyTestCase;
+use Rector\Tests\Reporting\Source\MakeClassFinalRector;
+
+final class NeededSkipVerifierTest extends AbstractLazyTestCase
+{
+    private string $tempDirectory;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->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, '<?php class SomeClass {}', null);
+
+        // already final, so the rule changes nothing - skipping it is unnecessary
+        $unnecessaryFilePath = $this->tempDirectory . '/unnecessary_skip.php';
+        FileSystem::write($unnecessaryFilePath, '<?php final class AlreadyFinal {}', null);
+
+        SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, true);
+        SimpleParameterProvider::setParameter(Option::SKIP, [
+            MakeClassFinalRector::class => [$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('<?php class SomeClass {}', FileSystem::read($neededFilePath));
+    }
+
+    public function testReportsNothingWithoutCapturedSkips(): void
+    {
+        $rectorConfig = $this->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;
+    }
+}