Skip to content

Don't reuse cleaned ASTs for analysed files - #6310

Open
binaryfire wants to merge 1 commit into
phpstan:2.2.xfrom
binaryfire:fix/parser-cache-before-analysed-files
Open

Don't reuse cleaned ASTs for analysed files#6310
binaryfire wants to merge 1 commit into
phpstan:2.2.xfrom
binaryfire:fix/parser-cache-before-analysed-files

Conversation

@binaryfire

@binaryfire binaryfire commented Aug 31, 2026

Copy link
Copy Markdown

PathRoutingParser can't choose the parser for analysed files until the analysed file list is set. If an eagerly created extension uses ReflectionProvider in its constructor, CachedParser can store a cleaned AST and return it later during analysis. The affected extension types are property and method class reflection extensions, dynamic function, method, and static method return type extensions, and function, method, and static method type-specifying extensions. Rules and collectors are created after the analysed file list is set, so they can't trigger it.

This PR tracks whether cached ASTs contain the full file representation and upgrades cleaned entries when an analysed file needs one. It adds unit coverage for both cache entry paths and an e2e case for early reflection. In a cold Hypervel analysis, the new per-process route memo reached at most 1,787 entries.

@ondrejmirtes

Copy link
Copy Markdown
Member

Please reproduce the problem as well with e2e test.

With what kind of extension this happens?

The parser route for a file can change after the analysed file list is
installed. Track whether source-keyed entries contain the richest file
representation so a cleaned entry is reparsed when the route changes,
while preserving cached node identity for repeated simple parses.

Add unit coverage for both early cache paths and an end-to-end fixture
where an eagerly created extension reflects an analysed class.
@binaryfire
binaryfire force-pushed the fix/parser-cache-before-analysed-files branch from 1a0db04 to 1cd9747 Compare August 31, 2026 08:40
@binaryfire binaryfire changed the title Do not cache ASTs before analysed files are set Don't reuse cleaned ASTs for analysed files Aug 31, 2026
@binaryfire

Copy link
Copy Markdown
Author

This can happen with extensions from the collections PHPStan loads while creating TypeSpecifier: property and method class reflection extensions, dynamic function, method, and static method return type extensions, and function, method, and static method type-specifying extensions. If one uses ReflectionProvider in its constructor to inspect a class that's also being analysed, it can trigger parsing before the analysed file list is set. Rules and collectors are created later, so they can't trigger it.

I've added a minimal e2e case using a MethodsClassReflectionExtension. On the old code, the cold run reports return.missing because analysis reuses the earlier AST without the method body. The updated code upgrades that entry and the analysis passes.

@ondrejmirtes

Copy link
Copy Markdown
Member

Doing work in constructor is a bad idea for many reasons. Did you personally encounter or find an extension that does this on GitHub?

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