diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index c957a105144..f0d1a19ad6e 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -1004,22 +1004,26 @@ jobs: echo "$OUTPUT" # two unmatched messages, an unmatched entry, and a count higher than the occurrences in the PHP baseline, # an unmatched message, an unmatched entry, and a count higher than the occurrences in the NEON baseline; - # not the entry in vendor/, and not a count lower than the occurrences (--generate-baseline does that) - ../bashunit assert contains '7 of the errors can be fixed automatically. Run PHPStan again with --fix' "$OUTPUT" + # and two entries without a path in the NEON baseline, reported in it; + # not the entries in vendor/, and not a count lower than the occurrences (--generate-baseline does that) + ../bashunit assert contains '9 of the errors can be fixed automatically. Run PHPStan again with --fix' "$OUTPUT" + ../bashunit assert contains 'baseline.neon:21:Ignored error pattern #^Unmatched without a path$# was not matched' "$OUTPUT" OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse --fix --generate-baseline") echo "$OUTPUT" ../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, the second one goes in the next round - ../bashunit assert contains 'Round 2: fixed 2 errors in 2 files' "$OUTPUT" - ../bashunit assert contains '[OK] 7 errors fixed in 2 files' "$OUTPUT" + # 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" + ../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 OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse --error-format raw") echo "$OUTPUT" ../bashunit assert contains 'Ignored error pattern #^Unmatched in vendor$#' "$OUTPUT" + ../bashunit assert contains 'Ignored error pattern #^Unmatched in vendor without a path$# was not matched' "$OUTPUT" ../bashunit assert contains 'is expected to occur 1 time, but occurred 2 times.' "$OUTPUT" ../bashunit assert not_contains 'Unmatched one' "$OUTPUT" ../bashunit assert not_contains 'NEON' "$OUTPUT" diff --git a/e2e/fix-baseline/baseline-original.neon b/e2e/fix-baseline/baseline-original.neon index b31a8187b86..cdab5299174 100644 --- a/e2e/fix-baseline/baseline-original.neon +++ b/e2e/fix-baseline/baseline-original.neon @@ -16,3 +16,8 @@ parameters: - '#^Unmatched in a NEON list$#' - '#^Undefined variable\: \$h$#' path: src/Foo.php + + # entries without a path are reported in this file + - '#^Unmatched without a path$#' + - + rawMessage: 'Unmatched without a path either' diff --git a/e2e/fix-baseline/vendor-baseline.php b/e2e/fix-baseline/vendor-baseline.php index f6862a300cd..efaf918d114 100644 --- a/e2e/fix-baseline/vendor-baseline.php +++ b/e2e/fix-baseline/vendor-baseline.php @@ -5,4 +5,5 @@ 'message' => '#^Unmatched in vendor$#', 'path' => __DIR__ . '/../../../src/Foo.php', ], + '#^Unmatched in vendor without a path$#', ]]]; diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 51a6d0e58d5..fc79f566e9e 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -57,7 +57,7 @@ parameters: - rawMessage: 'Call to static method expand() of internal class Nette\DI\Helpers from outside its root namespace Nette.' identifier: staticMethod.internalClass - count: 2 + count: 3 path: src/Analyser/Ignore/IgnoreErrorsConfigFixer.php - diff --git a/src/Analyser/Ignore/IgnoreErrorsConfigFixer.php b/src/Analyser/Ignore/IgnoreErrorsConfigFixer.php index e16d619c194..6a4ce9296aa 100644 --- a/src/Analyser/Ignore/IgnoreErrorsConfigFixer.php +++ b/src/Analyser/Ignore/IgnoreErrorsConfigFixer.php @@ -69,6 +69,7 @@ * * The entry is looked for in the whole config cascade, except for files of installed Composer packages. * The ignoreErrors parameter is merged from all of them, so the entry can come from more than one file. + * Entries without a path, including the ones written as a plain string pattern, are fixed too. * * Baselines can have tens of thousands of entries and each fixed error needs its own diff, which covers * only the changed lines. In a PHP config file, a fix is an edit of the source range of the parsed node, @@ -83,6 +84,7 @@ * format: 'php', * items: array, * removableNode: Node|null, + * line: int, * } * @phpstan-type NeonConfigEntry = array{ * file: string, @@ -90,7 +92,8 @@ * format: 'neon', * items: array, * path: non-empty-list, - * entryValue: NeonArrayNode, + * entryValue: NeonNode, + * line: int, * } * @phpstan-type ConfigEntry = PhpConfigEntry|NeonConfigEntry * @phpstan-type Edit = array{start: int, end: int, replacement: string, line: int} @@ -101,13 +104,16 @@ final class IgnoreErrorsConfigFixer private const ENTRY_KEYS = ['message', 'messages', 'rawMessage', 'rawMessages', 'identifier', 'identifiers', 'path', 'paths', 'count', 'reportUnmatched']; + /** key of the entries without a path in getEntriesByPath(), a normalized path is never empty */ + private const NO_PATH = ''; + private const MATCHED_FIELDS = [ 'message' => 'messages', 'rawMessage' => 'rawMessages', 'identifier' => 'identifiers', ]; - /** @var array>|null entries by normalized path */ + /** @var array>|null entries by normalized path, or by self::NO_PATH */ private ?array $entriesByPath = null; /** @var array contents of the config files the entries were found in */ @@ -136,10 +142,28 @@ public function __construct( } /** - * @param ExpandedIgnoredErrorData $ignoreError + * Config file and line of the entry, for reporting an entry without a path. + * + * @param string|ExpandedIgnoredErrorData $ignoreError + * @return array{string, int}|null + */ + public function findEntryLocation(string|array $ignoreError): ?array + { + $entries = $this->findEntries($ignoreError); + if ($entries === []) { + return null; + } + + $entry = $entries[0][0]; + + return [$entry['file'], $entry['line']]; + } + + /** + * @param string|ExpandedIgnoredErrorData $ignoreError * @return list */ - public function removeUnmatched(array $ignoreError): array + public function removeUnmatched(string|array $ignoreError): array { $editsByFile = []; $neonChangesByFile = []; @@ -360,18 +384,22 @@ private function createDiff(string $code, array $edits): ?string * Entries the expanded entry was made of. The list item is set when the entry matched through a list * like `messages` that has other items too, only that item can then be removed. * - * @param ExpandedIgnoredErrorData $ignoreError + * @param string|ExpandedIgnoredErrorData $ignoreError * @return list the entry, and the list field and position of the item */ - private function findEntries(array $ignoreError): array + private function findEntries(string|array $ignoreError): array { + if (is_string($ignoreError)) { + $ignoreError = ['message' => $ignoreError]; + } + $path = $ignoreError['realPath'] ?? $ignoreError['path'] ?? null; - if ($path === null) { + if ($path === null && isset($ignoreError['paths'])) { return []; } $found = []; - foreach ($this->getEntriesByPath()[$this->normalizePath($path)] ?? [] as $entry) { + foreach ($this->getEntriesByPath()[$path !== null ? $this->normalizePath($path) : self::NO_PATH] ?? [] as $entry) { $listField = null; $position = null; foreach (self::MATCHED_FIELDS as $field => $matchedListField) { @@ -432,6 +460,12 @@ private function getEntriesByPath(): array $entries = str_ends_with($configFile, '.php') ? $this->findPhpEntries($configFile) : $this->findNeonEntries($configFile); foreach ($entries as $entry) { $path = $entry['values']['path'] ?? null; + if ($path === null) { + if (!isset($entry['values']['paths'])) { + $this->entriesByPath[self::NO_PATH][] = $entry; + } + continue; + } if (!is_string($path)) { continue; } @@ -496,6 +530,39 @@ private function findPhpEntries(string $file): array foreach ($node->items as $item) { $removableNodes[spl_object_id($item->value)] = $item; + + // plain string patterns, unlike entry arrays, are told apart from other strings only by being in an ignoreErrors list + if ( + !$item->key instanceof Node\Scalar\String_ + || $item->key->value !== 'ignoreErrors' + || !$item->value instanceof Expr\Array_ + ) { + continue; + } + + foreach ($item->value->items as $patternItem) { + if ($patternItem->key !== null || $patternItem->value instanceof Expr\Array_) { + continue; + } + + try { + $pattern = Helpers::expand($evaluator->evaluateDirectly($patternItem->value), $parameters); + } catch (ConstExprEvaluationException | InvalidArgumentException) { + continue; + } + if (!is_string($pattern)) { + continue; + } + + $entries[] = [ + 'file' => $file, + 'values' => ['message' => $pattern], + 'format' => 'php', + 'items' => [], + 'removableNode' => $patternItem, + 'line' => $patternItem->getStartLine(), + ]; + } } $items = []; @@ -516,7 +583,7 @@ private function findPhpEntries(string $file): array $values[$item->key->value] = $value; } - if (!isset($values['path']) || (isset($values['count']) && !is_int($values['count']))) { + if ($values === [] || (isset($values['count']) && !is_int($values['count']))) { continue; } @@ -526,6 +593,7 @@ private function findPhpEntries(string $file): array 'format' => 'php', 'items' => $items, 'removableNode' => $removableNodes[spl_object_id($node)] ?? null, + 'line' => $node->getStartLine(), ]; } @@ -572,12 +640,9 @@ private function findNeonEntries(string $file): array $adapter = new NeonAdapter(['[parameters][ignoreErrors][][path]', '[parameters][ignoreErrors][][paths][]'], $parameters); $entries = []; foreach ($ignoreErrorsItem->value->items as $entryItem) { - if (!$entryItem->value instanceof NeonArrayNode) { - continue; - } - $items = []; - foreach ($entryItem->value->items as $item) { + // a plain string pattern has no items + foreach ($entryItem->value instanceof NeonArrayNode ? $entryItem->value->items : [] as $item) { $key = $item->key !== null ? $item->key->toValue() : null; if (!is_string($key) || !in_array($key, self::ENTRY_KEYS, true)) { continue 2; @@ -594,7 +659,10 @@ private function findNeonEntries(string $file): array continue; } - if (!is_array($values) || !isset($values['path']) || (isset($values['count']) && !is_int($values['count']))) { + if (is_string($values)) { + $values = ['message' => $values]; + } + if (!is_array($values) || $values === [] || (isset($values['count']) && !is_int($values['count']))) { continue; } @@ -605,6 +673,7 @@ private function findNeonEntries(string $file): array 'items' => $items, 'path' => [$root, $parametersItem, $parametersItem->value, $ignoreErrorsItem, $ignoreErrorsItem->value, $entryItem], 'entryValue' => $entryItem->value, + 'line' => $entryItem->startLine ?? 1, ]; } diff --git a/src/Analyser/Ignore/IgnoredErrorHelperResult.php b/src/Analyser/Ignore/IgnoredErrorHelperResult.php index e5ebbbf0fbf..f1bed8885d9 100644 --- a/src/Analyser/Ignore/IgnoredErrorHelperResult.php +++ b/src/Analyser/Ignore/IgnoredErrorHelperResult.php @@ -412,11 +412,26 @@ public function process( fixedErrorDiffs: $this->configFixer->removeUnmatched($unmatchedIgnoredError), ); } elseif (!$onlyFiles) { - $stringErrors[] = sprintf( + $message = sprintf( '%s %s was not matched in reported errors.', IgnoredError::getIgnoredErrorLabel($unmatchedIgnoredError), IgnoredError::stringifyPattern($unmatchedIgnoredError), ); + // without a file of its own, the error is reported in the config file the entry is in + $location = $this->configFixer->findEntryLocation($unmatchedIgnoredError); + if ($location === null) { + $stringErrors[] = $message; + continue; + } + + $errors[] = new Error( + $message, + $location[0], + $location[1], + canBeIgnored: false, + identifier: 'ignore.unmatched', + fixedErrorDiffs: $this->configFixer->removeUnmatched($unmatchedIgnoredError), + ); } } } diff --git a/tests/PHPStan/Analyser/Ignore/IgnoreErrorsConfigFixerTest.php b/tests/PHPStan/Analyser/Ignore/IgnoreErrorsConfigFixerTest.php index 0f8c91ddf65..7923ca94577 100644 --- a/tests/PHPStan/Analyser/Ignore/IgnoreErrorsConfigFixerTest.php +++ b/tests/PHPStan/Analyser/Ignore/IgnoreErrorsConfigFixerTest.php @@ -216,6 +216,90 @@ public function testNotFixable(string $code, array $ignoreError, bool $lowerCoun $this->assertSame([], $lowerCount ? $fixer->lowerCount($ignoreError, 1) : $fixer->removeUnmatched($ignoreError)); } + public static function dataEntryWithoutPath(): Generator + { + $php = " ['ignoreErrors' => [\n" + . "\t'#^A$#',\n" + . "\t['message' => '#^A$#', 'path' => __DIR__ . '/Foo.php'],\n" + . "\t['message' => '#^A$#', 'identifier' => 'foo.bar'],\n" + . "\t['rawMessage' => 'B'],\n" + . "]]];\n"; + + yield 'PHP string pattern, not the entries with a path or an identifier' => [ + $php, + 'php', + '#^A$#', + 3, + " ['ignoreErrors' => [\n" + . "\t['message' => '#^A$#', 'path' => __DIR__ . '/Foo.php'],\n" + . "\t['message' => '#^A$#', 'identifier' => 'foo.bar'],\n" + . "\t['rawMessage' => 'B'],\n" + . "]]];\n", + ]; + + yield 'PHP entry' => [ + $php, + 'php', + ['rawMessage' => 'B'], + 6, + " ['ignoreErrors' => [\n" + . "\t'#^A$#',\n" + . "\t['message' => '#^A$#', 'path' => __DIR__ . '/Foo.php'],\n" + . "\t['message' => '#^A$#', 'identifier' => 'foo.bar'],\n" + . "]]];\n", + ]; + + $neon = "parameters:\n\tignoreErrors:\n" + . "\t\t- '#^100%% sure$#'\n" + . "\t\t-\n\t\t\tmessage: '#^100%% sure$#'\n\t\t\tpath: Foo.php\n" + . "\t\t-\n\t\t\trawMessages:\n\t\t\t\t- B\n\t\t\t\t- C\n" + . "\tlevel: 8\n"; + + yield 'NEON string pattern' => [ + $neon, + 'neon', + '#^100% sure$#', + 3, + "parameters:\n\tignoreErrors:\n" + . "\t\t-\n\t\t\tmessage: '#^100%% sure$#'\n\t\t\tpath: Foo.php\n" + . "\t\t-\n\t\t\trawMessages:\n\t\t\t\t- B\n\t\t\t\t- C\n" + . "\tlevel: 8\n", + ]; + + yield 'NEON item of a list' => [ + $neon, + 'neon', + ['rawMessage' => 'C'], + 7, + "parameters:\n\tignoreErrors:\n" + . "\t\t- '#^100%% sure$#'\n" + . "\t\t-\n\t\t\tmessage: '#^100%% sure$#'\n\t\t\tpath: Foo.php\n" + . "\t\t-\n\t\t\trawMessages:\n\t\t\t\t- B\n" + . "\tlevel: 8\n", + ]; + } + + /** + * @param string|array{message?: string, rawMessage?: string} $ignoreError + */ + #[DataProvider('dataEntryWithoutPath')] + public function testEntryWithoutPath(string $code, string $extension, string|array $ignoreError, int $line, string $expected): void + { + [$fixer, $file] = $this->createFixer($code, $extension); + + $this->assertSame([$file, $line], $fixer->findEntryLocation($ignoreError)); + $this->assertSame($expected, $this->apply($file, $code, $fixer->removeUnmatched($ignoreError))); + } + + public function testEntryWithPathsIsNotFound(): void + { + [$fixer] = $this->createFixer("parameters:\n\tignoreErrors:\n\t\t-\n\t\t\tmessage: '#^A$#'\n\t\t\tpaths:\n\t\t\t\t- Foo.php\n\t\t\t\t- Bar.php\n", 'neon'); + $ignoreError = ['message' => '#^A$#', 'paths' => [1 => __DIR__ . '/Bar.php']]; + + $this->assertNull($fixer->findEntryLocation($ignoreError)); + $this->assertSame([], $fixer->removeUnmatched($ignoreError)); + } + public static function dataNeon(): Generator { $entries = "parameters:\n\tignoreErrors:\n"