From b886375a27e13f825ec8bc19db99a60b74542672 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 21 Jun 2026 14:36:27 +0200 Subject: [PATCH] [skip] track used skips as class => [paths] map, matching skip config shape --- .../Application/ParallelFileProcessor.php | 18 +++++++++++---- src/Reporting/UnusedSkipResolver.php | 11 +++++---- src/Skipper/Skipper/SkipSkipper.php | 6 ++--- src/Skipper/Skipper/UsedSkipCollector.php | 23 +++++++++++++++---- src/ValueObject/ProcessResult.php | 19 ++++++++++----- .../MissConfigurationReporterTest.php | 6 +++-- tests/Reporting/UnusedSkipResolverTest.php | 10 +++++--- .../Skipper/Skipper/UsedSkipCollectorTest.php | 16 ++++++------- 8 files changed, 72 insertions(+), 37 deletions(-) diff --git a/src/Parallel/Application/ParallelFileProcessor.php b/src/Parallel/Application/ParallelFileProcessor.php index 27d59b3a46d..16d3e2e1ba0 100644 --- a/src/Parallel/Application/ParallelFileProcessor.php +++ b/src/Parallel/Application/ParallelFileProcessor.php @@ -75,7 +75,7 @@ public function process( /** @var SystemError[] $systemErrors */ $systemErrors = []; - /** @var array $usedSkips */ + /** @var array> $usedSkips */ $usedSkips = []; $tcpServer = new TcpServer('127.0.0.1:0', $streamSelectLoop); @@ -196,12 +196,15 @@ function (array $json) use ( * file_diffs: array, * files_count: int, * system_errors_count: int, - * used_skips: string[] + * used_skips: array * } $json */ $totalChanged += $json[Bridge::TOTAL_CHANGED]; - foreach ($json[Bridge::USED_SKIPS] as $usedSkip) { - $usedSkips[$usedSkip] = true; + foreach ($json[Bridge::USED_SKIPS] as $skip => $paths) { + $usedSkips[$skip] ??= []; + foreach ($paths as $path) { + $usedSkips[$skip][$path] = true; + } } // decode arrays to objects @@ -286,6 +289,11 @@ function ($exitCode, string $stdErr) use (&$systemErrors, $processIdentifier): v )); } - return new ProcessResult($systemErrors, $fileDiffs, $totalChanged, array_keys($usedSkips)); + $mergedUsedSkips = []; + foreach ($usedSkips as $skip => $paths) { + $mergedUsedSkips[$skip] = array_keys($paths); + } + + return new ProcessResult($systemErrors, $fileDiffs, $totalChanged, $mergedUsedSkips); } } diff --git a/src/Reporting/UnusedSkipResolver.php b/src/Reporting/UnusedSkipResolver.php index 450e843281b..90469847990 100644 --- a/src/Reporting/UnusedSkipResolver.php +++ b/src/Reporting/UnusedSkipResolver.php @@ -106,7 +106,7 @@ private function resolveGlobalRelativePaths(): array } /** - * @param string[] $usedSkips + * @param array $usedSkips * @return string[] */ private function resolveUnusedRuleScopedSkips(array $usedSkips): array @@ -117,9 +117,9 @@ private function resolveUnusedRuleScopedSkips(array $usedSkips): array foreach ($this->resolveRelativePathsByClass() as $rectorClass => $relativePaths) { $unusedRelativePaths = []; foreach ($relativePaths as $path => $relativePath) { - // used skips are tracked scoped to their rule as "class|path", so the same path + // used skips are tracked scoped to their rule (class => [path]), so the same path // skipped under another rule does not mark this one used (see SkipSkipper) - if (! in_array($rectorClass . '|' . $path, $usedSkips, true)) { + if (! in_array($path, $usedSkips[$rectorClass] ?? [], true)) { $unusedRelativePaths[] = $relativePath; } } @@ -136,14 +136,15 @@ private function resolveUnusedRuleScopedSkips(array $usedSkips): array } /** - * @param string[] $usedSkips + * @param array $usedSkips * @return string[] */ private function resolveUnusedGlobalSkips(array $usedSkips): array { $unusedSkips = []; foreach ($this->resolveGlobalRelativePaths() as $path => $relativePath) { - if (! in_array($path, $usedSkips, true)) { + // global path skips are tracked by their own path key, with no nested paths + if (! array_key_exists($path, $usedSkips)) { $unusedSkips[] = $relativePath; } } diff --git a/src/Skipper/Skipper/SkipSkipper.php b/src/Skipper/Skipper/SkipSkipper.php index 511dc42f75a..cc654a67e94 100644 --- a/src/Skipper/Skipper/SkipSkipper.php +++ b/src/Skipper/Skipper/SkipSkipper.php @@ -30,11 +30,11 @@ public function doesMatchSkip(object | string $checker, string $filePath, array return true; } - // mark the specific matched path used, scoped to its rule via "class|path" - the same - // path can be skipped under multiple rules, so a path-only key would mark them all used + // mark the specific matched path used, scoped to its rule - the same path can be skipped + // under multiple rules, so the path is nested under its rule, not tracked on its own $matchedPath = $this->fileInfoMatcher->matchPattern($filePath, $skippedFiles); if ($matchedPath !== null) { - $this->usedSkipCollector->markUsed($skippedClass . '|' . $matchedPath); + $this->usedSkipCollector->markUsed($skippedClass, $matchedPath); return true; } } diff --git a/src/Skipper/Skipper/UsedSkipCollector.php b/src/Skipper/Skipper/UsedSkipCollector.php index 27c3abef42d..e15dd8e785e 100644 --- a/src/Skipper/Skipper/UsedSkipCollector.php +++ b/src/Skipper/Skipper/UsedSkipCollector.php @@ -13,20 +13,33 @@ final class UsedSkipCollector { /** - * @var array + * 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 markUsed(string $skip): void + public function markUsed(string $skip, ?string $path = null): void { - $this->usedSkips[$skip] = true; + $this->usedSkips[$skip] ??= []; + + if ($path !== null) { + $this->usedSkips[$skip][$path] = true; + } } /** - * @return string[] + * @return array */ public function provide(): array { - return array_keys($this->usedSkips); + $usedSkips = []; + foreach ($this->usedSkips as $skip => $paths) { + $usedSkips[$skip] = array_keys($paths); + } + + return $usedSkips; } } diff --git a/src/ValueObject/ProcessResult.php b/src/ValueObject/ProcessResult.php index 5adc7a101d6..1395b5f874e 100644 --- a/src/ValueObject/ProcessResult.php +++ b/src/ValueObject/ProcessResult.php @@ -14,7 +14,7 @@ final class ProcessResult /** * @param SystemError[] $systemErrors * @param FileDiff[] $fileDiffs - * @param string[] $usedSkips + * @param array $usedSkips */ public function __construct( private array $systemErrors, @@ -24,7 +24,11 @@ public function __construct( ) { Assert::allIsInstanceOf($systemErrors, SystemError::class); Assert::allIsInstanceOf($fileDiffs, FileDiff::class); - Assert::allString($usedSkips); + + Assert::allString(array_keys($usedSkips)); + foreach ($usedSkips as $usedSkip) { + Assert::allString($usedSkip); + } } /** @@ -62,13 +66,16 @@ public function addSystemErrors(array $systemErrors): void * their result from worker processes only. Merge those main-process marks back in, or they would * be wrongly reported as unused. * - * @param string[] $usedSkips + * @param array $usedSkips */ public function addUsedSkips(array $usedSkips): void { - Assert::allString($usedSkips); + foreach ($usedSkips as $skip => $paths) { + Assert::allString($paths); - $this->usedSkips = array_values(array_unique([...$this->usedSkips, ...$usedSkips])); + $existingPaths = $this->usedSkips[$skip] ?? []; + $this->usedSkips[$skip] = array_values(array_unique([...$existingPaths, ...$paths])); + } } public function getTotalChanged(): int @@ -77,7 +84,7 @@ public function getTotalChanged(): int } /** - * @return string[] + * @return array */ public function getUsedSkips(): array { diff --git a/tests/Reporting/MissConfigurationReporterTest.php b/tests/Reporting/MissConfigurationReporterTest.php index 4f9ff041f36..02732bb3a66 100644 --- a/tests/Reporting/MissConfigurationReporterTest.php +++ b/tests/Reporting/MissConfigurationReporterTest.php @@ -65,8 +65,10 @@ public function testReportsOnlyTrackableUnusedSkips(): void { SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, true); - // matched rule-scoped path is marked used scoped to its rule as "class|path" - $processResult = new ProcessResult([], [], 0, [AnotherClassToSkip::class . '|' . self::USED_RULE_MASK]); + // matched rule-scoped path is marked used scoped to its rule (class => [path]) + $processResult = new ProcessResult([], [], 0, [ + AnotherClassToSkip::class => [self::USED_RULE_MASK], + ]); $this->missConfigurationReporter->reportUnusedSkips($processResult); $output = $this->bufferedOutput->fetch(); diff --git a/tests/Reporting/UnusedSkipResolverTest.php b/tests/Reporting/UnusedSkipResolverTest.php index 943b42308f0..10bce74e339 100644 --- a/tests/Reporting/UnusedSkipResolverTest.php +++ b/tests/Reporting/UnusedSkipResolverTest.php @@ -53,8 +53,10 @@ public function testResolvesUnusedSkipsAsRuleAndPath(): void { SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, true); - // used skips are tracked scoped to their rule as "class|path" - $processResult = new ProcessResult([], [], 0, [AnotherClassToSkip::class . '|' . self::USED_RULE_MASK]); + // used skips are tracked scoped to their rule (class => [path]) + $processResult = new ProcessResult([], [], 0, [ + AnotherClassToSkip::class => [self::USED_RULE_MASK], + ]); $unusedSkips = $this->unusedSkipResolver->resolve($processResult); // rule-scoped unused skip is reported as "rule:" with the path nested below @@ -78,7 +80,9 @@ public function testSamePathUnderAnotherRuleDoesNotMarkSkipUsed(): void ]); // used skip is scoped to its rule, so the shared path stays unused under the other rule - $processResult = new ProcessResult([], [], 0, [AnotherClassToSkip::class . '|' . $sharedMask]); + $processResult = new ProcessResult([], [], 0, [ + AnotherClassToSkip::class => [$sharedMask], + ]); $unusedSkips = $this->unusedSkipResolver->resolve($processResult); // the never-matched rule still reports its shared path as unused diff --git a/tests/Skipper/Skipper/UsedSkipCollectorTest.php b/tests/Skipper/Skipper/UsedSkipCollectorTest.php index 9e20c92d67f..e013a5da95b 100644 --- a/tests/Skipper/Skipper/UsedSkipCollectorTest.php +++ b/tests/Skipper/Skipper/UsedSkipCollectorTest.php @@ -54,15 +54,15 @@ public function testCollectsOnlyMatchedSkips(): void $usedSkips = $this->usedSkipCollector->provide(); - // matched skips are collected - $this->assertContains(FifthElement::class, $usedSkips); + // matched skips are collected as keys + $this->assertArrayHasKey(FifthElement::class, $usedSkips); $this->assertNotEmpty(array_filter( - $usedSkips, + array_keys($usedSkips), static fn (string $usedSkip): bool => str_ends_with($usedSkip, 'SomeSkippedPath') )); // unmatched skip is never collected - $this->assertNotContains(self::UNUSED_SKIP_MARKER, $usedSkips); + $this->assertArrayNotHasKey(self::UNUSED_SKIP_MARKER, $usedSkips); } public function testCollectsMatchedPathNotRuleClassForRuleScopedSkip(): void @@ -83,11 +83,11 @@ public function testCollectsMatchedPathNotRuleClassForRuleScopedSkip(): void $usedSkips = $this->usedSkipCollector->provide(); - // the specific matched path is collected scoped to its rule as "class|path" - $this->assertContains(AnotherClassToSkip::class . '|' . '*/someDirectory/*', $usedSkips); - $this->assertNotContains(AnotherClassToSkip::class, $usedSkips); + // 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(AnotherClassToSkip::class . '|' . self::UNUSED_SKIP_MARKER, $usedSkips); + $this->assertNotContains(self::UNUSED_SKIP_MARKER, $usedSkips[AnotherClassToSkip::class]); } }