Skip to content

Reuse the errors of rules on CollectedDataNode from the result cache - #6840

Open
SanderMuller wants to merge 3 commits into
phpstan:2.3.xfrom
SanderMuller:cache-collected-data-rule-errors
Open

SanderMuller wants to merge 3 commits into
phpstan:2.3.xfrom
SanderMuller:cache-collected-data-rule-errors

Conversation

@SanderMuller

@SanderMuller SanderMuller commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

This follows the design from phpstan/phpstan#15447 (comment).

The rules on CollectedDataNode now run again only when their input changed. CollectedDataNode records which collectors a rule reads with get(), and whether it calls isOnlyFilesAnalysis(). The result cache stores the errors of each rule with a hash of that data. On the next run, a rule with the same input gets its stored errors back.

The ignore extensions and the local ignores still run on every error, also on reused ones. A rule that throws is not stored, so it runs every time. Each rule gets its own CollectedDataNode, so a service that memoizes per node cannot hide a read from the next rule.

To store the rule results, the result cache is now saved after the finalizer ran. That is the first commit, and it is not behind the toggle.

PHPStan self-analysis with dead-code-detector (build/phpstan.neon, 13 errors in both arms):

2.3.x this PR
warm, nothing changed 1.42 s (CPU 1.10 s) 0.45 s (CPU 0.40 s)
warm, one string literal changed 1.83 s (CPU 1.51 s) 0.86 s (CPU 0.77 s)
cold 15.13 s (CPU 110.1 s) 15.15 s (CPU 111.5 s)

Warm rows are the median of 5 rounds, cold of 9 rounds in both orders. The result cache grows from 39.4 MB to 39.7 MB.
I measured the table on an earlier version of this PR and did not measure it again. build/phpstan.neon includes bleeding edge, and no extension in it uses DeclarationDependencyTracker.

A rule can also depend on data the node does not see. larastan's UnusedViewsRule reads the Blade view files from disk. Such a rule declares that data with the DependencyTracker methods, for example trackDirectoryDependency(). The values are stored with its errors, and the rule runs again when one of them changes. Before this PR, these methods did nothing in a rule on CollectedDataNode.

Values that a class reflection extension tracks with DeclarationDependencyTracker reach these rules too. A rule does not say which classes it read, so each rule depends on the values of every class that the process saw. When one of these values changes, a rule that did not read that class can also run again.

The reuse is behind the reuseCollectedDataRuleErrors feature toggle, which is on in bleeding edge only. Without it, every rule runs on every run, as before.

This overlaps with #6373, which loads the cached collected data only when the rules run. This PR hashes the collected data on every warm run, so it reads it every time.

Refs phpstan/phpstan#15447

🤖 Generated with Claude Code

@SanderMuller
SanderMuller force-pushed the cache-collected-data-rule-errors branch from b36b4f2 to b6b5518 Compare October 25, 2026 20:23
@ondrejmirtes

Copy link
Copy Markdown
Member

This is nice but I worry it's also a BC break. Users before expect that CollectedDataNode rules always run.

A few ideas:

  1. Make this bleeding edge only.
  2. Verify the new APIs (https://phpstan.org/developing-extensions/result-cache-meta-extensions#tracking-dependencies-on-files) work inside these rules and that the rules are invalidated and re-run properly. With E2E tests.
  3. Identify most popular extensions like Larastan, Dead Code Detector etc. which make use of Collectors, and tell them about these changes once we merged this and release 2.3.3. Their problems should be solvable with the new APIs.

SanderMuller and others added 2 commits October 26, 2026 15:14
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@SanderMuller
SanderMuller force-pushed the cache-collected-data-rule-errors branch from b6b5518 to c96f229 Compare October 26, 2026 14:43
@SanderMuller

Copy link
Copy Markdown
Contributor Author

Thanks, I did 1 and 2:

  1. Reuse is now behind a reuseCollectedDataRuleErrors feature toggle, which is on in bleeding edge only. Without it, every rule on CollectedDataNode runs as before.
  2. The DependencyTracker methods did nothing in these rules before. Now the values a rule tracks are stored with its errors, and the rule runs again when one of them changes. The new E2E test has a rule like larastan's views rule. It tracks a directory, a file and a class that is not analysed, and the test changes each of them.

One gap is left: values that a class reflection extension tracks with DeclarationDependencyTracker do not reach these rules yet. Should I add that here or in a follow-up?

For 3, I will open issues on larastan, dead-code-detector and the other extensions with collectors after 2.3.3 is out.

@ondrejmirtes

Copy link
Copy Markdown
Member

values that a class reflection extension tracks with DeclarationDependencyTracker do not reach these rules yet

Yeah, these should all work

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) <noreply@anthropic.com>
@SanderMuller

Copy link
Copy Markdown
Contributor Author

Done in c7b1469. A rule does not say which classes it read. So each rule on CollectedDataNode now depends on the values of every class that the process saw. When one of these values changes, a rule that did not read that class can also run again.

The tracking E2E test now has a class reflection extension that describes a class from a file that is not analysed. When the test empties that file, the rule runs again and reports the view. Without this commit, the rule is reused and the error is missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants