From b93eb8a49d4ea9866a4aea78157f649f4b80314c Mon Sep 17 00:00:00 2001 From: "ondrej@mirtes.cz" Date: Sun, 1 Nov 2026 10:24:15 +0000 Subject: [PATCH] Re-analyse files with errors of rules on collected data when a symbol appears Rules on CollectedDataNode run in AnalyserResultFinalizer, and their errors are not in the cached error map: they re-run on the merged collected data on every run, so a cached copy would be reported twice. That map is what restore() sweeps when a new symbol appears - the recovery for an error that could record no dependency because the symbol it is about did not exist yet. A file whose only error came from collected data was never swept, so it kept the error after the symbol arrived, while a run with a cleared cache reported nothing. The rule results stored for reuseCollectedDataRuleErrors do not cover this: they are only stored with that toggle on. The cache now keeps the files with errors of these rules, taken from the finalizer result on every save regardless of the toggle, and restore() treats them like the files in the error map. Amp-Thread-ID: https://ampcode.com/threads/T-01a19695-f1c1-75fa-ac1f-125f79f272c7 --- .github/workflows/e2e-tests.yml | 14 +++++ .../appears/nsfns.php | 15 +++++ e2e/result-cache-collector-sweep/phpstan.neon | 5 ++ .../src/Caller.php | 8 +++ .../src/PlainCaller.php | 13 ++++ .../src/globalfns.php | 6 ++ .../tmp/.gitignore | 2 + src/Analyser/ResultCache/ResultCache.php | 10 +++ .../ResultCache/ResultCacheManager.php | 63 ++++++++++++++++--- .../ResultCache/ResultCacheProcessResult.php | 12 ++-- src/Command/AnalyseApplication.php | 2 +- src/Command/FixerWorkerRunner.php | 2 +- .../File/SimpleRelativePathHelperTest.php | 2 +- 13 files changed, 136 insertions(+), 18 deletions(-) create mode 100644 e2e/result-cache-collector-sweep/appears/nsfns.php create mode 100644 e2e/result-cache-collector-sweep/phpstan.neon create mode 100644 e2e/result-cache-collector-sweep/src/Caller.php create mode 100644 e2e/result-cache-collector-sweep/src/PlainCaller.php create mode 100644 e2e/result-cache-collector-sweep/src/globalfns.php create mode 100644 e2e/result-cache-collector-sweep/tmp/.gitignore diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 84cfa939732..99db5336ea1 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -1622,6 +1622,20 @@ jobs: OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw") echo "$OUTPUT" ../bashunit assert contains 'register a class in the container; re-analysing everything.' "$OUTPUT" + - script: | + cd e2e/result-cache-collector-sweep + # Two errors: one from an ordinary rule, one from a rule built on collected data. + OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + ../bashunit assert contains 'PlainCaller.php:8:Method ResultCacheE2ECollectorSweep\PlainCaller::doPlainCaller() has invalid return type ResultCacheE2ECollectorSweep\Missing.' "$OUTPUT" + ../bashunit assert contains 'Caller.php:7:Call to function resultCacheE2ECollectorHelper() on a separate line has no effect.' "$OUTPUT" + # One new file defines both missing symbols. The files with errors are re-analysed - including + # the file holding the error of the collector-backed rule, which is not in the cached error map. + cp appears/nsfns.php src/nsfns.php + OUTPUT=$(../bashunit assert exit_code "0" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + ../bashunit assert not_contains 'has invalid return type' "$OUTPUT" + ../bashunit assert not_contains 'on a separate line has no effect' "$OUTPUT" - script: | cd e2e/result-cache-extension-update composer install diff --git a/e2e/result-cache-collector-sweep/appears/nsfns.php b/e2e/result-cache-collector-sweep/appears/nsfns.php new file mode 100644 index 00000000000..1ae4ad8a1d9 --- /dev/null +++ b/e2e/result-cache-collector-sweep/appears/nsfns.php @@ -0,0 +1,15 @@ + $projectExtensionFiles * @param array $currentFileHashes * @param array $collectedDataRuleResults + * @param list $filesWithCollectedDataRuleErrors the files with errors of the rules on CollectedDataNode */ public function __construct( private array $filesToAnalyse, @@ -52,6 +53,7 @@ public function __construct( private array $projectExtensionFiles, private array $currentFileHashes, private array $collectedDataRuleResults = [], + private array $filesWithCollectedDataRuleErrors = [], ) { } @@ -203,4 +205,12 @@ public function getCollectedDataRuleResults(): array return $this->collectedDataRuleResults; } + /** + * @return list + */ + public function getFilesWithCollectedDataRuleErrors(): array + { + return $this->filesWithCollectedDataRuleErrors; + } + } diff --git a/src/Analyser/ResultCache/ResultCacheManager.php b/src/Analyser/ResultCache/ResultCacheManager.php index 078b7ca38ff..375799e8605 100644 --- a/src/Analyser/ResultCache/ResultCacheManager.php +++ b/src/Analyser/ResultCache/ResultCacheManager.php @@ -7,6 +7,7 @@ use PHPStan\Analyser\CollectedDataRuleResult; use PHPStan\Analyser\Error; use PHPStan\Analyser\FileAnalyserResult; +use PHPStan\Analyser\FinalizerResult; use PHPStan\Analyser\ValueDependencyCollector; use PHPStan\Collectors\CollectedData; use PHPStan\Command\Output; @@ -42,6 +43,7 @@ use function array_intersect; use function array_key_exists; use function array_keys; +use function array_map; use function array_merge; use function array_unique; use function array_values; @@ -106,7 +108,7 @@ final class ResultCacheManager */ private const EXTENSIONS_NOT_INVALIDATING_CACHE = ['xdebug', 'blackfire', 'phpstan_turbo']; - private const CACHE_VERSION = 'v23-collectedDataRuleResults'; + private const CACHE_VERSION = 'v24-filesWithCollectedDataRuleErrors'; /** * The recorded hash of a dependency that does not exist. A rule can depend on a path rather than on @@ -716,6 +718,18 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ? } } $collectedDataRuleResults = $transformer->absolutizeCollectedDataRuleResults($collectedDataRuleResults); + $storedFilesWithCollectedDataRuleErrors = $data['filesWithCollectedDataRuleErrors'] ?? []; + if (!is_array($storedFilesWithCollectedDataRuleErrors)) { + throw new RuntimeException('The cached files with errors of the rules on collected data are not an array.'); + } + $filesWithCollectedDataRuleErrors = []; + foreach ($storedFilesWithCollectedDataRuleErrors as $fileWithCollectedDataRuleError) { + if (!is_string($fileWithCollectedDataRuleError)) { + throw new RuntimeException('A cached file with errors of the rules on collected data is not a string.'); + } + + $filesWithCollectedDataRuleErrors[] = $transformer->absolutizePath($fileWithCollectedDataRuleError); + } } catch (Throwable $e) { @unlink($cacheFilePath); @@ -775,10 +789,20 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ? $changedAnalysedFiles[$analysedFile] = true; } + // The errors of the rules on CollectedDataNode are not in $errors: those rules run on the merged + // collected data on every run, a cached copy would be reported twice. But such an error waits + // for a symbol to appear just like any other, and re-analysing its file renews the file's share + // of the collected data. + $cachedFilesWithCollectedDataRuleErrors = array_fill_keys($filesWithCollectedDataRuleErrors, true); + $filteredFilesWithCollectedDataRuleErrors = []; + // only the stub files with errors so far, which are never analysed $filesWithErrorsAllChanged = $filteredErrors === []; foreach ($allAnalysedFiles as $analysedFile) { - if (!array_key_exists($analysedFile, $errors) || array_key_exists($analysedFile, $changedAnalysedFiles)) { + if ( + (!array_key_exists($analysedFile, $errors) && !array_key_exists($analysedFile, $cachedFilesWithCollectedDataRuleErrors)) + || array_key_exists($analysedFile, $changedAnalysedFiles) + ) { continue; } @@ -790,6 +814,9 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ? if (array_key_exists($analysedFile, $errors)) { $filteredErrors[$analysedFile] = $errors[$analysedFile]; } + if (array_key_exists($analysedFile, $cachedFilesWithCollectedDataRuleErrors)) { + $filteredFilesWithCollectedDataRuleErrors[] = $analysedFile; + } if (array_key_exists($analysedFile, $locallyIgnoredErrors)) { $filteredLocallyIgnoredErrors[$analysedFile] = $locallyIgnoredErrors[$analysedFile]; } @@ -881,6 +908,9 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ? } } + // in the order process() compares them in + sort($filteredFilesWithCollectedDataRuleErrors); + // the freshly computed metadata, so a file that stopped being scanned is not counted $scannedFiles = is_array($meta['scannedFiles'] ?? null) ? $meta['scannedFiles'] : []; foreach (array_keys($notAnalysedFiles) as $notAnalysedFile) { @@ -1036,6 +1066,9 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ? $filesToAnalyse[] = $fileWithError; } + foreach ($filteredFilesWithCollectedDataRuleErrors as $fileWithError) { + $filesToAnalyse[] = $fileWithError; + } } foreach ($packageSeededFiles as $packageSeededFile) { @@ -1105,6 +1138,7 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ? projectExtensionFiles: $data['projectExtensionFiles'], currentFileHashes: $currentFileHashes, collectedDataRuleResults: $collectedDataRuleResults, + filesWithCollectedDataRuleErrors: $filteredFilesWithCollectedDataRuleErrors, ); } @@ -1251,7 +1285,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache $projectConfigArray = $this->getPathTransformer()->relativizeProjectConfig($projectConfigArray); $meta['projectConfig'] = Neon::encode($projectConfigArray); } - $doSave = function (array $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, ?array $dependencies, ?array $usedTraitDependencies, ?array $valueDependencies, ?array $packageDependencies, array $exportedNodes, CachedExportedNodes $cachedExportedNodes, array $projectExtensionFiles, array $collectedDataRuleResults) use ($internalErrors, $resultCache, $output, $onlyFiles, $meta): bool { + $doSave = function (array $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, ?array $dependencies, ?array $usedTraitDependencies, ?array $valueDependencies, ?array $packageDependencies, array $exportedNodes, CachedExportedNodes $cachedExportedNodes, array $projectExtensionFiles, FinalizerResult $finalizerResult) use ($internalErrors, $resultCache, $output, $onlyFiles, $meta): bool { if ($onlyFiles) { if ($output->isVeryVerbose()) { $output->writeLineFormatted('Result cache was not saved because only files were passed as analysed paths.'); @@ -1337,8 +1371,16 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache } // in the order save() writes them, which is the order they are restored in + $collectedDataRuleResults = $finalizerResult->getCollectedDataRuleResults(); ksort($collectedDataRuleResults); + $filesWithCollectedDataRuleErrors = []; + foreach ($finalizerResult->getCollectorErrors() as $collectorError) { + $filesWithCollectedDataRuleErrors[$collectorError->getFilePath()] = true; + } + ksort($filesWithCollectedDataRuleErrors); + $filesWithCollectedDataRuleErrors = array_keys($filesWithCollectedDataRuleErrors); + $stubFiles = $this->getStubFiles(); if ( !$resultCache->isFullAnalysis() @@ -1350,6 +1392,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache && $unmatchedLineIgnores === $resultCache->getUnmatchedLineIgnores() && $collectedDataByFile === $resultCache->getCollectedData() && $collectedDataRuleResults === $resultCache->getCollectedDataRuleResults() + && $filesWithCollectedDataRuleErrors === $resultCache->getFilesWithCollectedDataRuleErrors() && $packageDependencies === $resultCache->getPackageDependencies() && $exportedNodes === $resultCache->getExportedNodes() && $cachedExportedNodes->getFiles() === $resultCache->getCachedExportedNodes()->getFiles() @@ -1368,7 +1411,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache $currentFileHashes[$file] = self::REANALYSE_FILE_HASH_PREFIX . $currentFileHashes[$file]; } - $this->save($resultCache->getLastFullAnalysisTime(), $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes, $projectExtensionFiles, $currentFileHashes, $meta, $stubFiles, $collectedDataRuleResults); + $this->save($resultCache->getLastFullAnalysisTime(), $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes, $projectExtensionFiles, $currentFileHashes, $meta, $stubFiles, $collectedDataRuleResults, $filesWithCollectedDataRuleErrors); if ($output->isVeryVerbose()) { $output->writeLineFormatted('Result cache is saved.'); @@ -1378,7 +1421,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache }; if ($resultCache->isFullAnalysis()) { - return new ResultCacheProcessResult($analyserResult, function (array $collectedDataRuleResults) use ($save, $output, $analyserResult, $projectConfigArray, $doSave, $freshErrorsByFile, $freshLocallyIgnoredErrorsByFile, $freshCollectedDataByFile): bool { + return new ResultCacheProcessResult($analyserResult, function (FinalizerResult $finalizerResult) use ($save, $output, $analyserResult, $projectConfigArray, $doSave, $freshErrorsByFile, $freshLocallyIgnoredErrorsByFile, $freshCollectedDataByFile): bool { if ($save === false) { if ($output->isVeryVerbose()) { $output->writeLineFormatted('Result cache was not saved because it was not requested.'); @@ -1392,7 +1435,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache $projectExtensionFiles = $this->getProjectExtensionFiles($projectConfigArray, $analyserResult->getDependencies()); } - return $doSave($freshErrorsByFile, $freshLocallyIgnoredErrorsByFile, $analyserResult->getLinesToIgnore(), $analyserResult->getUnmatchedLineIgnores(), $freshCollectedDataByFile, $analyserResult->getDependencies(), $analyserResult->getUsedTraitDependencies(), $analyserResult->getValueDependencies(), $analyserResult->getPackageDependencies(), $this->addNonAnalysedExportedNodes($analyserResult->getExportedNodes(), $analyserResult->getDependencies(), $analyserResult->getUsedTraitDependencies(), CachedExportedNodes::createEmpty()), CachedExportedNodes::createEmpty(), $projectExtensionFiles, $collectedDataRuleResults); + return $doSave($freshErrorsByFile, $freshLocallyIgnoredErrorsByFile, $analyserResult->getLinesToIgnore(), $analyserResult->getUnmatchedLineIgnores(), $freshCollectedDataByFile, $analyserResult->getDependencies(), $analyserResult->getUsedTraitDependencies(), $analyserResult->getValueDependencies(), $analyserResult->getPackageDependencies(), $this->addNonAnalysedExportedNodes($analyserResult->getExportedNodes(), $analyserResult->getDependencies(), $analyserResult->getUsedTraitDependencies(), CachedExportedNodes::createEmpty()), CachedExportedNodes::createEmpty(), $projectExtensionFiles, $finalizerResult); }); } @@ -1409,7 +1452,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache $linesToIgnore = $this->mergeLinesToIgnore($resultCache, $analyserResult->getLinesToIgnore()); $unmatchedLineIgnores = $this->mergeUnmatchedLineIgnores($resultCache, $analyserResult->getUnmatchedLineIgnores()); - $saveCallback = function (array $collectedDataRuleResults) use ($save, $resultCache, $projectConfigArray, $doSave, $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes): bool { + $saveCallback = function (FinalizerResult $finalizerResult) use ($save, $resultCache, $projectConfigArray, $doSave, $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes): bool { if ($save === false) { return false; } @@ -1434,7 +1477,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache $projectExtensionFiles[$file] = [$hash, true, $className]; } } - return $doSave($errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes, $projectExtensionFiles, $collectedDataRuleResults); + return $doSave($errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes, $projectExtensionFiles, $finalizerResult); }; $flatErrors = []; @@ -1894,6 +1937,7 @@ private function mergeUnmatchedLineIgnores(ResultCache $resultCache, array $fres * @param mixed[] $meta * @param array $stubFiles * @param array $collectedDataRuleResults + * @param list $filesWithCollectedDataRuleErrors */ private function save( int $lastFullAnalysisTime, @@ -1913,6 +1957,7 @@ private function save( array $meta, array $stubFiles, array $collectedDataRuleResults, + array $filesWithCollectedDataRuleErrors, ): void { $invertedDependencies = []; @@ -1999,6 +2044,7 @@ private function save( $packageDependencies = $transformer->relativizeFileKeyed($packageDependencies); $projectExtensionFiles = $transformer->relativizeFileKeyed($projectExtensionFiles); $collectedDataRuleResults = $transformer->relativizeCollectedDataRuleResults($collectedDataRuleResults); + $filesWithCollectedDataRuleErrors = array_map(static fn (string $path): string => $transformer->relativizePath($path), $filesWithCollectedDataRuleErrors); $file = $this->cacheFilePath; @@ -2036,6 +2082,7 @@ private function save( $this->writeArrayFrame($handle, $file, 'unmatchedLineIgnores', $unmatchedLineIgnores); $this->writeArrayFrame($handle, $file, 'collectedData', $collectedData); $this->writeValueFrame($handle, $file, 'collectedDataRuleResults', $collectedDataRuleResults); + $this->writeValueFrame($handle, $file, 'filesWithCollectedDataRuleErrors', $filesWithCollectedDataRuleErrors); // An older PHPStan reading this file absolutizes the dependencies before it gets to the // cacheVersion check that makes it discard the file, and fails on a missing section. $this->writeArrayFrame($handle, $file, 'dependencies', []); diff --git a/src/Analyser/ResultCache/ResultCacheProcessResult.php b/src/Analyser/ResultCache/ResultCacheProcessResult.php index ee7b5621d9f..a79b65e0093 100644 --- a/src/Analyser/ResultCache/ResultCacheProcessResult.php +++ b/src/Analyser/ResultCache/ResultCacheProcessResult.php @@ -4,13 +4,13 @@ use Closure; use PHPStan\Analyser\AnalyserResult; -use PHPStan\Analyser\CollectedDataRuleResult; +use PHPStan\Analyser\FinalizerResult; final class ResultCacheProcessResult { /** - * @param Closure(array): bool $saveCallback + * @param Closure(FinalizerResult): bool $saveCallback */ public function __construct(private AnalyserResult $analyserResult, private Closure $saveCallback) { @@ -25,13 +25,11 @@ public function getAnalyserResult(): AnalyserResult * Writes the result cache, and says whether it was written. * * It is separate from ResultCacheManager::process() so that the caller can run the rules on - * CollectedDataNode on the merged result first, and save their results too. - * - * @param array $collectedDataRuleResults + * CollectedDataNode on the merged result first, and save what they found too. */ - public function save(array $collectedDataRuleResults): bool + public function save(FinalizerResult $finalizerResult): bool { - return ($this->saveCallback)($collectedDataRuleResults); + return ($this->saveCallback)($finalizerResult); } } diff --git a/src/Command/AnalyseApplication.php b/src/Command/AnalyseApplication.php index a25f577141c..da47355bd36 100644 --- a/src/Command/AnalyseApplication.php +++ b/src/Command/AnalyseApplication.php @@ -153,7 +153,7 @@ public function analyse( count($finalizerResult->getCollectedDataRuleResults()), )); } - $savedResultCache = $resultCacheResult->save($finalizerResult->getCollectedDataRuleResults()); + $savedResultCache = $resultCacheResult->save($finalizerResult); $analyserResult = $finalizerResult->getAnalyserResult(); $internalErrors = $analyserResult->getInternalErrors(); $errors = array_merge( diff --git a/src/Command/FixerWorkerRunner.php b/src/Command/FixerWorkerRunner.php index e2067070752..3b5f8c270fd 100644 --- a/src/Command/FixerWorkerRunner.php +++ b/src/Command/FixerWorkerRunner.php @@ -198,7 +198,7 @@ function (array $errors, array $locallyIgnoredErrors, array $analysedFiles) use true, ); $finalizerResult = $this->analyserResultFinalizer->finalize($resultCacheResult->getAnalyserResult(), $isOnlyFiles, false, null, null, $resultCache->getCollectedDataRuleResults()); - $resultCacheResult->save($finalizerResult->getCollectedDataRuleResults()); + $resultCacheResult->save($finalizerResult); $internalErrors = []; foreach ($finalizerResult->getAnalyserResult()->getInternalErrors() as $internalError) { diff --git a/tests/PHPStan/File/SimpleRelativePathHelperTest.php b/tests/PHPStan/File/SimpleRelativePathHelperTest.php index 49c93f3fabb..d6facefa457 100644 --- a/tests/PHPStan/File/SimpleRelativePathHelperTest.php +++ b/tests/PHPStan/File/SimpleRelativePathHelperTest.php @@ -35,7 +35,7 @@ public static function dataGetRelativePath(): array '/project/src', '/project/src/HelloWorld.php', 'HelloWorld.php', - ] + ], ]; }