Skip to content

Commit b93eb8a

Browse files
committed
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
1 parent 3dd39a6 commit b93eb8a

13 files changed

Lines changed: 136 additions & 18 deletions

File tree

‎.github/workflows/e2e-tests.yml‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1622,6 +1622,20 @@ jobs:
16221622
OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw")
16231623
echo "$OUTPUT"
16241624
../bashunit assert contains 'register a class in the container; re-analysing everything.' "$OUTPUT"
1625+
- script: |
1626+
cd e2e/result-cache-collector-sweep
1627+
# Two errors: one from an ordinary rule, one from a rule built on collected data.
1628+
OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw")
1629+
echo "$OUTPUT"
1630+
../bashunit assert contains 'PlainCaller.php:8:Method ResultCacheE2ECollectorSweep\PlainCaller::doPlainCaller() has invalid return type ResultCacheE2ECollectorSweep\Missing.' "$OUTPUT"
1631+
../bashunit assert contains 'Caller.php:7:Call to function resultCacheE2ECollectorHelper() on a separate line has no effect.' "$OUTPUT"
1632+
# One new file defines both missing symbols. The files with errors are re-analysed - including
1633+
# the file holding the error of the collector-backed rule, which is not in the cached error map.
1634+
cp appears/nsfns.php src/nsfns.php
1635+
OUTPUT=$(../bashunit assert exit_code "0" "../../bin/phpstan analyse -vv --error-format raw")
1636+
echo "$OUTPUT"
1637+
../bashunit assert not_contains 'has invalid return type' "$OUTPUT"
1638+
../bashunit assert not_contains 'on a separate line has no effect' "$OUTPUT"
16251639
- script: |
16261640
cd e2e/result-cache-extension-update
16271641
composer install
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace ResultCacheE2ECollectorSweep;
4+
5+
function resultCacheE2ECollectorHelper(): int
6+
{
7+
echo 'side effect';
8+
9+
return 1;
10+
}
11+
12+
class Missing
13+
{
14+
15+
}
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
parameters:
2+
level: 4
3+
tmpDir: tmp
4+
paths:
5+
- src
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace ResultCacheE2ECollectorSweep;
4+
5+
function caller(): void
6+
{
7+
resultCacheE2ECollectorHelper();
8+
}
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace ResultCacheE2ECollectorSweep;
4+
5+
class PlainCaller
6+
{
7+
8+
public function doPlainCaller(): ?Missing
9+
{
10+
return null;
11+
}
12+
13+
}
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<?php declare(strict_types = 1);
2+
3+
function resultCacheE2ECollectorHelper(): int
4+
{
5+
return 1;
6+
}
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
*
2+
!.*

‎src/Analyser/ResultCache/ResultCache.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ final class ResultCache
3131
* @param array<string, array{string, bool, string}> $projectExtensionFiles
3232
* @param array<string, string> $currentFileHashes
3333
* @param array<string, CollectedDataRuleResult> $collectedDataRuleResults
34+
* @param list<string> $filesWithCollectedDataRuleErrors the files with errors of the rules on CollectedDataNode
3435
*/
3536
public function __construct(
3637
private array $filesToAnalyse,
@@ -52,6 +53,7 @@ public function __construct(
5253
private array $projectExtensionFiles,
5354
private array $currentFileHashes,
5455
private array $collectedDataRuleResults = [],
56+
private array $filesWithCollectedDataRuleErrors = [],
5557
)
5658
{
5759
}
@@ -203,4 +205,12 @@ public function getCollectedDataRuleResults(): array
203205
return $this->collectedDataRuleResults;
204206
}
205207

208+
/**
209+
* @return list<string>
210+
*/
211+
public function getFilesWithCollectedDataRuleErrors(): array
212+
{
213+
return $this->filesWithCollectedDataRuleErrors;
214+
}
215+
206216
}

‎src/Analyser/ResultCache/ResultCacheManager.php‎

Lines changed: 55 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use PHPStan\Analyser\CollectedDataRuleResult;
88
use PHPStan\Analyser\Error;
99
use PHPStan\Analyser\FileAnalyserResult;
10+
use PHPStan\Analyser\FinalizerResult;
1011
use PHPStan\Analyser\ValueDependencyCollector;
1112
use PHPStan\Collectors\CollectedData;
1213
use PHPStan\Command\Output;
@@ -42,6 +43,7 @@
4243
use function array_intersect;
4344
use function array_key_exists;
4445
use function array_keys;
46+
use function array_map;
4547
use function array_merge;
4648
use function array_unique;
4749
use function array_values;
@@ -106,7 +108,7 @@ final class ResultCacheManager
106108
*/
107109
private const EXTENSIONS_NOT_INVALIDATING_CACHE = ['xdebug', 'blackfire', 'phpstan_turbo'];
108110

