Improve conditional type resolving performance#2030
Improve conditional type resolving performance#2030ondrejmirtes merged 16 commits intophpstan:1.9.xfrom
Conversation
|
653c00b |
|
This looks promising 😊 Please run |
|
I'm running |
|
Looks great 😊 |
|
This is something like resolving the conditional types as late as it can. phpstan-src/src/Analyser/MutatingScope.php Line 3904 in 42bddf0 when merging, we can only "intersect" conditional types. simple example if ($a) {
// conditional type $c='foo' => $b = 'foo'
} else {
// conditional type $c='bar' => $b = 'bar'
}
// Whats the conditional type here? Currently they are deleted while intersecting
// Maybe we can merge them like $a=truthty&&$c='foo' => $b='foo', $a=falsy&&$c='bar' => $b='bar' |
This reverts commit 653c00b.
|
Three failing tests are related to |
|
Unfortunately you're on your own, I hope you figure it out, I'm following it closely :) It'd take me a few days to load everything into my brain and it's already in yours so you'll be faster in finishing it I'm sure :) |
|
I debugged further bug-987 shows whats happening phpstan-src/tests/PHPStan/Analyser/data/bug-987.php Lines 38 to 51 in 579b075 /**
* @return string[]
*/
function hello(self $class, bool $boolOption): array
{
$myArray = $class->arrOrNull();
if ($boolOption && $myArray === null) {
throw new \Exception('');
}
// !$boolOption or $boolOption=true => $myArray=array<string>
if (!$boolOption) {
$myArray = $class->otherGetArray();
// $myArray=array<string> in inner scope
}
// inner scope filterByTruthyValue(!$boolOption) and outer scope filterByFalsyValue(!$boolOption) is merged
// In outer scope $boolOption=true, so it can resolve $boolOption=true => $myArray=array<string>
// Although, with changes in fix-conditional-performance this conditional is not resolved and still $myArray=array<string>|null
assertType('array<string>', $myArray);
return $myArray;
} |
|
I think adding this kind of conditional type phpstan-src/src/Analyser/NodeScopeResolver.php Lines 3420 to 3427 in f8f09cd but not sure yet. |
c6d605b to
a72f632
Compare
src/Analyser/MutatingScope.php
Outdated
| foreach ($scope->conditionalExpressions as $conditionalExprString => $conditionalExpressions) { | ||
| foreach (array_reverse($conditionalExpressions) as $conditionalExpression) { | ||
| $newConditionExpressionTypeHolders = []; | ||
| foreach ($conditionalExpression->getConditionExpressionTypeHolders() as $holderExprString => $conditionalTypeHolder) { | ||
| if ($holderExprString === $exprString && $expressionTypes[$holderExprString]->equals($conditionalTypeHolder)) { | ||
| continue; | ||
| } | ||
| $newConditionExpressionTypeHolders[$holderExprString] = $conditionalTypeHolder; | ||
| } | ||
| if ($newConditionExpressionTypeHolders === []) { | ||
| if ($conditionalExpression->getTypeHolder()->getCertainty()->no()) { | ||
| unset($expressionTypes[$conditionalExprString]); | ||
| } else { | ||
| $expressionTypes[$conditionalExprString] = $conditionalExpression->getTypeHolder(); | ||
| } | ||
| continue 2; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not satisfied enough with this implementation, but this works at least.
I think after conditional types merging logic has improved, we can delete matching ConditionExpressionTypeHolders
src/Analyser/MutatingScope.php
Outdated
| // tmp: causes an infinite loop by `getType` in `findPropertyReflectionFromNode` | ||
| // if ($expr instanceof PropertyFetch) { | ||
| // $propertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($expr, $this); | ||
| // if ($propertyReflection !== null) { | ||
| // $nativePropertyReflection = $propertyReflection->getNativeReflection(); | ||
| // if ($nativePropertyReflection !== null && $nativePropertyReflection->isReadOnly()) { | ||
| // return false; | ||
| // } | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Now the only thing to solve is this
There was a problem hiding this comment.
Waiting impatiently for the fix :)
only have to invalidate the current conditional expression
|
Hmm doesn't look correct. |
e07daae to
68afcd8
Compare
|
I'm very satisfied with this implementation. Gonna update the pull request description for explanation! |
…pecified and types are same
|
This pull request has been marked as ready for review. |
|
ondrejmirtes
left a comment
There was a problem hiding this comment.
Awesome, looks great 😊
- Please add a regression test for #5805, check the issue bot.
- Did you check all the failures? Are they improvements?
thank you!
Oh, thanks! I wasn't aware with the issue bot result change.
Sorry I forgot to comment about it. https://github.com/PrestaShop/PrestaShop/blob/38d8ad3ef7a2faa6b6db73b0a19768d2be3dc85c/controllers/admin/AdminImportController.php#L3094-L3101 https://github.com/PrestaShop/PrestaShop/blob/38d8ad3ef7a2faa6b6db73b0a19768d2be3dc85c/controllers/admin/AdminProductsController.php#L689 |
|
Very nice indeed 🎉 |
|
Confirm that added regression test fails with 1.9.x |
|
Thank you! I'll try to run it with Blackfire to compare 1.9.2 and the new 1.9.x to see what can be done, after the weekend. |
|
great job @rajyan 👍 |
resolves phpstan/phpstan#8397
resolves #2033
closes phpstan/phpstan#5805
I've improved the logic ofconditionalExpressionsto only resolve conditional types when needed, instead of resolving allconditionalExpressionsevery timefilterBySpecifiedTypesis called.I'm quite confident that this change can improve the performance :)
Now, all I have to do is to pass all the tests!
Update:
Instead of calling
getTypeeach time to check if conditional types's conditions are resolvable, I changed to only check if the conditions are specified and the specified type is same as the condition type.This became possible, because previous refactoring made
assignExpressionandspecifyExpressionTypeseparated correctly, and$scope->getTypecan be used to get the specified types:+1: