[skipper] Merge UsedSkipCollector into Skipper as single service

rules-tests/Renaming/Rector/Name/RenameClassRector/SkipUsedTrackingTest.php

@@ -6,7 +6,7 @@
 
 use Nette\Utils\FileSystem;
 use Rector\Renaming\Rector\Name\RenameClassRector;
-use Rector\Skipper\Skipper\UsedSkipCollector;
+use Rector\Skipper\Skipper\Skipper;
 use Rector\Testing\PHPUnit\AbstractRectorTestCase;
 
 /**
@@ -28,8 +28,8 @@ public function testMarksSkipUsedOnlyWhenRuleWouldChangeFile(): void
         // file does not use OldClass → rule would not change it → its skip is unnecessary
         $this->doTestFile(__DIR__ . '/FixtureSkipUsedTracking/skip_unused_no_old_class.php.inc');
 
-        $usedSkipCollector = $this->make(UsedSkipCollector::class);
-        $usedSkips = $usedSkipCollector->provide();
+        $skipper = $this->make(Skipper::class);
+        $usedSkips = $skipper->provideUsedSkips();
 
         $usedPaths = $usedSkips[RenameClassRector::class] ?? [];
 

src/Application/ApplicationFileProcessor.php

@@ -17,7 +17,7 @@
 use Rector\Parallel\ScheduleFactory;
 use Rector\PhpParser\Parser\ParserErrors;
 use Rector\Reporting\MissConfigurationReporter;
-use Rector\Skipper\Skipper\UsedSkipCollector;
+use Rector\Skipper\Skipper\Skipper;
 use Rector\Testing\PHPUnit\StaticPHPUnitEnvironment;
 use Rector\Util\ArrayParametersMerger;
 use Rector\ValueObject\Application\File;
@@ -50,7 +50,7 @@ public function __construct(
         private readonly FileProcessor $fileProcessor,
         private readonly ArrayParametersMerger $arrayParametersMerger,
         private readonly MissConfigurationReporter $missConfigurationReporter,
-        private readonly UsedSkipCollector $usedSkipCollector,
+        private readonly Skipper $skipper,
     ) {
     }
 
@@ -106,7 +106,7 @@ public function run(Configuration $configuration, InputInterface $input): Proces
 
         // path-only skips are matched in the main process while finding files; in parallel runs the
         // result comes from workers only, so merge those marks back in to avoid false "unused skip"
-        $processResult->addUsedSkips($this->usedSkipCollector->provide());
+        $processResult->addUsedSkips($this->skipper->provideUsedSkips());
 
         $this->restoreErrorHandler();
 
@@ -181,7 +181,7 @@ public function processFiles(
             }
         }
 
-        return new ProcessResult($systemErrors, $fileDiffs, $totalChanged, $this->usedSkipCollector->provide());
+        return new ProcessResult($systemErrors, $fileDiffs, $totalChanged, $this->skipper->provideUsedSkips());
     }
 
     private function processFile(File $file, Configuration $configuration): FileProcessResult

src/DependencyInjection/LazyContainerFactory.php

@@ -69,7 +69,6 @@
 use Rector\PHPStanStaticTypeMapper\TypeMapper\UnionTypeMapper;
 use Rector\PostRector\Application\PostFileProcessor;
 use Rector\Rector\AbstractRector;
-use Rector\Skipper\Skipper\UsedSkipCollector;
 use Symfony\Component\Console\Application;
 use Symfony\Component\Console\Style\SymfonyStyle;
 use Webmozart\Assert\Assert;
@@ -171,9 +170,6 @@ private function registerFileProcessing(RectorConfig $rectorConfig): void
         $rectorConfig->singleton(PostFileProcessor::class);
         $rectorConfig->singleton(RectorRegistry::class);
 
-        // shared state: collects used skips across the skipper, the path skipper and the file processor
-        $rectorConfig->singleton(UsedSkipCollector::class);
-
         $rectorConfig->singleton(
             DynamicSourceLocatorProvider::class,
             static function (RectorConfig $rectorConfig): DynamicSourceLocatorProvider {

src/FileSystem/FilesFinder.php

@@ -9,7 +9,7 @@
 use Rector\Caching\UnchangedFilesFilter;
 use Rector\Configuration\Option;
 use Rector\Configuration\Parameter\SimpleParameterProvider;
-use Rector\Skipper\Skipper\PathSkipper;
+use Rector\Skipper\Skipper\Skipper;
 use Rector\ValueObject\Configuration;
 use Symfony\Component\Finder\Finder;
 
@@ -22,7 +22,7 @@ public function __construct(
         private FilesystemTweaker $filesystemTweaker,
         private UnchangedFilesFilter $unchangedFilesFilter,
         private FileAndDirectoryFilter $fileAndDirectoryFilter,
-        private PathSkipper $pathSkipper,
+        private Skipper $skipper,
         private FilePathHelper $filePathHelper,
         private ChangedFilesDetector $changedFilesDetector,
         private FilePathFilter $filePathFilter,
@@ -48,7 +48,7 @@ public function findInDirectoriesAndFiles(
         $filteredFilePaths = $this->fileAndDirectoryFilter->filterFiles($filesAndDirectories);
         $filteredFilePaths = array_filter(
             $filteredFilePaths,
-            fn (string $filePath): bool => ! $this->pathSkipper->shouldSkip($filePath)
+            fn (string $filePath): bool => ! $this->skipper->shouldSkipFilePath($filePath)
         );
 
         // fallback append `.php` to be used for both $filteredFilePaths and $filteredFilePathsInDirectories
@@ -184,7 +184,7 @@ private function findInDirectories(
                 continue;
             }
 
-            if ($this->pathSkipper->shouldSkip($path)) {
+            if ($this->skipper->shouldSkipFilePath($path)) {
                 continue;
             }
 

src/Skipper/Skipper/PathSkipper.php

@@ -12,18 +12,11 @@
     public function __construct(
         private FileInfoMatcher $fileInfoMatcher,
         private SkippedPathsResolver $skippedPathsResolver,
-        private UsedSkipCollector $usedSkipCollector
     ) {
     }
 
-    public function shouldSkip(string $filePath): bool
+    public function matchSkippedPath(string $filePath): ?string
     {
-        $matchedPath = $this->fileInfoMatcher->matchPattern($filePath, $this->skippedPathsResolver->resolve());
-        if ($matchedPath === null) {
-            return false;
-        }
-
-        $this->usedSkipCollector->markUsed($matchedPath);
-        return true;
+        return $this->fileInfoMatcher->matchPattern($filePath, $this->skippedPathsResolver->resolve());
     }
 }

src/Skipper/Skipper/Skipper.php

@@ -26,18 +26,32 @@ final class Skipper
      */
     private null|array $skippedClassesToFiles = null;
 
+    /**
+     * Map of skip element (rule class or global path) to the set of paths matched under it.
+     * Rule-scoped skips collect their matched paths; skip-everywhere rules and global path skips
+     * keep an empty path set, the same shape as the "->withSkip()" config.
+     *
+     * @var array<string, array<string, true>>
+     */
+    private array $usedSkips = [];
+
     public function __construct(
         private readonly RectifiedAnalyzer $rectifiedAnalyzer,
         private readonly PathSkipper $pathSkipper,
         private readonly FileInfoMatcher $fileInfoMatcher,
         private readonly ReflectionProvider $reflectionProvider,
-        private readonly UsedSkipCollector $usedSkipCollector,
     ) {
     }
 
     public function shouldSkipFilePath(string $filePath): bool
     {
-        return $this->pathSkipper->shouldSkip($filePath);
+        $matchedPath = $this->pathSkipper->matchSkippedPath($filePath);
+        if ($matchedPath === null) {
+            return false;
+        }
+
+        $this->markUsed($matchedPath);
+        return true;
     }
 
     public function shouldSkipRectorAndFile(object $rector, string $filePath): bool
@@ -84,7 +98,23 @@ public function matchSkip(string|object $element, string $filePath): ?SkipMatch
 
     public function markSkipUsed(SkipMatch $skipMatch): void
     {
-        $this->usedSkipCollector->markUsed($skipMatch->getSkippedClass(), $skipMatch->getMatchedPath());
+        $this->markUsed($skipMatch->getSkippedClass(), $skipMatch->getMatchedPath());
+    }
+
+    /**
+     * Skip elements (rule classes and paths) that actually matched during the run,
+     * so unused skips can be reported and removed.
+     *
+     * @return array<string, string[]>
+     */
+    public function provideUsedSkips(): array
+    {
+        $usedSkips = [];
+        foreach ($this->usedSkips as $skip => $paths) {
+            $usedSkips[$skip] = array_keys($paths);
+        }
+
+        return $usedSkips;
     }
 
     /**
@@ -147,4 +177,13 @@ public function shouldSkipCurrentNode(string $rectorClass, Node $node): bool
     {
         return $this->rectifiedAnalyzer->hasRectified($rectorClass, $node);
     }
+
+    private function markUsed(string $skip, ?string $path = null): void
+    {
+        $this->usedSkips[$skip] ??= [];
+
+        if ($path !== null) {
+            $this->usedSkips[$skip][$path] = true;
+        }
+    }
 }

tests/Skipper/Skipper/SkipperTest.php

@@ -16,6 +16,11 @@
 
 final class SkipperTest extends AbstractLazyTestCase
 {
+    /**
+     * Uniquely named on purpose, so the shared skipper is never polluted by another test.
+     */
+    private const string UNUSED_SKIP_MARKER = '*/UniqueUnusedSkipMarker/*';
+
     private Skipper $skipper;
 
     protected function setUp(): void
