Skip to content

Commit dabe99a

Browse files
committed
Fix unmatched baseline entries in time linear in the baseline size
Fixing unmatched ignoreErrors entries and too high counts took time quadratic in the number of entries: 18.7 s for 8,000 unmatched entries of one path. - The entries are indexed by path and by the value of the first matched field, so findEntries() no longer goes through every entry of the path for each error. - Removing a NEON entry after the first one of a block list, and lowering a count, are edits of the tokens of the entry. Printing the whole ignoreErrors list again for each error with the format-preserving printer was linear in its length. The edit covers only the lines of the entry, so neighbouring entries are removed in the same round instead of conflicting. 8,000 unmatched entries now take 1.8 s, 20,000 take 4.0 s. The fixed config files are the same as before. Amp-Thread-ID: https://ampcode.com/threads/T-01a1b1c2-4005-7120-a2fa-17285a96ea5b
1 parent d9a7e64 commit dabe99a

3 files changed

Lines changed: 271 additions & 20 deletions

File tree

‎.github/workflows/e2e-tests.yml‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1013,10 +1013,9 @@ jobs:
10131013
../bashunit assert contains 'Errors cannot be fixed when generating the baseline.' "$OUTPUT"
10141014
OUTPUT=$(../bashunit assert exit_code "0" "../../bin/phpstan analyse --fix")
10151015
echo "$OUTPUT"
1016-
# removing the last two messages of the PHP list conflicts, and so does removing the NEON entry
1017-
# and the first message of the entry after it, and so does removing the two neighbouring entries without a path,
1018-
# the second one goes in the next round
1019-
../bashunit assert contains 'Round 2: fixed 3 errors in 2 files' "$OUTPUT"
1016+
# removing the last two messages of the PHP list conflicts, the second one goes in the next round;
1017+
# a NEON entry after the first one is removed with only its own lines, it does not conflict with its neighbours
1018+
../bashunit assert contains 'Round 2: fixed 1 error in 1 file' "$OUTPUT"
10201019
../bashunit assert contains '[OK] 9 errors fixed in 2 files' "$OUTPUT"
10211020
diff -u baseline-expected.php baseline.php
10221021
diff -u baseline-expected.neon baseline.neon

‎src/Analyser/Ignore/IgnoreErrorsConfigFixer.php‎

Lines changed: 213 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,10 @@
99
use Nette\Neon\Node as NeonNode;
1010
use Nette\Neon\Node\ArrayItemNode as NeonArrayItemNode;
1111
use Nette\Neon\Node\ArrayNode as NeonArrayNode;
12+
use Nette\Neon\Node\BlockArrayNode as NeonBlockArrayNode;
1213
use Nette\Neon\Node\LiteralNode as NeonLiteralNode;
1314
use Nette\Neon\Parser as NeonParser;
15+
use Nette\Neon\Token as NeonToken;
1416
use Nette\Neon\TokenStream as NeonTokenStream;
1517
use Nette\Utils\Strings;
1618
use PhpParser\ConstExprEvaluationException;
@@ -36,12 +38,14 @@
3638
use function array_pop;
3739
use function array_search;
3840
use function array_slice;
41+
use function array_unique;
3942
use function array_values;
4043
use function count;
4144
use function dirname;
4245
use function hash;
4346
use function implode;
4447
use function in_array;
48+
use function intdiv;
4549
use function is_array;
4650
use function is_int;
4751
use function is_string;
@@ -93,6 +97,7 @@
9397
* items: array<string, NeonArrayItemNode>,
9498
* path: non-empty-list<NeonNode>,
9599
* entryValue: NeonNode,
100+
* previousItem: NeonArrayItemNode|null,
96101
* line: int,
97102
* }
98103
* @phpstan-type ConfigEntry = PhpConfigEntry|NeonConfigEntry
@@ -113,13 +118,26 @@ final class IgnoreErrorsConfigFixer
113118
'identifier' => 'identifiers',
114119
];
115120

