From 1cd9747d1b822131b8f97c708cd6d7c6a1e4219e Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah Date: Mon, 31 Aug 2026 05:20:55 +0000 Subject: [PATCH] Upgrade cached ASTs for analysed files 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. --- .github/workflows/e2e-tests.yml | 6 ++ e2e/early-reflection-extension/.gitignore | 2 + e2e/early-reflection-extension/composer.json | 8 +++ e2e/early-reflection-extension/composer.lock | 18 ++++++ ...lectionMethodsClassReflectionExtension.php | 33 ++++++++++ e2e/early-reflection-extension/phpstan.neon | 11 ++++ .../src/AnalysedClass.php | 13 ++++ src/Parser/CachedParser.php | 29 ++++++--- src/Parser/PathRoutingParser.php | 40 +++++++++--- tests/PHPStan/Parser/CachedParserTest.php | 62 +++++++++++++++++++ 10 files changed, 206 insertions(+), 16 deletions(-) create mode 100644 e2e/early-reflection-extension/.gitignore create mode 100644 e2e/early-reflection-extension/composer.json create mode 100644 e2e/early-reflection-extension/composer.lock create mode 100644 e2e/early-reflection-extension/extension/EarlyReflectionMethodsClassReflectionExtension.php create mode 100644 e2e/early-reflection-extension/phpstan.neon create mode 100644 e2e/early-reflection-extension/src/AnalysedClass.php diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index fac692020a3..751752ffd0d 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -346,6 +346,12 @@ jobs: cd e2e/bug-11857 composer install ../../bin/phpstan + - script: | + cd e2e/early-reflection-extension + composer install + # The regression happens on the first reflection parse, before the reflection cache under tmp exists. + rm -rf tmp + ../../bin/phpstan - script: | cd e2e/in-trait OUTPUT=$(../bashunit -a exit_code "1" "../../bin/phpstan --error-format=raw") diff --git a/e2e/early-reflection-extension/.gitignore b/e2e/early-reflection-extension/.gitignore new file mode 100644 index 00000000000..f451f91a33d --- /dev/null +++ b/e2e/early-reflection-extension/.gitignore @@ -0,0 +1,2 @@ +/tmp +/vendor diff --git a/e2e/early-reflection-extension/composer.json b/e2e/early-reflection-extension/composer.json new file mode 100644 index 00000000000..07668e1463a --- /dev/null +++ b/e2e/early-reflection-extension/composer.json @@ -0,0 +1,8 @@ +{ + "autoload-dev": { + "classmap": [ + "extension/", + "src/" + ] + } +} diff --git a/e2e/early-reflection-extension/composer.lock b/e2e/early-reflection-extension/composer.lock new file mode 100644 index 00000000000..ba8d41762cc --- /dev/null +++ b/e2e/early-reflection-extension/composer.lock @@ -0,0 +1,18 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state", + "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", + "This file is @generated automatically" + ], + "content-hash": "d751713988987e9331980363e24189ce", + "packages": [], + "packages-dev": [], + "aliases": [], + "minimum-stability": "stable", + "stability-flags": {}, + "prefer-stable": false, + "prefer-lowest": false, + "platform": {}, + "platform-dev": {}, + "plugin-api-version": "2.9.0" +} diff --git a/e2e/early-reflection-extension/extension/EarlyReflectionMethodsClassReflectionExtension.php b/e2e/early-reflection-extension/extension/EarlyReflectionMethodsClassReflectionExtension.php new file mode 100644 index 00000000000..20fec273d05 --- /dev/null +++ b/e2e/early-reflection-extension/extension/EarlyReflectionMethodsClassReflectionExtension.php @@ -0,0 +1,33 @@ +getClass(AnalysedClass::class)->getNativeReflection(); + } + + #[Override] + public function hasMethod(ClassReflection $classReflection, string $methodName): bool + { + return false; + } + + #[Override] + public function getMethod(ClassReflection $classReflection, string $methodName): MethodReflection + { + throw new ShouldNotHappenException(); + } + +} diff --git a/e2e/early-reflection-extension/phpstan.neon b/e2e/early-reflection-extension/phpstan.neon new file mode 100644 index 00000000000..598cd0b14df --- /dev/null +++ b/e2e/early-reflection-extension/phpstan.neon @@ -0,0 +1,11 @@ +parameters: + level: 8 + tmpDir: tmp + paths: + - src + +services: + - + class: EarlyReflectionExtension\EarlyReflectionMethodsClassReflectionExtension + tags: + - phpstan.broker.methodsClassReflectionExtension diff --git a/e2e/early-reflection-extension/src/AnalysedClass.php b/e2e/early-reflection-extension/src/AnalysedClass.php new file mode 100644 index 00000000000..2dc7b036c79 --- /dev/null +++ b/e2e/early-reflection-extension/src/AnalysedClass.php @@ -0,0 +1,13 @@ + keyed by source code, weighing the length of that source */ private LruCache $cachedNodesByString; - /** @var array */ - private array $parsedByString = []; + /** @var array source code parsed into the richest file representation */ + private array $cachedNodesByStringAreRich = []; /** @var LruCache path => [mtime, size, source code] */ private LruCache $cachedSourceByFile; @@ -78,7 +78,18 @@ public function parseFile(string $file): array { $sourceCode = $this->readFile($file); $cachedNodes = $this->cachedNodesByString->get($sourceCode); - if ($cachedNodes !== null && !isset($this->parsedByString[$sourceCode])) { + if ($cachedNodes !== null && isset($this->cachedNodesByStringAreRich[$sourceCode])) { + return $cachedNodes; + } + + // An entry the routing parser produced before the analysed-file list + // arrived holds a cleaned AST. Serving it to a request that has to see + // the file in full is the bug this marker prevents. + $richRequest = !$this->originalParser instanceof PathRoutingParser + || $this->originalParser->shouldUseRichParser($file); + if ($cachedNodes !== null && !$richRequest) { + // Repeated file parses have to return the same node objects. A fixable rule can + // locate a node through another parser caller and match it by object identity. return $cachedNodes; } @@ -87,12 +98,13 @@ public function parseFile(string $file): array // upgrade an entry previously produced by parseString() in place - // no net change to the entry count, just refresh its LRU position $this->cachedNodesByString->replace($sourceCode, $nodes); - unset($this->parsedByString[$sourceCode]); - - return $nodes; + } else { + $this->store($sourceCode, $nodes); } - $this->store($sourceCode, $nodes); + if ($richRequest) { + $this->cachedNodesByStringAreRich[$sourceCode] = true; + } return $nodes; } @@ -109,7 +121,6 @@ public function parseString(string $sourceCode): array $nodes = $this->originalParser->parseString($sourceCode); $this->store($sourceCode, $nodes); - $this->parsedByString[$sourceCode] = true; return $nodes; } @@ -120,7 +131,7 @@ public function parseString(string $sourceCode): array private function store(string $sourceCode, array $nodes): void { foreach ($this->cachedNodesByString->set($sourceCode, $nodes, strlen($sourceCode)) as $evictedSourceCode) { - unset($this->parsedByString[$evictedSourceCode]); + unset($this->cachedNodesByStringAreRich[$evictedSourceCode]); } } diff --git a/src/Parser/PathRoutingParser.php b/src/Parser/PathRoutingParser.php index 53040aa27f9..77b69f867cf 100644 --- a/src/Parser/PathRoutingParser.php +++ b/src/Parser/PathRoutingParser.php @@ -21,6 +21,9 @@ final class PathRoutingParser implements Parser /** @var array filePath(string) => bool(true) */ private array $analysedFiles = []; + /** @var array */ + private array $shouldUseRichParserCache = []; + public function __construct( private FileHelper $fileHelper, private Parser $currentPhpVersionRichParser, @@ -38,16 +41,31 @@ public function __construct( public function setAnalysedFiles(array $files): void { $this->analysedFiles = array_fill_keys($files, true); + $this->shouldUseRichParserCache = []; } public function parseFile(string $file): array { - $normalizedPath = $this->fileHelper->normalizePath($file, '/'); - if (str_contains($normalizedPath, 'vendor/jetbrains/phpstorm-stubs')) { + if ($this->isPhp8StubFile($file)) { return $this->php8Parser->parseFile($file); } - if (str_contains($normalizedPath, 'vendor/phpstan/php-8-stubs/stubs')) { - return $this->php8Parser->parseFile($file); + + $parser = $this->shouldUseRichParser($file) + ? $this->currentPhpVersionRichParser + : $this->currentPhpVersionSimpleParser; + + return $parser->parseFile($this->fileHelper->normalizePath($file)); + } + + public function shouldUseRichParser(string $file): bool + { + return $this->shouldUseRichParserCache[$file] ??= $this->resolveShouldUseRichParser($file); + } + + private function resolveShouldUseRichParser(string $file): bool + { + if ($this->isPhp8StubFile($file)) { + return false; } $file = $this->fileHelper->normalizePath($file); @@ -64,16 +82,16 @@ public function parseFile(string $file): array if ($realFilePath !== false) { $normalizedRealFilePath = $this->fileHelper->normalizePath($realFilePath); if (isset($this->analysedFiles[$normalizedRealFilePath])) { - return $this->currentPhpVersionRichParser->parseFile($file); + return true; } } break; } - return $this->currentPhpVersionSimpleParser->parseFile($file); + return false; } - return $this->currentPhpVersionRichParser->parseFile($file); + return true; } public function parseString(string $sourceCode): array @@ -81,4 +99,12 @@ public function parseString(string $sourceCode): array return $this->currentPhpVersionSimpleParser->parseString($sourceCode); } + private function isPhp8StubFile(string $file): bool + { + $normalizedPath = $this->fileHelper->normalizePath($file, '/'); + + return str_contains($normalizedPath, 'vendor/jetbrains/phpstorm-stubs') + || str_contains($normalizedPath, 'vendor/phpstan/php-8-stubs/stubs'); + } + } diff --git a/tests/PHPStan/Parser/CachedParserTest.php b/tests/PHPStan/Parser/CachedParserTest.php index 820bb3b47c2..2e8a1659a2f 100644 --- a/tests/PHPStan/Parser/CachedParserTest.php +++ b/tests/PHPStan/Parser/CachedParserTest.php @@ -198,6 +198,68 @@ public function testParseTheSameFileWithDifferentMethod(): void $this->assertSame(2, $stmts[0]->stmts[1]->expr->expr->class->getAttribute(AnonymousClassVisitor::ATTRIBUTE_LINE_INDEX)); } + public function testParseFileBeforeAnalysedFilesAreSet(): void + { + $fileHelper = self::getContainer()->getByType(FileHelper::class); + $pathRoutingParser = new PathRoutingParser( + $fileHelper, + self::getContainer()->getService('currentPhpVersionRichParser'), + self::getContainer()->getService('currentPhpVersionSimpleDirectParser'), + self::getContainer()->getService('php8Parser'), + null, + ); + $parser = new CachedParser($pathRoutingParser, 500, 4194304); + $path = $fileHelper->normalizePath(__DIR__ . '/data/test.php'); + + $stmts = $parser->parseFile($path); + $this->assertInstanceOf(Namespace_::class, $stmts[0]); + $this->assertInstanceOf(Node\Stmt\Expression::class, $stmts[0]->stmts[0]); + $this->assertInstanceOf(Node\Expr\Assign::class, $stmts[0]->stmts[0]->expr); + $this->assertInstanceOf(Node\Expr\New_::class, $stmts[0]->stmts[0]->expr->expr); + $this->assertNull($stmts[0]->stmts[0]->expr->expr->class->getAttribute(AnonymousClassVisitor::ATTRIBUTE_LINE_INDEX)); + + $pathRoutingParser->setAnalysedFiles([$path]); + + $stmts = $parser->parseFile($path); + $this->assertInstanceOf(Namespace_::class, $stmts[0]); + $this->assertInstanceOf(Node\Stmt\Expression::class, $stmts[0]->stmts[0]); + $this->assertInstanceOf(Node\Expr\Assign::class, $stmts[0]->stmts[0]->expr); + $this->assertInstanceOf(Node\Expr\New_::class, $stmts[0]->stmts[0]->expr->expr); + $this->assertSame(1, $stmts[0]->stmts[0]->expr->expr->class->getAttribute(AnonymousClassVisitor::ATTRIBUTE_LINE_INDEX)); + } + + public function testParseStringEntryIsNotUpgradedBeforeAnalysedFilesAreSet(): void + { + $fileHelper = self::getContainer()->getByType(FileHelper::class); + $pathRoutingParser = new PathRoutingParser( + $fileHelper, + self::getContainer()->getService('currentPhpVersionRichParser'), + self::getContainer()->getService('currentPhpVersionSimpleDirectParser'), + self::getContainer()->getService('php8Parser'), + null, + ); + $parser = new CachedParser($pathRoutingParser, 500, 4194304); + $path = $fileHelper->normalizePath(__DIR__ . '/data/test.php'); + + $stringStmts = $parser->parseString(FileReader::read($path)); + $stmts = $parser->parseFile($path); + $this->assertSame($stringStmts, $stmts); + $this->assertInstanceOf(Namespace_::class, $stmts[0]); + $this->assertInstanceOf(Node\Stmt\Expression::class, $stmts[0]->stmts[0]); + $this->assertInstanceOf(Node\Expr\Assign::class, $stmts[0]->stmts[0]->expr); + $this->assertInstanceOf(Node\Expr\New_::class, $stmts[0]->stmts[0]->expr->expr); + $this->assertNull($stmts[0]->stmts[0]->expr->expr->class->getAttribute(AnonymousClassVisitor::ATTRIBUTE_LINE_INDEX)); + + $pathRoutingParser->setAnalysedFiles([$path]); + + $stmts = $parser->parseFile($path); + $this->assertInstanceOf(Namespace_::class, $stmts[0]); + $this->assertInstanceOf(Node\Stmt\Expression::class, $stmts[0]->stmts[0]); + $this->assertInstanceOf(Node\Expr\Assign::class, $stmts[0]->stmts[0]->expr); + $this->assertInstanceOf(Node\Expr\New_::class, $stmts[0]->stmts[0]->expr->expr); + $this->assertSame(1, $stmts[0]->stmts[0]->expr->expr->class->getAttribute(AnonymousClassVisitor::ATTRIBUTE_LINE_INDEX)); + } + public function testWithExprCacheHelper(): void { $fileHelper = self::getContainer()->getByType(FileHelper::class);