-
Notifications
You must be signed in to change notification settings - Fork 446
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
ArrayFilterFunctionReturnTypeReturnTypeExtension handle error types #2794
ArrayFilterFunctionReturnTypeReturnTypeExtension handle error types #2794
Conversation
You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x. |
@@ -54,9 +55,13 @@ public function getTypeFromFunctionCall(FunctionReflection $functionReflection, | |||
} | |||
|
|||
$arrayArgType = $scope->getType($arrayArg); | |||
$arrayArgType->isArray(); |
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.
This is a no-op.
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.
The right solution is:
$arrays = TypeUtils::toBenevolentUnion($arrayArgType)->getArrays();
$arrayArgType = TypeCombinator::union(...$arrays);
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.
Ah, whoops that was a debugging line I thought I had deleted. Thanks for the direction, I will try it out.
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.
Or maybe just would work too:
$arrayArgType = TypeUtils::toBenevolentUnion($arrayArgType);
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.
That worked!
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.
And please listen to the bot 😊 #2794 (comment)
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.
I'll rebase off of 1.10.x and change the PR, I was waiting for "this is a horrible wrong approach" or not. Rebasing now
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.
Okay, rebased off of 1.10.x and changed the PR base
0e0a367
to
cf5484c
Compare
Thank you! |
Thanks for the guidance along the way! |
Fixed also phpstan/phpstan#8638, thank you! |
Fixes phpstan/phpstan#10189