[SanderMuller]

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

[ondrejmirtes]

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]

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]

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

Yeah, these should all work

[SanderMuller]

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.

[ondrejmirtes]

Thank you! After 2.3.4 is out, please notify popular extensions.

[SanderMuller]

Thank you! After 2.3.4 is out, please notify popular extensions.

Two things on our side:

1. A rule that reads no get() data gets reused forever. pb30's ComposerRule only calls isOnlyFilesAnalysis(). Should such a rule just always run?
2. A rule can't opt out. todo-by fetches ticket status over HTTP, and the only workaround is a ResultCacheValueExtension per ticket. Do we want an opt-out?

[ondrejmirtes]

Show me the pb30 rule please.

[SanderMuller]

https://github.com/pb30/phpstan-composer-analysis/blob/4edd66c6b71c0bb3c08662943da319a1d67413a2/src/ComposerRule.php#L33-L40

It returns early on isOnlyFilesAnalysis(), and otherwise runs composer-dependency-analyser over the whole project. It never calls get(). The analyser reads composer.json and the source files itself. So a new shadow dependency in the code does not reach the rule on a warm run.

I looked at it again, and I don't think we need to change anything for this. It is the same problem as larastan: inputs the rule does not declare. Once it tracks composer.json and the scanned paths, the reuse is correct. That is what pb30/phpstan-composer-analysis#67 suggests. Point 2 is not needed either: todo-by can track one value for all its tickets, so it needs no opt-out.

One thing that does need a change on our side, reproduced on 2.3.4 with dead-code-detector 1.4.2: its -vvv output is wrong on a warm run. DeadCodeRule is also a DiagnoseExtension, and print() reads state that only processNode() fills. On reuse it prints "Usage debugging is unavailable in files-only analysis." and leaves out the usages over unknown types. Should the reuse be off with -vvv?

[ondrejmirtes]

I feel like rules that never call ->get should always run. Otherwise people would always have to write these value extensions, and it gets pretty awkward for both depAnalyzer and todo-by HTTP fetching.