Draft
Conversation
…e` wherever possible
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.
Hi, this is step ~7 out of 12 I have planned. The endgoal is to get rid of 17 % of performance penalty and repeated calls to MutatingScope::resolveType. Of course the performance improvement might not pan out (the new version might also have bottlenecks we'll not see until the very end), but I still like how the new code looks like so I think the rewrite is worth it.
Right now PHPStan traverses AST during analysis multiple times:
instanceofThere are many problems this causes. One of the most understandable examples is that MutatingScope actually also has to call NodeScopeResolver (and TypeSpecifier for that matter) when resolving type of BooleanAnd and others:
phpstan-src/src/Analyser/MutatingScope.php
Line 1069 in f1e8852
The ongoing refactoring aims to change that. I already introduced
ExprHandlerwhich consolidates code handling from NodeScopeResolver and MutatingScope into a single class for the same expression type.processExpris what originally happened in NodeScopeResolver andresolveTypeis what happened in MutatingScope. But this didn't fundamentally change the code logic and performance.My idea is to introduce
getTypeintoExpressionResult- which means that afterNodeScopeResolver::processExprNodefinishes, we don't have just the updated Scope, we also have the Type of the expression. This should reduce the need to callNodeScopeResolverwhen resolving a type, because we already have the right scope to based our type resolution on!Right now the refactoring duplicates and transforms the logic from
resolveTypeto be suitably run inprocessExpr, specifically intypeCallbackconstructor parameter of ExpressionResult. This means that calling$scope->getType($expr)inside typeCallback is forbidden. Instead, the code should ask$exprResult->getType()from previously calledprocessExprNode.In cases where the
$exprpassed intogetTypeis synthetic (virtual - is created withnewin the code, doesn't come from the parsed AST), we need to callprocessExprNodeand grab its type from ExpressionResult after.Thanks to Fibers on PHP 8.1+, the code in
resolveTypeis basically dead,MutatingScope::getType()should never be called. That's why I can live with the current duplication, in PHPStan 3.0 we'll be able to delete a lot of code. Currently it means that any changes to type resolution have to be edited twice - inresolveTypeand intypeCallback. If we fail to have equivalent logic, tests on PHP 7.4-8.0 or on PHP 8.1+ will fail.This is starting to get some really nice benefits. Previously, NullsafeShortCircuitingHelper had to be called recursively for every eligible expression, to see if there's
?->somewhere in the chain that would influence the final result. With the new way, this code only has to happen once, in NullsafePropertyFetchHandler + NullsafeMethodCallHandler.I imagine this will have other benefits, like no longer needing to unwrap assigns in TypeSpecifier
phpstan-src/src/Analyser/TypeSpecifier.php
Lines 1994 to 2001 in c14518f
In the next steps, I also will move the code from TypeSpecifier to respective ExprHandlers, in a new
specifyTypesmethod. And then I will copy and adjust the code in processExpr so thatExpressionResultcan inform about narrowed types in some form as well. And in PHPStan 3.0. we'll be able to delete all thespecifyTypesmethods as well.I encourage you to wrap your head around this. I look forward to your feedback!