-
Notifications
You must be signed in to change notification settings - Fork 445
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
Support union-types in array_chunk() #2635
Conversation
@@ -46,7 +48,8 @@ public function getTypeFromFunctionCall(FunctionReflection $functionReflection, | |||
$preserveKeys = false; | |||
} | |||
|
|||
if ($lengthType instanceof ConstantIntegerType && $lengthType->getValue() < 1) { |
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.
primary motivation was getting rid of the instanceof ConstantIntegerType
parts in this extension
throw new ShouldNotHappenException(); | ||
} | ||
|
||
$results[] = $constantArray->chunk($finiteType->getValue(), $preserveKeys); |
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 might get slow when big integer ranges are involved for $length
. any opinions about possible limits or different implementations - this might also be rare, as we only do it when $preserveKey
is a known boolean and the array is constant at the same time.
@@ -0,0 +1,18 @@ | |||
<?php declare(strict_types = 1); | |||
|
|||
namespace ArrayChunkPhp8; |
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.
failling tests before PR:
This pull request has been marked as ready for review. |
$results = []; | ||
foreach ($constantArrays as $constantArray) { | ||
foreach ($lengthType->getFiniteTypes() as $finiteType) { | ||
if (!$finiteType instanceof ConstantIntegerType || $finiteType->getValue() < 1) { |
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 a courageous assumption. I wouldn't throw ShouldNotHappenException
, it can break right after https://github.com/phpstan/phpstan-src/pull/2642/files gets merged.
So instead it'd be better to return the default return type, instead of crashing.
Also, I'd limit the possibilities to a small number of finite types, rather than have a performance problem on our hands. It can get rather big.
Also - you need to test this with $lengthType
of infinite interval like int<1, max>
or int<50, max>
.
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 you are
2985733
to
e11dd99
Compare
/** @var array{a: 0, b?: 1, c: 2, d: 3} $arr */ | ||
assertType('array{0: array{0: 0, 1?: 1|2, 2?: 2|3, 3?: 3}, 1?: array{0?: 2|3, 1?: 3}}|array{array{0}, array{0?: 1|2, 1?: 2}, array{0?: 2|3, 1?: 3}, array{0?: 3}}', array_chunk($arr, $oneToFour)); | ||
assertType('non-empty-list<non-empty-list<0|1|2|3>>', array_chunk($arr, $tooBig)); |
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 can see the FINITE_TYPES_LIMIT
in action
Thank you. |
No description provided.