diff --git a/e2e/fix-ignored-entries/fix-ignored-entries_test.sh b/e2e/fix-ignored-entries/fix-ignored-entries_test.sh index 399409a3b5c..114b46faf49 100644 --- a/e2e/fix-ignored-entries/fix-ignored-entries_test.sh +++ b/e2e/fix-ignored-entries/fix-ignored-entries_test.sh @@ -30,3 +30,24 @@ function test_fix_ignored_entries() { assert_contains 'Untouched.php:?:Ignored error pattern #^Stale$#' "$OUTPUT" assert_not_contains 'src/In' "$OUTPUT" } + +function test_fix_ignored_entries_in_list_of_files() { + composer install + rm -rf src tmp && cp -r original src && cp baseline-original.php baseline.php && cp baseline-original.neon baseline.neon + # a list of files is a partial analysis, like a git hook that passes the changed files, + # the ignores in the files the first round changed are still fixed in the second round + OUTPUT=$(../../bin/phpstan analyse --fix-ignored src/InComment.php src/InNeonBaseline.php src/InPhpBaseline.php src/Untouched.php src/functions.php 2>&1) && CODE=0 || CODE=$? + assert_same "0" "$CODE" + echo "$OUTPUT" + assert_contains 'Round 1: fixed 5 errors in 3 files' "$OUTPUT" + assert_contains 'Round 2: fixed 3 errors in 3 files' "$OUTPUT" + assert_contains '[OK] 8 errors fixed in 5 files' "$OUTPUT" + for file in InComment.php InNeonBaseline.php InPhpBaseline.php; do diff -u expected/$file src/$file; done + diff -u expected/baseline.php baseline.php + diff -u expected/baseline.neon baseline.neon + OUTPUT=$(../../bin/phpstan analyse --error-format raw 2>&1) && CODE=0 || CODE=$? + assert_same "1" "$CODE" + echo "$OUTPUT" + assert_contains 'Untouched.php:?:Ignored error pattern #^Stale$#' "$OUTPUT" + assert_not_contains 'src/In' "$OUTPUT" +} diff --git a/src/Analyser/Ignore/IgnoredErrorHelperResult.php b/src/Analyser/Ignore/IgnoredErrorHelperResult.php index f1bed8885d9..0d0390b67a8 100644 --- a/src/Analyser/Ignore/IgnoredErrorHelperResult.php +++ b/src/Analyser/Ignore/IgnoredErrorHelperResult.php @@ -7,8 +7,10 @@ use PHPStan\File\FileHelper; use PHPStan\ShouldNotHappenException; use function array_fill_keys; +use function array_flip; use function array_key_exists; use function array_keys; +use function array_map; use function count; use function is_array; use function is_string; @@ -91,12 +93,14 @@ private static function filterIgnoreErrorsByIdentifier(array $ignoreErrors, ?str /** * @param list $errors * @param string[] $analysedFiles + * @param string[] $filesChangedByFix Files changed by the previous rounds of --fix, their unmatched ignores are reported even for a list of files. */ public function process( array $errors, bool $onlyFiles, array $analysedFiles, bool $hasInternalErrors, + array $filesChangedByFix = [], ): IgnoredErrorHelperProcessedResult { $unmatchedIgnoredErrors = $this->ignoreErrors; @@ -355,6 +359,7 @@ public function process( } $analysedFilesKeys = array_fill_keys($analysedFiles, true); + $filesChangedByFixKeys = array_flip(array_map(fn (string $file): string => $this->fileHelper->normalizePath($file), $filesChangedByFix)); if (!$hasInternalErrors) { foreach ($unmatchedIgnoredErrors as $i => $unmatchedIgnoredError) { @@ -396,7 +401,10 @@ public function process( continue; } - if ($onlyFiles) { + // an earlier round of --fix can leave an ignore in a file it changed unmatched, like the one + // that matched an error fixed by --fix-ignored, so it is reported for a list of files too + // and the next round removes it + if ($onlyFiles && !array_key_exists($unmatchedIgnoredError['realPath'], $filesChangedByFixKeys)) { continue; } diff --git a/src/Command/AnalyseApplication.php b/src/Command/AnalyseApplication.php index d9c223b153d..78546f855d5 100644 --- a/src/Command/AnalyseApplication.php +++ b/src/Command/AnalyseApplication.php @@ -53,6 +53,7 @@ public function __construct( /** * @param string[] $files * @param mixed[]|null $projectConfigArray + * @param string[] $filesChangedByFix * @throws InceptionNotSuccessfulException */ public function analyse( @@ -67,6 +68,7 @@ public function analyse( ?string $tmpFile, ?string $insteadOfFile, InputInterface $input, + array $filesChangedByFix = [], ): AnalysisResult { $isResultCacheUsed = false; @@ -167,7 +169,7 @@ public function analyse( $workerCount = $analyserResult->getWorkerCount(); $isResultCacheUsed = !$resultCache->isFullAnalysis(); - $ignoredErrorHelperProcessedResult = $ignoredErrorHelperResult->process($errors, $onlyFiles, $files, $hasInternalErrors); + $ignoredErrorHelperProcessedResult = $ignoredErrorHelperResult->process($errors, $onlyFiles, $files, $hasInternalErrors, $filesChangedByFix); $fileSpecificErrors = $ignoredErrorHelperProcessedResult->getNotIgnoredErrors(); $notFileSpecificErrors = $ignoredErrorHelperProcessedResult->getOtherIgnoreMessages(); foreach ($ignoredErrorHelperProcessedResult->getIgnoredErrors() as [$ignoredError]) { diff --git a/src/Command/AnalyseCommand.php b/src/Command/AnalyseCommand.php index 114757add92..7b2a5e4d758 100644 --- a/src/Command/AnalyseCommand.php +++ b/src/Command/AnalyseCommand.php @@ -397,6 +397,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int $inceptionResult->getErrorOutput()->writeLineFormatted('Analysing files...'); } + /** @var string[] $filesChangedByFix */ + $filesChangedByFix = $input->getOption(self::OPTION_FIXED_IGNORED_IN); + try { $analysisResult = $application->analyse( $files, @@ -410,6 +413,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $inceptionResult->getEditorModeTmpFile(), $inceptionResult->getEditorModeInsteadOfFile(), $input, + $filesChangedByFix, ); } catch (Throwable $t) { if ($debug) { diff --git a/tests/PHPStan/Analyser/AnalyserTest.php b/tests/PHPStan/Analyser/AnalyserTest.php index 3a800ea97a0..5fb7be8581a 100644 --- a/tests/PHPStan/Analyser/AnalyserTest.php +++ b/tests/PHPStan/Analyser/AnalyserTest.php @@ -657,6 +657,44 @@ public function testDoNotReportUnmatchedIgnoredErrorsFromPathWithCountIfPathWasN $this->assertNoErrors($result); } + /** + * @return iterable, bool}> + */ + public static function dataReportUnmatchedIgnoredErrorFromPathInFileChangedByFix(): iterable + { + yield 'list of files' => [true, [], false]; + yield 'list of files, file changed by an earlier fix round' => [true, [__DIR__ . '/data/empty/empty.php'], true]; + yield 'list of files, file changed by an earlier fix round, path not normalized' => [true, [__DIR__ . '/data/empty/../empty/empty.php'], true]; + yield 'list of files, another file changed by an earlier fix round' => [true, [__DIR__ . '/data/two-fails.php'], false]; + yield 'full analysis' => [false, [], true]; + } + + /** + * @param list $filesChangedByFix + */ + #[DataProvider('dataReportUnmatchedIgnoredErrorFromPathInFileChangedByFix')] + public function testReportUnmatchedIgnoredErrorFromPathInFileChangedByFix(bool $onlyFiles, array $filesChangedByFix, bool $expectReported): void + { + $ignoreErrors = [ + [ + 'message' => '#Fail\.#', + 'path' => __DIR__ . '/data/empty/empty.php', + ], + ]; + $result = $this->runAnalyser($ignoreErrors, true, __DIR__ . '/data/empty/empty.php', $onlyFiles, false, [], [], $filesChangedByFix); + if (!$expectReported) { + $this->assertNoErrors($result); + return; + } + + $expectedPath = $this->getFileHelper()->normalizePath(__DIR__ . '/data/empty/empty.php'); + $this->assertCount(1, $result); + $this->assertInstanceOf(Error::class, $result[0]); + $this->assertSame('Ignored error pattern #Fail\.# in path ' . $expectedPath . ' was not matched in reported errors.', $result[0]->getMessage()); + $this->assertSame($expectedPath, $result[0]->getFilePath()); + $this->assertSame('ignore.unmatched', $result[0]->getIdentifier()); + } + public function testIgnoreNextLine(): void { $result = $this->runAnalyser([], false, [ @@ -851,6 +889,7 @@ public function testIgnoreErrorExplicitReportUnmatchedEnableMulti(): void * @param string|string[] $filePaths * @param list> $collectors * @param list> $collectedDataRules + * @param list $filesChangedByFix * @return string[]|Error[] */ private function runAnalyser( @@ -861,6 +900,7 @@ private function runAnalyser( bool $reportIgnoresWithoutComments = false, array $collectors = [], array $collectedDataRules = [], + array $filesChangedByFix = [], ): array { $analyser = $this->createAnalyser($reportIgnoresWithoutComments, $collectors); @@ -901,7 +941,13 @@ private function runAnalyser( ); $analyserResult = $finalizer->finalize($analyserResult, $onlyFiles, false)->getAnalyserResult(); - $ignoredErrorHelperProcessedResult = $ignoredErrorHelperResult->process($analyserResult->getErrors(), $onlyFiles, $normalizedFilePaths, $analyserResult->hasReachedInternalErrorsCountLimit()); + $ignoredErrorHelperProcessedResult = $ignoredErrorHelperResult->process( + $analyserResult->getErrors(), + $onlyFiles, + $normalizedFilePaths, + $analyserResult->hasReachedInternalErrorsCountLimit(), + $filesChangedByFix, + ); $errors = $ignoredErrorHelperProcessedResult->getNotIgnoredErrors(); $errors = array_merge($errors, $ignoredErrorHelperProcessedResult->getOtherIgnoreMessages()); if ($analyserResult->hasReachedInternalErrorsCountLimit()) {