From a778146b6ffce3fbce47fe7e40143cd9a1e240e2 Mon Sep 17 00:00:00 2001 From: Sander Muller Date: Sun, 25 Oct 2026 17:53:49 +0100 Subject: [PATCH 1/3] Save the result cache after the rules on CollectedDataNode ran ResultCacheManager::process() now returns the save as a callback, and both callers call it after AnalyserResultFinalizer::finalize(). Nothing else changes: the same data is saved. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../ResultCache/ResultCacheManager.php | 33 +++++++++++-------- .../ResultCache/ResultCacheProcessResult.php | 16 +++++++-- src/Command/AnalyseApplication.php | 2 +- src/Command/FixerWorkerRunner.php | 7 ++-- 4 files changed, 37 insertions(+), 21 deletions(-) diff --git a/src/Analyser/ResultCache/ResultCacheManager.php b/src/Analyser/ResultCache/ResultCacheManager.php index 72c1e6a005a..5df893a6323 100644 --- a/src/Analyser/ResultCache/ResultCacheManager.php +++ b/src/Analyser/ResultCache/ResultCacheManager.php @@ -1356,20 +1356,22 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache }; if ($resultCache->isFullAnalysis()) { - $saved = false; - if ($save !== false) { + return new ResultCacheProcessResult($analyserResult, function () 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.'); + } + + return false; + } + $projectExtensionFiles = []; if ($analyserResult->getDependencies() !== null) { $projectExtensionFiles = $this->getProjectExtensionFiles($projectConfigArray, $analyserResult->getDependencies()); } - $saved = $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); - } else { - if ($output->isVeryVerbose()) { - $output->writeLineFormatted('Result cache was not saved because it was not requested.'); - } - } - return new ResultCacheProcessResult($analyserResult, $saved); + 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); + }); } $errorsByFile = $this->mergeErrors($resultCache, $freshErrorsByFile); @@ -1385,8 +1387,11 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache $linesToIgnore = $this->mergeLinesToIgnore($resultCache, $analyserResult->getLinesToIgnore()); $unmatchedLineIgnores = $this->mergeUnmatchedLineIgnores($resultCache, $analyserResult->getUnmatchedLineIgnores()); - $saved = false; - if ($save !== false) { + $saveCallback = function () use ($save, $resultCache, $projectConfigArray, $doSave, $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes): bool { + if ($save === false) { + return false; + } + $projectExtensionFiles = []; foreach ($resultCache->getProjectExtensionFiles() as $file => [$hash, $isAnalysed, $className]) { if ($isAnalysed) { @@ -1407,8 +1412,8 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache $projectExtensionFiles[$file] = [$hash, true, $className]; } } - $saved = $doSave($errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes, $projectExtensionFiles); - } + return $doSave($errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes, $projectExtensionFiles); + }; $flatErrors = []; foreach ($errorsByFile as $fileErrors) { @@ -1442,7 +1447,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache peakMemoryUsageBytes: $analyserResult->getPeakMemoryUsageBytes(), processedFiles: $analyserResult->getProcessedFiles(), workerCount: $analyserResult->getWorkerCount(), - ), $saved); + ), $saveCallback); } /** diff --git a/src/Analyser/ResultCache/ResultCacheProcessResult.php b/src/Analyser/ResultCache/ResultCacheProcessResult.php index 9789eefcbb3..512adc7b794 100644 --- a/src/Analyser/ResultCache/ResultCacheProcessResult.php +++ b/src/Analyser/ResultCache/ResultCacheProcessResult.php @@ -2,12 +2,16 @@ namespace PHPStan\Analyser\ResultCache; +use Closure; use PHPStan\Analyser\AnalyserResult; final class ResultCacheProcessResult { - public function __construct(private AnalyserResult $analyserResult, private bool $saved) + /** + * @param Closure(): bool $saveCallback + */ + public function __construct(private AnalyserResult $analyserResult, private Closure $saveCallback) { } @@ -16,9 +20,15 @@ public function getAnalyserResult(): AnalyserResult return $this->analyserResult; } - public function isSaved(): bool + /** + * 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. + */ + public function save(): bool { - return $this->saved; + return ($this->saveCallback)(); } } diff --git a/src/Command/AnalyseApplication.php b/src/Command/AnalyseApplication.php index f5868057130..82855bfc183 100644 --- a/src/Command/AnalyseApplication.php +++ b/src/Command/AnalyseApplication.php @@ -140,6 +140,7 @@ public function analyse( $onlyFiles, $debug, )->getAnalyserResult(); + $savedResultCache = $resultCacheResult->save(); $internalErrors = $analyserResult->getInternalErrors(); $errors = array_merge( $analyserResult->getErrors(), @@ -154,7 +155,6 @@ public function analyse( $fileSpecificErrors = $ignoredErrorHelperProcessedResult->getNotIgnoredErrors(); $notFileSpecificErrors = $ignoredErrorHelperProcessedResult->getOtherIgnoreMessages(); $collectedData = $analyserResult->getCollectedData(); - $savedResultCache = $resultCacheResult->isSaved(); } return new AnalysisResult( diff --git a/src/Command/FixerWorkerRunner.php b/src/Command/FixerWorkerRunner.php index cb162b221b4..e93beee2b21 100644 --- a/src/Command/FixerWorkerRunner.php +++ b/src/Command/FixerWorkerRunner.php @@ -190,14 +190,15 @@ function (array $errors, array $locallyIgnoredErrors, array $analysedFiles) use ]); }, )->then(function (AnalyserResult $intermediateAnalyserResult) use ($resultCacheManager, $resultCache, $errorOutput, $isOnlyFiles, $ignoredErrorHelperResult, $inceptionFiles, $out): void { - $analyserResult = $resultCacheManager->process( + $resultCacheResult = $resultCacheManager->process( $intermediateAnalyserResult, $resultCache, $errorOutput, false, true, - )->getAnalyserResult(); - $finalizerResult = $this->analyserResultFinalizer->finalize($analyserResult, $isOnlyFiles, false); + ); + $finalizerResult = $this->analyserResultFinalizer->finalize($resultCacheResult->getAnalyserResult(), $isOnlyFiles, false); + $resultCacheResult->save(); $internalErrors = []; foreach ($finalizerResult->getAnalyserResult()->getInternalErrors() as $internalError) { From c96f229f4ef1c76113fba705e48fe778f0ab8e3e Mon Sep 17 00:00:00 2001 From: Sander Muller Date: Sun, 25 Oct 2026 18:15:49 +0100 Subject: [PATCH 2/3] Reuse the errors of rules on CollectedDataNode from the result cache CollectedDataNode records which collectors a rule reads and whether it reads isOnlyFilesAnalysis(). The result cache stores the errors of each rule with a hash of that input. On the next run, a rule whose input is the same returns its stored errors and does not run again. A rule can declare what else it reads with the DependencyTracker methods, for example trackFileDependency() or trackDirectoryDependency(). These values are stored with its errors, and the rule runs again when one of them changes. The ignore extensions and the local ignores still run on every error, also on reused errors. This is behind the reuseCollectedDataRuleErrors feature toggle, which is on in bleeding edge only. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/e2e-tests.yml | 51 ++++ .gitignore | 2 + conf/bleedingEdge.neon | 1 + conf/config.neon | 1 + conf/parametersSchema.neon | 1 + .../.gitignore | 1 + .../composer.json | 5 + .../composer.lock | 18 ++ .../extension/ViewCollector.php | 37 +++ .../extension/ViewRule.php | 69 +++++ .../ignored-views.txt | 0 .../lib/ExtraViews.php | 10 + .../phpstan.neon | 23 ++ .../src/Controller.php | 23 ++ .../src/functions.php | 8 + .../views/home.txt | 1 + .../phpstan.neon | 8 + .../src/Caller.php | 13 + .../src/Other.php | 13 + .../src/functions.php | 8 + src/Analyser/AnalyserResultFinalizer.php | 174 ++++++++++- src/Analyser/CollectedDataRuleResult.php | 74 +++++ src/Analyser/DependencyTracker.php | 3 + src/Analyser/Error.php | 2 +- src/Analyser/FinalizerResult.php | 19 ++ src/Analyser/ResultCache/ResultCache.php | 11 + .../ResultCache/ResultCacheManager.php | 35 ++- .../ResultCachePathTransformer.php | 31 ++ .../ResultCache/ResultCacheProcessResult.php | 11 +- src/Analyser/ValueDependencyCollector.php | 33 +- src/Command/AnalyseApplication.php | 15 +- src/Command/FixerWorkerRunner.php | 4 +- src/Node/CollectedDataNode.php | 10 +- src/Node/CollectedDataNodeReads.php | 42 +++ src/Testing/RuleTestCase.php | 2 + tests/PHPStan/Analyser/AnalyserTest.php | 2 + .../CollectedDataReuse/EarlyReturnRule.php | 50 ++++ .../CollectedDataReuse/FirstCollector.php | 26 ++ .../CollectedDataReuse/OnlyFilesRule.php | 37 +++ .../CollectedDataReuse/SecondCollector.php | 26 ++ .../CollectedDataReuse/ThrowingRule.php | 32 ++ .../CollectedDataReuse/TrackingRule.php | 61 ++++ .../UnregisteredValueExtension.php | 25 ++ .../UnregisteredValueRule.php | 41 +++ .../Analyser/CollectedDataRuleReuseTest.php | 281 ++++++++++++++++++ .../ResultCachePathTransformerTest.php | 48 +++ 46 files changed, 1352 insertions(+), 36 deletions(-) create mode 100644 e2e/result-cache-collected-data-rules-tracking/.gitignore create mode 100644 e2e/result-cache-collected-data-rules-tracking/composer.json create mode 100644 e2e/result-cache-collected-data-rules-tracking/composer.lock create mode 100644 e2e/result-cache-collected-data-rules-tracking/extension/ViewCollector.php create mode 100644 e2e/result-cache-collected-data-rules-tracking/extension/ViewRule.php create mode 100644 e2e/result-cache-collected-data-rules-tracking/ignored-views.txt create mode 100644 e2e/result-cache-collected-data-rules-tracking/lib/ExtraViews.php create mode 100644 e2e/result-cache-collected-data-rules-tracking/phpstan.neon create mode 100644 e2e/result-cache-collected-data-rules-tracking/src/Controller.php create mode 100644 e2e/result-cache-collected-data-rules-tracking/src/functions.php create mode 100644 e2e/result-cache-collected-data-rules-tracking/views/home.txt create mode 100644 e2e/result-cache-collected-data-rules/phpstan.neon create mode 100644 e2e/result-cache-collected-data-rules/src/Caller.php create mode 100644 e2e/result-cache-collected-data-rules/src/Other.php create mode 100644 e2e/result-cache-collected-data-rules/src/functions.php create mode 100644 src/Analyser/CollectedDataRuleResult.php create mode 100644 src/Node/CollectedDataNodeReads.php create mode 100644 tests/PHPStan/Analyser/CollectedDataReuse/EarlyReturnRule.php create mode 100644 tests/PHPStan/Analyser/CollectedDataReuse/FirstCollector.php create mode 100644 tests/PHPStan/Analyser/CollectedDataReuse/OnlyFilesRule.php create mode 100644 tests/PHPStan/Analyser/CollectedDataReuse/SecondCollector.php create mode 100644 tests/PHPStan/Analyser/CollectedDataReuse/ThrowingRule.php create mode 100644 tests/PHPStan/Analyser/CollectedDataReuse/TrackingRule.php create mode 100644 tests/PHPStan/Analyser/CollectedDataReuse/UnregisteredValueExtension.php create mode 100644 tests/PHPStan/Analyser/CollectedDataReuse/UnregisteredValueRule.php create mode 100644 tests/PHPStan/Analyser/CollectedDataRuleReuseTest.php diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 007a8d510a5..e0285c95e76 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -215,6 +215,57 @@ jobs: OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan -vvv") echo "$OUTPUT" ../bashunit assert contains 'Result cache restored.' "$OUTPUT" + - script: | + cd e2e/result-cache-collected-data-rules + OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + ../bashunit assert contains 'Caller.php:10:Call to function ResultCacheCollectedDataRules\pure() on a separate line has no effect.' "$OUTPUT" + # Nothing changed, so every rule on CollectedDataNode reuses its errors from the result cache. + OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + ../bashunit assert contains 'Caller.php:10:Call to function ResultCacheCollectedDataRules\pure() on a separate line has no effect.' "$OUTPUT" + echo "$OUTPUT" | grep -E 'Result cache reused the errors of ([0-9]+) of \1 rules on collected data\.' + ../bashunit assert contains 'Result cache was not rewritten because it is unchanged.' "$OUTPUT" + # src/Other.php is analysed again, but what it collects stays the same. + echo '// changed' >> src/Other.php + OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + ../bashunit assert contains 'Result cache restored. 1 file will be reanalysed.' "$OUTPUT" + ../bashunit assert contains 'Caller.php:10:Call to function ResultCacheCollectedDataRules\pure() on a separate line has no effect.' "$OUTPUT" + echo "$OUTPUT" | grep -E 'Result cache reused the errors of ([0-9]+) of \1 rules on collected data\.' + # Removing the call changes what src/Caller.php collects, so the rule runs again and the error goes away. + sed -i 's/^\t\tpure();$//' src/Caller.php + OUTPUT=$(../bashunit assert exit_code "0" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + ../bashunit assert not_contains 'on a separate line has no effect' "$OUTPUT" + - script: | + cd e2e/result-cache-collected-data-rules-tracking + composer install + OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + ../bashunit assert contains 'Controller.php:15:View about not found.' "$OUTPUT" + OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + echo "$OUTPUT" | grep -E 'Result cache reused the errors of ([0-9]+) of \1 rules on collected data\.' + # The rule tracks the views directory, so a new view runs it again. + printf 'about\n' > views/about.txt + OUTPUT=$(../bashunit assert exit_code "0" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + ../bashunit assert not_contains 'View about not found.' "$OUTPUT" + rm views/about.txt + OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + ../bashunit assert contains 'Controller.php:15:View about not found.' "$OUTPUT" + # The rule tracks the file of ignored views too. + printf 'about\n' > ignored-views.txt + OUTPUT=$(../bashunit assert exit_code "0" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + ../bashunit assert not_contains 'View about not found.' "$OUTPUT" + # And the class of extra views, which is not analysed. + sed -i "s/'contact'/'nothing'/" lib/ExtraViews.php + OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + ../bashunit assert contains 'Controller.php:20:View contact not found.' "$OUTPUT" - script: | cd e2e/bug-14514 composer install diff --git a/.gitignore b/.gitignore index 37f02026c37..9848a14eacf 100644 --- a/.gitignore +++ b/.gitignore @@ -22,6 +22,8 @@ e2e/bashunit /e2e/result-cache-moved-tmpdir/tmp-a /e2e/result-cache-moved-tmpdir/tmp-b /e2e/result-cache-stale-objects/tmp +/e2e/result-cache-collected-data-rules/tmp +/e2e/result-cache-collected-data-rules-tracking/tmp /e2e/result-cache-truncated/tmp /e2e/result-cache-worker-count/tmp e2e/restart-manager/probe.log diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index a14610a1aa4..7098877fb44 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -33,3 +33,4 @@ parameters: backedEnumFromParameterType: true unusedVariable: true unusedParameters: true + reuseCollectedDataRuleErrors: true diff --git a/conf/config.neon b/conf/config.neon index f623528506a..0eb6c0c764b 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -65,6 +65,7 @@ parameters: backedEnumFromParameterType: false unusedVariable: false unusedParameters: false + reuseCollectedDataRuleErrors: false fileExtensions: - php checkAdvancedIsset: false diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon index 154054ccb1f..62dab9d9c3b 100644 --- a/conf/parametersSchema.neon +++ b/conf/parametersSchema.neon @@ -62,6 +62,7 @@ parametersSchema: backedEnumFromParameterType: bool() unusedVariable: bool() unusedParameters: bool() + reuseCollectedDataRuleErrors: bool() ]) fileExtensions: listOf(string()) checkAdvancedIsset: bool() diff --git a/e2e/result-cache-collected-data-rules-tracking/.gitignore b/e2e/result-cache-collected-data-rules-tracking/.gitignore new file mode 100644 index 00000000000..61ead86667c --- /dev/null +++ b/e2e/result-cache-collected-data-rules-tracking/.gitignore @@ -0,0 +1 @@ +/vendor diff --git a/e2e/result-cache-collected-data-rules-tracking/composer.json b/e2e/result-cache-collected-data-rules-tracking/composer.json new file mode 100644 index 00000000000..e23fc929433 --- /dev/null +++ b/e2e/result-cache-collected-data-rules-tracking/composer.json @@ -0,0 +1,5 @@ +{ + "autoload-dev": { + "classmap": ["extension/"] + } +} diff --git a/e2e/result-cache-collected-data-rules-tracking/composer.lock b/e2e/result-cache-collected-data-rules-tracking/composer.lock new file mode 100644 index 00000000000..ba8d41762cc --- /dev/null +++ b/e2e/result-cache-collected-data-rules-tracking/composer.lock @@ -0,0 +1,18 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state", + "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", + "This file is @generated automatically" + ], + "content-hash": "d751713988987e9331980363e24189ce", + "packages": [], + "packages-dev": [], + "aliases": [], + "minimum-stability": "stable", + "stability-flags": {}, + "prefer-stable": false, + "prefer-lowest": false, + "platform": {}, + "platform-dev": {}, + "plugin-api-version": "2.9.0" +} diff --git a/e2e/result-cache-collected-data-rules-tracking/extension/ViewCollector.php b/e2e/result-cache-collected-data-rules-tracking/extension/ViewCollector.php new file mode 100644 index 00000000000..ec3bb3bdad4 --- /dev/null +++ b/e2e/result-cache-collected-data-rules-tracking/extension/ViewCollector.php @@ -0,0 +1,37 @@ + + */ +final class ViewCollector implements Collector +{ + + public function getNodeType(): string + { + return FuncCall::class; + } + + public function processNode(Node $node, Scope $scope) + { + if (!$node->name instanceof Name || $node->name->toLowerString() !== 'view') { + return null; + } + + $args = $node->getArgs(); + if (!isset($args[0]) || !$args[0]->value instanceof String_) { + return null; + } + + return [$args[0]->value->value, $node->getStartLine()]; + } + +} diff --git a/e2e/result-cache-collected-data-rules-tracking/extension/ViewRule.php b/e2e/result-cache-collected-data-rules-tracking/extension/ViewRule.php new file mode 100644 index 00000000000..aee351de744 --- /dev/null +++ b/e2e/result-cache-collected-data-rules-tracking/extension/ViewRule.php @@ -0,0 +1,69 @@ + + */ +final class ViewRule implements Rule +{ + + public function __construct( + private ReflectionProvider $reflectionProvider, + private string $viewsDirectory, + private string $ignoredViewsFile, + ) + { + } + + public function getNodeType(): string + { + return CollectedDataNode::class; + } + + /** + * @param Scope&DependencyTracker $scope + */ + public function processNode(Node $node, Scope $scope): array + { + $scope->trackDirectoryDependency($this->viewsDirectory, '*.txt'); + $scope->trackFileDependency($this->ignoredViewsFile); + $scope->trackClassDependency(ExtraViews::class); + + $ignoredViews = file($this->ignoredViewsFile, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES); + $extraViews = $this->reflectionProvider->getClass(ExtraViews::class)->getNativeReflection()->getConstants(); + $errors = []; + foreach ($node->get(ViewCollector::class) as $file => $views) { + foreach ($views as [$view, $line]) { + if (is_file($this->viewsDirectory . '/' . $view . '.txt') || in_array($view, $ignoredViews, true) || in_array($view, $extraViews, true)) { + continue; + } + + $errors[] = RuleErrorBuilder::message(sprintf('View %s not found.', $view)) + ->file($file) + ->line($line) + ->identifier('view.notFound') + ->build(); + } + } + + return $errors; + } + +} diff --git a/e2e/result-cache-collected-data-rules-tracking/ignored-views.txt b/e2e/result-cache-collected-data-rules-tracking/ignored-views.txt new file mode 100644 index 00000000000..e69de29bb2d diff --git a/e2e/result-cache-collected-data-rules-tracking/lib/ExtraViews.php b/e2e/result-cache-collected-data-rules-tracking/lib/ExtraViews.php new file mode 100644 index 00000000000..479c602de02 --- /dev/null +++ b/e2e/result-cache-collected-data-rules-tracking/lib/ExtraViews.php @@ -0,0 +1,10 @@ + $ignoreErrorExtensions */ @@ -34,11 +43,19 @@ public function __construct( private LocalIgnoresProcessor $localIgnoresProcessor, #[AutowiredParameter] private bool $reportUnmatchedIgnoredErrors, + private ValueDependencyCollector $valueDependencyCollector, + #[AutowiredParameter(ref: '%featureToggles.reuseCollectedDataRuleErrors%')] + private bool $reuseCollectedDataRuleErrors, ) { } - public function finalize(AnalyserResult $analyserResult, bool $onlyFiles, bool $debug): FinalizerResult + /** + * @param array $previousCollectedDataRuleResults the rule results of the + * previous run, from the result cache - a rule whose input did not change reuses its errors. + * Only with the reuseCollectedDataRuleErrors feature toggle, otherwise every rule runs. + */ + public function finalize(AnalyserResult $analyserResult, bool $onlyFiles, bool $debug, array $previousCollectedDataRuleResults = []): FinalizerResult { if (count($analyserResult->getCollectedData()) === 0) { return $this->addUnmatchedIgnoredErrors($this->mergeFilteredPhpErrors($analyserResult), [], []); @@ -50,15 +67,56 @@ public function finalize(AnalyserResult $analyserResult, bool $onlyFiles, bool $ } $nodeType = CollectedDataNode::class; - $node = new CollectedDataNode($analyserResult->getCollectedData(), $onlyFiles); + $collectedData = $analyserResult->getCollectedData(); + $node = new CollectedDataNode($collectedData, $onlyFiles); + + // Hashed once per collector type and run, in the same order however the files were merged. + $collectorHashes = []; + $hashCollector = static function (string $collectorType) use ($collectedData, &$collectorHashes): string { + if (!array_key_exists($collectorType, $collectorHashes)) { + $data = []; + foreach ($collectedData as $filePath => $collectedDataPerCollector) { + if (!isset($collectedDataPerCollector[$collectorType])) { + continue; + } - $file = 'N/A'; + $data[$filePath] = $collectedDataPerCollector[$collectorType]; + } + ksort($data); + $collectorHashes[$collectorType] = md5(serialize($data)); + } + + return $collectorHashes[$collectorType]; + }; + + $file = self::NO_FILE; $scope = $this->scopeFactory->create(ScopeContext::create($file)); $tempCollectorErrors = []; $internalErrors = $analyserResult->getInternalErrors(); + $collectedDataRuleResults = []; + $reusedRulesCount = 0; + $ruleCountsByClass = []; foreach ($this->ruleRegistry->getRules($nodeType) as $rule) { + $ruleClass = get_class($rule); + $ruleCountsByClass[$ruleClass] = ($ruleCountsByClass[$ruleClass] ?? 0) + 1; + $ruleKey = sprintf('%s#%d', $ruleClass, $ruleCountsByClass[$ruleClass]); + + $previousResult = $this->reuseCollectedDataRuleErrors ? $previousCollectedDataRuleResults[$ruleKey] ?? null : null; + if ($previousResult !== null && $this->canReuse($previousResult, $onlyFiles, $hashCollector)) { + $collectedDataRuleResults[$ruleKey] = $previousResult; + $reusedRulesCount++; + $this->addRuleErrors($previousResult->getErrors(), $node, $scope, $tempCollectorErrors); + continue; + } + + $reads = new CollectedDataNodeReads(); + $ruleNode = new CollectedDataNode($collectedData, $onlyFiles, $reads); + if ($this->reuseCollectedDataRuleErrors) { + $this->valueDependencyCollector->startCollectedDataRule(); + } try { - $ruleErrors = $rule->processNode($node, $scope); + $ruleErrors = $rule->processNode($ruleNode, $scope); + $ruleValueDependencies = $this->valueDependencyCollector->getCollectedDataRuleValues(); } catch (AnalysedCodeException $e) { $tempCollectorErrors[] = (new Error($e->getMessage(), $file, $node->getStartLine(), $e, tip: $e->getTip())) ->withIdentifier('phpstan.internal') @@ -96,21 +154,20 @@ public function finalize(AnalyserResult $analyserResult, bool $onlyFiles, bool $ shouldReportBug: true, ); continue; + } finally { + $this->valueDependencyCollector->finishCollectedDataRule(); } + $errors = []; foreach ($ruleErrors as $ruleError) { - $error = $this->ruleErrorTransformer->transform($ruleError, $scope, [], $node); - - if ($error->canBeIgnored()) { - foreach ($this->ignoreErrorExtensions->getAll() as $ignoreErrorExtension) { - if ($ignoreErrorExtension->shouldIgnore($error, $node, $scope)) { - continue 2; - } - } - } + $errors[] = $this->ruleErrorTransformer->transform($ruleError, $scope, [], $ruleNode); + } - $tempCollectorErrors[] = $error; + if ($this->reuseCollectedDataRuleErrors) { + $collectedDataRuleResults[$ruleKey] = $this->createRuleResult($reads, $onlyFiles, $hashCollector, $errors, $ruleValueDependencies); } + + $this->addRuleErrors($errors, $node, $scope, $tempCollectorErrors); } $errors = $analyserResult->getUnorderedErrors(); @@ -158,7 +215,87 @@ public function finalize(AnalyserResult $analyserResult, bool $onlyFiles, bool $ peakMemoryUsageBytes: $analyserResult->getPeakMemoryUsageBytes(), processedFiles: $analyserResult->getProcessedFiles(), workerCount: $analyserResult->getWorkerCount(), - ), $collectorErrors, $locallyIgnoredCollectorErrors); + ), $collectorErrors, $locallyIgnoredCollectorErrors, $collectedDataRuleResults, $reusedRulesCount); + } + + /** + * @param Closure(string): string $hashCollector + * @param list $errors + * @param array $valueDependencies + */ + private function createRuleResult(CollectedDataNodeReads $reads, bool $onlyFiles, Closure $hashCollector, array $errors, array $valueDependencies): CollectedDataRuleResult + { + $readCollectorHashes = []; + foreach ($reads->getCollectorTypes() as $collectorType) { + $readCollectorHashes[$collectorType] = $hashCollector($collectorType); + } + + $storedValueDependencies = []; + foreach ($valueDependencies as [$extensionClass, $key, $value]) { + $extension = $this->valueDependencyCollector->getExtension($extensionClass); + if ($extension === null) { + continue; + } + + $storedValueDependencies[] = [$extensionClass, $extension->keyToResultCache($key), $value]; + } + + return new CollectedDataRuleResult( + $readCollectorHashes, + $reads->isOnlyFilesRead() ? $onlyFiles : null, + $errors, + $storedValueDependencies, + ); + } + + /** + * @param Closure(string): string $hashCollector + */ + private function canReuse(CollectedDataRuleResult $previousResult, bool $onlyFiles, Closure $hashCollector): bool + { + $previousOnlyFiles = $previousResult->getOnlyFiles(); + if ($previousOnlyFiles !== null && $previousOnlyFiles !== $onlyFiles) { + return false; + } + + foreach ($previousResult->getCollectorHashes() as $collectorType => $hash) { + if ($hashCollector($collectorType) !== $hash) { + return false; + } + } + + // the values the rule declared through DependencyTracker - see ValueDependencyCollector + foreach ($previousResult->getValueDependencies() as [$extensionClass, $storedKey, $value]) { + $extension = $this->valueDependencyCollector->getExtension($extensionClass); + if ($extension === null) { + return false; + } + + if ($extension->getValue($extension->keyFromResultCache($storedKey)) !== $value) { + return false; + } + } + + return true; + } + + /** + * @param list $errors + * @param list $tempCollectorErrors + */ + private function addRuleErrors(array $errors, CollectedDataNode $node, Scope $scope, array &$tempCollectorErrors): void + { + foreach ($errors as $error) { + if ($error->canBeIgnored()) { + foreach ($this->ignoreErrorExtensions->getAll() as $ignoreErrorExtension) { + if ($ignoreErrorExtension->shouldIgnore($error, $node, $scope)) { + continue 2; + } + } + } + + $tempCollectorErrors[] = $error; + } } private function mergeFilteredPhpErrors(AnalyserResult $analyserResult): AnalyserResult @@ -187,15 +324,18 @@ private function mergeFilteredPhpErrors(AnalyserResult $analyserResult): Analyse /** * @param list $collectorErrors * @param list $locallyIgnoredCollectorErrors + * @param array $collectedDataRuleResults */ private function addUnmatchedIgnoredErrors( AnalyserResult $analyserResult, array $collectorErrors, array $locallyIgnoredCollectorErrors, + array $collectedDataRuleResults = [], + int $reusedRulesCount = 0, ): FinalizerResult { if (!$this->reportUnmatchedIgnoredErrors) { - return new FinalizerResult($analyserResult, $collectorErrors, $locallyIgnoredCollectorErrors); + return new FinalizerResult($analyserResult, $collectorErrors, $locallyIgnoredCollectorErrors, $collectedDataRuleResults, $reusedRulesCount); } $errors = $analyserResult->getUnorderedErrors(); @@ -252,6 +392,8 @@ private function addUnmatchedIgnoredErrors( ), $collectorErrors, $locallyIgnoredCollectorErrors, + $collectedDataRuleResults, + $reusedRulesCount, ); } diff --git a/src/Analyser/CollectedDataRuleResult.php b/src/Analyser/CollectedDataRuleResult.php new file mode 100644 index 00000000000..afe87c62968 --- /dev/null +++ b/src/Analyser/CollectedDataRuleResult.php @@ -0,0 +1,74 @@ + $collectorHashes collector type => hash of its data + * @param list $errors before the ignore extensions and the local ignores + * @param list $valueDependencies [extension class, key as the extension stores it, value] + */ + public function __construct( + private array $collectorHashes, + private ?bool $onlyFiles, + private array $errors, + private array $valueDependencies = [], + ) + { + } + + /** + * @return array + */ + public function getCollectorHashes(): array + { + return $this->collectorHashes; + } + + /** + * Null when the rule did not ask. + */ + public function getOnlyFiles(): ?bool + { + return $this->onlyFiles; + } + + /** + * @return list + */ + public function getErrors(): array + { + return $this->errors; + } + + /** + * @return list + */ + public function getValueDependencies(): array + { + return $this->valueDependencies; + } + + /** + * @param callable(string): string $transform + */ + public function transformPaths(callable $transform): self + { + $errors = []; + foreach ($this->errors as $error) { + $errors[] = $error->transformPaths($transform); + } + + return new self($this->collectorHashes, $this->onlyFiles, $errors, $this->valueDependencies); + } + +} diff --git a/src/Analyser/DependencyTracker.php b/src/Analyser/DependencyTracker.php index b48cbf2823b..5947fc3514e 100644 --- a/src/Analyser/DependencyTracker.php +++ b/src/Analyser/DependencyTracker.php @@ -23,6 +23,9 @@ * * It tracks what the analysis of the current file depends on besides the analysed code, so that * the result cache re-analyses the file when that changes - see ResultCacheValueExtension. + * In a rule for CollectedDataNode, it tracks what the errors of the rule depend on: with the + * reuseCollectedDataRuleErrors feature toggle, the result cache reuses the errors of such a rule + * while its collected data and the tracked values stay the same. * * Extensions that get no Scope because they describe a class - class reflection extensions - use * DeclarationDependencyTracker instead. diff --git a/src/Analyser/Error.php b/src/Analyser/Error.php index 4cfa6c7723c..9d116266e4f 100644 --- a/src/Analyser/Error.php +++ b/src/Analyser/Error.php @@ -470,7 +470,7 @@ private function transformTraitContexts(callable $transformPath): array { $result = []; foreach ($this->traitContexts as $contextFilePath => $contextFile) { - $result[$transformPath($contextFilePath)] = $contextFile; + $result[$transformPath($contextFilePath)] = $transformPath($contextFile); } return $result; diff --git a/src/Analyser/FinalizerResult.php b/src/Analyser/FinalizerResult.php index c196b25d99f..36416a4c39d 100644 --- a/src/Analyser/FinalizerResult.php +++ b/src/Analyser/FinalizerResult.php @@ -8,11 +8,14 @@ final class FinalizerResult /** * @param list $collectorErrors * @param list $locallyIgnoredCollectorErrors + * @param array $collectedDataRuleResults */ public function __construct( private AnalyserResult $analyserResult, private array $collectorErrors, private array $locallyIgnoredCollectorErrors, + private array $collectedDataRuleResults = [], + private int $reusedRulesCount = 0, ) { } @@ -46,4 +49,20 @@ public function getLocallyIgnoredCollectorErrors(): array return $this->locallyIgnoredCollectorErrors; } + /** + * @return array + */ + public function getCollectedDataRuleResults(): array + { + return $this->collectedDataRuleResults; + } + + /** + * How many of the rules on CollectedDataNode reused their errors from the result cache. + */ + public function getReusedRulesCount(): int + { + return $this->reusedRulesCount; + } + } diff --git a/src/Analyser/ResultCache/ResultCache.php b/src/Analyser/ResultCache/ResultCache.php index 99889f0a63d..cde318c1d01 100644 --- a/src/Analyser/ResultCache/ResultCache.php +++ b/src/Analyser/ResultCache/ResultCache.php @@ -2,6 +2,7 @@ namespace PHPStan\Analyser\ResultCache; +use PHPStan\Analyser\CollectedDataRuleResult; use PHPStan\Analyser\Error; use PHPStan\Analyser\FileAnalyserResult; use PHPStan\Collectors\CollectedData; @@ -29,6 +30,7 @@ final class ResultCache * @param array> $exportedNodes the decoded ones - see $cachedExportedNodes for the rest * @param array $projectExtensionFiles * @param array $currentFileHashes + * @param array $collectedDataRuleResults */ public function __construct( private array $filesToAnalyse, @@ -49,6 +51,7 @@ public function __construct( private CachedExportedNodes $cachedExportedNodes, private array $projectExtensionFiles, private array $currentFileHashes, + private array $collectedDataRuleResults = [], ) { } @@ -192,4 +195,12 @@ public function getCurrentFileHashes(): array return $this->currentFileHashes; } + /** + * @return array + */ + public function getCollectedDataRuleResults(): array + { + return $this->collectedDataRuleResults; + } + } diff --git a/src/Analyser/ResultCache/ResultCacheManager.php b/src/Analyser/ResultCache/ResultCacheManager.php index 5df893a6323..0296a633cc5 100644 --- a/src/Analyser/ResultCache/ResultCacheManager.php +++ b/src/Analyser/ResultCache/ResultCacheManager.php @@ -4,6 +4,7 @@ use Nette\Neon\Neon; use PHPStan\Analyser\AnalyserResult; +use PHPStan\Analyser\CollectedDataRuleResult; use PHPStan\Analyser\Error; use PHPStan\Analyser\FileAnalyserResult; use PHPStan\Analyser\ValueDependencyCollector; @@ -105,7 +106,7 @@ final class ResultCacheManager */ private const EXTENSIONS_NOT_INVALIDATING_CACHE = ['xdebug', 'blackfire', 'phpstan_turbo']; - private const CACHE_VERSION = 'v22-valueDependencies'; + private const CACHE_VERSION = 'v23-collectedDataRuleResults'; /** * The recorded hash of a dependency that does not exist. A rule can depend on a path rather than on @@ -705,6 +706,16 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ? $locallyIgnoredErrors = $data['locallyIgnoredErrorsCallback'](); $collectedData = $data['collectedDataCallback'](); $exportedNodes = $data['exportedNodesCallback'](); + $collectedDataRuleResults = $data['collectedDataRuleResults'] ?? []; + if (!is_array($collectedDataRuleResults)) { + throw new RuntimeException('The cached results of the rules on collected data are not an array.'); + } + foreach ($collectedDataRuleResults as $collectedDataRuleResult) { + if (!$collectedDataRuleResult instanceof CollectedDataRuleResult) { + throw new RuntimeException('A cached result of a rule on collected data is not a CollectedDataRuleResult.'); + } + } + $collectedDataRuleResults = $transformer->absolutizeCollectedDataRuleResults($collectedDataRuleResults); } catch (Throwable $e) { @unlink($cacheFilePath); @@ -1087,6 +1098,7 @@ public function restore(array $allAnalysedFiles, bool $debug, bool $onlyFiles, ? cachedExportedNodes: $cachedExportedNodes->only($keptCachedExportedNodes), projectExtensionFiles: $data['projectExtensionFiles'], currentFileHashes: $currentFileHashes, + collectedDataRuleResults: $collectedDataRuleResults, ); } @@ -1233,7 +1245,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) 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, array $collectedDataRuleResults) 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.'); @@ -1318,6 +1330,9 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache unset($errorsByFile[$file]); } + // in the order save() writes them, which is the order they are restored in + ksort($collectedDataRuleResults); + $stubFiles = $this->getStubFiles(); if ( !$resultCache->isFullAnalysis() @@ -1328,6 +1343,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache && $linesToIgnore === $resultCache->getLinesToIgnore() && $unmatchedLineIgnores === $resultCache->getUnmatchedLineIgnores() && $collectedDataByFile === $resultCache->getCollectedData() + && $collectedDataRuleResults === $resultCache->getCollectedDataRuleResults() && $packageDependencies === $resultCache->getPackageDependencies() && $exportedNodes === $resultCache->getExportedNodes() && $cachedExportedNodes->getFiles() === $resultCache->getCachedExportedNodes()->getFiles() @@ -1346,7 +1362,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); + $this->save($resultCache->getLastFullAnalysisTime(), $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes, $projectExtensionFiles, $currentFileHashes, $meta, $stubFiles, $collectedDataRuleResults); if ($output->isVeryVerbose()) { $output->writeLineFormatted('Result cache is saved.'); @@ -1356,7 +1372,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache }; if ($resultCache->isFullAnalysis()) { - return new ResultCacheProcessResult($analyserResult, function () use ($save, $output, $analyserResult, $projectConfigArray, $doSave, $freshErrorsByFile, $freshLocallyIgnoredErrorsByFile, $freshCollectedDataByFile): bool { + return new ResultCacheProcessResult($analyserResult, function (array $collectedDataRuleResults) 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.'); @@ -1370,7 +1386,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); + 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); }); } @@ -1387,7 +1403,7 @@ public function process(AnalyserResult $analyserResult, ResultCache $resultCache $linesToIgnore = $this->mergeLinesToIgnore($resultCache, $analyserResult->getLinesToIgnore()); $unmatchedLineIgnores = $this->mergeUnmatchedLineIgnores($resultCache, $analyserResult->getUnmatchedLineIgnores()); - $saveCallback = function () use ($save, $resultCache, $projectConfigArray, $doSave, $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes): bool { + $saveCallback = function (array $collectedDataRuleResults) use ($save, $resultCache, $projectConfigArray, $doSave, $errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes): bool { if ($save === false) { return false; } @@ -1412,7 +1428,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); + return $doSave($errorsByFile, $locallyIgnoredErrorsByFile, $linesToIgnore, $unmatchedLineIgnores, $collectedDataByFile, $dependencies, $usedTraitDependencies, $valueDependencies, $packageDependencies, $exportedNodes, $cachedExportedNodes, $projectExtensionFiles, $collectedDataRuleResults); }; $flatErrors = []; @@ -1871,6 +1887,7 @@ private function mergeUnmatchedLineIgnores(ResultCache $resultCache, array $fres * @param array $currentFileHashes * @param mixed[] $meta * @param array $stubFiles + * @param array $collectedDataRuleResults */ private function save( int $lastFullAnalysisTime, @@ -1889,6 +1906,7 @@ private function save( array $currentFileHashes, array $meta, array $stubFiles, + array $collectedDataRuleResults, ): void { $invertedDependencies = []; @@ -1951,6 +1969,7 @@ private function save( } ksort($exportedNodes); + ksort($collectedDataRuleResults); // The only point where the StubFilesExtensions may run: the analysis is over, bootstrapFiles // have been executed, so the extensions can rely on them. restore() reads these hashes back @@ -1973,6 +1992,7 @@ private function save( unset($invertedDependencies); $packageDependencies = $transformer->relativizeFileKeyed($packageDependencies); $projectExtensionFiles = $transformer->relativizeFileKeyed($projectExtensionFiles); + $collectedDataRuleResults = $transformer->relativizeCollectedDataRuleResults($collectedDataRuleResults); $file = $this->cacheFilePath; @@ -2009,6 +2029,7 @@ private function save( $this->writeArrayFrame($handle, $file, 'linesToIgnore', $linesToIgnore); $this->writeArrayFrame($handle, $file, 'unmatchedLineIgnores', $unmatchedLineIgnores); $this->writeArrayFrame($handle, $file, 'collectedData', $collectedData); + $this->writeValueFrame($handle, $file, 'collectedDataRuleResults', $collectedDataRuleResults); // 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/ResultCachePathTransformer.php b/src/Analyser/ResultCache/ResultCachePathTransformer.php index 4413298b015..78ae735942b 100644 --- a/src/Analyser/ResultCache/ResultCachePathTransformer.php +++ b/src/Analyser/ResultCache/ResultCachePathTransformer.php @@ -2,6 +2,8 @@ namespace PHPStan\Analyser\ResultCache; +use PHPStan\Analyser\AnalyserResultFinalizer; +use PHPStan\Analyser\CollectedDataRuleResult; use PHPStan\Analyser\Error; use PHPStan\Collectors\CollectedData; use PHPStan\Collectors\CollectorWithPaths; @@ -114,6 +116,35 @@ public function absolutizeErrors(array $errorsByFile): array return $result; } + /** + * @param array $collectedDataRuleResults + * @return array + */ + public function relativizeCollectedDataRuleResults(array $collectedDataRuleResults): array + { + $result = []; + foreach ($collectedDataRuleResults as $ruleKey => $ruleResult) { + $result[$ruleKey] = $ruleResult->transformPaths(fn (string $path): string => $path === AnalyserResultFinalizer::NO_FILE ? $path : $this->relativizeCompoundKey($path)); + } + + return $result; + } + + /** + * @param array $collectedDataRuleResults + * @return array + */ + public function absolutizeCollectedDataRuleResults(array $collectedDataRuleResults): array + { + $result = []; + foreach ($collectedDataRuleResults as $ruleKey => $ruleResult) { + // an error without a file keeps no file, absolutizing would prefix it with the directory + $result[$ruleKey] = $ruleResult->transformPaths(fn (string $path): string => $path === AnalyserResultFinalizer::NO_FILE ? $path : $this->absolutizeCompoundKey($path)); + } + + return $result; + } + /** * Collected data is opaque to the cache, so only the collector that produced it can rewrite the * paths inside it - it does so by implementing CollectorWithPaths. Collectors that carry no paths diff --git a/src/Analyser/ResultCache/ResultCacheProcessResult.php b/src/Analyser/ResultCache/ResultCacheProcessResult.php index 512adc7b794..ee7b5621d9f 100644 --- a/src/Analyser/ResultCache/ResultCacheProcessResult.php +++ b/src/Analyser/ResultCache/ResultCacheProcessResult.php @@ -4,12 +4,13 @@ use Closure; use PHPStan\Analyser\AnalyserResult; +use PHPStan\Analyser\CollectedDataRuleResult; final class ResultCacheProcessResult { /** - * @param Closure(): bool $saveCallback + * @param Closure(array): bool $saveCallback */ public function __construct(private AnalyserResult $analyserResult, private Closure $saveCallback) { @@ -24,11 +25,13 @@ 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. + * CollectedDataNode on the merged result first, and save their results too. + * + * @param array $collectedDataRuleResults */ - public function save(): bool + public function save(array $collectedDataRuleResults): bool { - return ($this->saveCallback)(); + return ($this->saveCallback)($collectedDataRuleResults); } } diff --git a/src/Analyser/ValueDependencyCollector.php b/src/Analyser/ValueDependencyCollector.php index a056ccf6da2..a65b6acbe93 100644 --- a/src/Analyser/ValueDependencyCollector.php +++ b/src/Analyser/ValueDependencyCollector.php @@ -39,8 +39,10 @@ * by the files analysed after it, so it is what the scope's file declares that depends on the * value: a change of it re-analyses the scope's file and every file depending on it. * - * Outside the analysis of a file - in the rules for CollectedDataNode, which run again on every - * run anyway - nothing is collected. + * A rule for CollectedDataNode runs outside the analysis of a file. What it declares is collected + * for the rule itself, between startCollectedDataRule() and finishCollectedDataRule(), and stored + * with its errors: the next run reuses them only while every value is still the same. Outside of + * both, nothing is collected. * * DeclarationDependencyTracker records what a class declares depends on, through extensions that * describe a class rather than analyse code. Their result is remembered for the rest of the process @@ -59,6 +61,9 @@ final class ValueDependencyCollector private ?string $analysedFile = null; + /** @var array|null id => [extension class, key, value], while a rule for CollectedDataNode runs */ + private ?array $collectedDataRuleValues = null; + /** @var array id => [extension class, key, value] */ private array $values = []; @@ -100,6 +105,12 @@ public function startFile(string $analysedFile): void */ public function record(string $extensionClass, string $key, Scope $scope, bool $insideWalk): void { + if ($this->collectedDataRuleValues !== null) { + $id = self::getId($extensionClass, $key); + $this->collectedDataRuleValues[$id] ??= [$extensionClass, $key, $this->getRegisteredExtension($extensionClass)->getValue($key)]; + return; + } + if ($this->analysedFile === null) { return; } @@ -154,6 +165,24 @@ public function recordClass(string $className, Scope $scope, bool $insideWalk): $this->record(ClassResultCacheValueExtension::class, ClassResultCacheValueExtension::createKey($className), $scope, $insideWalk); } + public function startCollectedDataRule(): void + { + $this->collectedDataRuleValues = []; + } + + /** + * @return array id => [extension class, key, value] + */ + public function getCollectedDataRuleValues(): array + { + return $this->collectedDataRuleValues ?? []; + } + + public function finishCollectedDataRule(): void + { + $this->collectedDataRuleValues = null; + } + public function getFileKey(string $file): string { return $this->fileHelper->normalizePath($file); diff --git a/src/Command/AnalyseApplication.php b/src/Command/AnalyseApplication.php index 82855bfc183..f74503ac2dd 100644 --- a/src/Command/AnalyseApplication.php +++ b/src/Command/AnalyseApplication.php @@ -135,12 +135,21 @@ public function analyse( $processedFiles = $intermediateAnalyserResult->getProcessedFiles(); $resultCacheResult = $resultCacheManager->process($intermediateAnalyserResult, $resultCache, $errorOutput, $onlyFiles, true); - $analyserResult = $this->analyserResultFinalizer->finalize( + $finalizerResult = $this->analyserResultFinalizer->finalize( $this->switchTmpFileInAnalyserResult($resultCacheResult->getAnalyserResult(), $insteadOfFile, $tmpFile), $onlyFiles, $debug, - )->getAnalyserResult(); - $savedResultCache = $resultCacheResult->save(); + $resultCache->getCollectedDataRuleResults(), + ); + if ($finalizerResult->getReusedRulesCount() > 0 && $errorOutput->isVeryVerbose()) { + $errorOutput->writeLineFormatted(sprintf( + 'Result cache reused the errors of %d of %d rules on collected data.', + $finalizerResult->getReusedRulesCount(), + count($finalizerResult->getCollectedDataRuleResults()), + )); + } + $savedResultCache = $resultCacheResult->save($finalizerResult->getCollectedDataRuleResults()); + $analyserResult = $finalizerResult->getAnalyserResult(); $internalErrors = $analyserResult->getInternalErrors(); $errors = array_merge( $analyserResult->getErrors(), diff --git a/src/Command/FixerWorkerRunner.php b/src/Command/FixerWorkerRunner.php index e93beee2b21..71f134c11bc 100644 --- a/src/Command/FixerWorkerRunner.php +++ b/src/Command/FixerWorkerRunner.php @@ -197,8 +197,8 @@ function (array $errors, array $locallyIgnoredErrors, array $analysedFiles) use false, true, ); - $finalizerResult = $this->analyserResultFinalizer->finalize($resultCacheResult->getAnalyserResult(), $isOnlyFiles, false); - $resultCacheResult->save(); + $finalizerResult = $this->analyserResultFinalizer->finalize($resultCacheResult->getAnalyserResult(), $isOnlyFiles, false, $resultCache->getCollectedDataRuleResults()); + $resultCacheResult->save($finalizerResult->getCollectedDataRuleResults()); $internalErrors = []; foreach ($finalizerResult->getAnalyserResult()->getInternalErrors() as $internalError) { diff --git a/src/Node/CollectedDataNode.php b/src/Node/CollectedDataNode.php index 14a12815fe1..db5f89ff32e 100644 --- a/src/Node/CollectedDataNode.php +++ b/src/Node/CollectedDataNode.php @@ -18,7 +18,7 @@ final class CollectedDataNode extends NodeAbstract implements VirtualNode /** * @param CollectorData $collectedData */ - public function __construct(private array $collectedData, private bool $onlyFiles) + public function __construct(private array $collectedData, private bool $onlyFiles, private ?CollectedDataNodeReads $reads = null) { parent::__construct([]); } @@ -31,6 +31,10 @@ public function __construct(private array $collectedData, private bool $onlyFile */ public function get(string $collectorType): array { + if ($this->reads !== null) { + $this->reads->addCollectorType($collectorType); + } + $result = []; foreach ($this->collectedData as $filePath => $collectedDataPerCollector) { if (!isset($collectedDataPerCollector[$collectorType])) { @@ -52,6 +56,10 @@ public function get(string $collectorType): array */ public function isOnlyFilesAnalysis(): bool { + if ($this->reads !== null) { + $this->reads->addOnlyFiles(); + } + return $this->onlyFiles; } diff --git a/src/Node/CollectedDataNodeReads.php b/src/Node/CollectedDataNodeReads.php new file mode 100644 index 00000000000..4b35e6874b7 --- /dev/null +++ b/src/Node/CollectedDataNodeReads.php @@ -0,0 +1,42 @@ + */ + private array $collectorTypes = []; + + private bool $onlyFilesRead = false; + + public function addCollectorType(string $collectorType): void + { + $this->collectorTypes[$collectorType] = true; + } + + public function addOnlyFiles(): void + { + $this->onlyFilesRead = true; + } + + /** + * @return list + */ + public function getCollectorTypes(): array + { + return array_keys($this->collectorTypes); + } + + public function isOnlyFilesRead(): bool + { + return $this->onlyFilesRead; + } + +} diff --git a/src/Testing/RuleTestCase.php b/src/Testing/RuleTestCase.php index af028257400..99f8770e99c 100644 --- a/src/Testing/RuleTestCase.php +++ b/src/Testing/RuleTestCase.php @@ -304,6 +304,8 @@ private function gatherAnalyserErrorsWithDelayedErrors(array $files): array self::createScopeFactory($reflectionProvider, self::getContainer()->getService('typeSpecifier')), new LocalIgnoresProcessor(), true, + self::getContainer()->getByType(ValueDependencyCollector::class), + false, ); return [ diff --git a/tests/PHPStan/Analyser/AnalyserTest.php b/tests/PHPStan/Analyser/AnalyserTest.php index 060c45d6af5..8c68cedc877 100644 --- a/tests/PHPStan/Analyser/AnalyserTest.php +++ b/tests/PHPStan/Analyser/AnalyserTest.php @@ -812,6 +812,8 @@ private function runAnalyser( ), new LocalIgnoresProcessor(), $reportUnmatchedIgnoredErrors, + self::getContainer()->getByType(ValueDependencyCollector::class), + false, ); $analyserResult = $finalizer->finalize($analyserResult, $onlyFiles, false)->getAnalyserResult(); diff --git a/tests/PHPStan/Analyser/CollectedDataReuse/EarlyReturnRule.php b/tests/PHPStan/Analyser/CollectedDataReuse/EarlyReturnRule.php new file mode 100644 index 00000000000..974bd6c4072 --- /dev/null +++ b/tests/PHPStan/Analyser/CollectedDataReuse/EarlyReturnRule.php @@ -0,0 +1,50 @@ + + */ +final class EarlyReturnRule implements Rule +{ + + public int $runs = 0; + + public function getNodeType(): string + { + return CollectedDataNode::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $this->runs++; + + $first = $node->get(FirstCollector::class); + if ($first === []) { + return []; + } + + $errors = []; + foreach ($node->get(SecondCollector::class) as $file => $values) { + foreach ($values as $value) { + $errors[] = RuleErrorBuilder::message('Second collected ' . $value . '.') + ->file($file) + ->line(1) + ->identifier('tests.second') + ->build(); + } + } + + return $errors; + } + +} diff --git a/tests/PHPStan/Analyser/CollectedDataReuse/FirstCollector.php b/tests/PHPStan/Analyser/CollectedDataReuse/FirstCollector.php new file mode 100644 index 00000000000..21dcae3ccd9 --- /dev/null +++ b/tests/PHPStan/Analyser/CollectedDataReuse/FirstCollector.php @@ -0,0 +1,26 @@ + + */ +final class FirstCollector implements Collector +{ + + public function getNodeType(): string + { + return Node::class; + } + + public function processNode(Node $node, Scope&DependencyTracker $scope) + { + return null; + } + +} diff --git a/tests/PHPStan/Analyser/CollectedDataReuse/OnlyFilesRule.php b/tests/PHPStan/Analyser/CollectedDataReuse/OnlyFilesRule.php new file mode 100644 index 00000000000..f5e1805c064 --- /dev/null +++ b/tests/PHPStan/Analyser/CollectedDataReuse/OnlyFilesRule.php @@ -0,0 +1,37 @@ + + */ +final class OnlyFilesRule implements Rule +{ + + public int $runs = 0; + + public function getNodeType(): string + { + return CollectedDataNode::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $this->runs++; + + if ($node->isOnlyFilesAnalysis()) { + return []; + } + + return [ + RuleErrorBuilder::message('Full analysis.')->file(__FILE__)->line(2)->identifier('tests.full')->build(), + ]; + } + +} diff --git a/tests/PHPStan/Analyser/CollectedDataReuse/SecondCollector.php b/tests/PHPStan/Analyser/CollectedDataReuse/SecondCollector.php new file mode 100644 index 00000000000..574e0dff4e7 --- /dev/null +++ b/tests/PHPStan/Analyser/CollectedDataReuse/SecondCollector.php @@ -0,0 +1,26 @@ + + */ +final class SecondCollector implements Collector +{ + + public function getNodeType(): string + { + return Node::class; + } + + public function processNode(Node $node, Scope&DependencyTracker $scope) + { + return null; + } + +} diff --git a/tests/PHPStan/Analyser/CollectedDataReuse/ThrowingRule.php b/tests/PHPStan/Analyser/CollectedDataReuse/ThrowingRule.php new file mode 100644 index 00000000000..bfe08ed4778 --- /dev/null +++ b/tests/PHPStan/Analyser/CollectedDataReuse/ThrowingRule.php @@ -0,0 +1,32 @@ + + */ +final class ThrowingRule implements Rule +{ + + public int $runs = 0; + + public function getNodeType(): string + { + return CollectedDataNode::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $this->runs++; + $node->get(FirstCollector::class); + + throw new FunctionNotFoundException('notThere'); + } + +} diff --git a/tests/PHPStan/Analyser/CollectedDataReuse/TrackingRule.php b/tests/PHPStan/Analyser/CollectedDataReuse/TrackingRule.php new file mode 100644 index 00000000000..6b52f2b2fd0 --- /dev/null +++ b/tests/PHPStan/Analyser/CollectedDataReuse/TrackingRule.php @@ -0,0 +1,61 @@ + + */ +final class TrackingRule implements Rule +{ + + public int $runs = 0; + + public function __construct(private string $file, private string $directory, private string $valueFile) + { + } + + public function getNodeType(): string + { + return CollectedDataNode::class; + } + + /** + * @param Scope&DependencyTracker $scope + */ + public function processNode(Node $node, Scope $scope): array + { + $this->runs++; + + $scope->trackFileDependency($this->file); + $scope->trackDirectoryDependency($this->directory, '*.txt'); + $scope->trackValueDependency(FileResultCacheValueExtension::class, $this->valueFile); + + $contents = file_get_contents($this->file); + $files = glob($this->directory . '/*.txt'); + $value = file_get_contents($this->valueFile); + + return [ + RuleErrorBuilder::message(sprintf('File says %s, directory has %d files, value is %s.', $contents === false ? 'nothing' : $contents, $files === false ? 0 : count($files), $value === false ? 'nothing' : $value)) + ->file(__FILE__) + ->line(1) + ->identifier('tests.tracking') + ->build(), + ]; + } + +} diff --git a/tests/PHPStan/Analyser/CollectedDataReuse/UnregisteredValueExtension.php b/tests/PHPStan/Analyser/CollectedDataReuse/UnregisteredValueExtension.php new file mode 100644 index 00000000000..07680c5c8cf --- /dev/null +++ b/tests/PHPStan/Analyser/CollectedDataReuse/UnregisteredValueExtension.php @@ -0,0 +1,25 @@ + + */ +final class UnregisteredValueRule implements Rule +{ + + public function getNodeType(): string + { + return CollectedDataNode::class; + } + + /** + * @param Scope&DependencyTracker $scope + */ + public function processNode(Node $node, Scope $scope): array + { + $scope->trackValueDependency(UnregisteredValueExtension::class, 'key'); + + return [ + RuleErrorBuilder::message('Unregistered value.') + ->file(__FILE__) + ->line(1) + ->identifier('tests.unregistered') + ->build(), + ]; + } + +} diff --git a/tests/PHPStan/Analyser/CollectedDataRuleReuseTest.php b/tests/PHPStan/Analyser/CollectedDataRuleReuseTest.php new file mode 100644 index 00000000000..ec971096fa0 --- /dev/null +++ b/tests/PHPStan/Analyser/CollectedDataRuleReuseTest.php @@ -0,0 +1,281 @@ +finalize([$rule], [ + self::B => [SecondCollector::class => ['b1']], + ], false, []); + $this->assertSame(1, $rule->runs); + $this->assertSame([], $this->getMessages($result)); + + // SecondCollector changed, but the rule did not ask for it + $result = $this->finalize([$rule], [ + self::B => [SecondCollector::class => ['b2']], + ], false, $result->getCollectedDataRuleResults()); + $this->assertSame(1, $rule->runs); + $this->assertSame(1, $result->getReusedRulesCount()); + + // FirstCollector changed, so the rule runs and now asks for both + $result = $this->finalize([$rule], [ + self::A => [FirstCollector::class => ['a1']], + self::B => [SecondCollector::class => ['b2']], + ], false, $result->getCollectedDataRuleResults()); + $this->assertSame(2, $rule->runs); + $this->assertSame(['Second collected b2.'], $this->getMessages($result)); + + // SecondCollector changed again, and this time the rule asked for it + $result = $this->finalize([$rule], [ + self::A => [FirstCollector::class => ['a1']], + self::B => [SecondCollector::class => ['b3']], + ], false, $result->getCollectedDataRuleResults()); + $this->assertSame(3, $rule->runs); + $this->assertSame(['Second collected b3.'], $this->getMessages($result)); + + // nothing changed: the errors come from the previous run + $result = $this->finalize([$rule], [ + self::A => [FirstCollector::class => ['a1']], + self::B => [SecondCollector::class => ['b3']], + ], false, $result->getCollectedDataRuleResults()); + $this->assertSame(3, $rule->runs); + $this->assertSame(1, $result->getReusedRulesCount()); + $this->assertSame(['Second collected b3.'], $this->getMessages($result)); + } + + public function testOrderOfFilesDoesNotMatter(): void + { + $rule = new EarlyReturnRule(); + $result = $this->finalize([$rule], [ + self::A => [FirstCollector::class => ['a1']], + self::B => [SecondCollector::class => ['b1']], + self::C => [SecondCollector::class => ['c1']], + ], false, []); + + // a merged result cache lists the files in another order than a full analysis does + $result = $this->finalize([$rule], [ + self::C => [SecondCollector::class => ['c1']], + self::A => [FirstCollector::class => ['a1']], + self::B => [SecondCollector::class => ['b1']], + ], false, $result->getCollectedDataRuleResults()); + $this->assertSame(1, $rule->runs); + $this->assertSame(1, $result->getReusedRulesCount()); + } + + public function testOnlyFilesAnalysis(): void + { + $onlyFilesRule = new OnlyFilesRule(); + $earlyReturnRule = new EarlyReturnRule(); + $collectedData = [ + self::A => [FirstCollector::class => ['a1']], + ]; + + $result = $this->finalize([$onlyFilesRule, $earlyReturnRule], $collectedData, false, []); + $this->assertSame(['Full analysis.'], $this->getMessages($result)); + + // only the rule that asked for isOnlyFilesAnalysis() runs again + $result = $this->finalize([$onlyFilesRule, $earlyReturnRule], $collectedData, true, $result->getCollectedDataRuleResults()); + $this->assertSame(2, $onlyFilesRule->runs); + $this->assertSame(1, $earlyReturnRule->runs); + $this->assertSame([], $this->getMessages($result)); + } + + public function testRuleThatThrowsIsNotReused(): void + { + $rule = new ThrowingRule(); + $collectedData = [ + self::A => [FirstCollector::class => ['a1']], + ]; + + $result = $this->finalize([$rule], $collectedData, false, []); + $this->assertSame([], $result->getCollectedDataRuleResults()); + + $result = $this->finalize([$rule], $collectedData, false, $result->getCollectedDataRuleResults()); + $this->assertSame(2, $rule->runs); + $this->assertSame(['Function notThere not found while trying to analyse it - discovering symbols is probably not configured properly.'], $this->getMessages($result)); + } + + public function testTwoRulesOfTheSameClassKeepTheirOwnResults(): void + { + $first = new OnlyFilesRule(); + $second = new EarlyReturnRule(); + $third = new EarlyReturnRule(); + $collectedData = [ + self::A => [FirstCollector::class => ['a1']], + self::B => [SecondCollector::class => ['b1']], + ]; + + $result = $this->finalize([$first, $second, $third], $collectedData, false, []); + $this->assertCount(3, $result->getCollectedDataRuleResults()); + + $result = $this->finalize([$first, $second, $third], $collectedData, false, $result->getCollectedDataRuleResults()); + $this->assertSame(3, $result->getReusedRulesCount()); + $this->assertSame(['Full analysis.', 'Second collected b1.', 'Second collected b1.'], $this->getMessages($result)); + } + + public function testRuleRunsAgainWhenATrackedValueChanged(): void + { + $directory = sys_get_temp_dir() . '/phpstan-collected-data-reuse-' . uniqid(); + mkdir($directory); + $file = $directory . '/value.php'; + file_put_contents($file, 'one'); + file_put_contents($directory . '/a.txt', ''); + $valueFile = $directory . '/value.data'; + file_put_contents($valueFile, 'a'); + + $rule = new TrackingRule($file, $directory, $valueFile); + $collectedData = [ + self::A => [FirstCollector::class => ['a1']], + ]; + + $result = $this->finalize([$rule], $collectedData, false, []); + $this->assertSame(1, $rule->runs); + $this->assertSame(['File says one, directory has 1 files, value is a.'], $this->getMessages($result)); + + // nothing changed + $result = $this->finalize([$rule], $collectedData, false, $result->getCollectedDataRuleResults()); + $this->assertSame(1, $rule->runs); + $this->assertSame(1, $result->getReusedRulesCount()); + $this->assertSame(['File says one, directory has 1 files, value is a.'], $this->getMessages($result)); + + // the tracked file changed - in size too, FileContentHasher memoizes by mtime and size + file_put_contents($file, 'three'); + clearstatcache(); + $result = $this->finalize([$rule], $collectedData, false, $result->getCollectedDataRuleResults()); + $this->assertSame(2, $rule->runs); + $this->assertSame(['File says three, directory has 1 files, value is a.'], $this->getMessages($result)); + + // a file appeared in the tracked directory + file_put_contents($directory . '/b.txt', ''); + $result = $this->finalize([$rule], $collectedData, false, $result->getCollectedDataRuleResults()); + $this->assertSame(3, $rule->runs); + $this->assertSame(['File says three, directory has 2 files, value is a.'], $this->getMessages($result)); + + // a file that does not match the pattern does not count + file_put_contents($directory . '/c.md', ''); + $result = $this->finalize([$rule], $collectedData, false, $result->getCollectedDataRuleResults()); + $this->assertSame(3, $rule->runs); + $this->assertSame(1, $result->getReusedRulesCount()); + + // the value declared through trackValueDependency() changed + file_put_contents($valueFile, 'bb'); + clearstatcache(); + $result = $this->finalize([$rule], $collectedData, false, $result->getCollectedDataRuleResults()); + $this->assertSame(4, $rule->runs); + $this->assertSame(['File says three, directory has 2 files, value is bb.'], $this->getMessages($result)); + + unlink($valueFile); + unlink($directory . '/c.md'); + unlink($directory . '/b.txt'); + unlink($directory . '/a.txt'); + unlink($file); + rmdir($directory); + } + + public function testEveryRuleRunsWithoutTheFeatureToggle(): void + { + $rule = new EarlyReturnRule(); + $collectedData = [ + self::A => [FirstCollector::class => ['a1']], + self::B => [SecondCollector::class => ['b1']], + ]; + + $result = $this->finalize([$rule], $collectedData, false, [], false); + $this->assertSame([], $result->getCollectedDataRuleResults()); + + $previousResults = $this->finalize([$rule], $collectedData, false, [])->getCollectedDataRuleResults(); + $this->assertCount(1, $previousResults); + $this->assertSame(2, $rule->runs); + + $result = $this->finalize([$rule], $collectedData, false, $previousResults, false); + $this->assertSame(3, $rule->runs); + $this->assertSame(0, $result->getReusedRulesCount()); + $this->assertSame(['Second collected b1.'], $this->getMessages($result)); + + // without the toggle, the declared values are not asked for, as before + $result = $this->finalize([new UnregisteredValueRule()], $collectedData, false, [], false); + $this->assertSame(['Unregistered value.'], $this->getMessages($result)); + } + + /** + * @param list> $rules + * @param CollectorData $collectedData + * @param array $previousResults + */ + private function finalize(array $rules, array $collectedData, bool $onlyFiles, array $previousResults, bool $reuse = true): FinalizerResult + { + $finalizer = new AnalyserResultFinalizer( + new DirectRegistry($rules), + self::getContainer()->getExtensionsCollection(IgnoreErrorExtension::class), + self::getContainer()->getByType(RuleErrorTransformer::class), + self::createScopeFactory(self::createReflectionProvider(), self::getContainer()->getService('typeSpecifier')), + new LocalIgnoresProcessor(), + false, + self::getContainer()->getByType(ValueDependencyCollector::class), + $reuse, + ); + + return $finalizer->finalize(new AnalyserResult( + unorderedErrors: [], + filteredPhpErrors: [], + allPhpErrors: [], + locallyIgnoredErrors: [], + linesToIgnore: [], + unmatchedLineIgnores: [], + internalErrors: [], + collectedData: $collectedData, + dependencies: [], + usedTraitDependencies: [], + valueDependencies: null, + packageDependencies: [], + exportedNodes: [], + reachedInternalErrorsCountLimit: false, + peakMemoryUsageBytes: 0, + processedFiles: [], + ), $onlyFiles, false, $previousResults); + } + + /** + * @return list + */ + private function getMessages(FinalizerResult $result): array + { + return array_map(static fn (Error $error): string => $error->getMessage(), $result->getCollectorErrors()); + } + +} diff --git a/tests/PHPStan/Analyser/ResultCache/ResultCachePathTransformerTest.php b/tests/PHPStan/Analyser/ResultCache/ResultCachePathTransformerTest.php index 4d396844674..551bd449f08 100644 --- a/tests/PHPStan/Analyser/ResultCache/ResultCachePathTransformerTest.php +++ b/tests/PHPStan/Analyser/ResultCache/ResultCachePathTransformerTest.php @@ -2,6 +2,9 @@ namespace PHPStan\Analyser\ResultCache; +use PHPStan\Analyser\AnalyserResultFinalizer; +use PHPStan\Analyser\CollectedDataRuleResult; +use PHPStan\Analyser\Error; use PHPUnit\Framework\TestCase; use function str_replace; use const DIRECTORY_SEPARATOR; @@ -103,6 +106,51 @@ public function testNormalizeMetaIsTheRoundTripFixedPoint(): void $this->assertSame($canonical, $transformer->absolutizeMeta($transformer->relativizeMeta($raw))); } + /** + * The stored errors of rules on CollectedDataNode keep trait contexts, and both the key and the + * "path (in context of class X)" value hold a path. + */ + public function testCollectedDataRuleResultsTraitContextsAreRelativized(): void + { + $transformer = new ResultCachePathTransformer(self::raw('/opt/project/vendor/phpstan/phpstan')); + $traitFile = self::canonical('/opt/project/src/SomeTrait.php'); + $error = new Error( + 'Message', + $traitFile . ' (in context of class Foo)', + 10, + filePath: $traitFile, + traitContexts: [$traitFile => $traitFile . ' (in context of class Foo)'], + ); + $results = ['SomeRule#0' => new CollectedDataRuleResult([], null, [$error])]; + + $relativized = $transformer->relativizeCollectedDataRuleResults($results); + $this->assertSame( + ['../../../src/SomeTrait.php' => '../../../src/SomeTrait.php (in context of class Foo)'], + $relativized['SomeRule#0']->getErrors()[0]->getTraitContexts(), + ); + + $absolutized = $transformer->absolutizeCollectedDataRuleResults($relativized); + $this->assertSame( + [$traitFile => $traitFile . ' (in context of class Foo)'], + $absolutized['SomeRule#0']->getErrors()[0]->getTraitContexts(), + ); + } + + /** + * A rule on CollectedDataNode can report an error without a file. It has no path to relativize, + * and absolutizing it must not prefix it with a directory. + */ + public function testCollectedDataRuleResultsKeepAnErrorWithoutFile(): void + { + $transformer = new ResultCachePathTransformer(self::raw('/opt/project/vendor/phpstan/phpstan')); + $error = new Error('Message', AnalyserResultFinalizer::NO_FILE, null); + $results = ['SomeRule#0' => new CollectedDataRuleResult([], null, [$error])]; + + $roundTrip = $transformer->absolutizeCollectedDataRuleResults($transformer->relativizeCollectedDataRuleResults($results)); + $this->assertSame(AnalyserResultFinalizer::NO_FILE, $roundTrip['SomeRule#0']->getErrors()[0]->getFile()); + $this->assertSame(AnalyserResultFinalizer::NO_FILE, $roundTrip['SomeRule#0']->getErrors()[0]->getFilePath()); + } + /** * The path as its source spells it: '/'-separated, with a drive letter on Windows. */ From c7b14694c5c2da681266f2c735050b951085d893 Mon Sep 17 00:00:00 2001 From: Sander Muller Date: Tue, 27 Oct 2026 09:32:27 +0100 Subject: [PATCH 3/3] Rerun a rule on CollectedDataNode when what a class declares changed A class reflection extension records with DeclarationDependencyTracker what a class declares depends on. A rule on CollectedDataNode can read such a class, but it is not known which classes it read. An extension also describes a class only once per process, so a second rule that reads the class records nothing. So a rule now gets the values of every class seen in the process. A changed value runs the rule again. A value of a class that the rule did not read causes an extra run only. Every rule now has the same values, so the finalizer asks for each current value once per run, not once per rule. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/e2e-tests.yml | 6 ++ .../extension/MoreViewsExtension.php | 43 +++++++++ .../extension/ViewMethodReflection.php | 90 +++++++++++++++++++ .../extension/ViewRule.php | 6 +- .../lib/MoreViews.php | 11 +++ .../more-views.txt | 1 + .../phpstan.neon | 6 ++ .../src/Controller.php | 5 ++ src/Analyser/AnalyserResultFinalizer.php | 10 ++- src/Analyser/ValueDependencyCollector.php | 16 +++- .../CollectedDataReuse/DescribedClassRule.php | 48 ++++++++++ .../Analyser/CollectedDataRuleReuseTest.php | 49 +++++++++- 12 files changed, 283 insertions(+), 8 deletions(-) create mode 100644 e2e/result-cache-collected-data-rules-tracking/extension/MoreViewsExtension.php create mode 100644 e2e/result-cache-collected-data-rules-tracking/extension/ViewMethodReflection.php create mode 100644 e2e/result-cache-collected-data-rules-tracking/lib/MoreViews.php create mode 100644 e2e/result-cache-collected-data-rules-tracking/more-views.txt create mode 100644 tests/PHPStan/Analyser/CollectedDataReuse/DescribedClassRule.php diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index e0285c95e76..5ee2556e697 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -266,6 +266,12 @@ jobs: OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw") echo "$OUTPUT" ../bashunit assert contains 'Controller.php:20:View contact not found.' "$OUTPUT" + # And a method that a class reflection extension describes from a file that is not + # analysed, tracked through DeclarationDependencyTracker. No analysed file changes. + printf '' > more-views.txt + OUTPUT=$(../bashunit assert exit_code "1" "../../bin/phpstan analyse -vv --error-format raw") + echo "$OUTPUT" + ../bashunit assert contains 'Controller.php:25:View faq not found.' "$OUTPUT" - script: | cd e2e/bug-14514 composer install diff --git a/e2e/result-cache-collected-data-rules-tracking/extension/MoreViewsExtension.php b/e2e/result-cache-collected-data-rules-tracking/extension/MoreViewsExtension.php new file mode 100644 index 00000000000..50ed386fa88 --- /dev/null +++ b/e2e/result-cache-collected-data-rules-tracking/extension/MoreViewsExtension.php @@ -0,0 +1,43 @@ +getName() !== MoreViews::class) { + return false; + } + + $this->declarationDependencyTracker->trackFileDependency($classReflection, $this->moreViewsFile); + + return in_array($methodName, file($this->moreViewsFile, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES), true); + } + + public function getMethod(ClassReflection $classReflection, string $methodName): MethodReflection + { + return new ViewMethodReflection($classReflection, $methodName, new StringType()); + } + +} diff --git a/e2e/result-cache-collected-data-rules-tracking/extension/ViewMethodReflection.php b/e2e/result-cache-collected-data-rules-tracking/extension/ViewMethodReflection.php new file mode 100644 index 00000000000..435df1cfb57 --- /dev/null +++ b/e2e/result-cache-collected-data-rules-tracking/extension/ViewMethodReflection.php @@ -0,0 +1,90 @@ +declaringClass; + } + + public function isStatic(): bool + { + return false; + } + + public function isPrivate(): bool + { + return false; + } + + public function isPublic(): bool + { + return true; + } + + public function getDocComment(): ?string + { + return null; + } + + public function getName(): string + { + return $this->name; + } + + public function getPrototype(): ClassMemberReflection + { + return $this; + } + + public function getVariants(): array + { + return [new FunctionVariant(TemplateTypeMap::createEmpty(), null, [], false, $this->returnType)]; + } + + public function isDeprecated(): TrinaryLogic + { + return TrinaryLogic::createNo(); + } + + public function getDeprecatedDescription(): ?string + { + return null; + } + + public function isFinal(): TrinaryLogic + { + return TrinaryLogic::createNo(); + } + + public function isInternal(): TrinaryLogic + { + return TrinaryLogic::createNo(); + } + + public function getThrowType(): ?Type + { + return null; + } + + public function hasSideEffects(): TrinaryLogic + { + return TrinaryLogic::createNo(); + } + +} diff --git a/e2e/result-cache-collected-data-rules-tracking/extension/ViewRule.php b/e2e/result-cache-collected-data-rules-tracking/extension/ViewRule.php index aee351de744..47edd41db22 100644 --- a/e2e/result-cache-collected-data-rules-tracking/extension/ViewRule.php +++ b/e2e/result-cache-collected-data-rules-tracking/extension/ViewRule.php @@ -17,7 +17,8 @@ /** * Reports a view without a file in the views directory, unless the view is listed in a file of * ignored views or is a constant of ExtraViews. None of them is analysed, so the rule declares - * all three. + * all three. A view can also be a method of MoreViews, which MoreViewsExtension describes and + * declares itself. * * @implements Rule */ @@ -48,10 +49,11 @@ public function processNode(Node $node, Scope $scope): array $ignoredViews = file($this->ignoredViewsFile, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES); $extraViews = $this->reflectionProvider->getClass(ExtraViews::class)->getNativeReflection()->getConstants(); + $moreViews = $this->reflectionProvider->getClass(MoreViews::class); $errors = []; foreach ($node->get(ViewCollector::class) as $file => $views) { foreach ($views as [$view, $line]) { - if (is_file($this->viewsDirectory . '/' . $view . '.txt') || in_array($view, $ignoredViews, true) || in_array($view, $extraViews, true)) { + if (is_file($this->viewsDirectory . '/' . $view . '.txt') || in_array($view, $ignoredViews, true) || in_array($view, $extraViews, true) || $moreViews->hasMethod($view)) { continue; } diff --git a/e2e/result-cache-collected-data-rules-tracking/lib/MoreViews.php b/e2e/result-cache-collected-data-rules-tracking/lib/MoreViews.php new file mode 100644 index 00000000000..94631e41e18 --- /dev/null +++ b/e2e/result-cache-collected-data-rules-tracking/lib/MoreViews.php @@ -0,0 +1,11 @@ +ruleRegistry->getRules($nodeType) as $rule) { $ruleClass = get_class($rule); $ruleCountsByClass[$ruleClass] = ($ruleCountsByClass[$ruleClass] ?? 0) + 1; $ruleKey = sprintf('%s#%d', $ruleClass, $ruleCountsByClass[$ruleClass]); $previousResult = $this->reuseCollectedDataRuleErrors ? $previousCollectedDataRuleResults[$ruleKey] ?? null : null; - if ($previousResult !== null && $this->canReuse($previousResult, $onlyFiles, $hashCollector)) { + if ($previousResult !== null && $this->canReuse($previousResult, $onlyFiles, $hashCollector, $currentValues)) { $collectedDataRuleResults[$ruleKey] = $previousResult; $reusedRulesCount++; $this->addRuleErrors($previousResult->getErrors(), $node, $scope, $tempCollectorErrors); @@ -250,8 +251,9 @@ private function createRuleResult(CollectedDataNodeReads $reads, bool $onlyFiles /** * @param Closure(string): string $hashCollector + * @param array $currentValues id => value, asked for once per run - every rule shares the values of the classes */ - private function canReuse(CollectedDataRuleResult $previousResult, bool $onlyFiles, Closure $hashCollector): bool + private function canReuse(CollectedDataRuleResult $previousResult, bool $onlyFiles, Closure $hashCollector, array &$currentValues): bool { $previousOnlyFiles = $previousResult->getOnlyFiles(); if ($previousOnlyFiles !== null && $previousOnlyFiles !== $onlyFiles) { @@ -271,7 +273,9 @@ private function canReuse(CollectedDataRuleResult $previousResult, bool $onlyFil return false; } - if ($extension->getValue($extension->keyFromResultCache($storedKey)) !== $value) { + $id = ValueDependencyCollector::getId($extensionClass, $storedKey); + $currentValues[$id] ??= $extension->getValue($extension->keyFromResultCache($storedKey)); + if ($currentValues[$id] !== $value) { return false; } } diff --git a/src/Analyser/ValueDependencyCollector.php b/src/Analyser/ValueDependencyCollector.php index a65b6acbe93..9394281606d 100644 --- a/src/Analyser/ValueDependencyCollector.php +++ b/src/Analyser/ValueDependencyCollector.php @@ -49,6 +49,8 @@ * and reused by every file analysed after it without asking the extension again, so these are kept * for the whole process too, by class, and every file depending on the class - or on a class * extending it or implementing it - gets them as its own dependencies when its analysis finishes. + * A rule for CollectedDataNode gets those of every class, because it is not known which classes + * it read. * * @phpstan-type ValueDependencies = array{ * values: array, @@ -171,11 +173,23 @@ public function startCollectedDataRule(): void } /** + * With what every class seen in this process declares depends on, see recordForClass(): the + * rule may have read any of them, and an extension describes a class only once per process. + * * @return array id => [extension class, key, value] */ public function getCollectedDataRuleValues(): array { - return $this->collectedDataRuleValues ?? []; + if ($this->collectedDataRuleValues === null) { + return []; + } + + $values = $this->collectedDataRuleValues; + foreach ($this->classDeclarations as $classValues) { + $values += $classValues; + } + + return $values; } public function finishCollectedDataRule(): void diff --git a/tests/PHPStan/Analyser/CollectedDataReuse/DescribedClassRule.php b/tests/PHPStan/Analyser/CollectedDataReuse/DescribedClassRule.php new file mode 100644 index 00000000000..b73998f02c6 --- /dev/null +++ b/tests/PHPStan/Analyser/CollectedDataReuse/DescribedClassRule.php @@ -0,0 +1,48 @@ + + */ +final class DescribedClassRule implements Rule +{ + + public int $runs = 0; + + public function __construct(private string $file) + { + } + + public function getNodeType(): string + { + return CollectedDataNode::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $this->runs++; + + $contents = file_get_contents($this->file); + + return [ + RuleErrorBuilder::message(sprintf('Class is described by %s.', $contents === false ? 'nothing' : $contents)) + ->file(__FILE__) + ->line(1) + ->identifier('tests.describedClass') + ->build(), + ]; + } + +} diff --git a/tests/PHPStan/Analyser/CollectedDataRuleReuseTest.php b/tests/PHPStan/Analyser/CollectedDataRuleReuseTest.php index ec971096fa0..9a71818d42c 100644 --- a/tests/PHPStan/Analyser/CollectedDataRuleReuseTest.php +++ b/tests/PHPStan/Analyser/CollectedDataRuleReuseTest.php @@ -2,6 +2,7 @@ namespace PHPStan\Analyser; +use PHPStan\Analyser\CollectedDataReuse\DescribedClassRule; use PHPStan\Analyser\CollectedDataReuse\EarlyReturnRule; use PHPStan\Analyser\CollectedDataReuse\FirstCollector; use PHPStan\Analyser\CollectedDataReuse\OnlyFilesRule; @@ -9,7 +10,9 @@ use PHPStan\Analyser\CollectedDataReuse\ThrowingRule; use PHPStan\Analyser\CollectedDataReuse\TrackingRule; use PHPStan\Analyser\CollectedDataReuse\UnregisteredValueRule; +use PHPStan\Analyser\ResultCache\ResultCacheValueExtension; use PHPStan\Collectors\CollectedData; +use PHPStan\File\FileHelper; use PHPStan\Node\CollectedDataNode; use PHPStan\Rules\DirectRegistry; use PHPStan\Rules\Rule; @@ -207,6 +210,48 @@ public function testRuleRunsAgainWhenATrackedValueChanged(): void rmdir($directory); } + public function testRuleRunsAgainWhenWhatAClassDeclaresChanged(): void + { + $file = sys_get_temp_dir() . '/phpstan-collected-data-reuse-' . uniqid() . '.txt'; + file_put_contents($file, 'one'); + $classReflection = self::createReflectionProvider()->getClass(FirstCollector::class); + + $rule = new DescribedClassRule($file); + $collectedData = [ + self::A => [FirstCollector::class => ['a1']], + ]; + + // a new process each run: an extension describes the class once, before the rule asks for it + /** @param array $previousResults */ + $finalize = function (array $previousResults) use ($rule, $collectedData, $file, $classReflection): FinalizerResult { + $valueDependencyCollector = new ValueDependencyCollector( + self::getContainer()->getExtensionsCollection(ResultCacheValueExtension::class), + self::getContainer()->getByType(FileHelper::class), + ); + (new ValueDependencyDeclarationDependencyTracker($valueDependencyCollector))->trackFileDependency($classReflection, $file); + + return $this->finalize([$rule], $collectedData, false, $previousResults, true, $valueDependencyCollector); + }; + + $result = $finalize([]); + $this->assertSame(1, $rule->runs); + $this->assertSame(['Class is described by one.'], $this->getMessages($result)); + + // nothing changed + $result = $finalize($result->getCollectedDataRuleResults()); + $this->assertSame(1, $rule->runs); + $this->assertSame(1, $result->getReusedRulesCount()); + + // the file the extension described the class from changed - in size too, FileContentHasher memoizes by mtime and size + file_put_contents($file, 'three'); + clearstatcache(); + $result = $finalize($result->getCollectedDataRuleResults()); + $this->assertSame(2, $rule->runs); + $this->assertSame(['Class is described by three.'], $this->getMessages($result)); + + unlink($file); + } + public function testEveryRuleRunsWithoutTheFeatureToggle(): void { $rule = new EarlyReturnRule(); @@ -237,7 +282,7 @@ public function testEveryRuleRunsWithoutTheFeatureToggle(): void * @param CollectorData $collectedData * @param array $previousResults */ - private function finalize(array $rules, array $collectedData, bool $onlyFiles, array $previousResults, bool $reuse = true): FinalizerResult + private function finalize(array $rules, array $collectedData, bool $onlyFiles, array $previousResults, bool $reuse = true, ?ValueDependencyCollector $valueDependencyCollector = null): FinalizerResult { $finalizer = new AnalyserResultFinalizer( new DirectRegistry($rules), @@ -246,7 +291,7 @@ private function finalize(array $rules, array $collectedData, bool $onlyFiles, a self::createScopeFactory(self::createReflectionProvider(), self::getContainer()->getService('typeSpecifier')), new LocalIgnoresProcessor(), false, - self::getContainer()->getByType(ValueDependencyCollector::class), + $valueDependencyCollector ?? self::getContainer()->getByType(ValueDependencyCollector::class), $reuse, );