-
Notifications
You must be signed in to change notification settings - Fork 434
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
Implement ParameterOutTypeExtensions #3083
base: 1.11.x
Are you sure you want to change the base?
Conversation
This pull request has been marked as ready for review. |
I am not yet sure this feature is useful for the preg_match inference, because the type depends on context: https://github.com/phpstan/phpstan-src/pull/2589/files#diff-65e7da030813493bd5e549d34d89e84b206ed1f44cfc490ffaa52dcba136bd9bR128 |
Thinking more about it: maybe we need a param out type which depends on the function call return type for the preg_match case |
My question is: How important that is? Can it work without this condition in the first release? I have some ideas but it'd require some exploration (conditional types). |
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.
Otherwise 👍
we will see when looking into it. I think this PR as is also makes sense in general. I think conditional param-out types could work. |
d7fcc22
to
b872fd6
Compare
if ($currentParameter === null) { | ||
throw new ShouldNotHappenException(); | ||
} | ||
|
||
$argValue = $arg->value; | ||
if ($argValue instanceof Variable && is_string($argValue->name)) { |
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.
why does this code path only exec on variables and not e.g. on PropertyFetch
?
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.
You can explore that in another PR.
src/Analyser/NodeScopeResolver.php
Outdated
$argValue = $arg->value; | ||
if ($argValue instanceof Variable && is_string($argValue->name)) { | ||
|
||
$paramOutTypes = $this->getParameterOutExtensionsTypes($callLike, $calleeReflection, $currentParameter, $scope); |
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.
Will this realistically ever return multiple types? I feel like the union should be done inside this method, and the method should return ?Type
.
if ($currentParameter === null) { | ||
throw new ShouldNotHappenException(); | ||
} | ||
|
||
$argValue = $arg->value; | ||
if ($argValue instanceof Variable && is_string($argValue->name)) { |
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.
You can explore that in another PR.
refs #2589 (comment)