-
Notifications
You must be signed in to change notification settings - Fork 461
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think its worth checking for the array case, or is c5beb9c good enough?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...?
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. |
@staabm @ondrejmirtes I don't know if you are aware of this, but this exact commit also fixes this bug: /** @param array<mixed>|false $a */
function foo($a): void
{
while (count($a) > 0) {
array_shift($a);
}
} Before this commit: After this commit: I thought it was worth bringing this up in case it's a side effect. Maybe we should add it as a test case? |
I will have a look. Thanks for noticing |
simliar to #2811