109-
private const CACHE_VERSION = 'v23-collectedDataRuleResults';
111+
private const CACHE_VERSION = 'v24-filesWithCollectedDataRuleErrors';
110112

111113
/**
112114
* 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, ?
716718
}
717719
}
718720
$collectedDataRuleResults = $transformer->absolutizeCollectedDataRuleResults($collectedDataRuleResults);
721+
$storedFilesWithCollectedDataRuleErrors = $data['filesWithCollectedDataRuleErrors'] ?? [];
722+
if (!is_array($storedFilesWithCollectedDataRuleErrors)) {
723+
throw new RuntimeException('The cached files with errors of the rules on collected data are not an array.');
724+
}
725+
$filesWithCollectedDataRuleErrors = [];
726+
foreach ($storedFilesWithCollectedDataRuleErrors as $fileWithCollectedDataRuleError) {
727+
if (!is_string($fileWithCollectedDataRuleError)) {
728+
throw new RuntimeException('A cached file with errors of the rules on collected data is not a string.');
729+
}
730+
731+
$filesWithCollectedDataRuleErrors[] = $transformer->absolutizePath($fileWithCollectedDataRuleError);
732+
}
719733
} catch (Throwable $e) {
720734
@unlink($cacheFilePath);
721735

@@ -775,10 +789,20 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ?
775789

776790
$changedAnalysedFiles[$analysedFile] = true;
777791
}
792+
// The errors of the rules on CollectedDataNode are not in $errors: those rules run on the merged
793+
// collected data on every run, a cached copy would be reported twice. But such an error waits
794+
// for a symbol to appear just like any other, and re-analysing its file renews the file's share
795+
// of the collected data.
796+
$cachedFilesWithCollectedDataRuleErrors = array_fill_keys($filesWithCollectedDataRuleErrors, true);
797+
$filteredFilesWithCollectedDataRuleErrors = [];
798+
778799
// only the stub files with errors so far, which are never analysed
779800
$filesWithErrorsAllChanged = $filteredErrors === [];
780801
foreach ($allAnalysedFiles as $analysedFile) {
781-
if (!array_key_exists($analysedFile, $errors) || array_key_exists($analysedFile, $changedAnalysedFiles)) {
802+
if (
803+
(!array_key_exists($analysedFile, $errors) && !array_key_exists($analysedFile, $cachedFilesWithCollectedDataRuleErrors))
804+
|| array_key_exists($analysedFile, $changedAnalysedFiles)
805+
) {
782806
continue;
783807
}
784808

@@ -790,6 +814,9 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ?
790814
if (array_key_exists($analysedFile, $errors)) {
791815
$filteredErrors[$analysedFile] = $errors[$analysedFile];
792816
}
817+
if (array_key_exists($analysedFile, $cachedFilesWithCollectedDataRuleErrors)) {
818+
$filteredFilesWithCollectedDataRuleErrors[] = $analysedFile;
819+
}
793820
if (array_key_exists($analysedFile, $locallyIgnoredErrors)) {
794821
$filteredLocallyIgnoredErrors[$analysedFile] = $locallyIgnoredErrors[$analysedFile];
795822
}
@@ -881,6 +908,9 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ?
881908
}
882909
}
883910

911+
// in the order process() compares them in
912+
sort($filteredFilesWithCollectedDataRuleErrors);
913+
884914
// the freshly computed metadata, so a file that stopped being scanned is not counted
885915
$scannedFiles = is_array($meta['scannedFiles'] ?? null) ? $meta['scannedFiles'] : [];
886916
foreach (array_keys($notAnalysedFiles) as $notAnalysedFile) {
@@ -1036,6 +1066,9 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ?
10361066

10371067
$filesToAnalyse[] = $fileWithError;
10381068
}
1069+
foreach ($filteredFilesWithCollectedDataRuleErrors as $fileWithError) {
1070+
$filesToAnalyse[] = $fileWithError;
1071+
}
10391072
}
10401073

10411074
foreach ($packageSeededFiles as $packageSeededFile) {
@@ -1105,6 +1138,7 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ?
11051138
projectExtensionFiles: $data['projectExtensionFiles'],
11061139
currentFileHashes: $currentFileHashes,
11071140
collectedDataRuleResults: $collectedDataRuleResults,
1141+
filesWithCollectedDataRuleErrors: $filteredFilesWithCollectedDataRuleErrors,
11081142
);
11091143
}
11101144

@@ -1251,7 +1285,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache
12511285
$projectConfigArray = $this->getPathTransformer()->relativizeProjectConfig($projectConfigArray);
12521286
$meta['projectConfig'] = Neon::encode($projectConfigArray);
12531287
}
1254-
$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 {
1288+
$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 {
12551289
if ($onlyFiles) {
12561290
if ($output->isVeryVerbose()) {
12571291
$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
13371371
}
13381372

13391373
// in the order save() writes them, which is the order they are restored in
1374+
$collectedDataRuleResults = $finalizerResult->getCollectedDataRuleResults();
13401375
ksort($collectedDataRuleResults);
13411376

1377+
$filesWithCollectedDataRuleErrors = [];
1378+
foreach ($finalizerResult->getCollectorErrors() as $collectorError) {
1379+
$filesWithCollectedDataRuleErrors[$collectorError->getFilePath()] = true;
1380+
}
1381+
ksort($filesWithCollectedDataRuleErrors);
1382+
$filesWithCollectedDataRuleErrors = array_keys($filesWithCollectedDataRuleErrors);
1383+
13421384
$stubFiles = $this->getStubFiles();
13431385
if (
13441386
!$resultCache->isFullAnalysis()
@@ -1350,6 +1392,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache
13501392
&& $unmatchedLineIgnores === $resultCache->getUnmatchedLineIgnores()
13511393
&& $collectedDataByFile === $resultCache->getCollectedData()
13521394
&& $collectedDataRuleResults === $resultCache->getCollectedDataRuleResults()
1395+
&& $filesWithCollectedDataRuleErrors === $resultCache->getFilesWithCollectedDataRuleErrors()
13531396
&& $packageDependencies === $resultCache->getPackageDependencies()
13541397
&& $exportedNodes === $resultCache->getExportedNodes()
13551398
&& $cachedExportedNodes->getFiles() === $resultCache->getCachedExportedNodes()->getFiles()
@@ -1368,7 +1411,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache
13681411
$currentFileHashes[$file] = self::REANALYSE_FILE_HASH_PREFIX . $currentFileHashes[$file];
13691412
}
13701413

1371-
$this->save($resultCache->getLastFullAnalysisTime(), $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes, $projectExtensionFiles, $currentFileHashes, $meta, $stubFiles, $collectedDataRuleResults);
1414+
$this->save($resultCache->getLastFullAnalysisTime(), $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes, $projectExtensionFiles, $currentFileHashes, $meta, $stubFiles, $collectedDataRuleResults, $filesWithCollectedDataRuleErrors);
13721415

13731416
if ($output->isVeryVerbose()) {
13741417
$output->writeLineFormatted('Result cache is saved.');
@@ -1378,7 +1421,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache
13781421
};
13791422

13801423
if ($resultCache->isFullAnalysis()) {
1381-
return new ResultCacheProcessResult($analyserResult, function (array $collectedDataRuleResults) use ($save, $output, $analyserResult, $projectConfigArray, $doSave, $freshErrorsByFile, $freshLocallyIgnoredErrorsByFile, $freshCollectedDataByFile): bool {
1424+
return new ResultCacheProcessResult($analyserResult, function (FinalizerResult $finalizerResult) use ($save, $output, $analyserResult, $projectConfigArray, $doSave, $freshErrorsByFile, $freshLocallyIgnoredErrorsByFile, $freshCollectedDataByFile): bool {
13821425
if ($save === false) {
13831426
if ($output->isVeryVerbose()) {
13841427
$output->writeLineFormatted('Result cache was not saved because it was not requested.');
@@ -1392,7 +1435,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache
13921435
$projectExtensionFiles = $this->getProjectExtensionFiles($projectConfigArray, $analyserResult->getDependencies());
13931436
}
13941437

1395-
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);
1438+
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);
13961439
});
13971440
}
13981441

@@ -1409,7 +1452,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache
14091452
$linesToIgnore = $this->mergeLinesToIgnore($resultCache, $analyserResult->getLinesToIgnore());
14101453
$unmatchedLineIgnores = $this->mergeUnmatchedLineIgnores($resultCache, $analyserResult->getUnmatchedLineIgnores());
14111454

1412-
$saveCallback = function (array $collectedDataRuleResults) use ($save, $resultCache, $projectConfigArray, $doSave, $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes): bool {
1455+
$saveCallback = function (FinalizerResult $finalizerResult) use ($save, $resultCache, $projectConfigArray, $doSave, $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes): bool {
14131456
if ($save === false) {
14141457
return false;
14151458
}
@@ -1434,7 +1477,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache
14341477
$projectExtensionFiles[$file] = [$hash, true, $className];
14351478
}
14361479
}
1437-
return $doSave($errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes, $projectExtensionFiles, $collectedDataRuleResults);
1480+
return $doSave($errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes, $projectExtensionFiles, $finalizerResult);
14381481
};
14391482

14401483
$flatErrors = [];
@@ -1894,6 +1937,7 @@ private function mergeUnmatchedLineIgnores(ResultCache $resultCache, array $fres
18941937
* @param mixed[] $meta
18951938
* @param array<string, string> $stubFiles
18961939
* @param array<string, CollectedDataRuleResult> $collectedDataRuleResults
1940+
* @param list<string> $filesWithCollectedDataRuleErrors
18971941
*/
18981942
private function save(
18991943
int $lastFullAnalysisTime,
@@ -1913,6 +1957,7 @@ private function save(
19131957
array $meta,
19141958
array $stubFiles,
19151959
array $collectedDataRuleResults,
1960+
array $filesWithCollectedDataRuleErrors,
19161961
): void
19171962
{
19181963
$invertedDependencies = [];
@@ -1999,6 +2044,7 @@ private function save(
19992044
$packageDependencies = $transformer->relativizeFileKeyed($packageDependencies);
20002045
$projectExtensionFiles = $transformer->relativizeFileKeyed($projectExtensionFiles);
20012046
$collectedDataRuleResults = $transformer->relativizeCollectedDataRuleResults($collectedDataRuleResults);
2047+
$filesWithCollectedDataRuleErrors = array_map(static fn (string $path): string => $transformer->relativizePath($path), $filesWithCollectedDataRuleErrors);
20022048

20032049
$file = $this->cacheFilePath;
20042050

@@ -2036,6 +2082,7 @@ private function save(
20362082
$this->writeArrayFrame($handle, $file, 'unmatchedLineIgnores', $unmatchedLineIgnores);
20372083
$this->writeArrayFrame($handle, $file, 'collectedData', $collectedData);
20382084
$this->writeValueFrame($handle, $file, 'collectedDataRuleResults', $collectedDataRuleResults);
2085+
$this->writeValueFrame($handle, $file, 'filesWithCollectedDataRuleErrors', $filesWithCollectedDataRuleErrors);
20392086
// An older PHPStan reading this file absolutizes the dependencies before it gets to the
20402087
// cacheVersion check that makes it discard the file, and fails on a missing section.
20412088
$this->writeArrayFrame($handle, $file, 'dependencies', []);

‎src/Analyser/ResultCache/ResultCacheProcessResult.php‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,13 @@
44

55
use Closure;
66
use PHPStan\Analyser\AnalyserResult;
7-
use PHPStan\Analyser\CollectedDataRuleResult;
7+
use PHPStan\Analyser\FinalizerResult;
88

99
final class ResultCacheProcessResult
1010
{
1111

1212
/**
13-
* @param Closure(array<string, CollectedDataRuleResult>): bool $saveCallback
13+
* @param Closure(FinalizerResult): bool $saveCallback
1414
*/
1515
public function __construct(private AnalyserResult $analyserResult, private Closure $saveCallback)
1616
{
@@ -25,13 +25,11 @@ public function getAnalyserResult(): AnalyserResult
2525
* Writes the result cache, and says whether it was written.
2626
*
2727
* It is separate from ResultCacheManager::process() so that the caller can run the rules on
28-
* CollectedDataNode on the merged result first, and save their results too.
29-
*
30-
* @param array<string, CollectedDataRuleResult> $collectedDataRuleResults
28+
* CollectedDataNode on the merged result first, and save what they found too.
3129
*/
32-
public function save(array $collectedDataRuleResults): bool
30+
public function save(FinalizerResult $finalizerResult): bool
3331
{
34-
return ($this->saveCallback)($collectedDataRuleResults);
32+
return ($this->saveCallback)($finalizerResult);
3533
}
3634

3735
}

0 commit comments

Comments
 (0)