Skip to content
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

Validate array function callback #555

Merged

Conversation

BackEndTea
Copy link
Contributor

Fixes phpstan/phpstan#3883

This adds type checks for callables on array_map, array_reduce and array_walk.

Currently array_filter is not checked, as the params for the callable depend on the mode param, so that may need an extension of some kind.

This alos fixes new issues found by these checks

stubs/array.stub Outdated Show resolved Hide resolved
src/Parallel/ParallelAnalyser.php Outdated Show resolved Hide resolved
tests/PHPStan/Rules/Functions/data/array_map.php Outdated Show resolved Hide resolved
@BackEndTea BackEndTea force-pushed the validate-array-function-callback branch from af19c52 to 061b13e Compare June 12, 2021 11:23
@ondrejmirtes ondrejmirtes force-pushed the validate-array-function-callback branch 2 times, most recently from 827de8a to 989ea3f Compare June 13, 2021 12:32
@ondrejmirtes
Copy link
Member

Hi, thank you for this. I pushed some commits with improvements. The reason why I postponed doing this is because most functions seem pretty complicated to be described with just generics.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be worth finishing this and merging this, I'd like more examples of these functions that would benefit from this. Like usort() etc.

@ondrejmirtes ondrejmirtes force-pushed the validate-array-function-callback branch from de33a9d to 5b3fe9b Compare June 13, 2021 13:34
@BackEndTea
Copy link
Contributor Author

Hi, thank you for this. I pushed some commits with improvements. The reason why I postponed doing this is because most functions seem pretty complicated to be described with just generics.

True, looking into it the more commonly used functions like array_map, need more than just generics.

Prehaps some kind of DynamicParameterTypeExtension? though i'm not sure how to go about implementing that, that should be something for another PR.

@BackEndTea
Copy link
Contributor Author

I've added the stubs for usort uasort and uksort. And removed array_map, as it is incorrect.

StubPhpDocProvider::findFunctionPhpDoc() would need a similar logic like findMethodPhpDoc already has - the $positionalParameterNames parameter and calling changeParameterNamesByMapping.

I'm not sure how i should go about this

@ondrejmirtes
Copy link
Member

The way it works is:

  1. The stub has parameters $a and $b. PHPStan notices that $a has string and $b has int.
  2. The real function has parameters $x and $y. Because $x is the first one, the type for $a should be used for $x. Because $y is the second one, the type for $b should be used. So $x has string and $y has int.

How to implement it here:

  1. Edit the stub so that the parameters are always different from PHP. That way you can easily test the logic.
  2. Make findFunctionPhpDoc also work with $positionalParameterNames same as findMethodPhpDoc does it.
  3. Observe how $positionalParameterNames are passed into the findMethodPhpDoc method and do the same thing for findFunctionPhpDoc.
  4. The tests testing the stubs should now pass.

@BackEndTea
Copy link
Contributor Author

The (extra) failures on PHP 8 are due to param names

@ondrejmirtes
Copy link
Member

The (extra) failures on PHP 8 are due to param names

The tests need to account for this with PHP_VERSION_ID >= 80000 ternary operators filling in the correct names. Using sprintf() might be best for this. Thanks.

@BackEndTea
Copy link
Contributor Author

I can't figure out why for that one job the tests would fail.

@ondrejmirtes ondrejmirtes merged commit a9dd9af into phpstan:master Jun 15, 2021
@ondrejmirtes
Copy link
Member

I'll look into it. Thanks!

@BackEndTea
Copy link
Contributor Author

Thanks for the merge and the help @ondrejmirtes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants