Skip to content

Commit 2328ea6

Browse files
TomasVotrubaTomasVotruba
authored
[skip] track used skips as class => [paths] map (#8074)
1 parent 19dcdb7 commit 2328ea6

8 files changed

Lines changed: 72 additions & 37 deletions

File tree

‎src/Parallel/Application/ParallelFileProcessor.php‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public function process(
7575
/** @var SystemError[] $systemErrors */
7676
$systemErrors = [];
7777

78-
/** @var array<string, true> $usedSkips */
78+
/** @var array<string, array<string, true>> $usedSkips */
7979
$usedSkips = [];
8080

8181
$tcpServer = new TcpServer('127.0.0.1:0', $streamSelectLoop);
@@ -196,12 +196,15 @@ function (array $json) use (
196196
* file_diffs: array<string, mixed>,
197197
* files_count: int,
198198
* system_errors_count: int,
199-
* used_skips: string[]
199+
* used_skips: array<string, string[]>
200200
* } $json */
201201
$totalChanged += $json[Bridge::TOTAL_CHANGED];
202202

203-
foreach ($json[Bridge::USED_SKIPS] as $usedSkip) {
204-
$usedSkips[$usedSkip] = true;
203+
foreach ($json[Bridge::USED_SKIPS] as $skip => $paths) {
204+
$usedSkips[$skip] ??= [];
205+
foreach ($paths as $path) {
206+
$usedSkips[$skip][$path] = true;
207+
}
205208
}
206209

207210
// decode arrays to objects
@@ -286,6 +289,11 @@ function ($exitCode, string $stdErr) use (&$systemErrors, $processIdentifier): v
286289
));
287290
}
288291

289-
return new ProcessResult($systemErrors, $fileDiffs, $totalChanged, array_keys($usedSkips));
292+
$mergedUsedSkips = [];
293+
foreach ($usedSkips as $skip => $paths) {
294+
$mergedUsedSkips[$skip] = array_keys($paths);
295+
}
296+
297+
return new ProcessResult($systemErrors, $fileDiffs, $totalChanged, $mergedUsedSkips);
290298
}
291299
}

‎src/Reporting/UnusedSkipResolver.php‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ private function resolveGlobalRelativePaths(): array
106106
}
107107

