From 87dffb226f354bc215eaddca55e10755528f6bef Mon Sep 17 00:00:00 2001 From: "ondrej@mirtes.cz" Date: Fri, 6 Nov 2026 18:48:50 +0000 Subject: [PATCH] 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 --- .github/workflows/e2e-tests.yml | 7 +- .../Ignore/IgnoreErrorsConfigFixer.php | 227 ++++++++++++++++-- .../Ignore/IgnoreErrorsConfigFixerTest.php | 57 ++++- 3 files changed, 271 insertions(+), 20 deletions(-) diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index aa99478278d..3c4f774a775 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -1013,10 +1013,9 @@ jobs: ../bashunit assert contains 'Errors cannot be fixed when generating the baseline.' "$OUTPUT" OUTPUT=$(../bashunit assert exit_code "0" "../../bin/phpstan analyse --fix") echo "$OUTPUT" - # removing the last two messages of the PHP list conflicts, and so does removing the NEON entry - # and the first message of the entry after it, and so does removing the two neighbouring entries without a path, - # the second one goes in the next round - ../bashunit assert contains 'Round 2: fixed 3 errors in 2 files' "$OUTPUT" + # removing the last two messages of the PHP list conflicts, the second one goes in the next round; + # a NEON entry after the first one is removed with only its own lines, it does not conflict with its neighbours + ../bashunit assert contains 'Round 2: fixed 1 error in 1 file' "$OUTPUT" ../bashunit assert contains '[OK] 9 errors fixed in 2 files' "$OUTPUT" diff -u baseline-expected.php baseline.php diff -u baseline-expected.neon baseline.neon diff --git a/src/Analyser/Ignore/IgnoreErrorsConfigFixer.php b/src/Analyser/Ignore/IgnoreErrorsConfigFixer.php index 6a4ce9296aa..53efc463b1c 100644 --- a/src/Analyser/Ignore/IgnoreErrorsConfigFixer.php +++ b/src/Analyser/Ignore/IgnoreErrorsConfigFixer.php @@ -9,8 +9,10 @@ use Nette\Neon\Node as NeonNode; use Nette\Neon\Node\ArrayItemNode as NeonArrayItemNode; use Nette\Neon\Node\ArrayNode as NeonArrayNode; +use Nette\Neon\Node\BlockArrayNode as NeonBlockArrayNode; use Nette\Neon\Node\LiteralNode as NeonLiteralNode; use Nette\Neon\Parser as NeonParser; +use Nette\Neon\Token as NeonToken; use Nette\Neon\TokenStream as NeonTokenStream; use Nette\Utils\Strings; use PhpParser\ConstExprEvaluationException; @@ -36,12 +38,14 @@ use function array_pop; use function array_search; use function array_slice; +use function array_unique; use function array_values; use function count; use function dirname; use function hash; use function implode; use function in_array; +use function intdiv; use function is_array; use function is_int; use function is_string; @@ -93,6 +97,7 @@ * items: array, * path: non-empty-list, * entryValue: NeonNode, + * previousItem: NeonArrayItemNode|null, * line: int, * } * @phpstan-type ConfigEntry = PhpConfigEntry|NeonConfigEntry @@ -113,13 +118,26 @@ final class IgnoreErrorsConfigFixer 'identifier' => 'identifiers', ]; - /** @var array>|null entries by normalized path, or by self::NO_PATH */ + /** key of the entries with none of the MATCHED_FIELDS in getEntriesByPath() */ + private const NO_MATCHED_FIELD = "\0"; + + /** + * Entries by normalized path, or by self::NO_PATH, and by getMatchKeys(). A baseline can have + * thousands of entries with the same path, scanning all of them for each unmatched error would be quadratic. + * + * @var array>>|null + */ private ?array $entriesByPath = null; /** @var array contents of the config files the entries were found in */ private array $files = []; - /** @var array parsed NEON config files */ + /** + * Parsed NEON config files. The offsets of the tokens in the code without \r, and the offsets of its line breaks, + * let an edit of one entry be made without printing the whole ignoreErrors list again. + * + * @var array, offsets: list, lineBreaks: list}> + */ private array $neonFiles = []; /** @@ -167,10 +185,12 @@ public function removeUnmatched(string|array $ignoreError): array { $editsByFile = []; $neonChangesByFile = []; + $neonRemovedEntriesByFile = []; foreach ($this->findEntries($ignoreError) as [$entry, $listField, $position]) { if ($entry['format'] === 'neon') { if ($listField === null || $position === null) { $neonChangesByFile[$entry['file']][] = [$entry['path'], null]; + $neonRemovedEntriesByFile[$entry['file']][] = $entry; continue; } @@ -215,7 +235,10 @@ public function removeUnmatched(string|array $ignoreError): array } foreach ($neonChangesByFile as $file => $changes) { - $edit = $this->createNeonEdit($file, $changes); + // more changes of one file are printed together, they could remove all entries of the list + $edit = count($changes) === 1 && count($neonRemovedEntriesByFile[$file] ?? []) === 1 + ? $this->createNeonRemovalEdit($neonRemovedEntriesByFile[$file][0]) ?? $this->createNeonEdit($file, $changes) + : $this->createNeonEdit($file, $changes); if ($edit === null) { return []; } @@ -252,7 +275,7 @@ public function lowerCount(array $ignoreError, int $count): array return []; } - $edit = $this->createNeonEdit($entry['file'], [ + $edit = $this->createNeonCountEdit($entry['file'], $countItem->value, $count) ?? $this->createNeonEdit($entry['file'], [ [[...$entry['path'], $entry['entryValue'], $countItem, $countItem->value], new NeonLiteralNode($count)], ]); @@ -398,8 +421,16 @@ private function findEntries(string|array $ignoreError): array return []; } + $matchKey = self::NO_MATCHED_FIELD; + foreach (array_keys(self::MATCHED_FIELDS) as $field) { + if (isset($ignoreError[$field])) { + $matchKey = $field . "\0" . $ignoreError[$field]; + break; + } + } + $found = []; - foreach ($this->getEntriesByPath()[$path !== null ? $this->normalizePath($path) : self::NO_PATH] ?? [] as $entry) { + foreach ($this->getEntriesByPath()[$path !== null ? $this->normalizePath($path) : self::NO_PATH][$matchKey] ?? [] as $entry) { $listField = null; $position = null; foreach (self::MATCHED_FIELDS as $field => $matchedListField) { @@ -443,7 +474,7 @@ private function findEntries(string|array $ignoreError): array } /** - * @return array> + * @return array>> */ private function getEntriesByPath(): array { @@ -462,7 +493,9 @@ private function getEntriesByPath(): array $path = $entry['values']['path'] ?? null; if ($path === null) { if (!isset($entry['values']['paths'])) { - $this->entriesByPath[self::NO_PATH][] = $entry; + foreach (self::getMatchKeys($entry['values']) as $matchKey) { + $this->entriesByPath[self::NO_PATH][$matchKey][] = $entry; + } } continue; } @@ -470,13 +503,48 @@ private function getEntriesByPath(): array continue; } - $this->entriesByPath[$this->normalizePath($path)][] = $entry; + $normalizedPath = $this->normalizePath($path); + foreach (self::getMatchKeys($entry['values']) as $matchKey) { + $this->entriesByPath[$normalizedPath][$matchKey][] = $entry; + } } } return $this->entriesByPath; } + /** + * findEntries() matches an error only with an entry that has the first of the MATCHED_FIELDS the error has, + * and none of the fields before it. The entry is found by that field and each value it can match. + * + * @param array $values + * @return list + */ + private static function getMatchKeys(array $values): array + { + foreach (self::MATCHED_FIELDS as $field => $listField) { + if (!array_key_exists($field, $values) && !array_key_exists($listField, $values)) { + continue; + } + + $matched = []; + if (is_string($values[$field] ?? null)) { + $matched[] = $values[$field]; + } + foreach (is_array($values[$listField] ?? null) ? $values[$listField] : [] as $listValue) { + if (!is_string($listValue)) { + continue; + } + + $matched[] = $listValue; + } + + return array_values(array_unique(array_map(static fn (string $value): string => $field . "\0" . $value, $matched))); + } + + return [self::NO_MATCHED_FIELD]; + } + /** * Every array that looks like an ignoreErrors entry, wherever it is in the file. Such an array is removable * when it is an item of another array, or when it is added in a statement like `$ignoreErrors[] = [...];`. @@ -626,6 +694,20 @@ private function findNeonEntries(string $file): array $this->files[$file] = ['code' => $code, 'hash' => hash('sha256', $code)]; $codeWithoutCr = str_replace("\r", '', $code); + $tokenList = array_values($tokens->getTokens()); + $offsets = []; + $offset = 0; + foreach ($tokenList as $token) { + $offsets[] = $offset; + $offset += strlen($token->value); + } + $offsets[] = $offset; + $lineBreaks = []; + $lineBreak = strpos($codeWithoutCr, "\n"); + while ($lineBreak !== false) { + $lineBreaks[] = $lineBreak; + $lineBreak = strpos($codeWithoutCr, "\n", $lineBreak + 1); + } $this->neonFiles[$file] = [ 'tokens' => $tokens, 'root' => $root, @@ -633,13 +715,19 @@ private function findNeonEntries(string $file): array // nette/neon removes \r, it is put back when every line break had it 'crlf' => $codeWithoutCr !== $code, 'mixedLineBreaks' => $codeWithoutCr !== $code && str_replace("\n", "\r\n", $codeWithoutCr) !== $code, + 'tokenList' => $tokenList, + 'offsets' => $offsets, + 'lineBreaks' => $lineBreaks, ]; $parameters = $this->getParameters(); // relative paths are resolved like when the config file is loaded $adapter = new NeonAdapter(['[parameters][ignoreErrors][][path]', '[parameters][ignoreErrors][][paths][]'], $parameters); $entries = []; + $previousItem = null; foreach ($ignoreErrorsItem->value->items as $entryItem) { + $entryPreviousItem = $previousItem; + $previousItem = $entryItem; $items = []; // a plain string pattern has no items foreach ($entryItem->value instanceof NeonArrayNode ? $entryItem->value->items : [] as $item) { @@ -673,6 +761,7 @@ private function findNeonEntries(string $file): array 'items' => $items, 'path' => [$root, $parametersItem, $parametersItem->value, $ignoreErrorsItem, $ignoreErrorsItem->value, $entryItem], 'entryValue' => $entryItem->value, + 'previousItem' => $ignoreErrorsItem->value instanceof NeonBlockArrayNode ? $entryPreviousItem : null, 'line' => $entryItem->startLine ?? 1, ]; } @@ -704,7 +793,7 @@ private static function findNeonItem(NeonNode $node, string $key): ?NeonArrayIte */ private function createNeonEdit(string $file, array $changes): ?array { - ['tokens' => $tokens, 'root' => $root, 'codeWithoutCr' => $codeWithoutCr, 'crlf' => $crlf, 'mixedLineBreaks' => $mixedLineBreaks] = $this->neonFiles[$file]; + ['tokens' => $tokens, 'root' => $root, 'codeWithoutCr' => $codeWithoutCr, 'mixedLineBreaks' => $mixedLineBreaks] = $this->neonFiles[$file]; if ($mixedLineBreaks) { return null; } @@ -726,10 +815,100 @@ private function createNeonEdit(string $file, array $changes): ?array $replacement .= substr($codeWithoutCr, $offset, $printedChange['start'] - $offset) . $printedChange['replacement']; $offset = $printedChange['end']; } - $end = $offset; - if ($crlf) { - $start += substr_count($codeWithoutCr, "\n", length: $start); - $end += substr_count($codeWithoutCr, "\n", length: $end); + + return $this->createEditOfCodeWithoutCr($file, $start, $offset, $replacement); + } + + /** + * Removes a whole entry of a block list with its lines, and with the lines between it and the previous entry, + * like comments, the same as printing the list without it. Unlike printing the list, the time does not grow + * with its length, and the edit covers only the lines of the entry, so that the next entry can be removed + * in the same round. The first entry is left to the printer, it removes the empty lines after it as well. + * + * @param NeonConfigEntry $entry + * @return Edit|null + */ + private function createNeonRemovalEdit(array $entry): ?array + { + $previousItem = $entry['previousItem']; + $entryItem = $entry['path'][count($entry['path']) - 1]; + if ( + $previousItem === null + || $previousItem->endTokenPos === null + || $entryItem->startTokenPos === null + || $entryItem->endTokenPos === null + || $this->neonFiles[$entry['file']]['mixedLineBreaks'] + ) { + return null; + } + + ['tokenList' => $tokens, 'offsets' => $offsets, 'codeWithoutCr' => $codeWithoutCr] = $this->neonFiles[$entry['file']]; + $previousLineBreak = self::findNeonLineBreak($tokens, $previousItem->endTokenPos + 1); + for ($i = $previousLineBreak; $i < $entryItem->startTokenPos; $i++) { + // only empty lines, comments and indentation are between the entries of a block list + if (!in_array($tokens[$i]->type, [NeonToken::Newline, NeonToken::Whitespace, NeonToken::Comment], true)) { + return null; + } + } + if (!isset($tokens[$previousLineBreak]) || $previousLineBreak >= $entryItem->startTokenPos) { + return null; + } + + $lineBreak = self::findNeonLineBreak($tokens, $entryItem->endTokenPos + 1); + + return $this->createEditOfCodeWithoutCr( + $entry['file'], + $offsets[$previousLineBreak] + 1, + isset($tokens[$lineBreak]) ? $offsets[$lineBreak] + 1 : strlen($codeWithoutCr), + '', + ); + } + + /** + * Replaces the count with a new literal, the same as the printer prints it. + * + * @return Edit|null + */ + private function createNeonCountEdit(string $file, NeonNode $countNode, int $count): ?array + { + ['tokenList' => $tokens, 'offsets' => $offsets, 'mixedLineBreaks' => $mixedLineBreaks] = $this->neonFiles[$file]; + $pos = $countNode->startTokenPos; + if ( + $mixedLineBreaks + || !$countNode instanceof NeonLiteralNode + || $pos === null + || $countNode->endTokenPos !== $pos + || ($tokens[$pos]->type ?? null) !== NeonToken::Literal + ) { + return null; + } + + return $this->createEditOfCodeWithoutCr($file, $offsets[$pos], $offsets[$pos + 1], (string) $count); + } + + /** + * Position of the first line break token from $pos, or the count of the tokens. + * + * @param list $tokens + */ + private static function findNeonLineBreak(array $tokens, int $pos): int + { + while (isset($tokens[$pos]) && $tokens[$pos]->type !== NeonToken::Newline) { + $pos++; + } + + return $pos; + } + + /** + * @return Edit + */ + private function createEditOfCodeWithoutCr(string $file, int $start, int $end, string $replacement): array + { + $lineBreaksBeforeStart = $this->countNeonLineBreaksBefore($file, $start); + if ($this->neonFiles[$file]['crlf']) { + $end += $this->countNeonLineBreaksBefore($file, $end); + $start += $lineBreaksBeforeStart; $replacement = str_replace("\n", "\r\n", $replacement); } @@ -737,10 +916,30 @@ private function createNeonEdit(string $file, array $changes): ?array 'start' => $start, 'end' => $end, 'replacement' => $replacement, - 'line' => substr_count($this->files[$file]['code'], "\n", length: $start) + 1, + 'line' => $lineBreaksBeforeStart + 1, ]; } + /** + * The number of line breaks before the offset in the code without \r. + */ + private function countNeonLineBreaksBefore(string $file, int $offset): int + { + $lineBreaks = $this->neonFiles[$file]['lineBreaks']; + $low = 0; + $high = count($lineBreaks); + while ($low < $high) { + $mid = intdiv($low + $high, 2); + if ($lineBreaks[$mid] < $offset) { + $low = $mid + 1; + } else { + $high = $mid; + } + } + + return $low; + } + /** * Replaces the changed nodes and clones their ancestors, the original tree stays as it is. * The clones keep the token positions of the original nodes, so the printer knows what they stand for. diff --git a/tests/PHPStan/Analyser/Ignore/IgnoreErrorsConfigFixerTest.php b/tests/PHPStan/Analyser/Ignore/IgnoreErrorsConfigFixerTest.php index 7923ca94577..c44a8877c72 100644 --- a/tests/PHPStan/Analyser/Ignore/IgnoreErrorsConfigFixerTest.php +++ b/tests/PHPStan/Analyser/Ignore/IgnoreErrorsConfigFixerTest.php @@ -13,6 +13,7 @@ use PHPStan\Testing\PHPStanTestCase; use PHPUnit\Framework\Attributes\DataProvider; use function file_put_contents; +use function sprintf; use function sys_get_temp_dir; use function uniqid; use function unlink; @@ -352,6 +353,35 @@ public static function dataNeon(): Generator "parameters:\n\tignoreErrors: []\n", ]; + yield 'last entry' => [ + "parameters:\n\tignoreErrors:\n\t\t- '#^A$#'\n\n\t\t# B\n\t\t-\n\t\t\tmessage: '#^B$#' # b\n\t\t\tpath: Foo.php\n\tlevel: 8\n", + static fn (IgnoreErrorsConfigFixer $fixer, string $path) => $fixer->removeUnmatched(['message' => '#^B$#', 'path' => $path, 'realPath' => $path]), + "parameters:\n\tignoreErrors:\n\t\t- '#^A$#'\n\tlevel: 8\n", + ]; + + yield 'entry of a generated baseline' => [ + "parameters:\n\tignoreErrors:\n" + . "\t\t-\n\t\t\tmessage: '#^A$#'\n\t\t\tidentifier: a.b\n\t\t\tcount: 1\n\t\t\tpath: Foo.php\n\n" + . "\t\t-\n\t\t\tmessage: '#^B$#'\n\t\t\tidentifier: a.b\n\t\t\tcount: 2\n\t\t\tpath: Foo.php\n\n" + . "\t\t-\n\t\t\tmessage: '#^C$#'\n\t\t\tidentifier: a.b\n\t\t\tcount: 1\n\t\t\tpath: Foo.php\n", + static fn (IgnoreErrorsConfigFixer $fixer, string $path) => $fixer->removeUnmatched(['message' => '#^B$#', 'identifier' => 'a.b', 'count' => 2, 'path' => $path, 'realPath' => $path]), + "parameters:\n\tignoreErrors:\n" + . "\t\t-\n\t\t\tmessage: '#^A$#'\n\t\t\tidentifier: a.b\n\t\t\tcount: 1\n\t\t\tpath: Foo.php\n\n" + . "\t\t-\n\t\t\tmessage: '#^C$#'\n\t\t\tidentifier: a.b\n\t\t\tcount: 1\n\t\t\tpath: Foo.php\n", + ]; + + yield 'CRLF line breaks, second entry' => [ + "parameters:\r\n\tignoreErrors:\r\n\t\t-\r\n\t\t\tmessage: '#^A$#'\r\n\t\t\tcount: 3\r\n\t\t\tpath: Foo.php\r\n\t\t-\r\n\t\t\tmessage: '#^B$#'\r\n\t\t\tpath: Foo.php\r\n\tlevel: 8\r\n", + static fn (IgnoreErrorsConfigFixer $fixer, string $path) => $fixer->removeUnmatched(['message' => '#^B$#', 'path' => $path, 'realPath' => $path]), + "parameters:\r\n\tignoreErrors:\r\n\t\t-\r\n\t\t\tmessage: '#^A$#'\r\n\t\t\tcount: 3\r\n\t\t\tpath: Foo.php\r\n\tlevel: 8\r\n", + ]; + + yield 'CRLF line breaks, lower count' => [ + "parameters:\r\n\tignoreErrors:\r\n\t\t-\r\n\t\t\tmessage: '#^A$#'\r\n\t\t\tcount: 3\r\n\t\t\tpath: Foo.php\r\n", + static fn (IgnoreErrorsConfigFixer $fixer, string $path) => $fixer->lowerCount(['message' => '#^A$#', 'count' => 3, 'path' => $path, 'realPath' => $path], 1), + "parameters:\r\n\tignoreErrors:\r\n\t\t-\r\n\t\t\tmessage: '#^A$#'\r\n\t\t\tcount: 1\r\n\t\t\tpath: Foo.php\r\n", + ]; + yield 'CRLF line breaks' => [ "parameters:\r\n\tignoreErrors:\r\n\t\t-\r\n\t\t\tmessage: '#^A$#'\r\n\t\t\tpath: Foo.php\r\n\tlevel: 8\r\n", static fn (IgnoreErrorsConfigFixer $fixer, string $path) => $fixer->removeUnmatched(['message' => '#^A$#', 'path' => $path, 'realPath' => $path]), @@ -393,9 +423,32 @@ public function testNeonRemovingAllItemsOfListTakesTwoRounds(): void $fixer->removeUnmatched(['message' => '#^C$#', 'path' => $path, 'realPath' => $path]), $fixer->removeUnmatched(['message' => '#^D$#', 'path' => $path, 'realPath' => $path]), ]); - $this->assertSame(2, $result['appliedCount']); + // the entries after the first one are removed with only their own lines, the next one too + $this->assertSame(3, $result['appliedCount']); + $this->assertSame( + "parameters:\n\tignoreErrors:\n\t\t-\n\t\t\tmessages:\n\t\t\t\t- '#^B$#'\n\t\t\tpath: Foo.php\n", + $result['contents'][$file], + ); + } + + public function testNeonRemovingAllEntriesInOneRoundKeepsAList(): void + { + $code = "parameters:\n\tignoreErrors:\n"; + for ($i = 0; $i < 4; $i++) { + $code .= sprintf("\t\t-\n\t\t\tmessage: '#^E%d$#'\n\t\t\tcount: 1\n\t\t\tpath: Foo.php\n\n", $i); + } + $code .= "\tlevel: 8\n"; + [$fixer, $file] = $this->createFixer($code, 'neon'); + $path = __DIR__ . '/Foo.php'; + + $diffGroups = []; + for ($i = 0; $i < 4; $i++) { + $diffGroups[] = $fixer->removeUnmatched(['message' => sprintf('#^E%d$#', $i), 'count' => 1, 'path' => $path, 'realPath' => $path]); + } + $result = self::getContainer()->getByType(Patcher::class)->apply([$file => $code], $diffGroups); + $this->assertSame(3, $result['appliedCount']); $this->assertSame( - "parameters:\n\tignoreErrors:\n\t\t-\n\t\t\tmessages:\n\t\t\t\t- '#^B$#'\n\t\t\tpath: Foo.php\n\t\t-\n\t\t\tmessage: '#^D$#'\n\t\t\tpath: Foo.php\n", + "parameters:\n\tignoreErrors:\n\t\t-\n\t\t\tmessage: '#^E1$#'\n\t\t\tcount: 1\n\t\t\tpath: Foo.php\n\n\tlevel: 8\n", $result['contents'][$file], ); }