From 5f118207f8c2d89090d496a0011a99e8d370d79e Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sat, 10 Oct 2026 23:38:58 +0200 Subject: [PATCH] [skipper] Merge UsedSkipCollector into Skipper as single service --- .../SkipUsedTrackingTest.php | 6 +- src/Application/ApplicationFileProcessor.php | 8 +- .../LazyContainerFactory.php | 4 - src/FileSystem/FilesFinder.php | 8 +- src/Skipper/Skipper/PathSkipper.php | 11 +-- src/Skipper/Skipper/Skipper.php | 45 ++++++++- src/Skipper/Skipper/UsedSkipCollector.php | 45 --------- tests/Skipper/Skipper/SkipperTest.php | 60 ++++++++++++ .../Skipper/Skipper/UsedSkipCollectorTest.php | 93 ------------------- 9 files changed, 115 insertions(+), 165 deletions(-) delete mode 100644 src/Skipper/Skipper/UsedSkipCollector.php delete mode 100644 tests/Skipper/Skipper/UsedSkipCollectorTest.php diff --git a/rules-tests/Renaming/Rector/Name/RenameClassRector/SkipUsedTrackingTest.php b/rules-tests/Renaming/Rector/Name/RenameClassRector/SkipUsedTrackingTest.php index 8c1421abc99..a658750c742 100644 --- a/rules-tests/Renaming/Rector/Name/RenameClassRector/SkipUsedTrackingTest.php +++ b/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] ?? []; diff --git a/src/Application/ApplicationFileProcessor.php b/src/Application/ApplicationFileProcessor.php index 8c58b6e3129..4979b351b6b 100644 --- a/src/Application/ApplicationFileProcessor.php +++ b/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 diff --git a/src/DependencyInjection/LazyContainerFactory.php b/src/DependencyInjection/LazyContainerFactory.php index 1a06549745f..f7dce52357f 100644 --- a/src/DependencyInjection/LazyContainerFactory.php +++ b/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 { diff --git a/src/FileSystem/FilesFinder.php b/src/FileSystem/FilesFinder.php index a7f4ce50790..d6039d6dba5 100644 --- a/src/FileSystem/FilesFinder.php +++ b/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; } diff --git a/src/Skipper/Skipper/PathSkipper.php b/src/Skipper/Skipper/PathSkipper.php index 5616ad782e3..a2e9b2277b5 100644 --- a/src/Skipper/Skipper/PathSkipper.php +++ b/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()); } } diff --git a/src/Skipper/Skipper/Skipper.php b/src/Skipper/Skipper/Skipper.php index 884be924823..446c84c4589 100644 --- a/src/Skipper/Skipper/Skipper.php +++ b/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> + */ + 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 + */ + 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; + } + } } diff --git a/src/Skipper/Skipper/UsedSkipCollector.php b/src/Skipper/Skipper/UsedSkipCollector.php deleted file mode 100644 index e15dd8e785e..00000000000 --- a/src/Skipper/Skipper/UsedSkipCollector.php +++ /dev/null @@ -1,45 +0,0 @@ -withSkip()" config. - * - * @var array> - */ - private array $usedSkips = []; - - public function markUsed(string $skip, ?string $path = null): void - { - $this->usedSkips[$skip] ??= []; - - if ($path !== null) { - $this->usedSkips[$skip][$path] = true; - } - } - - /** - * @return array - */ - public function provide(): array - { - $usedSkips = []; - foreach ($this->usedSkips as $skip => $paths) { - $usedSkips[$skip] = array_keys($paths); - } - - return $usedSkips; - } -} diff --git a/tests/Skipper/Skipper/SkipperTest.php b/tests/Skipper/Skipper/SkipperTest.php index 39182dbd001..6ce1cb67a0b 100644 --- a/tests/Skipper/Skipper/SkipperTest.php +++ b/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]); + } } diff --git a/tests/Skipper/Skipper/UsedSkipCollectorTest.php b/tests/Skipper/Skipper/UsedSkipCollectorTest.php deleted file mode 100644 index a64611a9ce3..00000000000 --- a/tests/Skipper/Skipper/UsedSkipCollectorTest.php +++ /dev/null @@ -1,93 +0,0 @@ -skipper = $this->make(Skipper::class); - $this->usedSkipCollector = $this->make(UsedSkipCollector::class); - } - - protected function tearDown(): void - { - SimpleParameterProvider::setParameter(Option::SKIP, []); - } - - public function testCollectsOnlyMatchedSkips(): void - { - $this->skipper->shouldSkipFilePath('tests/Skipper/Skipper/Fixture/SomeSkippedPath/any.txt'); - $this->skipper->shouldSkipRectorAndFile(new FifthElement(), __DIR__ . '/Fixture/someFile'); - - $usedSkips = $this->usedSkipCollector->provide(); - - // 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->usedSkipCollector->provide(); - - // 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]); - } -}