108108
/**
109-
* @param string[] $usedSkips
109+
* @param array<string, string[]> $usedSkips
110110
* @return string[]
111111
*/
112112
private function resolveUnusedRuleScopedSkips(array $usedSkips): array
@@ -117,9 +117,9 @@ private function resolveUnusedRuleScopedSkips(array $usedSkips): array
117117
foreach ($this->resolveRelativePathsByClass() as $rectorClass => $relativePaths) {
118118
$unusedRelativePaths = [];
119119
foreach ($relativePaths as $path => $relativePath) {
120-
// used skips are tracked scoped to their rule as "class|path", so the same path
120+
// used skips are tracked scoped to their rule (class => [path]), so the same path
121121
// skipped under another rule does not mark this one used (see SkipSkipper)
122-
if (! in_array($rectorClass . '|' . $path, $usedSkips, true)) {
122+
if (! in_array($path, $usedSkips[$rectorClass] ?? [], true)) {
123123
$unusedRelativePaths[] = $relativePath;
124124
}
125125
}
@@ -136,14 +136,15 @@ private function resolveUnusedRuleScopedSkips(array $usedSkips): array
136136
}
137137

138138
/**
139-
* @param string[] $usedSkips
139+
* @param array<string, string[]> $usedSkips
140140
* @return string[]
141141
*/
142142
private function resolveUnusedGlobalSkips(array $usedSkips): array
143143
{
144144
$unusedSkips = [];
145145
foreach ($this->resolveGlobalRelativePaths() as $path => $relativePath) {
146-
if (! in_array($path, $usedSkips, true)) {
146+
// global path skips are tracked by their own path key, with no nested paths
147+
if (! array_key_exists($path, $usedSkips)) {
147148
$unusedSkips[] = $relativePath;
148149
}
149150
}

‎src/Skipper/Skipper/SkipSkipper.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,11 @@ public function doesMatchSkip(object | string $checker, string $filePath, array
3030
return true;
3131
}
3232

33-
// mark the specific matched path used, scoped to its rule via "class|path" - the same
34-
// path can be skipped under multiple rules, so a path-only key would mark them all used
33+
// mark the specific matched path used, scoped to its rule - the same path can be skipped
34+
// under multiple rules, so the path is nested under its rule, not tracked on its own
3535
$matchedPath = $this->fileInfoMatcher->matchPattern($filePath, $skippedFiles);
3636
if ($matchedPath !== null) {
37-
$this->usedSkipCollector->markUsed($skippedClass . '|' . $matchedPath);
37+
$this->usedSkipCollector->markUsed($skippedClass, $matchedPath);
3838
return true;
3939
}
4040
}

‎src/Skipper/Skipper/UsedSkipCollector.php‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,20 +13,33 @@
1313
final class UsedSkipCollector
1414
{
1515
/**
16-
* @var array<string, true>
16+
* Map of skip element (rule class or global path) to the set of paths matched under it.
17+
* Rule-scoped skips collect their matched paths; skip-everywhere rules and global path skips
18+
* keep an empty path set, the same shape as the "->withSkip()" config.
19+
*
20+
* @var array<string, array<string, true>>
1721
*/
1822
private array $usedSkips = [];
1923

20-
public function markUsed(string $skip): void
24+
public function markUsed(string $skip, ?string $path = null): void
2125
{
22-
$this->usedSkips[$skip] = true;
26+
$this->usedSkips[$skip] ??= [];
27+
28+
if ($path !== null) {
29+
$this->usedSkips[$skip][$path] = true;
30+
}
2331
}
2432

2533
/**
26-
* @return string[]
34+
* @return array<string, string[]>
2735
*/
2836
public function provide(): array
2937
{
30-
return array_keys($this->usedSkips);
38+
$usedSkips = [];
39+
foreach ($this->usedSkips as $skip => $paths) {
40+
$usedSkips[$skip] = array_keys($paths);
41+
}
42+
43+
return $usedSkips;
3144
}
3245
}

‎src/ValueObject/ProcessResult.php‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ final class ProcessResult
1414
/**
1515
* @param SystemError[] $systemErrors
1616
* @param FileDiff[] $fileDiffs
17-
* @param string[] $usedSkips
17+
* @param array<string, string[]> $usedSkips
1818
*/
1919
public function __construct(
2020
private array $systemErrors,
@@ -24,7 +24,11 @@ public function __construct(
2424
) {
2525
Assert::allIsInstanceOf($systemErrors, SystemError::class);
2626
Assert::allIsInstanceOf($fileDiffs, FileDiff::class);
27-
Assert::allString($usedSkips);
27+
28+
Assert::allString(array_keys($usedSkips));
29+
foreach ($usedSkips as $usedSkip) {
30+
Assert::allString($usedSkip);
31+
}
2832
}
2933

3034
/**
@@ -62,13 +66,16 @@ public function addSystemErrors(array $systemErrors): void
6266
* their result from worker processes only. Merge those main-process marks back in, or they would
6367
* be wrongly reported as unused.
6468
*
65-
* @param string[] $usedSkips
69+
* @param array<string, string[]> $usedSkips
6670
*/
6771
public function addUsedSkips(array $usedSkips): void
6872
{
69-
Assert::allString($usedSkips);
73+
foreach ($usedSkips as $skip => $paths) {
74+
Assert::allString($paths);
7075

71-
$this->usedSkips = array_values(array_unique([...$this->usedSkips, ...$usedSkips]));
76+
$existingPaths = $this->usedSkips[$skip] ?? [];
77+
$this->usedSkips[$skip] = array_values(array_unique([...$existingPaths, ...$paths]));
78+
}
7279
}
7380

7481
public function getTotalChanged(): int
@@ -77,7 +84,7 @@ public function getTotalChanged(): int
7784
}
7885

7986
/**
80-
* @return string[]
87+
* @return array<string, string[]>
8188
*/
8289
public function getUsedSkips(): array
8390
{

‎tests/Reporting/MissConfigurationReporterTest.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,10 @@ public function testReportsOnlyTrackableUnusedSkips(): void
6565
{
6666
SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, true);
6767

68-
// matched rule-scoped path is marked used scoped to its rule as "class|path"
69-
$processResult = new ProcessResult([], [], 0, [AnotherClassToSkip::class . '|' . self::USED_RULE_MASK]);
68+
// matched rule-scoped path is marked used scoped to its rule (class => [path])
69+
$processResult = new ProcessResult([], [], 0, [
70+
AnotherClassToSkip::class => [self::USED_RULE_MASK],
71+
]);
7072
$this->missConfigurationReporter->reportUnusedSkips($processResult);
7173

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

‎tests/Reporting/UnusedSkipResolverTest.php‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,10 @@ public function testResolvesUnusedSkipsAsRuleAndPath(): void
5353
{
5454
SimpleParameterProvider::setParameter(Option::REPORT_UNUSED_SKIPS, true);
5555

56-
// used skips are tracked scoped to their rule as "class|path"
57-
$processResult = new ProcessResult([], [], 0, [AnotherClassToSkip::class . '|' . self::USED_RULE_MASK]);
56+
// used skips are tracked scoped to their rule (class => [path])
57+
$processResult = new ProcessResult([], [], 0, [
58+
AnotherClassToSkip::class => [self::USED_RULE_MASK],
59+
]);
5860
$unusedSkips = $this->unusedSkipResolver->resolve($processResult);
5961

6062
// rule-scoped unused skip is reported as "rule:" with the path nested below
@@ -78,7 +80,9 @@ public function testSamePathUnderAnotherRuleDoesNotMarkSkipUsed(): void
7880
]);
7981

8082
// used skip is scoped to its rule, so the shared path stays unused under the other rule
81-
$processResult = new ProcessResult([], [], 0, [AnotherClassToSkip::class . '|' . $sharedMask]);
83+
$processResult = new ProcessResult([], [], 0, [
84+
AnotherClassToSkip::class => [$sharedMask],
85+
]);
8286
$unusedSkips = $this->unusedSkipResolver->resolve($processResult);
8387

8488
// the never-matched rule still reports its shared path as unused

‎tests/Skipper/Skipper/UsedSkipCollectorTest.php‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -54,15 +54,15 @@ public function testCollectsOnlyMatchedSkips(): void
5454

5555
$usedSkips = $this->usedSkipCollector->provide();
5656

57-
// matched skips are collected
58-
$this->assertContains(FifthElement::class, $usedSkips);
57+
// matched skips are collected as keys
58+
$this->assertArrayHasKey(FifthElement::class, $usedSkips);
5959
$this->assertNotEmpty(array_filter(
60-
$usedSkips,
60+
array_keys($usedSkips),
6161
static fn (string $usedSkip): bool => str_ends_with($usedSkip, 'SomeSkippedPath')
6262
));
6363

6464
// unmatched skip is never collected
65-
$this->assertNotContains(self::UNUSED_SKIP_MARKER, $usedSkips);
65+
$this->assertArrayNotHasKey(self::UNUSED_SKIP_MARKER, $usedSkips);
6666
}
6767

6868
public function testCollectsMatchedPathNotRuleClassForRuleScopedSkip(): void
@@ -83,11 +83,11 @@ public function testCollectsMatchedPathNotRuleClassForRuleScopedSkip(): void
8383

8484
$usedSkips = $this->usedSkipCollector->provide();
8585

86-
// the specific matched path is collected scoped to its rule as "class|path"
87-
$this->assertContains(AnotherClassToSkip::class . '|' . '*/someDirectory/*', $usedSkips);
88-
$this->assertNotContains(AnotherClassToSkip::class, $usedSkips);
86+
// the specific matched path is collected nested under its rule class
87+
$this->assertArrayHasKey(AnotherClassToSkip::class, $usedSkips);
88+
$this->assertContains('*/someDirectory/*', $usedSkips[AnotherClassToSkip::class]);
8989

9090
// the unmatched sibling path under the same rule is never collected
91-
$this->assertNotContains(AnotherClassToSkip::class . '|' . self::UNUSED_SKIP_MARKER, $usedSkips);
91+
$this->assertNotContains(self::UNUSED_SKIP_MARKER, $usedSkips[AnotherClassToSkip::class]);
9292
}
9393
}

0 commit comments

Comments
 (0)