Implement array_all type specifying for arrow functions#5134
Implement array_all type specifying for arrow functions#5134uekann wants to merge 17 commits intophpstan:2.1.xfrom
Conversation
|
I'm out this weekend and will get to any feedback on Monday. |
2d9dbbb to
e44e476
Compare
|
This pull request has been marked as ready for review. |
845b403 to
407c372
Compare
|
@staabm
I also considered implementing support so that passing other functions annotated with |
|
This pull request has been marked as ready for review. |
| public function specifyTypes(FunctionReflection $functionReflection, FuncCall $node, Scope $scope, TypeSpecifierContext $context): SpecifiedTypes | ||
| { | ||
| $args = $node->getArgs(); | ||
| if (!$context->truthy() || count($args) < 2) { |
There was a problem hiding this comment.
I never know whether its $context->truthy() or $context->true().
please add a test with a === true comparison
(the escaped mutant at least tells us, its not covered by tests)
There was a problem hiding this comment.
I added a test for the === true comparison, but for a function returning bool it does not distinguish between $context->true() and $context->truthy(): both lead to the same narrowing on the true branch. So the test covers the syntax, but it does not really prove which context check is the correct one here.
| @@ -0,0 +1,179 @@ | |||
| <?php // lint >= 8.4 | |||
There was a problem hiding this comment.
does this test really depend on php 8.4? I think this can be lowered without problems?
There was a problem hiding this comment.
I understand that array_all is a function added in PHP 8.4. Therefore, the tests are set for 8.4 and later.
I ran it on my machine and it passed on 8.3 (I'm not sure why this is), so should I remove the mention of 8.4 and later?
There was a problem hiding this comment.
good point. it works on 8.3 because we have symfony/polyfills.
leave it as is than, thanks.
| $callable instanceof Expr\Closure && | ||
| count($callable->stmts) === 1 && | ||
| $callable->stmts[0] instanceof Stmt\Return_ && | ||
| isset($callable->stmts[0]->expr) | ||
| ) { | ||
| $callableExpr = $callable->stmts[0]->expr; |
There was a problem hiding this comment.
I don't think this is the right approach, but don't have a better idea atm either
There was a problem hiding this comment.
I initially considered performing type specification for arbitrary closures, but I concluded that this would require tracking the result of executing statements, which does not seem realistic. I also do not think that would align well with PHPStan’s design.
For that reason, I implemented it so that type specification is applied only to closures that contain nothing but a return statement.
Another option would be to apply type specification only to closures produced by converting a function to a first-class callable. I considered this because assertions written in PHPDoc can exist on the original function. However, at the time of implementation, that assertion information was lost when the function was converted to a closure, so this did not seem realistic either.
(The issue I opened about that, phpstan/phpstan#14249, has since been resolved, so this may become a viable option in the future. However, at least for now, I also cannot think of a better alternative, so I think this approach is acceptable as a temporary solution.)
|
@ondrejmirtes |
c2e15f0 to
85be1cc
Compare
85be1cc to
14e0f3d
Compare
| } elseif ( | ||
| $callable instanceof Expr\Closure && | ||
| count($callable->stmts) === 1 && | ||
| $callable->stmts[0] instanceof Stmt\Return_ && | ||
| isset($callable->stmts[0]->expr) | ||
| ) { |
There was a problem hiding this comment.
I agree with @staabm that this might not be satisfying.
The first thing I thought then was to check how it was done for array_filter (in ArrayFilterFunctionReturnTypeHelper) and they seems to do something similar
if ($callbackArg instanceof Closure && count($callbackArg->stmts) === 1 && count($callbackArg->params) > 0) {
$statement = $callbackArg->stmts[0];
if ($statement instanceof Return_ && $statement->expr !== null) {
But it handles also
elseif (
($callbackArg instanceof FuncCall || $callbackArg instanceof MethodCall || $callbackArg instanceof StaticCall)
&& $callbackArg->isFirstClassCallable()
)
and
else {
$constantStrings = $scope->getType($callbackArg)->getConstantStrings();
if (count($constantStrings) > 0) {
So I think we could have more case to handle.
This PR partially resolves phpstan/phpstan#12939 by specifying array element types when array_all returns true.
To keep this initial PR focused, it currently supports only ArrowFunction (e.g.,
array_all($arr, fn($v, $k) => is_int($v) && is_string($k))). It correctly recognizes union and intersection types when the arrow function uses||(or) and&&(and).The following features are left as future scope:
Standard Closures: (e.g.,
function($v) { ... })String and First-class callables: (e.g.,
'is_int'oris_int(...)). Note that these currently throw an ArgumentCountError in PHP anyway, asarray_all()passes both value and key, but functions like is_int expect exactly one argument (see: https://3v4l.org/2DQ9a#veol).Custom assertions: Support for
@phpstan-assert-if-trueand similar annotations is not included in this PR, as mapping variables to types in those contexts is currently difficult and requires further work.