Cache parsed ASTs behind the path routing, one cache per parser - #6569
Open
SanderMuller wants to merge 1 commit into
Open
SanderMuller wants to merge 1 commit into
SanderMuller wants to merge 1 commit into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
defaultAnalysisParserwas oneCachedParserin front ofPathRoutingParser.CachedParserkeys ASTs by source code, andPathRoutingParsersends a file outside the analysed paths toCleaningParser, 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 asreturn.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.
PathRoutingParsernow wraps oneCachedParserfor the rich parser and one for the cleaning parser, anddefaultAnalysisParseris 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-15231is theduplicate-file/variant from the reporter's repro repo:src/Zeta.phpis analysed, and a bootstrap autoloader loads a byte-identicallib/Zeta.php. On 2.2.x it reports 2return.missingerrors, 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.neonreplaces 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:--debug: median 83.6s before and 83.5s after.--debugrun, as-vprints 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.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 tophp8Parser(phpstorm-stubs and php-8-stubs) are no longer cached. On a--debugrun oversrc/Analyser/ExprHandler, none of the 41 stub files was parsed twice.Closes phpstan/phpstan#15231
🤖 Generated with Claude Code