diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index ac31c5bd29a..0c5f06510af 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -993,6 +993,32 @@ jobs: OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -c cycle.neon --fix") echo "$OUTPUT" ../bashunit assert contains 'Fixing stopped after 10 rounds' "$OUTPUT" + - script: | + cd e2e/fix-baseline + composer install + mkdir -p vendor/acme/rules && cp vendor-baseline.php vendor/acme/rules/baseline.php + rm -rf tmp && cp baseline-original.php baseline.php + OUTPUT=$(AI_AGENT=1 ../bashunit assert exit_code "1" "../../bin/phpstan analyse --error-format raw") + echo "$OUTPUT" + # two unmatched messages and an unmatched entry of the PHP baseline, and a count higher than the occurrences; + # not the entries in NEON and in vendor/, and not a count lower than the occurrences (--generate-baseline does that) + ../bashunit assert contains '4 of the errors can be fixed automatically. Run PHPStan again with --fix' "$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 list conflicts, the second one goes in the next round + ../bashunit assert contains 'Round 2: fixed 1 error in 1 file' "$OUTPUT" + ../bashunit assert contains '[OK] 4 errors fixed in 1 file' "$OUTPUT" + diff -u baseline-expected.php baseline.php + OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse --error-format raw") + echo "$OUTPUT" + ../bashunit assert contains 'Ignored error pattern #^Unmatched in NEON$#' "$OUTPUT" + ../bashunit assert contains 'Ignored error pattern #^Unmatched in vendor$#' "$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 'occurred only' "$OUTPUT" - script: | cd e2e/editor-mode OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw") diff --git a/e2e/fix-baseline/.gitignore b/e2e/fix-baseline/.gitignore new file mode 100644 index 00000000000..0685d4cefe4 --- /dev/null +++ b/e2e/fix-baseline/.gitignore @@ -0,0 +1,4 @@ +/vendor +/composer.lock +/tmp +/baseline.php diff --git a/e2e/fix-baseline/baseline-expected.php b/e2e/fix-baseline/baseline-expected.php new file mode 100644 index 00000000000..7eb08b2450c --- /dev/null +++ b/e2e/fix-baseline/baseline-expected.php @@ -0,0 +1,35 @@ + '#^Undefined variable\: \$[abc]$#', + 'identifier' => 'variable.undefined', + 'count' => 3, + 'path' => __DIR__ . '/src/Foo.php', +]; +$ignoreErrors[] = [ + 'message' => '#^Function FixBaselineE2E\\\\bar\(\) should return string but returns int\.$#', + 'identifier' => 'return.type', + 'count' => 1, + 'path' => __DIR__ . '/src/Foo.php', +]; +$ignoreErrors[] = [ + 'messages' => [ + '#^Undefined variable\: \$d$#', + ], + 'path' => __DIR__ . '/src/Foo.php', +]; +$ignoreErrors[] = [ + 'message' => '#^Undefined variable\: \$e$#', + 'identifier' => 'variable.undefined', + 'count' => 1, + 'path' => __DIR__ . '/src/Foo.php', +]; +$ignoreErrors[] = [ + 'message' => '#^Undefined variable\: \$f$#', + 'identifier' => 'variable.undefined', + 'count' => 1, + 'path' => __DIR__ . '/src/Foo.php', +]; + +return ['parameters' => ['ignoreErrors' => $ignoreErrors]]; diff --git a/e2e/fix-baseline/baseline-original.php b/e2e/fix-baseline/baseline-original.php new file mode 100644 index 00000000000..f97eb4b0335 --- /dev/null +++ b/e2e/fix-baseline/baseline-original.php @@ -0,0 +1,43 @@ + '#^Variable \$\w+ might not be defined\.$#', + 'identifier' => 'variable.undefined', + 'count' => 2, + 'path' => __DIR__ . '/src/Foo.php', +]; +$ignoreErrors[] = [ + 'message' => '#^Undefined variable\: \$[abc]$#', + 'identifier' => 'variable.undefined', + 'count' => 5, + 'path' => __DIR__ . '/src/Foo.php', +]; +$ignoreErrors[] = [ + 'message' => '#^Function FixBaselineE2E\\\\bar\(\) should return string but returns int\.$#', + 'identifier' => 'return.type', + 'count' => 1, + 'path' => __DIR__ . '/src/Foo.php', +]; +$ignoreErrors[] = [ + 'messages' => [ + '#^Undefined variable\: \$d$#', + '#^Unmatched one$#', + '#^Unmatched two$#', + ], + 'path' => __DIR__ . '/src/Foo.php', +]; +$ignoreErrors[] = [ + 'message' => '#^Undefined variable\: \$e$#', + 'identifier' => 'variable.undefined', + 'count' => 1, + 'path' => __DIR__ . '/src/Foo.php', +]; +$ignoreErrors[] = [ + 'message' => '#^Undefined variable\: \$f$#', + 'identifier' => 'variable.undefined', + 'count' => 1, + 'path' => __DIR__ . '/src/Foo.php', +]; + +return ['parameters' => ['ignoreErrors' => $ignoreErrors]]; diff --git a/e2e/fix-baseline/composer.json b/e2e/fix-baseline/composer.json new file mode 100644 index 00000000000..14336fdabe8 --- /dev/null +++ b/e2e/fix-baseline/composer.json @@ -0,0 +1,7 @@ +{ + "autoload": { + "psr-4": { + "FixBaselineE2E\\": "src/" + } + } +} diff --git a/e2e/fix-baseline/phpstan.neon b/e2e/fix-baseline/phpstan.neon new file mode 100644 index 00000000000..74b50d8f290 --- /dev/null +++ b/e2e/fix-baseline/phpstan.neon @@ -0,0 +1,15 @@ +includes: + - baseline.php + # a config file of an installed package is never fixed + - vendor/acme/rules/baseline.php + +parameters: + level: 8 + paths: + - src + tmpDir: tmp + ignoreErrors: + # NEON config files are not fixed yet + - + message: '#^Unmatched in NEON$#' + path: src/Foo.php diff --git a/e2e/fix-baseline/src/Foo.php b/e2e/fix-baseline/src/Foo.php new file mode 100644 index 00000000000..345d9d28857 --- /dev/null +++ b/e2e/fix-baseline/src/Foo.php @@ -0,0 +1,27 @@ + ['ignoreErrors' => [ + [ + 'message' => '#^Unmatched in vendor$#', + 'path' => __DIR__ . '/../../../src/Foo.php', + ], +]]]; diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index fcc988632b3..166d7e965f3 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -54,6 +54,12 @@ parameters: count: 1 path: src/Analyser/ExprHandler/Helper/IncDecTypeHelper.php + - + rawMessage: 'Call to static method expand() of internal class Nette\DI\Helpers from outside its root namespace Nette.' + identifier: staticMethod.internalClass + count: 1 + path: src/Analyser/Ignore/IgnoreErrorsConfigFixer.php + - rawMessage: Casting to string something that's already string. identifier: cast.useless diff --git a/src/Analyser/Ignore/IgnoreErrorsConfigFixer.php b/src/Analyser/Ignore/IgnoreErrorsConfigFixer.php new file mode 100644 index 00000000000..202dd798408 --- /dev/null +++ b/src/Analyser/Ignore/IgnoreErrorsConfigFixer.php @@ -0,0 +1,483 @@ +, + * items: array, + * removableNode: Node|null, + * } + * @phpstan-type Edit = array{start: int, end: int, replacement: string, line: int} + */ +#[AutowiredService] +final class IgnoreErrorsConfigFixer +{ + + private const ENTRY_KEYS = ['message', 'messages', 'rawMessage', 'rawMessages', 'identifier', 'identifiers', 'path', 'paths', 'count', 'reportUnmatched']; + + private const MATCHED_FIELDS = [ + 'message' => 'messages', + 'rawMessage' => 'rawMessages', + 'identifier' => 'identifiers', + ]; + + /** @var array>|null entries by normalized path */ + private ?array $entriesByPath = null; + + /** @var array contents of the config files the entries were found in */ + private array $files = []; + + /** + * @param list $allConfigFiles + */ + public function __construct( + #[AutowiredParameter(ref: '@currentPhpVersionPhpParser')] + private Parser $parser, + private PackageDependencyResolver $packageDependencyResolver, + private FileHelper $fileHelper, + #[AutowiredParameter] + private array $allConfigFiles, + #[AutowiredParameter] + private string $rootDir, + #[AutowiredParameter] + private string $currentWorkingDirectory, + ) + { + } + + /** + * @param ExpandedIgnoredErrorData $ignoreError + * @return list + */ + public function removeUnmatched(array $ignoreError): array + { + $editsByFile = []; + foreach ($this->findEntries($ignoreError) as [$entry, $listItem, $listNode]) { + $code = $this->files[$entry['file']]['code']; + if ($listItem === null || $listNode === null) { + if ($entry['removableNode'] === null) { + return []; + } + + $editsByFile[$entry['file']][] = $this->createRemovalEdit($code, $entry['removableNode']); + continue; + } + + // The edit replaces the whole list, so that removing another item of it conflicts. + // Otherwise all items could be removed in one round, leaving an entry without messages. + // The next round removes the whole entry once its last item is unmatched. + $itemEdit = $this->createRemovalEdit($code, $listItem); + $listStart = $listNode->getStartFilePos(); + $listEnd = $listNode->getEndFilePos() + 1; + $editsByFile[$entry['file']][] = [ + 'start' => $listStart, + 'end' => $listEnd, + 'replacement' => substr($code, $listStart, $itemEdit['start'] - $listStart) . substr($code, $itemEdit['end'], $listEnd - $itemEdit['end']), + 'line' => $listNode->getStartLine(), + ]; + } + + return $this->createDiffs($editsByFile); + } + + /** + * @param ExpandedIgnoredErrorData $ignoreError + * @return list + */ + public function lowerCount(array $ignoreError, int $count): array + { + $entries = $this->findEntries($ignoreError); + if (count($entries) !== 1) { + return []; + } + + [$entry, $listItem] = $entries[0]; + if ( + $listItem !== null + || !isset($entry['items']['count']) + // another entry of the same error is somewhere this class does not look into, like a NEON file + || $entry['values']['count'] !== ($ignoreError['count'] ?? null) + ) { + return []; + } + + $countNode = $entry['items']['count']->value; + + return $this->createDiffs([ + $entry['file'] => [[ + 'start' => $countNode->getStartFilePos(), + 'end' => $countNode->getEndFilePos() + 1, + 'replacement' => (string) $count, + 'line' => $countNode->getStartLine(), + ]], + ]); + } + + /** + * Removes the node, with the comma separating it from the other items when it is an array item, + * and with its lines when nothing else is on them. + * + * @return Edit + */ + private function createRemovalEdit(string $code, Node $node): array + { + $start = $node->getStartFilePos(); + $end = $node->getEndFilePos() + 1; + $line = $node->getStartLine(); + if ($node instanceof Node\ArrayItem) { + $afterEnd = $end + strspn($code, " \t\r\n", $end); + if (($code[$afterEnd] ?? '') === ',') { + $end = $afterEnd + 1; + $end += strspn($code, " \t", $end); + } else { + // the last item without a trailing comma, the comma before it goes + $beforeStart = $start - 1; + while ($beforeStart >= 0 && in_array($code[$beforeStart], [' ', "\t", "\r", "\n"], true)) { + $beforeStart--; + } + if ($beforeStart >= 0 && $code[$beforeStart] === ',') { + $line -= substr_count($code, "\n", $beforeStart, $start - $beforeStart); + $start = $beforeStart; + } + } + } + + $lineBreak = $start > 0 ? strrpos($code, "\n", $start - 1 - strlen($code)) : false; + $lineStart = $lineBreak === false ? 0 : $lineBreak + 1; + $lineBreak = strpos($code, "\n", $end - 1); + $lineEnd = $lineBreak === false ? strlen($code) : $lineBreak + 1; + if ( + trim(substr($code, $lineStart, $start - $lineStart)) === '' + && trim(substr($code, $end, $lineEnd - $end)) === '' + ) { + $start = $lineStart; + $end = $lineEnd; + } + + return ['start' => $start, 'end' => $end, 'replacement' => '', 'line' => $line]; + } + + /** + * @param array> $editsByFile + * @return list + */ + private function createDiffs(array $editsByFile): array + { + $diffs = []; + foreach ($editsByFile as $file => $edits) { + $diff = $this->createDiff($this->files[$file]['code'], $edits); + if ($diff === null) { + // the changes are applied together or not at all + return []; + } + + $diffs[] = new FixedErrorDiff($file, $this->files[$file]['hash'], $diff); + } + + return $diffs; + } + + /** + * A unified diff without context lines, the lines of each edit make one hunk. + * + * @param list $edits + */ + private function createDiff(string $code, array $edits): ?string + { + usort($edits, static fn (array $a, array $b): int => $a['start'] <=> $b['start']); + + $diff = ''; + $previousLineEnd = 0; + $lineOffset = 0; + foreach ($edits as $edit) { + $lineBreak = $edit['start'] > 0 ? strrpos($code, "\n", $edit['start'] - 1 - strlen($code)) : false; + $lineStart = $lineBreak === false ? 0 : $lineBreak + 1; + $lineBreak = strpos($code, "\n", $edit['end'] > $edit['start'] ? $edit['end'] - 1 : $edit['start']); + $lineEnd = $lineBreak === false ? strlen($code) : $lineBreak + 1; + if ($lineStart < $previousLineEnd) { + return null; + } + $previousLineEnd = $lineEnd; + + $before = substr($code, $lineStart, $edit['start'] - $lineStart); + $after = substr($code, $edit['end'], $lineEnd - $edit['end']); + $newCode = $before . $edit['replacement'] . $after; + + $oldLines = self::splitStringByLines(substr($code, $lineStart, $lineEnd - $lineStart)); + $newLines = self::splitStringByLines($newCode); + $diff .= sprintf('@@ -%d,%d +%d,%d @@', $edit['line'], count($oldLines), $edit['line'] + $lineOffset, count($newLines)) . "\n"; + foreach ($oldLines as $oldLine) { + // the last line of a file without a line break, the next diff line must not continue it + $diff .= '-' . rtrim($oldLine, "\r\n") . "\n"; + } + $diff .= implode('', array_map(static fn (string $newLine): string => '+' . $newLine, $newLines)); + $lineOffset += count($newLines) - count($oldLines); + } + + return $diff; + } + + /** + * 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 + * @return list + */ + private function findEntries(array $ignoreError): array + { + $path = $ignoreError['realPath'] ?? $ignoreError['path'] ?? null; + if ($path === null) { + return []; + } + + $found = []; + foreach ($this->getEntriesByPath()[$this->normalizePath($path)] ?? [] as $entry) { + $listItem = null; + $listNode = null; + foreach (self::MATCHED_FIELDS as $field => $listField) { + $expected = $ignoreError[$field] ?? null; + if ($expected === null) { + if (array_key_exists($field, $entry['values']) || array_key_exists($listField, $entry['values'])) { + continue 2; + } + continue; + } + + if (($entry['values'][$field] ?? null) === $expected) { + continue; + } + + $list = $entry['values'][$listField] ?? null; + if (!is_array($list)) { + continue 2; + } + + $index = array_search($expected, $list, true); + if ($index === false) { + continue 2; + } + + if (count($list) === 1) { + continue; + } + + $listNode = $entry['items'][$listField]->value; + if (!$listNode instanceof Expr\Array_ || count($listNode->items) !== count($list)) { + return []; + } + + $position = array_search($index, array_keys($list), true); + if ($position === false) { + return []; + } + + $listItem = $listNode->items[$position]; + } + + $found[] = [$entry, $listItem, $listNode]; + } + + return $found; + } + + /** + * @return array> + */ + private function getEntriesByPath(): array + { + if ($this->entriesByPath !== null) { + return $this->entriesByPath; + } + + $this->entriesByPath = []; + foreach ($this->allConfigFiles as $configFile) { + if (str_starts_with($configFile, 'phar://') || $this->packageDependencyResolver->isFileOfInstalledPackage($configFile)) { + continue; + } + + // NEON config files are not fixed yet + if (!str_ends_with($configFile, '.php')) { + continue; + } + + foreach ($this->findPhpEntries($configFile) as $entry) { + $path = $entry['values']['path'] ?? null; + if (!is_string($path)) { + continue; + } + + $this->entriesByPath[$this->normalizePath($path)][] = $entry; + } + } + + return $this->entriesByPath; + } + + /** + * 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[] = [...];`. + * + * @return list + */ + private function findPhpEntries(string $file): array + { + try { + $code = FileReader::read($file); + $stmts = $this->parser->parse($code); + } catch (CouldNotReadFileException | PhpParserError) { + return []; + } + if ($stmts === null) { + return []; + } + + $this->files[$file] = ['code' => $code, 'hash' => hash('sha256', $code)]; + + $evaluator = new ConstExprEvaluator(static function (Expr $expr) use ($file): string { + if ($expr instanceof Node\Scalar\MagicConst\Dir) { + return dirname($file); + } + if ($expr instanceof Node\Scalar\MagicConst\File) { + return $file; + } + + throw new ConstExprEvaluationException(); + }); + + $parameters = [ + 'rootDir' => $this->rootDir, + 'currentWorkingDirectory' => $this->currentWorkingDirectory, + 'env' => Environment::getCleanedArray(), + ]; + + $removableNodes = []; + $entries = []; + // in the order of the source, so the parent of an array comes before it + foreach ((new NodeFinder())->find($stmts, static fn (Node $node): bool => $node instanceof Expr\Array_ || $node instanceof Node\Stmt\Expression) as $node) { + if ($node instanceof Node\Stmt\Expression) { + if ( + $node->expr instanceof Expr\Assign + && $node->expr->var instanceof Expr\ArrayDimFetch + && $node->expr->var->dim === null + ) { + $removableNodes[spl_object_id($node->expr->expr)] = $node; + } + continue; + } + if (!$node instanceof Expr\Array_) { + continue; + } + + foreach ($node->items as $item) { + $removableNodes[spl_object_id($item->value)] = $item; + } + + $items = []; + $values = []; + foreach ($node->items as $item) { + if (!$item->key instanceof Node\Scalar\String_ || !in_array($item->key->value, self::ENTRY_KEYS, true)) { + continue 2; + } + + try { + // %% is escaped % in parameters, and other parameters can be used as well + $value = Helpers::expand($evaluator->evaluateDirectly($item->value), $parameters); + } catch (ConstExprEvaluationException | InvalidArgumentException) { + continue 2; + } + + $items[$item->key->value] = $item; + $values[$item->key->value] = $value; + } + + if (!isset($values['path']) || (isset($values['count']) && !is_int($values['count']))) { + continue; + } + + $entries[] = [ + 'file' => $file, + 'values' => $values, + 'items' => $items, + 'removableNode' => $removableNodes[spl_object_id($node)] ?? null, + ]; + } + + return $entries; + } + + private function normalizePath(string $path): string + { + return $this->fileHelper->normalizePath($this->fileHelper->absolutizePath($path)); + } + + /** + * @return string[] + */ + private static function splitStringByLines(string $input): array + { + return Strings::split($input, '/(.*\R)/', PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_NO_EMPTY); + } + +} diff --git a/src/Analyser/Ignore/IgnoredErrorHelper.php b/src/Analyser/Ignore/IgnoredErrorHelper.php index dd165407ba6..50a7fc58845 100644 --- a/src/Analyser/Ignore/IgnoredErrorHelper.php +++ b/src/Analyser/Ignore/IgnoredErrorHelper.php @@ -37,6 +37,7 @@ final class IgnoredErrorHelper */ public function __construct( private FileHelper $fileHelper, + private IgnoreErrorsConfigFixer $configFixer, #[AutowiredParameter] private array $ignoreErrors, #[AutowiredParameter] @@ -170,7 +171,7 @@ public function initialize(): IgnoredErrorHelperResult } } - return new IgnoredErrorHelperResult($this->fileHelper, $errors, $otherIgnoreErrors, $ignoreErrorsByFile, $expandedIgnoreErrors, $this->reportUnmatchedIgnoredErrors); + return new IgnoredErrorHelperResult($this->fileHelper, $this->configFixer, $errors, $otherIgnoreErrors, $ignoreErrorsByFile, $expandedIgnoreErrors, $this->reportUnmatchedIgnoredErrors); } } diff --git a/src/Analyser/Ignore/IgnoredErrorHelperResult.php b/src/Analyser/Ignore/IgnoredErrorHelperResult.php index d8c8fc28267..e5ebbbf0fbf 100644 --- a/src/Analyser/Ignore/IgnoredErrorHelperResult.php +++ b/src/Analyser/Ignore/IgnoredErrorHelperResult.php @@ -51,6 +51,7 @@ final class IgnoredErrorHelperResult */ public function __construct( private FileHelper $fileHelper, + private IgnoreErrorsConfigFixer $configFixer, private array $errors, private array $otherIgnoreErrors, private array $ignoreErrorsByFile, @@ -374,14 +375,21 @@ public function process( $matchedLine = $matchedAt[$i]['line'] ?? null; // $realCount is at least 1 (it was incremented in the closure) // and strictly less than count, so count is always >= 2. - $errors[] = (new Error(sprintf( - '%s %s is expected to occur %d times, but occurred only %d %s.', - IgnoredError::getIgnoredErrorLabel($unmatchedIgnoredError), - IgnoredError::stringifyPattern($unmatchedIgnoredError), - $unmatchedIgnoredError['count'], - $realCount, - $realCount === 1 ? 'time' : 'times', - ), $matchedFile ?? '', $matchedLine, false))->withIdentifier('ignore.count'); + $errors[] = new Error( + sprintf( + '%s %s is expected to occur %d times, but occurred only %d %s.', + IgnoredError::getIgnoredErrorLabel($unmatchedIgnoredError), + IgnoredError::stringifyPattern($unmatchedIgnoredError), + $unmatchedIgnoredError['count'], + $realCount, + $realCount === 1 ? 'time' : 'times', + ), + $matchedFile ?? '', + $matchedLine, + false, + identifier: 'ignore.count', + fixedErrorDiffs: $this->configFixer->lowerCount($unmatchedIgnoredError, $realCount), + ); } } elseif (isset($unmatchedIgnoredError['realPath'])) { if (!array_key_exists($unmatchedIgnoredError['realPath'], $analysedFilesKeys)) { @@ -392,7 +400,7 @@ public function process( continue; } - $errors[] = (new Error( + $errors[] = new Error( sprintf( '%s %s was not matched in reported errors.', IgnoredError::getIgnoredErrorLabel($unmatchedIgnoredError), @@ -400,7 +408,9 @@ public function process( ), $unmatchedIgnoredError['realPath'], canBeIgnored: false, - ))->withIdentifier('ignore.unmatched'); + identifier: 'ignore.unmatched', + fixedErrorDiffs: $this->configFixer->removeUnmatched($unmatchedIgnoredError), + ); } elseif (!$onlyFiles) { $stringErrors[] = sprintf( '%s %s was not matched in reported errors.', diff --git a/src/Analyser/ResultCache/ResultCacheManager.php b/src/Analyser/ResultCache/ResultCacheManager.php index f43162ea10c..218c09e0e0d 100644 --- a/src/Analyser/ResultCache/ResultCacheManager.php +++ b/src/Analyser/ResultCache/ResultCacheManager.php @@ -25,7 +25,6 @@ use PHPStan\File\CouldNotReadFileException; use PHPStan\File\CouldNotWriteFileException; use PHPStan\File\FileFinder; -use PHPStan\File\FileHelper; use PHPStan\File\FileStatSignatures; use PHPStan\Internal\ArrayHelper; use PHPStan\Internal\ComposerHelper; @@ -203,7 +202,6 @@ public function __construct( #[AutowiredParameter(ref: '@fileFinderScan')] private FileFinder $scanFileFinder, private StubFilesProvider $stubFilesProvider, - private FileHelper $fileHelper, private PackageDependencyResolver $packageDependencyResolver, private ExtensionVersionProvider $extensionVersionProvider, #[AutowiredParameter(ref: '%resultCachePath%')] @@ -2852,16 +2850,6 @@ private function getProjectExtensionFiles(?array $projectConfig, array $dependen $this->alreadyProcessed = []; $projectExtensionFiles = []; if ($projectConfig !== null) { - $vendorDirs = []; - foreach ($this->composerAutoloaderProjectPaths as $autoloaderProjectPath) { - $composer = ComposerHelper::getComposerConfig($autoloaderProjectPath); - if ($composer === null) { - continue; - } - $vendorDirectory = ComposerHelper::getVendorDirFromComposerConfig($autoloaderProjectPath, $composer); - $vendorDirs[] = $this->fileHelper->normalizePath($vendorDirectory); - } - $classes = ProjectConfigHelper::getServiceClassNames($projectConfig); foreach ($classes as $class) { try { @@ -2882,7 +2870,7 @@ private function getProjectExtensionFiles(?array $projectConfig, array $dependen $allServiceFiles = $this->getAllDependencies($fileName, $dependencies); if (count($allServiceFiles) === 0) { - if ($this->isFileOfInstalledPackage($this->fileHelper->normalizePath($fileName), $vendorDirs)) { + if ($this->packageDependencyResolver->isFileOfInstalledPackage($fileName)) { continue; } @@ -2903,29 +2891,6 @@ private function getProjectExtensionFiles(?array $projectConfig, array $dependen return $projectExtensionFiles; } - /** - * Whether the file belongs to a package Composer installed, which only changes when Composer - * changes it - editing one is not a thing to do, and a version change is noticed through the - * metadata instead. A package from a path repository is the exception: it is the project's own - * code, sitting in vendor/ because Composer copied it there, and it is edited in place. - * - * @param list $vendorDirs - */ - private function isFileOfInstalledPackage(string $normalizedFileName, array $vendorDirs): bool - { - foreach ($vendorDirs as $vendorDir) { - if (!str_starts_with($normalizedFileName, $vendorDir)) { - continue; - } - - $package = $this->packageDependencyResolver->resolvePackage($normalizedFileName); - - return $package === null || !$this->packageDependencyResolver->isPathPackage($package); - } - - return false; - } - /** * @param array> $dependencies * @return array diff --git a/src/Dependency/PackageDependencyResolver.php b/src/Dependency/PackageDependencyResolver.php index b75f550da9c..e3d85c23f84 100644 --- a/src/Dependency/PackageDependencyResolver.php +++ b/src/Dependency/PackageDependencyResolver.php @@ -47,6 +47,9 @@ final class PackageDependencyResolver /** @var array|null names of the installed packages that came from a path repository */ private ?array $pathPackages = null; + /** @var list|null normalized vendor directories of the analysed project */ + private ?array $vendorDirs = null; + /** @var array{classes: array, functions: array, constants: array}|null symbol => extension name */ private ?array $versionedExtensionSymbols = null; @@ -87,6 +90,29 @@ public function isPathPackage(string $package): bool return array_key_exists($package, $this->getPathPackages()); } + /** + * Whether the file belongs to a package Composer installed, which only changes when Composer + * changes it - editing one is not a thing to do. A package from a path repository is the exception: + * it is the project's own code, sitting in vendor/ because Composer copied it there, and it is edited in place. + */ + public function isFileOfInstalledPackage(string $file): bool + { + $this->resolveInstalledPackages(); + + $normalizedFile = $this->fileHelper->normalizePath($file); + foreach ($this->vendorDirs as $vendorDir) { + if (!str_starts_with($normalizedFile, $vendorDir)) { + continue; + } + + $package = $this->resolvePackage($normalizedFile); + + return $package === null || !$this->isPathPackage($package); + } + + return false; + } + private function doResolvePackage(string $file): ?string { // Normalize with a forward slash regardless of platform: normalizePath() defaults to @@ -282,22 +308,26 @@ private function getPathPackages(): array * * @phpstan-assert !null $this->installPathToPackage * @phpstan-assert !null $this->pathPackages + * @phpstan-assert !null $this->vendorDirs */ private function resolveInstalledPackages(): void { - if ($this->installPathToPackage !== null && $this->pathPackages !== null) { + if ($this->installPathToPackage !== null && $this->pathPackages !== null && $this->vendorDirs !== null) { return; } $map = []; $pathPackages = []; + $vendorDirs = []; foreach ($this->composerAutoloaderProjectPaths as $autoloadPath) { $composer = ComposerHelper::getComposerConfig($autoloadPath); if ($composer === null) { continue; } - $composerDirectory = ComposerHelper::getVendorDirFromComposerConfig($autoloadPath, $composer) . '/composer'; + $vendorDirectory = ComposerHelper::getVendorDirFromComposerConfig($autoloadPath, $composer); + $vendorDirs[] = $this->fileHelper->normalizePath($vendorDirectory); + $composerDirectory = $vendorDirectory . '/composer'; foreach ($this->readPathPackageNames($composerDirectory . '/installed.json') as $pathPackage) { $pathPackages[$pathPackage] = true; @@ -350,6 +380,7 @@ private function resolveInstalledPackages(): void $this->installPathToPackage = $map; $this->pathPackages = $pathPackages; + $this->vendorDirs = $vendorDirs; } /** diff --git a/tests/PHPStan/Analyser/AnalyserTest.php b/tests/PHPStan/Analyser/AnalyserTest.php index 2c571c8eda8..e1d88c6677e 100644 --- a/tests/PHPStan/Analyser/AnalyserTest.php +++ b/tests/PHPStan/Analyser/AnalyserTest.php @@ -8,6 +8,7 @@ use PhpParser\Parser\Php7; use PHPStan\Analyser\Generics\TemplateArgumentObserver; use PHPStan\Analyser\Ignore\IgnoredErrorHelper; +use PHPStan\Analyser\Ignore\IgnoreErrorsConfigFixer; use PHPStan\Analyser\Ignore\IgnoreLexer; use PHPStan\Analyser\Ignore\UnmatchedIgnoreFixer; use PHPStan\Collectors\Registry as CollectorRegistry; @@ -802,6 +803,7 @@ private function runAnalyser( $ignoredErrorHelper = new IgnoredErrorHelper( $this->getFileHelper(), + self::getContainer()->getByType(IgnoreErrorsConfigFixer::class), $ignoreErrors, $reportUnmatchedIgnoredErrors, ); diff --git a/tests/PHPStan/Analyser/Ignore/IgnoreErrorsConfigFixerTest.php b/tests/PHPStan/Analyser/Ignore/IgnoreErrorsConfigFixerTest.php new file mode 100644 index 00000000000..6c37694a3c7 --- /dev/null +++ b/tests/PHPStan/Analyser/Ignore/IgnoreErrorsConfigFixerTest.php @@ -0,0 +1,286 @@ + ['ignoreErrors' => \$ignoreErrors]];\n"; + + public static function dataRemoveUnmatched(): Generator + { + yield 'entry between other entries' => [ + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'count' => 1,\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . "\$ignoreErrors[] = [\n\t'message' => '#^B$#',\n\t'count' => 2,\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . "\$ignoreErrors[] = [\n\t'message' => '#^C$#',\n\t'count' => 1,\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END, + ['message' => '#^B$#', 'count' => 2, 'path' => '/Foo.php'], + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'count' => 1,\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . "\$ignoreErrors[] = [\n\t'message' => '#^C$#',\n\t'count' => 1,\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END, + ]; + + yield 'same message in another path is kept' => [ + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'path' => __DIR__ . '/Bar.php',\n];\n" + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END, + ['message' => '#^A$#', 'path' => '/Foo.php'], + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'path' => __DIR__ . '/Bar.php',\n];\n" + . self::BASELINE_END, + ]; + + yield 'entry with an identifier is told apart from one without it' => [ + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'identifier' => 'foo.bar',\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END, + ['message' => '#^A$#', 'identifier' => 'foo.bar', 'path' => '/Foo.php'], + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END, + ]; + + yield 'escaped percent sign in the message' => [ + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^100%% sure$#',\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END, + ['message' => '#^100% sure$#', 'path' => '/Foo.php'], + self::BASELINE_START . self::BASELINE_END, + ]; + + yield 'duplicate entries merged into one are all removed' => [ + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . "\$ignoreErrors[] = [\n\t'message' => '#^B$#',\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END, + ['message' => '#^A$#', 'count' => 2, 'path' => '/Foo.php'], + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^B$#',\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END, + ]; + + yield 'middle item of a multi-line list' => [ + " ['ignoreErrors' => [\n\t[\n\t\t'messages' => [\n\t\t\t'#^A$#',\n\t\t\t'#^B$#',\n\t\t\t'#^C$#',\n\t\t],\n\t\t'path' => __DIR__ . '/Foo.php',\n\t],\n]]];\n", + ['message' => '#^B$#', 'path' => '/Foo.php'], + " ['ignoreErrors' => [\n\t[\n\t\t'messages' => [\n\t\t\t'#^A$#',\n\t\t\t'#^C$#',\n\t\t],\n\t\t'path' => __DIR__ . '/Foo.php',\n\t],\n]]];\n", + ]; + + yield 'first item of a one-line list' => [ + " ['ignoreErrors' => [\n\t['messages' => ['#^A$#', '#^B$#'], 'path' => __DIR__ . '/Foo.php'],\n]]];\n", + ['message' => '#^A$#', 'path' => '/Foo.php'], + " ['ignoreErrors' => [\n\t['messages' => ['#^B$#'], 'path' => __DIR__ . '/Foo.php'],\n]]];\n", + ]; + + yield 'last item of a one-line list' => [ + " ['ignoreErrors' => [\n\t['messages' => ['#^A$#', '#^B$#'], 'path' => __DIR__ . '/Foo.php'],\n]]];\n", + ['message' => '#^B$#', 'path' => '/Foo.php'], + " ['ignoreErrors' => [\n\t['messages' => ['#^A$#'], 'path' => __DIR__ . '/Foo.php'],\n]]];\n", + ]; + + yield 'only item of a list removes the entry' => [ + " ['ignoreErrors' => [\n\t['identifiers' => ['foo.bar'], 'path' => __DIR__ . '/Foo.php'],\n\t['identifier' => 'baz.qux', 'path' => __DIR__ . '/Foo.php'],\n]]];\n", + ['identifier' => 'foo.bar', 'path' => '/Foo.php'], + " ['ignoreErrors' => [\n\t['identifier' => 'baz.qux', 'path' => __DIR__ . '/Foo.php'],\n]]];\n", + ]; + + yield 'last entry of a list without a trailing comma' => [ + " ['ignoreErrors' => [\n\t[\n\t\t'message' => '#^A$#',\n\t\t'path' => __DIR__ . '/Foo.php',\n\t],\n\t[\n\t\t'message' => '#^B$#',\n\t\t'path' => __DIR__ . '/Foo.php',\n\t]\n]]];\n", + ['message' => '#^B$#', 'path' => '/Foo.php'], + " ['ignoreErrors' => [\n\t[\n\t\t'message' => '#^A$#',\n\t\t'path' => __DIR__ . '/Foo.php',\n\t]\n]]];\n", + ]; + } + + /** + * @param array{message?: string, identifier?: string, count?: int, path: string} $ignoreError path relative to this directory + */ + #[DataProvider('dataRemoveUnmatched')] + public function testRemoveUnmatched(string $code, array $ignoreError, string $expected): void + { + [$fixer, $file] = $this->createFixer($code); + $ignoreError['path'] = __DIR__ . $ignoreError['path']; + $ignoreError['realPath'] = $ignoreError['path']; + + $this->assertSame($expected, $this->apply($file, $code, $fixer->removeUnmatched($ignoreError))); + } + + public function testRemovingAllItemsOfListTakesTwoRounds(): void + { + $code = " ['ignoreErrors' => [\n\t[\n\t\t'messages' => [\n\t\t\t'#^A$#',\n\t\t\t'#^B$#',\n\t\t],\n\t\t'path' => __DIR__ . '/Foo.php',\n\t],\n\t['message' => '#^C$#', 'path' => __DIR__ . '/Foo.php'],\n]]];\n"; + [$fixer, $file] = $this->createFixer($code); + $path = __DIR__ . '/Foo.php'; + $patcher = self::getContainer()->getByType(Patcher::class); + + $result = $patcher->apply([$file => $code], [ + $fixer->removeUnmatched(['message' => '#^A$#', 'path' => $path, 'realPath' => $path]), + $fixer->removeUnmatched(['message' => '#^B$#', 'path' => $path, 'realPath' => $path]), + ]); + $this->assertSame(1, $result['appliedCount']); + $this->assertSame( + " ['ignoreErrors' => [\n\t[\n\t\t'messages' => [\n\t\t\t'#^B$#',\n\t\t],\n\t\t'path' => __DIR__ . '/Foo.php',\n\t],\n\t['message' => '#^C$#', 'path' => __DIR__ . '/Foo.php'],\n]]];\n", + $result['contents'][$file], + ); + + // the next round + $code = $result['contents'][$file]; + [$fixer, $file] = $this->createFixer($code); + $this->assertSame( + " ['ignoreErrors' => [\n\t['message' => '#^C$#', 'path' => __DIR__ . '/Foo.php'],\n]]];\n", + $this->apply($file, $code, $fixer->removeUnmatched(['message' => '#^B$#', 'path' => $path, 'realPath' => $path])), + ); + } + + public function testLowerCount(): void + { + $code = self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'count' => 5,\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END; + [$fixer, $file] = $this->createFixer($code); + $path = __DIR__ . '/Foo.php'; + + $this->assertSame( + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'count' => 2,\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END, + $this->apply($file, $code, $fixer->lowerCount(['message' => '#^A$#', 'count' => 5, 'path' => $path, 'realPath' => $path], 2)), + ); + } + + public static function dataNotFixable(): Generator + { + // the merged count is spread over the entries, it is not clear which one to lower + yield 'count of duplicate entries' => [ + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'count' => 2,\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'count' => 3,\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END, + ['message' => '#^A$#', 'count' => 5, 'path' => '/Foo.php'], + true, + ]; + + // the rest of the count comes from an entry in a config file that is not fixed, like a NEON file + yield 'count of an entry merged with an entry in another file' => [ + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'count' => 2,\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END, + ['message' => '#^A$#', 'count' => 3, 'path' => '/Foo.php'], + true, + ]; + + yield 'count of an entry that is not in the file' => [ + self::BASELINE_START + . "\$ignoreErrors[] = [\n\t'message' => '#^A$#',\n\t'count' => 2,\n\t'path' => __DIR__ . '/Foo.php',\n];\n" + . self::BASELINE_END, + ['message' => '#^B$#', 'count' => 2, 'path' => '/Foo.php'], + true, + ]; + + yield 'entry in a variable is not removed on its own' => [ + " '#^A$#', 'path' => __DIR__ . '/Foo.php'];\nreturn ['parameters' => ['ignoreErrors' => [\$entry]]];\n", + ['message' => '#^A$#', 'path' => '/Foo.php'], + false, + ]; + } + + /** + * @param array{message?: string, identifier?: string, count?: int, path: string} $ignoreError path relative to this directory + */ + #[DataProvider('dataNotFixable')] + public function testNotFixable(string $code, array $ignoreError, bool $lowerCount): void + { + [$fixer] = $this->createFixer($code); + $ignoreError['path'] = __DIR__ . $ignoreError['path']; + $ignoreError['realPath'] = $ignoreError['path']; + + $this->assertSame([], $lowerCount ? $fixer->lowerCount($ignoreError, 1) : $fixer->removeUnmatched($ignoreError)); + } + + public function testNeonIsNotFixed(): void + { + $file = __DIR__ . '/' . uniqid('baseline-') . '.neon'; + file_put_contents($file, "parameters:\n\tignoreErrors:\n\t\t-\n\t\t\tmessage: '#^A$#'\n\t\t\tpath: Foo.php\n"); + try { + $fixer = $this->createFixerForFiles([$file]); + $path = __DIR__ . '/Foo.php'; + $this->assertSame([], $fixer->removeUnmatched(['message' => '#^A$#', 'path' => $path, 'realPath' => $path])); + } finally { + @unlink($file); + } + } + + /** + * The config file sits next to this test, so __DIR__ . '/Foo.php' in it is __DIR__ . '/Foo.php' here. + * + * @return array{IgnoreErrorsConfigFixer, string} + */ + private function createFixer(string $code): array + { + $file = __DIR__ . '/' . uniqid('baseline-') . '.php'; + file_put_contents($file, $code); + $this->filesToDelete[] = $file; + + return [$this->createFixerForFiles([$file]), $file]; + } + + /** @var list */ + private array $filesToDelete = []; + + #[Override] + protected function tearDown(): void + { + foreach ($this->filesToDelete as $file) { + @unlink($file); + } + $this->filesToDelete = []; + parent::tearDown(); + } + + /** + * @param list $configFiles + */ + private function createFixerForFiles(array $configFiles): IgnoreErrorsConfigFixer + { + $container = self::getContainer(); + + return new IgnoreErrorsConfigFixer( + $container->getService('currentPhpVersionPhpParser'), + $container->getByType(PackageDependencyResolver::class), + $container->getByType(FileHelper::class), + $configFiles, + sys_get_temp_dir(), + __DIR__, + ); + } + + /** + * @param list $diffs + */ + private function apply(string $file, string $code, array $diffs): string + { + $this->assertCount(1, $diffs); + $result = self::getContainer()->getByType(Patcher::class)->apply([$file => $code], [$diffs]); + $this->assertSame(1, $result['appliedCount']); + + return $result['contents'][$file]; + } + +} diff --git a/tests/PHPStan/Analyser/ResultCache/ResultCacheManagerCollectedDataTest.php b/tests/PHPStan/Analyser/ResultCache/ResultCacheManagerCollectedDataTest.php index 423eee7fcfa..33a477d968b 100644 --- a/tests/PHPStan/Analyser/ResultCache/ResultCacheManagerCollectedDataTest.php +++ b/tests/PHPStan/Analyser/ResultCache/ResultCacheManagerCollectedDataTest.php @@ -301,7 +301,6 @@ private function createManager(): ResultCacheManager exportedNodeFetcher: $container->getByType(ExportedNodeFetcher::class), scanFileFinder: $container->getService('fileFinderScan'), stubFilesProvider: $container->getByType(StubFilesProvider::class), - fileHelper: $container->getByType(FileHelper::class), packageDependencyResolver: $container->getByType(PackageDependencyResolver::class), extensionVersionProvider: $container->getByType(ExtensionVersionProvider::class), cacheFilePath: $this->cacheFilePath(),