New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix slow enum-union with lots of cases #2985
Conversation
not sure where to put the regression test, since we need a composer.json/lock. in the past we used should I do a end-2-end test? if so, how/where to put a "timeout" for it? alternatively I can try to reduce the existing test to something without external composer dependencies |
(added regression test) |
@@ -200,8 +201,7 @@ public static function union(Type ...$types): Type | |||
if ($types[$i] instanceof StringType && !$types[$i] instanceof ClassStringType) { | |||
$hasGenericScalarTypes[ConstantStringType::class] = true; | |||
} | |||
$enumCases = $types[$i]->getEnumCases(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the issue is with getEnumCases
be slow.
Shouldn't be introduced a new method isSomething
, implemented by all types in order to avoid the error message
Doing instanceof PHPStan\\\\Type\\\\Enum\\\\EnumCaseObjectType is error\\-prone and deprecated\\
?
Then
if ($types[$i]->isSomething()->yes()) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure its worth a new type-method just for this bottleneck
Thank you! |
closes phpstan/phpstan#10772
this effectively reverts 0a39b42 but fixes a nasty perf problem
the reproducer of #10772 took >2 minutes before this change and finishes instantly after it
Todo: regression test