-
Notifications
You must be signed in to change notification settings - Fork 504
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
Update ConstantArrayType::accepts() for lists #3381
Conversation
@@ -39,7 +39,6 @@ public function testRule(): void | |||
[ | |||
'Generator expects value type array{DateTime, DateTime, stdClass, DateTimeImmutable}, array{0: DateTime, 1: DateTime, 2: stdClass, 4: DateTimeImmutable} given.', | |||
74, | |||
'Array does not have offset 3.', |
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.
Since it's decided by the early return about list we don't have the information anymore.
- Not sure if it's an ok loss or we need to keep this (and how)
- Not sure if we need an help "This must be a list"
Your call.
@@ -257,6 +255,8 @@ public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $opt | |||
return; | |||
} | |||
} | |||
|
|||
$this->isList = TrinaryLogic::createNo(); |
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 fix is needed or
will fail.PHPStan was considering that
$this->tuple = [true, true, true];
for ($i = 0; $i < 3; ++$i)
{
$this->tuple[$i] = false;
}
// Here, `$this->tuple` is array{bool, bool, bool} but is not considered as a list
I added a non regression test array-is-list-offset.php which fails without this fix.
The idea is to not touch the list
property if we're in the case
if ($match) {
$this->valueTypes = $valueTypes;
return;
}
were all the constantScalar keys are already matching.
This assertion is false
|
4612d46
to
d6b0d0f
Compare
This pull request has been marked as ready for review. |
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.
Please separate the changes outside of ConstantArrayType::accepts()
to another PR and add proper tests there.
I'd expect new test methods added to ConstantArrayTypeBuilderTest.
d6b0d0f
to
3630205
Compare
3630205
to
0a2e927
Compare
0a2e927
to
13b6747
Compare
With all the existing fix merged, this PR is now minimal. This commit does the fix with a test 13b6747 But as shown by https://github.com/phpstan/phpstan-src/actions/runs/10943488747/job/30383079314?pr=3381 we're getting an error
because of the code
This is because of the unset call which means the array may not be a list in the PHPStan definition. It's easily fixed with an array_values call but
I didn't see an easy way to solve this situation unless PHPStan stop to consider that
and
are the same thing when it comes from the phpdoc. That would require to change phpstan-src/src/PhpDoc/TypeNodeResolver.php Line 973 in e8027e7
to something like
which would do I can try create another PR with such change but I'd like your opinion on this @ondrejmirtes. |
@@ -303,6 +303,10 @@ public function acceptsWithReason(Type $type, bool $strictTypes): AcceptsResult | |||
return AcceptsResult::createFromBoolean(count($type->keyTypes) === 0); | |||
} | |||
|
|||
if ($this->isList()->yes() && $type->isList()->no()) { | |||
return AcceptsResult::createNo(); |
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.
What about $type->isList()->maybe()
? I think that at that point the $result
below should be assigned to createMaybe()
and not createYes()
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 102b53b
but what about the following issue #3381 (comment) ?
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 don't really know what to do about that. Maybe this whole change is a hack and we need to approach it differently.
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 your right and this things is a hack, or at least not the right fix because it's not really related to list.
PHPStan sometimes consider an array is ordered and sometimes not.
An easy reproducer is https://phpstan.org/r/1ffe9cb5-65b1-4a7c-9010-3aa24560b462 vs real world example https://3v4l.org/TqTBn
Maybe ConstantArray::getKeysOrValuesArray
is wrong... or a ordered-array
(or unordered-array
) need to be introduced...
At least this PR helped to find some others bugs.
First fix of phpstan/phpstan#11600
but does not close the issue since the second part is not fixed (and is a hard one...).