-
Notifications
You must be signed in to change notification settings - Fork 566
Implement array_all type specifying for arrow functions #5134
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
base: 2.1.x
Are you sure you want to change the base?
Changes from all commits
1caf455
5b3ffca
4224d8c
baf2d90
f3caba9
da079f8
1e8072a
892da77
1683345
f375cbd
1a78e37
ef20fa9
ce44787
1464a9f
d0fe080
e8d86be
14e0f3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| <?php declare(strict_types = 1); | ||
|
|
||
| namespace PHPStan\Type\Php; | ||
|
|
||
| use PhpParser\Node\Expr; | ||
| use PhpParser\Node\Expr\FuncCall; | ||
| use PhpParser\Node\Expr\Variable; | ||
| use PhpParser\Node\Stmt; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\Analyser\SpecifiedTypes; | ||
| use PHPStan\Analyser\TypeSpecifier; | ||
| use PHPStan\Analyser\TypeSpecifierAwareExtension; | ||
| use PHPStan\Analyser\TypeSpecifierContext; | ||
| use PHPStan\DependencyInjection\AutowiredService; | ||
| use PHPStan\Reflection\FunctionReflection; | ||
| use PHPStan\Type\ArrayType; | ||
| use PHPStan\Type\FunctionTypeSpecifyingExtension; | ||
| use PHPStan\Type\MixedType; | ||
| use PHPStan\Type\Type; | ||
| use function array_find; | ||
| use function count; | ||
| use function is_string; | ||
| use function strtolower; | ||
|
|
||
| #[AutowiredService] | ||
| final class ArrayAllFunctionTypeSpecifyingExtension implements FunctionTypeSpecifyingExtension, TypeSpecifierAwareExtension | ||
| { | ||
|
|
||
| private TypeSpecifier $typeSpecifier; | ||
|
|
||
| public function isFunctionSupported(FunctionReflection $functionReflection, FuncCall $node, TypeSpecifierContext $context): bool | ||
| { | ||
| return strtolower($functionReflection->getName()) === 'array_all' | ||
| && !$context->null(); | ||
| } | ||
|
|
||
| public function specifyTypes(FunctionReflection $functionReflection, FuncCall $node, Scope $scope, TypeSpecifierContext $context): SpecifiedTypes | ||
| { | ||
| $args = $node->getArgs(); | ||
| if (!$context->truthy() || count($args) < 2) { | ||
|
Check warning on line 40 in src/Type/Php/ArrayAllFunctionTypeSpecifyingExtension.php
|
||
| return new SpecifiedTypes(); | ||
| } | ||
|
|
||
| $array = $args[0]->value; | ||
| $callable = $args[1]->value; | ||
| if ($callable instanceof Expr\ArrowFunction) { | ||
| $callableExpr = $callable->expr; | ||
| } elseif ( | ||
| $callable instanceof Expr\Closure && | ||
| count($callable->stmts) === 1 && | ||
| $callable->stmts[0] instanceof Stmt\Return_ && | ||
| isset($callable->stmts[0]->expr) | ||
| ) { | ||
|
Comment on lines
+48
to
+53
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 But it handles also and So I think we could have more case to handle. |
||
| $callableExpr = $callable->stmts[0]->expr; | ||
|
Comment on lines
+49
to
+54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is the right approach, but don't have a better idea atm either
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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.) |
||
| } else { | ||
| return new SpecifiedTypes(); | ||
| } | ||
|
|
||
| $callableParams = $callable->params; | ||
| $specifiedTypesInFuncCall = $this->typeSpecifier->specifyTypesInCondition($scope, $callableExpr, $context)->getSureTypes(); | ||
|
|
||
| if ( | ||
| isset($callableParams[0]) && | ||
| $callableParams[0]->var instanceof Variable && | ||
| is_string($callableParams[0]->var->name) | ||
| ) { | ||
| $valueType = $this->fetchTypeByVariable($specifiedTypesInFuncCall, $callableParams[0]->var->name); | ||
| } | ||
|
|
||
| if ( | ||
| isset($callableParams[1]) && | ||
| $callableParams[1]->var instanceof Variable && | ||
| is_string($callableParams[1]->var->name) | ||
| ) { | ||
| $keyType = $this->fetchTypeByVariable($specifiedTypesInFuncCall, $callableParams[1]->var->name); | ||
| } | ||
|
|
||
| if (isset($keyType) || isset($valueType)) { | ||
| return $this->typeSpecifier->create( | ||
| $array, | ||
| new ArrayType($keyType ?? new MixedType(), $valueType ?? new MixedType()), | ||
| $context, | ||
| $scope, | ||
| ); | ||
| } | ||
|
|
||
| return new SpecifiedTypes(); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<string, list{Expr, Type}> $specifiedTypes | ||
| */ | ||
| private function fetchTypeByVariable(array $specifiedTypes, string $variableName): ?Type | ||
| { | ||
| $specifiedTypeOfKey = array_find( | ||
| $specifiedTypes, | ||
| static fn ($specifiedType) => $specifiedType[0] instanceof Expr\Variable && $specifiedType[0]->name === $variableName, | ||
| ); | ||
|
|
||
| if (isset($specifiedTypeOfKey)) { | ||
| return $specifiedTypeOfKey[1]; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| public function setTypeSpecifier(TypeSpecifier $typeSpecifier): void | ||
| { | ||
| $this->typeSpecifier = $typeSpecifier; | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,191 @@ | ||
| <?php // lint >= 8.4 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this test really depend on php 8.4? I think this can be lowered without problems?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point. it works on 8.3 because we have symfony/polyfills. leave it as is than, thanks. |
||
|
|
||
| namespace ArrayAll; | ||
|
|
||
| use DateTime; | ||
| use DateTimeImmutable; | ||
|
|
||
| use function PHPStan\Testing\assertType; | ||
|
|
||
| class Foo { | ||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test1($array) { | ||
| if (array_all($array, fn ($value) => is_int($value))) { | ||
| assertType("array<int>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
staabm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| assertType("array<mixed>", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test2($array) { | ||
| if (array_all($array, fn ($value, $key) => is_string($key))) { | ||
| assertType("array<string, mixed>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| assertType("array", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test3($array) { | ||
| if (array_all($array, fn ($value, $key) => is_string($key) && is_int($value))) { | ||
| assertType("array<string, int>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| assertType("array<mixed>", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test4($array) { | ||
| if (array_all($array, fn ($value) => is_string($value) && is_numeric($value))) { | ||
| assertType("array<numeric-string>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| assertType("array<mixed>", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test5($array) { | ||
| if (array_all($array, fn ($value) => is_bool($value) || is_float($value))) { | ||
| assertType("array<bool|float>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| assertType("array<mixed>", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test6($array) { | ||
| if (array_all($array, fn ($value) => is_float(1))) { | ||
| assertType("array<mixed>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| assertType("array<mixed>", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test7($array) { | ||
| if (array_all($array, fn ($value) => $value instanceof DateTime)) { | ||
| assertType("array<DateTime>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| assertType("array<mixed>", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test8($array) { | ||
| if (array_all($array, fn ($value) => $value instanceof DateTime || $value instanceof DateTimeImmutable)) { | ||
| assertType("array<DateTime|DateTimeImmutable>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| assertType("array<mixed>", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param list<mixed> $array | ||
| */ | ||
| public function test9($array) { | ||
| if (array_all($array, fn ($value, $key) => is_int($key))) { | ||
| assertType("list<mixed>", $array); | ||
| } else { | ||
| assertType("list<mixed>", $array); | ||
| } | ||
| assertType("list<mixed>", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param non-empty-array<mixed> $array | ||
| */ | ||
| public function test10($array) { | ||
| if (array_all($array, fn ($value, $key) => is_int($key))) { | ||
| assertType("non-empty-array<int, mixed>", $array); | ||
| } else { | ||
| assertType("non-empty-array<mixed>", $array); | ||
| } | ||
| assertType("non-empty-array", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test11($array) { | ||
| if (array_all($array, function ($value) {return is_int($value);})) { | ||
| assertType("array<int>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| assertType("array<mixed>", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test12($array) { | ||
| if (array_all($array, function ($value) {$value = 1; return is_int($value);})) { | ||
| assertType("array<mixed>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| assertType("array<mixed>", $array); | ||
| } | ||
staabm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test13($array) { | ||
| if (array_all($array, function ($value, $key) {return is_int($value) && is_string($key);})) { | ||
| assertType("array<string, int>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| assertType("array<mixed>", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test14($array) { | ||
| if (array_all($array, function ($value, $key) {return;})) { | ||
| assertType("array<mixed>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| assertType("array<mixed>", $array); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed> $array | ||
| */ | ||
| public function test15($array) { | ||
| if (array_all($array, fn ($value) => is_int($value)) === true) { | ||
| assertType("array<int>", $array); | ||
| } else { | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| assertType("array<mixed>", $array); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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 never know whether its
$context->truthy()or$context->true().please add a test with a
=== truecomparison(the escaped mutant at least tells us, its not covered by tests)
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 added a test for the
=== truecomparison, 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.