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