Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions e2e/parallel-unused-skips/custom/config/rector.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@

$rectorConfig->reportUnusedSkips();

// matched in the worker - must be excluded from the unused report (proves parallel aggregation)
// plus a glob that never matches - must be reported as unused
// 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
$rectorConfig->skip([
SimplifyUselessVariableRector::class => ['*/src/*'],
'*/NonexistentUnused/*',
SimplifyUselessVariableRector::class => [
dirname(__DIR__, 2) . '/src/SomeClass.php',
dirname(__DIR__, 2) . '/src/NonexistentUnused.php',
],
]);
};
2 changes: 1 addition & 1 deletion e2e/parallel-unused-skips/expected-output.diff
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
[WARNING] This skip is unused, it never matched any element. You can remove it
from "->withSkip()"

* */NonexistentUnused/*
* ./parallel-unused-skips/src/NonexistentUnused.php
22 changes: 16 additions & 6 deletions src/Reporting/MissConfigurationReporter.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,24 @@ public function reportUnusedSkips(ProcessResult $processResult): void
return;
}

// only path-scoped class skips are trackable at runtime; skip-everywhere rule skips
// 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
$pathScopedClassSkips = array_keys(array_filter(
$this->skippedClassResolver->resolve(),
static fn (?array $paths): bool => $paths !== null
));
$skippedClassPaths = [];
foreach ($this->skippedClassResolver->resolve() as $paths) {
if ($paths === null) {
continue;
}

$skippedClassPaths = [...$skippedClassPaths, ...$paths];
}

$configuredSkips = [...$pathScopedClassSkips, ...$this->skippedPathsResolver->resolve()];
$configuredSkips = [...$skippedClassPaths, ...$this->skippedPathsResolver->resolve()];

// skip mask paths like "*/some/*"; they are hard to spot and report false positives
$configuredSkips = array_filter(
$configuredSkips,
static fn (string $skip): bool => ! str_contains($skip, '*')
);

$unusedSkips = array_values(array_diff($configuredSkips, $processResult->getUsedSkips()));
if ($unusedSkips === []) {
Expand Down
21 changes: 16 additions & 5 deletions src/Skipper/Matcher/FileInfoMatcher.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,26 @@ public function __construct(
*/
public function doesFileInfoMatchPatterns(string $filePath, array $filePatterns): bool
{
$filePath = PathNormalizer::normalize($filePath);
return $this->matchPattern($filePath, $filePatterns) !== null;
}

/**
* Returns the original (un-normalized) pattern that matched, so callers can report the exact
* configured path. Returns null when no pattern matches.
*
* @param string[] $filePatterns
*/
public function matchPattern(string $filePath, array $filePatterns): ?string
{
$normalizedFilePath = PathNormalizer::normalize($filePath);
foreach ($filePatterns as $filePattern) {
$filePattern = PathNormalizer::normalize($filePattern);
if ($this->doesFileMatchPattern($filePath, $filePattern)) {
return true;
$normalizedFilePattern = PathNormalizer::normalize($filePattern);
if ($this->doesFileMatchPattern($normalizedFilePath, $normalizedFilePattern)) {
return $filePattern;
}
}

return false;
return null;
}

/**
Expand Down
6 changes: 4 additions & 2 deletions src/Skipper/Skipper/SkipSkipper.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ public function doesMatchSkip(object | string $checker, string $filePath, array
return true;
}

if ($this->fileInfoMatcher->doesFileInfoMatchPatterns($filePath, $skippedFiles)) {
$this->usedSkipCollector->markUsed($skippedClass);
// mark the specific matched path used, so unused paths under the same rule are reported
$matchedPath = $this->fileInfoMatcher->matchPattern($filePath, $skippedFiles);
if ($matchedPath !== null) {
$this->usedSkipCollector->markUsed($matchedPath);
return true;
}
}
Expand Down
35 changes: 21 additions & 14 deletions tests/Reporting/MissConfigurationReporterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@

final class MissConfigurationReporterTest extends AbstractLazyTestCase
{
private const string UNUSED_GLOB = '*/UnusedGlobMarker/*';
private const string UNUSED_ENTITY_PATH = '/app/bundles/UserBundle/Entity';

private const string USED_ENTITY_PATH = '/app/bundles/AdminBundle/Entity';

private const string MASK_PATH = '*/SomeMask/*';

private BufferedOutput $bufferedOutput;

Expand All @@ -44,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,
// path-scoped class skip, never matched - must be reported
FifthElement::class => ['*/some/*'],
// path-scoped class skip, matched - must not be reported
AnotherClassToSkip::class => ['*/other/*'],
// glob path skip, never matched - must be reported
self::UNUSED_GLOB,
// 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,
]);
}

Expand All @@ -63,20 +67,23 @@ public function testReportsOnlyTrackableUnusedSkips(): void
{
SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, true);

$processResult = new ProcessResult([], [], 0, [AnotherClassToSkip::class]);
// matched path is marked used by its concrete path, not by the rule class
$processResult = new ProcessResult([], [], 0, [self::USED_ENTITY_PATH]);
$this->missConfigurationReporter->reportUnusedSkips($processResult);

$output = $this->bufferedOutput->fetch();

// unused, trackable skips are reported
$this->assertStringContainsString('FifthElement', $output);
$this->assertStringContainsString('UnusedGlobMarker', $output);
// unused concrete path is reported by path, not rule class
$this->assertStringContainsString('UserBundle', $output);

// matched concrete path is excluded
$this->assertStringNotContainsString('AdminBundle', $output);

// mask path is excluded (hard to spot, false-positive prone)
$this->assertStringNotContainsString('SomeMask', $output);

// skip-everywhere rule skip is excluded (not trackable at runtime)
$this->assertStringNotContainsString('ThreeMan', $output);

// matched skip is excluded
$this->assertStringNotContainsString('AnotherClassToSkip', $output);
}

public function testReportsNothingWhenDisabled(): void
Expand Down
Loading