phpstan-assert with recursive count() results in type loss #2812
Conversation
|
This pull request has been marked as ready for review. |
|
There's a conflict, please rebase :) |
| } elseif (count($items, COUNT_RECURSIVE) === 5) { | ||
| assertType('array{int, int, int, int, int}', $items); |
There was a problem hiding this comment.
hmm.. I copied this over from the above tests, but I am not sure whether thats correct in the recursive case..
There was a problem hiding this comment.
aahh since the type of the list is int its correct (count cannot find more elements by recursing).
There was a problem hiding this comment.
Yeah but it's not going to be correct if one of the elements is an array in which case this assumption should not proceed.
There was a problem hiding this comment.
do you think its worth checking for the array case, or is c5beb9c good enough?
There was a problem hiding this comment.
instead of adding inference of the count("non-recursive-type", COUNT_RECURSIVE)-case I think it would make more sense to create a new rule which errors when COUNT_RECURSIVE is used on a type which is not recursive?
| function recursiveCount(array $items):void { | ||
| assertType('list<array<int>|int>', $items); | ||
| if (count($items, COUNT_RECURSIVE) === 3) { | ||
| assertType('non-empty-list<array<int>|int>', $items); |
There was a problem hiding this comment.
we can't judge for the constant array case when recursive - therefore separate expectations here
|
This pull request has been marked as ready for review. |
src/Analyser/TypeSpecifier.php
Outdated
| $isNormalCount = (new ConstantIntegerType(COUNT_NORMAL))->isSuperTypeOf($mode)->yes(); | ||
|
|
||
| if (!$isNormalCount) { | ||
| $isNormalCount = $argType->getIterableValueType()->isScalar()->yes(); |
There was a problem hiding this comment.
This is too restrictive. For example objects cannot be counted recursively, not even countables: https://3v4l.org/3GCpt
So you need to ask isArray and work with uncertainty - TrinaryLogic. Think about the result when it's definitely not an array, when it can be an array and when it's definitely an array.
| $isNormalCount = (new ConstantIntegerType(COUNT_NORMAL))->isSuperTypeOf($mode)->yes(); | ||
|
|
||
| if (!$isNormalCount) { | ||
| $isNormalCount = $argType->getIterableValueType()->isArray()->no(); |
There was a problem hiding this comment.
I think this still misses some scenarios.
We should have different results for $isNormalCount yes/maybe/no.
And also different results for $argType->getIterableValueType()->isArray() yes/maybe/no.
If you create a matrix:
no + no
no + maybe
no + yes
maybe + no
maybe + maybe
maybe + yes
yes + no
yes + maybe
yes + yes
You'll see that some scenarios aren't covered correctly.
There was a problem hiding this comment.
thanks, I added tests covering this testmatrix in
tests/PHPStan/Analyser/data/count-maybe.php
| function doBar3(float $notCountable, float $invalidMode): void | ||
| { | ||
| if (count($notCountable, $invalidMode) > 0) { | ||
| assertType('float', $notCountable); |
There was a problem hiding this comment.
should we resolve to a *ERROR* on invalid mode? I was not sure, as we already have a error at the count level because of the invalid argument type...?
staabm
force-pushed
the
count-fix
branch
2 times, most recently
from
de4d77b
to
f831bf4
Compare
|
This pull request has been marked as ready for review. |
|
squashed, rebased against 1.11 and re-targetted the PR. not sure why CI thinks there is a merge commit contained |
Infer constant array type from specifying recursive count() on a list type fix inference with smaller/smaller equal on recursive count() add another test fix fix more tests more tests support recursive count on non-recursive element count recursive based on array type more tests fix test lists fix built support countables more assertions cs
|
Thank you. |
simliar to #2811