From c786386d0232353922db756cce848c064d9f16a6 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sat, 20 Jun 2026 14:19:22 +0200 Subject: [PATCH] improve skipper with nested */dir/* paths --- .../custom/config/rector.php | 9 ++-- .../expected-output.diff | 2 +- src/Reporting/MissConfigurationReporter.php | 15 +++---- src/Skipper/Matcher/FileInfoMatcher.php | 3 ++ .../MissConfigurationReporterTest.php | 35 ++++++++-------- tests/Skipper/Matcher/FileInfoMatcherTest.php | 41 +++++++++++++++++++ .../Skipper/Skipper/UsedSkipCollectorTest.php | 27 ++++++++++++ 7 files changed, 103 insertions(+), 29 deletions(-) create mode 100644 tests/Skipper/Matcher/FileInfoMatcherTest.php diff --git a/e2e/parallel-unused-skips/custom/config/rector.php b/e2e/parallel-unused-skips/custom/config/rector.php index a987270fa34..e67dec18e2f 100644 --- a/e2e/parallel-unused-skips/custom/config/rector.php +++ b/e2e/parallel-unused-skips/custom/config/rector.php @@ -14,12 +14,13 @@ $rectorConfig->reportUnusedSkips(); - // two concrete paths under one rule: first matched in the worker (proves parallel aggregation), - // second never matches and must be reported per-path as unused + // two rule-scoped masks: first matched in the worker (proves parallel aggregation), + // second never matches and must be reported per-path as unused. + // a global mask "*/NonexistentGlobal/*" would NOT be reported, only rule-scoped ones are $rectorConfig->skip([ SimplifyUselessVariableRector::class => [ - dirname(__DIR__, 2) . '/src/SomeClass.php', - dirname(__DIR__, 2) . '/src/NonexistentUnused.php', + '*/src/*', + '*/NonexistentUnused/*', ], ]); }; diff --git a/e2e/parallel-unused-skips/expected-output.diff b/e2e/parallel-unused-skips/expected-output.diff index eeea8a9d939..de57957a29e 100644 --- a/e2e/parallel-unused-skips/expected-output.diff +++ b/e2e/parallel-unused-skips/expected-output.diff @@ -3,4 +3,4 @@ [WARNING] This skip is unused, it never matched any element. You can remove it from "->withSkip()" - * ./parallel-unused-skips/src/NonexistentUnused.php + * */NonexistentUnused/* diff --git a/src/Reporting/MissConfigurationReporter.php b/src/Reporting/MissConfigurationReporter.php index 4d209358596..bb0cd432458 100644 --- a/src/Reporting/MissConfigurationReporter.php +++ b/src/Reporting/MissConfigurationReporter.php @@ -36,8 +36,9 @@ public function reportUnusedSkips(ProcessResult $processResult): void return; } - // only concrete path-scoped skips are trackable at runtime; skip-everywhere rule skips - // (null path) are forgotten from the container at boot, so they never reach the skipper + // rule-scoped path skips are trackable at runtime; skip-everywhere rule skips + // (null path) are forgotten from the container at boot, so they never reach the skipper. + // rule-scoped paths are intentional, so they are reported even as mask paths $skippedClassPaths = []; foreach ($this->skippedClassResolver->resolve() as $paths) { if ($paths === null) { @@ -47,14 +48,14 @@ public function reportUnusedSkips(ProcessResult $processResult): void $skippedClassPaths = [...$skippedClassPaths, ...$paths]; } - $configuredSkips = [...$skippedClassPaths, ...$this->skippedPathsResolver->resolve()]; - - // skip mask paths like "*/some/*"; they are hard to spot and report false positives - $configuredSkips = array_filter( - $configuredSkips, + // global mask paths like "*/some/*" are hard to spot and report false positives, skip them + $globalPaths = array_filter( + $this->skippedPathsResolver->resolve(), static fn (string $skip): bool => ! str_contains($skip, '*') ); + $configuredSkips = [...$skippedClassPaths, ...$globalPaths]; + $unusedSkips = array_values(array_diff($configuredSkips, $processResult->getUsedSkips())); if ($unusedSkips === []) { return; diff --git a/src/Skipper/Matcher/FileInfoMatcher.php b/src/Skipper/Matcher/FileInfoMatcher.php index 0b3fde447ef..bfb3345a83c 100644 --- a/src/Skipper/Matcher/FileInfoMatcher.php +++ b/src/Skipper/Matcher/FileInfoMatcher.php @@ -9,6 +9,9 @@ use Rector\Skipper\Fnmatcher; use Rector\Skipper\RealpathMatcher; +/** + * @see \Rector\Tests\Skipper\Matcher\FileInfoMatcherTest + */ final readonly class FileInfoMatcher { public function __construct( diff --git a/tests/Reporting/MissConfigurationReporterTest.php b/tests/Reporting/MissConfigurationReporterTest.php index fbc79c26a7e..b047f66a7dc 100644 --- a/tests/Reporting/MissConfigurationReporterTest.php +++ b/tests/Reporting/MissConfigurationReporterTest.php @@ -21,11 +21,11 @@ final class MissConfigurationReporterTest extends AbstractLazyTestCase { - private const string UNUSED_ENTITY_PATH = '/app/bundles/UserBundle/Entity'; + private const string UNUSED_RULE_MASK = '*/dead-rule/*'; - private const string USED_ENTITY_PATH = '/app/bundles/AdminBundle/Entity'; + private const string USED_RULE_MASK = '*/matched-rule/*'; - private const string MASK_PATH = '*/SomeMask/*'; + private const string GLOBAL_MASK = '*/global-mask/*'; private BufferedOutput $bufferedOutput; @@ -48,12 +48,12 @@ protected function setUp(): void SimpleParameterProvider::setParameter(Option::SKIP, [ // skip-everywhere rule skip - forgotten at boot, not trackable, must be excluded ThreeMan::class, - // concrete path-scoped class skip, never matched - must be reported - FifthElement::class => [self::UNUSED_ENTITY_PATH], - // concrete path-scoped class skip, matched - must not be reported - AnotherClassToSkip::class => [self::USED_ENTITY_PATH], - // mask path skip - hard to spot, false-positive prone, must not be reported - self::MASK_PATH, + // rule-scoped path skip, never matched - intentional, must be reported even as mask + FifthElement::class => [self::UNUSED_RULE_MASK], + // rule-scoped path skip, matched - must not be reported + AnotherClassToSkip::class => [self::USED_RULE_MASK], + // global mask path skip - hard to spot, false-positive prone, must not be reported + self::GLOBAL_MASK, ]); } @@ -67,20 +67,21 @@ public function testReportsOnlyTrackableUnusedSkips(): void { SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, true); - // matched path is marked used by its concrete path, not by the rule class - $processResult = new ProcessResult([], [], 0, [self::USED_ENTITY_PATH]); + // matched rule-scoped path is marked used by its path, not by the rule class + $processResult = new ProcessResult([], [], 0, [self::USED_RULE_MASK]); $this->missConfigurationReporter->reportUnusedSkips($processResult); $output = $this->bufferedOutput->fetch(); - // unused concrete path is reported by path, not rule class - $this->assertStringContainsString('UserBundle', $output); + // unused rule-scoped path is reported by path, not rule class + $this->assertStringContainsString('dead-rule', $output); + $this->assertStringNotContainsString('FifthElement', $output); - // matched concrete path is excluded - $this->assertStringNotContainsString('AdminBundle', $output); + // matched rule-scoped path is excluded + $this->assertStringNotContainsString('matched-rule', $output); - // mask path is excluded (hard to spot, false-positive prone) - $this->assertStringNotContainsString('SomeMask', $output); + // global mask path is excluded (hard to spot, false-positive prone) + $this->assertStringNotContainsString('global-mask', $output); // skip-everywhere rule skip is excluded (not trackable at runtime) $this->assertStringNotContainsString('ThreeMan', $output); diff --git a/tests/Skipper/Matcher/FileInfoMatcherTest.php b/tests/Skipper/Matcher/FileInfoMatcherTest.php new file mode 100644 index 00000000000..862608b55c6 --- /dev/null +++ b/tests/Skipper/Matcher/FileInfoMatcherTest.php @@ -0,0 +1,41 @@ +fileInfoMatcher = $this->make(FileInfoMatcher::class); + } + + public function testMatchPatternReturnsTheOriginalMatchedPattern(): void + { + $matchedPattern = $this->fileInfoMatcher->matchPattern('/project/src/Foo.php', ['*/tests/*', '*/src/*']); + + // the original, un-normalized pattern is returned, so callers can report the exact path + $this->assertSame('*/src/*', $matchedPattern); + } + + public function testMatchPatternReturnsNullWhenNoPatternMatches(): void + { + $matchedPattern = $this->fileInfoMatcher->matchPattern('/project/src/Foo.php', ['*/tests/*']); + + $this->assertNull($matchedPattern); + } + + public function testDoesFileInfoMatchPatternsStillReportsBoolean(): void + { + $this->assertTrue($this->fileInfoMatcher->doesFileInfoMatchPatterns('/project/src/Foo.php', ['*/src/*'])); + $this->assertFalse($this->fileInfoMatcher->doesFileInfoMatchPatterns('/project/src/Foo.php', ['*/tests/*'])); + } +} diff --git a/tests/Skipper/Skipper/UsedSkipCollectorTest.php b/tests/Skipper/Skipper/UsedSkipCollectorTest.php index dc201c1d7f5..060b361f1ca 100644 --- a/tests/Skipper/Skipper/UsedSkipCollectorTest.php +++ b/tests/Skipper/Skipper/UsedSkipCollectorTest.php @@ -10,6 +10,7 @@ use Rector\Skipper\Skipper\UsedSkipCollector; use Rector\Testing\PHPUnit\AbstractLazyTestCase; use Rector\Tests\Skipper\Skipper\Fixture\Element\FifthElement; +use Rector\Tests\Skipper\Skipper\Source\AnotherClassToSkip; final class UsedSkipCollectorTest extends AbstractLazyTestCase { @@ -63,4 +64,30 @@ public function testCollectsOnlyMatchedSkips(): void // unmatched skip is never collected $this->assertNotContains(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->shouldSkipElementAndFilePath( + AnotherClassToSkip::class, + __DIR__ . '/Fixture/someDirectory/anotherFile.php' + ); + + $usedSkips = $this->usedSkipCollector->provide(); + + // the specific matched path is collected, not the rule class + $this->assertContains('*/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); + } }