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], ); }