From f4f753630f2a699f26a4c951b710c3908ee15a52 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sat, 20 Jun 2026 16:02:20 +0200 Subject: [PATCH] [reporting] report unused skips in JSON output too JSON output forces VERBOSITY_QUIET, which silenced the unused-skips warning. Extract the computation into UnusedSkipResolver and surface it as an unused_skips key in the JSON payload, so editor/CI runs using --output-format=json see unused skips as well. --- .../Output/Factory/JsonOutputFactory.php | 14 +++- .../Output/JsonOutputFormatter.php | 11 ++- src/Reporting/MissConfigurationReporter.php | 44 +---------- src/Reporting/UnusedSkipResolver.php | 74 +++++++++++++++++++ .../MissConfigurationReporterTest.php | 6 +- tests/Reporting/UnusedSkipResolverTest.php | 74 +++++++++++++++++++ 6 files changed, 174 insertions(+), 49 deletions(-) create mode 100644 src/Reporting/UnusedSkipResolver.php create mode 100644 tests/Reporting/UnusedSkipResolverTest.php diff --git a/src/ChangesReporting/Output/Factory/JsonOutputFactory.php b/src/ChangesReporting/Output/Factory/JsonOutputFactory.php index aa75152d3ab..fb8b09c3ee9 100644 --- a/src/ChangesReporting/Output/Factory/JsonOutputFactory.php +++ b/src/ChangesReporting/Output/Factory/JsonOutputFactory.php @@ -15,8 +15,14 @@ */ final class JsonOutputFactory { - public static function create(ProcessResult $processResult, Configuration $configuration): string - { + /** + * @param string[] $unusedSkips + */ + public static function create( + ProcessResult $processResult, + Configuration $configuration, + array $unusedSkips = [] + ): string { $errorsJson = [ 'totals' => [ 'changed_files' => $processResult->getTotalChanged(), @@ -52,6 +58,10 @@ public static function create(ProcessResult $processResult, Configuration $confi $errorsJson['errors'] = $errorsData; } + if ($unusedSkips !== []) { + $errorsJson['unused_skips'] = $unusedSkips; + } + return Json::encode($errorsJson, pretty: true); } diff --git a/src/ChangesReporting/Output/JsonOutputFormatter.php b/src/ChangesReporting/Output/JsonOutputFormatter.php index 82e1f94f84f..45afd09ec93 100644 --- a/src/ChangesReporting/Output/JsonOutputFormatter.php +++ b/src/ChangesReporting/Output/JsonOutputFormatter.php @@ -6,6 +6,7 @@ use Rector\ChangesReporting\Contract\Output\OutputFormatterInterface; use Rector\ChangesReporting\Output\Factory\JsonOutputFactory; +use Rector\Reporting\UnusedSkipResolver; use Rector\ValueObject\Configuration; use Rector\ValueObject\ProcessResult; @@ -13,6 +14,11 @@ { public const string NAME = 'json'; + public function __construct( + private UnusedSkipResolver $unusedSkipResolver + ) { + } + public function getName(): string { return self::NAME; @@ -20,6 +26,9 @@ public function getName(): string public function report(ProcessResult $processResult, Configuration $configuration): void { - echo JsonOutputFactory::create($processResult, $configuration) . PHP_EOL; + // console output is silenced in json mode, so unused skips are surfaced in the payload + $unusedSkips = $this->unusedSkipResolver->resolve($processResult); + + echo JsonOutputFactory::create($processResult, $configuration, $unusedSkips) . PHP_EOL; } } diff --git a/src/Reporting/MissConfigurationReporter.php b/src/Reporting/MissConfigurationReporter.php index 05e4ae0fa9e..f62aab65275 100644 --- a/src/Reporting/MissConfigurationReporter.php +++ b/src/Reporting/MissConfigurationReporter.php @@ -8,8 +8,6 @@ use Rector\Configuration\Parameter\SimpleParameterProvider; use Rector\Configuration\VendorMissAnalyseGuard; use Rector\PostRector\Contract\Rector\PostRectorInterface; -use Rector\Skipper\SkipCriteriaResolver\SkippedClassResolver; -use Rector\Skipper\SkipCriteriaResolver\SkippedPathsResolver; use Rector\ValueObject\ProcessResult; use Symfony\Component\Console\Style\SymfonyStyle; @@ -21,8 +19,7 @@ public function __construct( private SymfonyStyle $symfonyStyle, private VendorMissAnalyseGuard $vendorMissAnalyseGuard, - private SkippedClassResolver $skippedClassResolver, - private SkippedPathsResolver $skippedPathsResolver, + private UnusedSkipResolver $unusedSkipResolver, ) { } @@ -32,44 +29,7 @@ public function __construct( */ public function reportUnusedSkips(ProcessResult $processResult): void { - if (! SimpleParameterProvider::provideBoolParameter(Option::REPORT_UNUSED_SKIPS, false)) { - return; - } - - // map of trackable skip path => human-readable display; skips are tracked at runtime by - // their path, but rule-scoped ones are printed as "rule => path" so the user knows exactly - // what to remove. Skip-everywhere rule skips (null path) are forgotten from the container - // at boot, so they never reach the skipper and cannot be tracked. - $skipDisplaysByPath = []; - foreach ($this->skippedClassResolver->resolve() as $rectorClass => $paths) { - if ($paths === null) { - continue; - } - - // rule-scoped paths are intentional, so they are reported even as mask paths - foreach ($paths as $path) { - $skipDisplaysByPath[$path] = $rectorClass . ' => ' . $path; - } - } - - // global mask paths like "*/some/*" are hard to spot and report false positives, skip them - foreach ($this->skippedPathsResolver->resolve() as $globalPath) { - if (str_contains($globalPath, '*')) { - continue; - } - - $skipDisplaysByPath[$globalPath] = $globalPath; - } - - $usedSkips = $processResult->getUsedSkips(); - - $unusedSkips = []; - foreach ($skipDisplaysByPath as $path => $skipDisplay) { - if (! in_array($path, $usedSkips, true)) { - $unusedSkips[] = $skipDisplay; - } - } - + $unusedSkips = $this->unusedSkipResolver->resolve($processResult); if ($unusedSkips === []) { return; } diff --git a/src/Reporting/UnusedSkipResolver.php b/src/Reporting/UnusedSkipResolver.php new file mode 100644 index 00000000000..71300b60ea3 --- /dev/null +++ b/src/Reporting/UnusedSkipResolver.php @@ -0,0 +1,74 @@ +withSkip()" that never matched any element during the run. + * Rule-scoped skips are returned as "rule => path" so the user knows exactly what to remove; + * global skips are returned as a plain path. Returns an empty array unless + * "->reportUnusedSkips()" is enabled. + * + * @return string[] + */ + public function resolve(ProcessResult $processResult): array + { + if (! SimpleParameterProvider::provideBoolParameter(Option::REPORT_UNUSED_SKIPS, false)) { + return []; + } + + // map of trackable skip path => human-readable display; skips are tracked at runtime by + // their path, but rule-scoped ones are printed as "rule => path" so the user knows exactly + // what to remove. Skip-everywhere rule skips (null path) are forgotten from the container + // at boot, so they never reach the skipper and cannot be tracked. + $skipDisplaysByPath = []; + foreach ($this->skippedClassResolver->resolve() as $rectorClass => $paths) { + if ($paths === null) { + continue; + } + + // rule-scoped paths are intentional, so they are reported even as mask paths + foreach ($paths as $path) { + $skipDisplaysByPath[$path] = $rectorClass . ' => ' . $path; + } + } + + // global mask paths like "*/some/*" are hard to spot and report false positives, skip them + foreach ($this->skippedPathsResolver->resolve() as $globalPath) { + if (str_contains($globalPath, '*')) { + continue; + } + + $skipDisplaysByPath[$globalPath] = $globalPath; + } + + $usedSkips = $processResult->getUsedSkips(); + + $unusedSkips = []; + foreach ($skipDisplaysByPath as $path => $skipDisplay) { + if (! in_array($path, $usedSkips, true)) { + $unusedSkips[] = $skipDisplay; + } + } + + return $unusedSkips; + } +} diff --git a/tests/Reporting/MissConfigurationReporterTest.php b/tests/Reporting/MissConfigurationReporterTest.php index 0c4e90fc1f3..5f111802c25 100644 --- a/tests/Reporting/MissConfigurationReporterTest.php +++ b/tests/Reporting/MissConfigurationReporterTest.php @@ -8,8 +8,7 @@ use Rector\Configuration\Parameter\SimpleParameterProvider; use Rector\Configuration\VendorMissAnalyseGuard; use Rector\Reporting\MissConfigurationReporter; -use Rector\Skipper\SkipCriteriaResolver\SkippedClassResolver; -use Rector\Skipper\SkipCriteriaResolver\SkippedPathsResolver; +use Rector\Reporting\UnusedSkipResolver; use Rector\Testing\PHPUnit\AbstractLazyTestCase; use Rector\Tests\Skipper\Skipper\Fixture\Element\FifthElement; use Rector\Tests\Skipper\Skipper\Fixture\Element\ThreeMan; @@ -41,8 +40,7 @@ protected function setUp(): void $this->missConfigurationReporter = new MissConfigurationReporter( $symfonyStyle, new VendorMissAnalyseGuard(), - $this->make(SkippedClassResolver::class), - $this->make(SkippedPathsResolver::class), + $this->make(UnusedSkipResolver::class), ); SimpleParameterProvider::setParameter(Option::SKIP, [ diff --git a/tests/Reporting/UnusedSkipResolverTest.php b/tests/Reporting/UnusedSkipResolverTest.php new file mode 100644 index 00000000000..756db90cdee --- /dev/null +++ b/tests/Reporting/UnusedSkipResolverTest.php @@ -0,0 +1,74 @@ +unusedSkipResolver = $this->make(UnusedSkipResolver::class); + + SimpleParameterProvider::setParameter(Option::SKIP, [ + // skip-everywhere rule skip - forgotten at boot, not trackable, excluded + ThreeMan::class, + // rule-scoped path skip, never matched - intentional, reported even as mask + FifthElement::class => [self::UNUSED_RULE_MASK], + // rule-scoped path skip, matched - excluded + AnotherClassToSkip::class => [self::USED_RULE_MASK], + // global mask path skip - hard to spot, false-positive prone, excluded + self::GLOBAL_MASK, + ]); + } + + protected function tearDown(): void + { + SimpleParameterProvider::setParameter(Option::SKIP, []); + SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, false); + } + + public function testResolvesUnusedSkipsAsRuleAndPath(): void + { + SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, true); + + $processResult = new ProcessResult([], [], 0, [self::USED_RULE_MASK]); + $unusedSkips = $this->unusedSkipResolver->resolve($processResult); + + // rule-scoped unused skip is reported as "rule => path" + $this->assertContains(FifthElement::class . ' => ' . self::UNUSED_RULE_MASK, $unusedSkips); + + // matched rule-scoped skip, global mask and skip-everywhere rule are excluded + $this->assertNotContains(AnotherClassToSkip::class . ' => ' . self::USED_RULE_MASK, $unusedSkips); + $this->assertNotContains(self::GLOBAL_MASK, $unusedSkips); + $this->assertNotContains(ThreeMan::class, $unusedSkips); + } + + public function testResolvesNothingWhenDisabled(): void + { + SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, false); + + $processResult = new ProcessResult([], [], 0, []); + + $this->assertSame([], $this->unusedSkipResolver->resolve($processResult)); + } +}