@@ -93,4 +98,59 @@ public static function provideRectorAndFile(): Iterator
         yield [new NotSkippedClass(), __DIR__ . '/Fixture/someFile', false];
         yield [new NotSkippedClass(), __DIR__ . '/Fixture/someOtherFile', false];
     }
+
+    public function testCollectsOnlyMatchedSkips(): void
+    {
+        SimpleParameterProvider::setParameter(Option::SKIP, [
+            // path skip that will be matched
+            __DIR__ . '/Fixture/SomeSkippedPath',
+
+            // path skip that is never matched
+            self::UNUSED_SKIP_MARKER,
+
+            // class skip that will be matched
+            FifthElement::class,
+        ]);
+
+        $this->skipper->shouldSkipFilePath('tests/Skipper/Skipper/Fixture/SomeSkippedPath/any.txt');
+        $this->skipper->shouldSkipRectorAndFile(new FifthElement(), __DIR__ . '/Fixture/someFile');
+
+        $usedSkips = $this->skipper->provideUsedSkips();
+
+        // matched skips are collected as keys
+        $this->assertArrayHasKey(FifthElement::class, $usedSkips);
+        $this->assertNotEmpty(array_filter(
+            array_keys($usedSkips),
+            static fn (string $usedSkip): bool => str_ends_with($usedSkip, 'SomeSkippedPath')
+        ));
+
+        // unmatched skip is never collected
+        $this->assertArrayNotHasKey(self::UNUSED_SKIP_MARKER, $usedSkips);
+    }
+
+    public function testCollectsMatchedPathNotRuleClassForRuleScopedSkip(): void
+    {
+        SimpleParameterProvider::setParameter(Option::SKIP, [
+            AnotherClassToSkip::class => [
+                // matched path
+                '*/someDirectory/*',
+                // sibling path that never matches
+                self::UNUSED_SKIP_MARKER,
+            ],
+        ]);
+
+        $this->skipper->shouldSkipRectorAndFile(
+            new AnotherClassToSkip(),
+            __DIR__ . '/Fixture/someDirectory/anotherFile.php'
+        );
+
+        $usedSkips = $this->skipper->provideUsedSkips();
+
+        // the specific matched path is collected nested under its rule class
+        $this->assertArrayHasKey(AnotherClassToSkip::class, $usedSkips);
+        $this->assertContains('*/someDirectory/*', $usedSkips[AnotherClassToSkip::class]);
+
+        // the unmatched sibling path under the same rule is never collected
+        $this->assertNotContains(self::UNUSED_SKIP_MARKER, $usedSkips[AnotherClassToSkip::class]);
+    }
 }