Repository navigation
Track every class, function and constant lookup instead of re-analysing the files with errors - #7032
Open
ondrejmirtes wants to merge 16 commits into
Open
ondrejmirtes wants to merge 16 commits into
ondrejmirtes wants to merge 16 commits into
Conversation
A class looked up and not found is recorded as a value dependency of the analysed file on ClassResultCacheValueExtension, whose value is "missing" until the class exists. When it appears, exactly the files that asked are re-analysed - also the ones without an error, which the re-analysis of the files with errors never reached: narrowing by class_exists(), a class name compared as a string, a rule asking the reflection provider. A lookup made while resolving something remembered for the rest of the process - the ancestors of a class, a PHPDoc type, the initializer of a class constant - is recorded for the file declaring it too, so the files depending on that file are re-analysed as well. With that, a symbol change that only adds classes, interfaces, traits or enums no longer re-analyses all the files with errors. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
…ng the files with errors Each class, function and constant the reflection layer is asked for is recorded as a value dependency of whatever asked, found or not: ClassResultCacheValueExtension, FunctionResultCacheValueExtension (keyed by the lowercased name) and ConstantResultCacheValueExtension (lowercased namespace, case-sensitive name), each "found" or "missing". When a symbol appears or goes away, exactly the files that looked it up are re-analysed. An unqualified function or constant in a namespace looks up the namespaced name before the global one, and both lookups are recorded, so declaring App\strlen() re-analyses the files calling strlen() in App (phpstan/phpstan#15453). What the reflection layer finds by name - from a string, a PHPDoc, a rule asking - also makes the file depend on the file declaring it, followed through what that file's declarations found. MemoizingReflectionProvider remembers its answers for the whole process, so a remembered answer is recorded again once per file, declaration and rule on collected data; the turbo extension answers from the memo only when it was recorded in the current context. Rules on CollectedDataNode record the values they found with their reused result. With that, nothing re-analyses all the files with errors any more: not a new or changed file declaring symbols, a deleted file, a changed scanned file, a changed Composer package or extension version. The filesWithCollectedDataRuleErrors section existed only for that, it is gone. E2E tests in result-cache-symbol-lookups assert the re-analysed files with result-cache-info and compare the warm result with a cold run, for symbols appearing, going away, shadowing global functions and constants, in scanned files and for rules on collected data. Tests that asserted the re-analysis of unrelated files with errors now expect fewer files. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
ClassResultCacheValueExtension hashed the file of each class and ancestor. For a built-in class that is the stub it was reflected from, which depends on the source locator that reflected it first: a rule on collected data asking for SoapClient after the analysis recorded a different value than the next run computed, and its result was never reused (result-cache-collected-data-rules-tracking). A built-in class changes only with PHP, its extensions or PHPStan, which invalidate the result cache anyway. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
The downgrade does not rewrite match expressions or nullsafe method calls, so they become a switch and null checks. issetStringOffset() in ReflectionAccess.cpp lost its last caller, which -Werror rejects. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
The symbol scan lists more files than declare a function or a constant: a method looks like a function to it, and define() may be called anywhere. A function or constant that does not exist parsed those files on each lookup, which the result cache now does on every run to check that it is still missing. On wordpress-develop that was 1.8 seconds of each warm run.
Instead of reaching it through a static instance registered by the container factory. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
…in the name resolution memo
new FullyQualified('strval') and new Name('\strval') - the name of a callable
string - shared one key and resolve differently, so the answer for '\strval'
depended on which was asked first. The result cache now asks for the found
functions by their fully qualified names during the analysis, which made
array_map('\strval', ...) reported or not depending on the order the files
were analysed in.
Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
…ols depended on What a class, a function or a file declares is resolved once per process. The symbols found while resolving it were kept by the file declaring it, so a file depending on any symbol of that file depended on all of them - and on whatever was resolved there before it was analysed. They are kept by the declared class or function now, and a file follows only the declarations of the symbols it depends on, and every declaration of its own. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
…ot have Like classes already: almost every function name reaching this locator is a userland one no earlier locator found - also every namespaced name an unqualified call is looked up by first - and the result cache asks for each missing function again on every run. Asking the cache means hashing the key and stat()ing a file that can never exist. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
A project with many Composer packages has a directory locator for each classmap directory, and a name none of them declares - which the result cache asks about for every missing class and function on every run - was looked up in each of them. They are asked through one map of all their symbols now, in the same order. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
…rom the collision detector Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
A class, function or constant a lookup did not find when the result cache was written is looked up again only when a new or changed analysed or scanned file declares that name, or when a Composer package or PHP extension version changed. Before, every missing name was looked up on every restore, which slowed down warm runs that changed nothing. The files of the project's own Composer autoload entries - PSR-4, PSR-0, classmap and files, of autoload and autoload-dev, and of packages from path repositories - are now tracked like scanned files: a new file or a changed one is noticed and read for what it declares, its exported nodes decide which dependents are re-analysed. An entry containing the vendor directory, like a namespace mapped to the project root, is walked without it, and nothing under tmpDir counts as a scanned file. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
RuleTestCase and the finalizer tests build AnalyserResultFinalizer themselves and were missing the argument added with the declared-names diff. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
A files entry of the project can point into the vendor directory, and a Composer update then changed a scanned file of an installed package, which threw the whole result cache away - the file is tracked with its package already. Expanding a directory holding the vendor directory, like a namespace mapped to the project root, took every file in it, not only the PHP ones. A class appearing in a classmap file under excludePaths is not found by a cold run either, which the new e2e test checks. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
…lt is kept The value of every class a rule on collected data found was computed as soon as the rule looked it up, parsing the files declaring the class and its ancestors - also for a rule reading no collected data, whose result is never kept and which runs every time. On a project with such a rule looking up thousands of classes, a warm run that analysed nothing spent most of its time there. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
A package from a path repository is now tracked file by file, so the fixture's package is recorded as downloaded to still cover the full analysis after an edit of a vendor file. Amp-Thread-ID: https://ampcode.com/threads/T-01a1c5c8-8d5b-7459-a22c-beb8e7ba8a3b
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.
Closes phpstan/phpstan#15211
Closes phpstan/phpstan#15453
The result cache no longer re-analyses all files with errors when a symbol appears or goes away. Each class, function and constant the reflection layer is asked for is recorded as a value dependency of whatever asked, whether it was found or missing. When a symbol appears or disappears, exactly the files that looked it up are re-analysed, including files that had no error but whose result depended on the symbol's absence:
class_exists()/function_exists()/defined()narrowing, class names in strings,new \ReflectionFunction('...'), PHPDoc types likescalarthat become a class, custom rules callinghasClass(), and rules onCollectedDataNode.Commits
ClassResultCacheValueExtensionholdsmissinguntil the class exists. Lookups made while resolving something the reflection remembers for the rest of the process (ancestors, PHPDoc types, class constant initializers) are also recorded for the declaring file, so its dependents follow.FunctionResultCacheValueExtensionandConstantResultCacheValueExtension.App\strlen()re-analyses the files callingstrlen()inApp(#15453).MemoizingReflectionProviderrecords a remembered answer again once per file, declaration and collected-data rule. The turbo extension answers from the memo only when the answer was recorded in the current context.filesWithCollectedDataRuleErrorscache section, which existed only for it, are removed. The cache version is bumped.Tests
e2e/result-cache-missing-classesande2e/result-cache-symbol-lookupshave 42 tests. Each one asserts the exactfilesToAnalyseCountfromresult-cache-info --jsonand compares the warm run with a cold run.define(), conditional declarations, a Composer package adding a class, parameter defaults and global type aliases.result-cache-scanned-2,result-cache-scanned-exported-nodes), and the verbose messages no longer mention "the files with errors".Measured on 9 replayed Laravel commits (about 1,700 files, level 6): warm runs re-analysed 1 to 19 files, or 322 for a commit adding a trait to
Model, where the sweep re-analysed about 930. Each warm run matched its cold run. Restoring took about 1.8 s.Known limitation, unchanged: a
class_alias()in a file that a bootstrap filerequires is not tracked. Only the files listed inbootstrapFilesare hashed.Also checked: the whole unit suite with and without the turbo extension, smoke, method and signature parity, and byte-identical output with and without turbo on Laravel.