116-
/** @var array<string, list<ConfigEntry>>|null entries by normalized path, or by self::NO_PATH */
121+
/** key of the entries with none of the MATCHED_FIELDS in getEntriesByPath() */
122+
private const NO_MATCHED_FIELD = "\0";
123+
124+
/**
125+
* Entries by normalized path, or by self::NO_PATH, and by getMatchKeys(). A baseline can have
126+
* thousands of entries with the same path, scanning all of them for each unmatched error would be quadratic.
127+
*
128+
* @var array<string, array<string, list<ConfigEntry>>>|null
129+
*/
117130
private ?array $entriesByPath = null;
118131

119132
/** @var array<string, array{code: string, hash: string}> contents of the config files the entries were found in */
120133
private array $files = [];
121134

122-
/** @var array<string, array{tokens: NeonTokenStream, root: NeonNode, codeWithoutCr: string, crlf: bool, mixedLineBreaks: bool}> parsed NEON config files */
135+
/**
136+
* Parsed NEON config files. The offsets of the tokens in the code without \r, and the offsets of its line breaks,
137+
* let an edit of one entry be made without printing the whole ignoreErrors list again.
138+
*
139+
* @var array<string, array{tokens: NeonTokenStream, root: NeonNode, codeWithoutCr: string, crlf: bool, mixedLineBreaks: bool, tokenList: list<NeonToken>, offsets: list<int>, lineBreaks: list<int>}>
140+
*/
123141
private array $neonFiles = [];
124142

125143
/**
@@ -167,10 +185,12 @@ public function removeUnmatched(string|array $ignoreError): array
167185
{
168186
$editsByFile = [];
169187
$neonChangesByFile = [];
188+
$neonRemovedEntriesByFile = [];
170189
foreach ($this->findEntries($ignoreError) as [$entry, $listField, $position]) {
171190
if ($entry['format'] === 'neon') {
172191
if ($listField === null || $position === null) {
173192
$neonChangesByFile[$entry['file']][] = [$entry['path'], null];
193+
$neonRemovedEntriesByFile[$entry['file']][] = $entry;
174194
continue;
175195
}
176196

@@ -215,7 +235,10 @@ public function removeUnmatched(string|array $ignoreError): array
215235
}
216236

217237
foreach ($neonChangesByFile as $file => $changes) {
218-
$edit = $this->createNeonEdit($file, $changes);
238+
// more changes of one file are printed together, they could remove all entries of the list
239+
$edit = count($changes) === 1 && count($neonRemovedEntriesByFile[$file] ?? []) === 1
240+
? $this->createNeonRemovalEdit($neonRemovedEntriesByFile[$file][0]) ?? $this->createNeonEdit($file, $changes)
241+
: $this->createNeonEdit($file, $changes);
219242
if ($edit === null) {
220243
return [];
221244
}
@@ -252,7 +275,7 @@ public function lowerCount(array $ignoreError, int $count): array
252275
return [];
253276
}
254277

255-
$edit = $this->createNeonEdit($entry['file'], [
278+
$edit = $this->createNeonCountEdit($entry['file'], $countItem->value, $count) ?? $this->createNeonEdit($entry['file'], [
256279
[[...$entry['path'], $entry['entryValue'], $countItem, $countItem->value], new NeonLiteralNode($count)],
257280
]);
258281

@@ -398,8 +421,16 @@ private function findEntries(string|array $ignoreError): array
398421
return [];
399422
}
400423

424+
$matchKey = self::NO_MATCHED_FIELD;
425+
foreach (array_keys(self::MATCHED_FIELDS) as $field) {
426+
if (isset($ignoreError[$field])) {
427+
$matchKey = $field . "\0" . $ignoreError[$field];
428+
break;
429+
}
430+
}
431+
401432
$found = [];
402-
foreach ($this->getEntriesByPath()[$path !== null ? $this->normalizePath($path) : self::NO_PATH] ?? [] as $entry) {
433+
foreach ($this->getEntriesByPath()[$path !== null ? $this->normalizePath($path) : self::NO_PATH][$matchKey] ?? [] as $entry) {
403434
$listField = null;
404435
$position = null;
405436
foreach (self::MATCHED_FIELDS as $field => $matchedListField) {
@@ -443,7 +474,7 @@ private function findEntries(string|array $ignoreError): array
443474
}
444475

445476
/**
446-
* @return array<string, list<ConfigEntry>>
477+
* @return array<string, array<string, list<ConfigEntry>>>
447478
*/
448479
private function getEntriesByPath(): array
449480
{
@@ -462,21 +493,58 @@ private function getEntriesByPath(): array
462493
$path = $entry['values']['path'] ?? null;
463494
if ($path === null) {
464495
if (!isset($entry['values']['paths'])) {
465-
$this->entriesByPath[self::NO_PATH][] = $entry;
496+
foreach (self::getMatchKeys($entry['values']) as $matchKey) {
497+
$this->entriesByPath[self::NO_PATH][$matchKey][] = $entry;
498+
}
466499
}
467500
continue;
468501
}
469502
if (!is_string($path)) {
470503
continue;
471504
}
472505

473-
$this->entriesByPath[$this->normalizePath($path)][] = $entry;
506+
$normalizedPath = $this->normalizePath($path);
507+
foreach (self::getMatchKeys($entry['values']) as $matchKey) {
508+
$this->entriesByPath[$normalizedPath][$matchKey][] = $entry;
509+
}
474510
}
475511
}
476512

