-
Notifications
You must be signed in to change notification settings - Fork 525
Fix item-type in list to constant-array conversion with count() #3309
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
Conversation
1e63d51
to
afe2113
Compare
This pull request has been marked as ready for review. |
1133d1a
to
7574911
Compare
src/Analyser/TypeSpecifier.php
Outdated
$valueTypesBuilder = ConstantArrayTypeBuilder::createEmpty(); | ||
for ($i = 0; $i < $constantType->getValue(); $i++) { | ||
$offsetType = new ConstantIntegerType($i); | ||
$valueTypesBuilder->setOffsetValueType($offsetType, $innerType->getOffsetValueType($offsetType)); |
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 doing intersect with HasOffsetType instead of the for loop?
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 tried using HasOffsetValueType
but it still requires the loop
- $valueTypesBuilder = ConstantArrayTypeBuilder::createEmpty();
+ $hasOffsetTypes = [];
for ($i = 0; $i < $constantType->getValue(); $i++) {
- $offsetType = new ConstantIntegerType($i);
- $valueTypesBuilder->setOffsetValueType($offsetType, $argType->getOffsetValueType($offsetType));
+ $hasOffsetTypes[] = new HasOffsetValueType(new ConstantIntegerType($i), $argType->getOffsetValueType(new ConstantIntegerType(
$i)));
}
- $valueTypes = $this->create($exprNode->getArgs()[0]->value, $valueTypesBuilder->getArray(), $context, false, $scope, $rootExpr);
+ $valueTypes = $this->create($exprNode->getArgs()[0]->value, TypeCombinator::intersect($argType, ...$hasOffsetTypes), $context, false,
$scope, $rootExpr);
and it leads to less precise types, I think (at least less readable)
--- Expected
+++ Actual
@@ @@
-'array{ListCount\A, ListCount\A, ListCount\A}'
+'list<ListCount\A>&hasOffsetValue(0, ListCount\A)&hasOffsetValue(1, ListCount\A)&hasOffsetValue(2, ListCount\A)'
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.
to drop the loop we could maybe use new HasOffsetValueType(IntegerRangeType::fromInterval(0, $constantType->getValue(), $offsetValueType)
but this doesn't work because $offsetValueType is different per offset
This pull request has been marked as ready for review. |
7574911
to
18d20a8
Compare
Conflict :) |
18d20a8
to
95d749e
Compare
This pull request has been marked as ready for review. |
if ( | ||
$isNormalCount->yes() | ||
&& $type->isList()->yes() | ||
&& $sizeType instanceof ConstantIntegerType |
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.
Note to future me: Misses int-range support?
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.
here we go: #3335
Thank you! |
before this PR we were loosing precision in this process as item-types were kind of generalized before.
I guess the original implementation was done before
Type->getOffsetValueType()
was a thing.