Skip to content

Cache parsed ASTs behind the path routing, one cache per parser - #6569

Open
SanderMuller wants to merge 1 commit into
phpstan:2.2.xfrom
SanderMuller:parser-cache-per-route
Open

SanderMuller wants to merge 1 commit into
phpstan:2.2.xfrom
SanderMuller:parser-cache-per-route

Conversation

@SanderMuller

Copy link
Copy Markdown
Contributor

defaultAnalysisParser was one CachedParser in front of PathRoutingParser. CachedParser keys ASTs by source code, and PathRoutingParser sends a file outside the analysed paths to CleaningParser, which strips function bodies. A file outside the analysed paths can have the same bytes as an analysed file. When it is parsed first, the analysed file gets the stripped AST from the cache. Each method in it with a return type is then reported as return.missing.

This happens when a bootstrap autoloader loads a copy of an analysed class. It also happens on a case-insensitive filesystem when the autoloader loads the same file under a second spelling of its path. The reporter hit the second case with October CMS, which lowercases directory segments.

This PR moves the cache behind the routing. PathRoutingParser now wraps one CachedParser for the rich parser and one for the cleaning parser, and defaultAnalysisParser is the router itself. A stripped AST can no longer reach an analysed file.

The bug is older than 2.2.10. It is in 2.2.8, #6069 hid it in 2.2.9, and the revert of that PR in 2.2.10 brought it back.

Test

e2e/bug-15231 is the duplicate-file/ variant from the reporter's repro repo: src/Zeta.php is analysed, and a bootstrap autoloader loads a byte-identical lib/Zeta.php. On 2.2.x it reports 2 return.missing errors, and with this change it reports none.

It is an e2e test and not a unit test, because the fix is in conf/parsers.neon. In tests, src/Testing/TestCase.neon replaces the cleaning parser with the rich parser.

Cost

Measured on a real-world project of about 4,500 files, from source, cold, 3 interleaved rounds against 2.2.x at d88544ac9:

  • CPU, parallel: median 135.4s before and 137.0s after, with ranges of 133.9-145.8s and 134.0-144.5s.
  • CPU, --debug: median 83.6s before and 83.5s after.
  • Peak memory of a --debug run, as -v prints it: 620 MB before and 634 MB after, in all 4 runs. The two caches each keep the existing limits, so the rich and the stripped ASTs no longer share one budget.
  • The reported errors are identical, 3260 in both.

Two paths lose cache hits they had before. PathRoutingParser::parseString() uses the cleaning parser, so a string can no longer get the rich AST of an analysed file with the same source. The files that go to php8Parser (phpstorm-stubs and php-8-stubs) are no longer cached. On a --debug run over src/Analyser/ExprHandler, none of the 41 stub files was parsed twice.

Closes phpstan/phpstan#15231

🤖 Generated with Claude Code

CachedParser keys ASTs by source code, and it sat in front of
PathRoutingParser. A file outside the analysed paths goes to the cleaning
parser, which strips function bodies. When such a file has the same bytes
as an analysed file and is parsed first, the analysed file gets the
stripped AST from the cache, and every method in it is reported as
return.missing.

PathRoutingParser now wraps one CachedParser for the rich parser and one
for the cleaning parser, and defaultAnalysisParser is the router.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.

1 participant