From 8bd7601df7bb1df515360f0135c997d75863efd9 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 21 Jun 2026 13:18:37 +0200 Subject: [PATCH] [skip] fix match class + path matching in reporting of unused skips --- src/Reporting/UnusedSkipResolver.php | 4 ++- src/Skipper/Skipper/SkipSkipper.php | 5 ++-- .../MissConfigurationReporterTest.php | 4 +-- tests/Reporting/UnusedSkipResolverTest.php | 25 ++++++++++++++++++- .../Skipper/Skipper/UsedSkipCollectorTest.php | 6 ++--- 5 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src/Reporting/UnusedSkipResolver.php b/src/Reporting/UnusedSkipResolver.php index 0c0274bd16d..450e843281b 100644 --- a/src/Reporting/UnusedSkipResolver.php +++ b/src/Reporting/UnusedSkipResolver.php @@ -117,7 +117,9 @@ private function resolveUnusedRuleScopedSkips(array $usedSkips): array foreach ($this->resolveRelativePathsByClass() as $rectorClass => $relativePaths) { $unusedRelativePaths = []; foreach ($relativePaths as $path => $relativePath) { - if (! in_array($path, $usedSkips, true)) { + // used skips are tracked scoped to their rule as "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)) { $unusedRelativePaths[] = $relativePath; } } diff --git a/src/Skipper/Skipper/SkipSkipper.php b/src/Skipper/Skipper/SkipSkipper.php index bf42c8551e3..511dc42f75a 100644 --- a/src/Skipper/Skipper/SkipSkipper.php +++ b/src/Skipper/Skipper/SkipSkipper.php @@ -30,10 +30,11 @@ public function doesMatchSkip(object | string $checker, string $filePath, array return true; } - // mark the specific matched path used, so unused paths under the same rule are reported + // 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 $matchedPath = $this->fileInfoMatcher->matchPattern($filePath, $skippedFiles); if ($matchedPath !== null) { - $this->usedSkipCollector->markUsed($matchedPath); + $this->usedSkipCollector->markUsed($skippedClass . '|' . $matchedPath); return true; } } diff --git a/tests/Reporting/MissConfigurationReporterTest.php b/tests/Reporting/MissConfigurationReporterTest.php index 5f111802c25..4f9ff041f36 100644 --- a/tests/Reporting/MissConfigurationReporterTest.php +++ b/tests/Reporting/MissConfigurationReporterTest.php @@ -65,8 +65,8 @@ public function testReportsOnlyTrackableUnusedSkips(): void { SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, true); - // matched rule-scoped path is marked used by its path, not by the rule class - $processResult = new ProcessResult([], [], 0, [self::USED_RULE_MASK]); + // matched rule-scoped path is marked used scoped to its rule as "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 eb926349e66..943b42308f0 100644 --- a/tests/Reporting/UnusedSkipResolverTest.php +++ b/tests/Reporting/UnusedSkipResolverTest.php @@ -53,7 +53,8 @@ public function testResolvesUnusedSkipsAsRuleAndPath(): void { SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, true); - $processResult = new ProcessResult([], [], 0, [self::USED_RULE_MASK]); + // used skips are tracked scoped to their rule as "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 @@ -65,6 +66,28 @@ public function testResolvesUnusedSkipsAsRuleAndPath(): void $this->assertNotContains(ThreeMan::class, $unusedSkips); } + public function testSamePathUnderAnotherRuleDoesNotMarkSkipUsed(): void + { + SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, true); + + $sharedMask = '*/shared-mask/*'; + SimpleParameterProvider::setParameter(Option::SKIP, [ + // same path skipped under two rules - only the first one actually matched a file + AnotherClassToSkip::class => [$sharedMask], + FifthElement::class => [$sharedMask], + ]); + + // used skip is scoped to its rule, so the shared path stays unused under the other rule + $processResult = new ProcessResult([], [], 0, [AnotherClassToSkip::class . '|' . $sharedMask]); + $unusedSkips = $this->unusedSkipResolver->resolve($processResult); + + // the never-matched rule still reports its shared path as unused + $this->assertContains(FifthElement::class . ':' . "\n * " . $sharedMask, $unusedSkips); + + // the matched rule is excluded + $this->assertNotContains(AnotherClassToSkip::class . ':' . "\n * " . $sharedMask, $unusedSkips); + } + public function testReportsUnusedSkipAsRelativePath(): void { SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, true); diff --git a/tests/Skipper/Skipper/UsedSkipCollectorTest.php b/tests/Skipper/Skipper/UsedSkipCollectorTest.php index 060b361f1ca..9e20c92d67f 100644 --- a/tests/Skipper/Skipper/UsedSkipCollectorTest.php +++ b/tests/Skipper/Skipper/UsedSkipCollectorTest.php @@ -83,11 +83,11 @@ public function testCollectsMatchedPathNotRuleClassForRuleScopedSkip(): void $usedSkips = $this->usedSkipCollector->provide(); - // the specific matched path is collected, not the rule class - $this->assertContains('*/someDirectory/*', $usedSkips); + // 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 unmatched sibling path under the same rule is never collected - $this->assertNotContains(self::UNUSED_SKIP_MARKER, $usedSkips); + $this->assertNotContains(AnotherClassToSkip::class . '|' . self::UNUSED_SKIP_MARKER, $usedSkips); } }