477513
return $this->entriesByPath;
478514
}
479515

516+
/**
517+
* findEntries() matches an error only with an entry that has the first of the MATCHED_FIELDS the error has,
518+
* and none of the fields before it. The entry is found by that field and each value it can match.
519+
*
520+
* @param array<string, mixed> $values
521+
* @return list<string>
522+
*/
523+
private static function getMatchKeys(array $values): array
524+
{
525+
foreach (self::MATCHED_FIELDS as $field => $listField) {
526+
if (!array_key_exists($field, $values) && !array_key_exists($listField, $values)) {
527+
continue;
528+
}
529+
530+
$matched = [];
531+
if (is_string($values[$field] ?? null)) {
532+
$matched[] = $values[$field];
533+
}
534+
foreach (is_array($values[$listField] ?? null) ? $values[$listField] : [] as $listValue) {
535+
if (!is_string($listValue)) {
536+
continue;
537+
}
538+
539+
$matched[] = $listValue;
540+
}
541+
542+
return array_values(array_unique(array_map(static fn (string $value): string => $field . "\0" . $value, $matched)));
543+
}
544+
545+
return [self::NO_MATCHED_FIELD];
546+
}
547+
480548
/**
481549
* Every array that looks like an ignoreErrors entry, wherever it is in the file. Such an array is removable
482550
* when it is an item of another array, or when it is added in a statement like `$ignoreErrors[] = [...];`.
@@ -626,20 +694,40 @@ private function findNeonEntries(string $file): array
626694

627695
$this->files[$file] = ['code' => $code, 'hash' => hash('sha256', $code)];
628696
$codeWithoutCr = str_replace("\r", '', $code);
697+
$tokenList = array_values($tokens->getTokens());
698+
$offsets = [];
699+
$offset = 0;
700+
foreach ($tokenList as $token) {
701+
$offsets[] = $offset;
702+
$offset += strlen($token->value);
703+
}
704+
$offsets[] = $offset;
705+
$lineBreaks = [];
706+
$lineBreak = strpos($codeWithoutCr, "\n");
707+
while ($lineBreak !== false) {
708+
$lineBreaks[] = $lineBreak;
709+
$lineBreak = strpos($codeWithoutCr, "\n", $lineBreak + 1);
710+
}
629711
$this->neonFiles[$file] = [
630712
'tokens' => $tokens,
631713
'root' => $root,
632714
'codeWithoutCr' => $codeWithoutCr,
633715
// nette/neon removes \r, it is put back when every line break had it
634716
'crlf' => $codeWithoutCr !== $code,
635717
'mixedLineBreaks' => $codeWithoutCr !== $code && str_replace("\n", "\r\n", $codeWithoutCr) !== $code,
718+
'tokenList' => $tokenList,
719+
'offsets' => $offsets,
720+
'lineBreaks' => $lineBreaks,
636721
];
637722

638723
$parameters = $this->getParameters();
639724
// relative paths are resolved like when the config file is loaded
640725
$adapter = new NeonAdapter(['[parameters][ignoreErrors][][path]', '[parameters][ignoreErrors][][paths][]'], $parameters);
641726
$entries = [];
727+
$previousItem = null;
642728
foreach ($ignoreErrorsItem->value->items as $entryItem) {
729+
$entryPreviousItem = $previousItem;
730+
$previousItem = $entryItem;
643731
$items = [];
644732
// a plain string pattern has no items
645733
foreach ($entryItem->value instanceof NeonArrayNode ? $entryItem->value->items : [] as $item) {
@@ -673,6 +761,7 @@ private function findNeonEntries(string $file): array
673761
'items' => $items,
674762
'path' => [$root, $parametersItem, $parametersItem->value, $ignoreErrorsItem, $ignoreErrorsItem->value, $entryItem],
675763
'entryValue' => $entryItem->value,
764+
'previousItem' => $ignoreErrorsItem->value instanceof NeonBlockArrayNode ? $entryPreviousItem : null,
676765
'line' => $entryItem->startLine ?? 1,
677766
];
678767
}
@@ -704,7 +793,7 @@ private static function findNeonItem(NeonNode $node, string $key): ?NeonArrayIte
704793
*/
705794
private function createNeonEdit(string $file, array $changes): ?array
706795
{
707-
['tokens' => $tokens, 'root' => $root, 'codeWithoutCr' => $codeWithoutCr, 'crlf' => $crlf, 'mixedLineBreaks' => $mixedLineBreaks] = $this->neonFiles[$file];
796+
['tokens' => $tokens, 'root' => $root, 'codeWithoutCr' => $codeWithoutCr, 'mixedLineBreaks' => $mixedLineBreaks] = $this->neonFiles[$file];
708797
if ($mixedLineBreaks) {
709798
return null;
710799
}
@@ -726,21 +815,131 @@ private function createNeonEdit(string $file, array $changes): ?array
726815
$replacement .= substr($codeWithoutCr, $offset, $printedChange['start'] - $offset) . $printedChange['replacement'];
727816
$offset = $printedChange['end'];
728817
}
729-
$end = $offset;
730-
if ($crlf) {
731-
$start += substr_count($codeWithoutCr, "\n", length: $start);
732-
$end += substr_count($codeWithoutCr, "\n", length: $end);
818+
819+
return $this->createEditOfCodeWithoutCr($file, $start, $offset, $replacement);
820+
}
821+
822+
/**
823+
* Removes a whole entry of a block list with its lines, and with the lines between it and the previous entry,
824+
* like comments, the same as printing the list without it. Unlike printing the list, the time does not grow
825+
* with its length, and the edit covers only the lines of the entry, so that the next entry can be removed
826+
* in the same round. The first entry is left to the printer, it removes the empty lines after it as well.
827+
*
828+
* @param NeonConfigEntry $entry
829+
* @return Edit|null
830+
*/
831+
private function createNeonRemovalEdit(array $entry): ?array
832+
{
833+
$previousItem = $entry['previousItem'];
834+
$entryItem = $entry['path'][count($entry['path']) - 1];
835+
if (
836+
$previousItem === null
837+
|| $previousItem->endTokenPos === null
838+
|| $entryItem->startTokenPos === null
839+
|| $entryItem->endTokenPos === null
840+
|| $this->neonFiles[$entry['file']]['mixedLineBreaks']
841+
) {
842+
return null;
843+
}
844+
845+
['tokenList' => $tokens, 'offsets' => $offsets, 'codeWithoutCr' => $codeWithoutCr] = $this->neonFiles[$entry['file']];
846+
$previousLineBreak = self::findNeonLineBreak($tokens, $previousItem->endTokenPos + 1);
847+
for ($i = $previousLineBreak; $i < $entryItem->startTokenPos; $i++) {
848+
// only empty lines, comments and indentation are between the entries of a block list
849+
if (!in_array($tokens[$i]->type, [NeonToken::Newline, NeonToken::Whitespace, NeonToken::Comment], true)) {
850+
return null;
851+
}
852+
}
853+
if (!isset($tokens[$previousLineBreak]) || $previousLineBreak >= $entryItem->startTokenPos) {
854+
return null;
855+
}
856+
857+
$lineBreak = self::findNeonLineBreak($tokens, $entryItem->endTokenPos + 1);
858+
859+
return $this->createEditOfCodeWithoutCr(
860+
$entry['file'],
861+
$offsets[$previousLineBreak] + 1,
862+
isset($tokens[$lineBreak]) ? $offsets[$lineBreak] + 1 : strlen($codeWithoutCr),
863+
'',
864+
);
865+
}
866+
867+
/**
868+
* Replaces the count with a new literal, the same as the printer prints it.
869+
*
870+
* @return Edit|null
871+
*/
872+
private function createNeonCountEdit(string $file, NeonNode $countNode, int $count): ?array
873+
{
874+
['tokenList' => $tokens, 'offsets' => $offsets, 'mixedLineBreaks' => $mixedLineBreaks] = $this->neonFiles[$file];
875+
$pos = $countNode->startTokenPos;
876+
if (
877+
$mixedLineBreaks
878+
|| !$countNode instanceof NeonLiteralNode
879+
|| $pos === null
880+
|| $countNode->endTokenPos !== $pos
881+
|| ($tokens[$pos]->type ?? null) !== NeonToken::Literal
882+
) {
883+
return null;
884+
}
885+
886+
return $this->createEditOfCodeWithoutCr($file, $offsets[$pos], $offsets[$pos + 1], (string) $count);
887+
}
888+
889+
/**
890+
* Position of the first line break token from $pos, or the count of the tokens.
891+
*
892+
* @param list<NeonToken> $tokens
893+
*/
894+
private static function findNeonLineBreak(array $tokens, int $pos): int
895+
{
896+
while (isset($tokens[$pos]) && $tokens[$pos]->type !== NeonToken::Newline) {
897+
$pos++;
898+
}
899+
900+
return $pos;
901+
}
902+
903+
/**
904+
* @return Edit
905+
*/
906+
private function createEditOfCodeWithoutCr(string $file, int $start, int $end, string $replacement): array
907+
{
908+
$lineBreaksBeforeStart = $this->countNeonLineBreaksBefore($file, $start);
909+
if ($this->neonFiles[$file]['crlf']) {
910+
$end += $this->countNeonLineBreaksBefore($file, $end);
911+
$start += $lineBreaksBeforeStart;
733912
$replacement = str_replace("\n", "\r\n", $replacement);
734913
}
735914

736915
return [
737916
'start' => $start,
738917
'end' => $end,
739918
'replacement' => $replacement,
740-
'line' => substr_count($this->files[$file]['code'], "\n", length: $start) + 1,
919+
'line' => $lineBreaksBeforeStart + 1,
741920
];
742921
}
743922

923+
/**
924+
* The number of line breaks before the offset in the code without \r.
925+
*/
926+
private function countNeonLineBreaksBefore(string $file, int $offset): int
927+
{
928+
$lineBreaks = $this->neonFiles[$file]['lineBreaks'];
929+
$low = 0;
930+
$high = count($lineBreaks);
931+
while ($low < $high) {
932+
$mid = intdiv($low + $high, 2);
933+
if ($lineBreaks[$mid] < $offset) {
934+
$low = $mid + 1;
935+
} else {
936+
$high = $mid;
937+
}
938+
}
939+
940+
return $low;
941+
}
942+
744943
/**
745944
* Replaces the changed nodes and clones their ancestors, the original tree stays as it is.
746945
* The clones keep the token positions of the original nodes, so the printer knows what they stand for.

0 commit comments

Comments
 (0)