-
Notifications
You must be signed in to change notification settings - Fork 519
Fix args are mistakenly handled as immediately-invoked #4145
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
Conversation
This pull request has been marked as ready for review. |
a6017ac
to
2f3b833
Compare
src/Analyser/NodeScopeResolver.php
Outdated
if ( | ||
$parameterType === null | ||
|| $parameterType instanceof MixedType | ||
|| $parameterType->isCallable()->no() |
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 think we need a new TypeUtils
method. findCallableType
perhaps. It'd return the type for isCallable()->yes()
. Additionally it should recursively inspect unions for types like ?callable
or ?Closure
.
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.
But I think this logic is wrong. What mainly has to be handled is $parameterCallImmediately->maybe()
. That's where you need to ask about this. And also in the else
section where it's $callCallbackImmediately = $calleeReflection instanceof FunctionReflection;
right 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.
This logic fails for a couple reasons:
- If the parameter is
mixed
but marked with@param-immediately-invoked-callable
it'd get silently ignored. - It'd still mark parameter as immediately invoked if the type is for example
string
(which is maybe-callable).
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.
thanks for the in-detail feedback. there are a lot of things to consider, I did not see yet.
additionally phpstan/phpstan#13331 (comment) was useful.
I tried adding more tests and adding comments about expectations, of what I understood from the comments.
thank you
the php engine might invoke the callables whenever it wants. the assumption about a function invoking the callable immediately fits for userland functions good enough though
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.
Additionally please add a regression test for 12119
src/Type/TypeUtils.php
Outdated
return $type; | ||
} | ||
|
||
if ($type instanceof UnionType || $type instanceof IntersectionType) { |
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 don't think we need to go into intersections. Once one of the intersected types is callable, the whole type is callable.
Thank you! |
closes phpstan/phpstan#13288
closes phpstan/phpstan#13311
closes phpstan/phpstan#13331
closes phpstan/phpstan#13307
closes phpstan/phpstan#12119