-
Couldn't load subscription status.
- Fork 544
Fix bug #10862 #3065
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
Fix bug #10862 #3065
Conversation
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.
Usually php feature detects are implemented in PhpVersion class
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.
Great!! It's exactly these kinds of comments, I need!
I'll try and figure out how that's working! 😊
I'm not sure this is the right solution, but since ConstantArrayTypeBuilder is not created by DI, I don't know how else to insert the PhpVersion class into the class.
I'm not sure this is the right solution, but since
ConstantArrayTypeBuilderis not created by DI, I don't know how else to insert thePhpVersionclass into the class.
$phpVersion = PhpVersionStaticAccessor::getInstance();
just directly where you need it apparently. It's already used somewhere in ConstantArrayType
I'm not sure this is the right solution, but since
ConstantArrayTypeBuilderis not created by DI, I don't know how else to insert thePhpVersionclass into the class.$phpVersion = PhpVersionStaticAccessor::getInstance();just directly where you need it apparently. It's already used somewhere in
ConstantArrayType
Greeeeat - thank you very much!! 😃
I feel like this fix should be done differently.
Because PHPStan has a wrong assumption even on previous PHP versions: https://phpstan.org/r/492b583a-dc4b-4be2-a4c9-2232e039c19c (2nd key in $a should be -3 on all PHP versions, not just PHP 8.3).
Only on PHP 8.3 $b should be array{-4: 1, -3: 2}, otherwise array{-4: 1, 0: 2}.
My suggestion: Fix the behaviour in ConstantArrayTypeBuilder regardless of PHP version. And then in InitializerExprTypeResolver::getArrayType() (DI is available there, hooray!) do the condition for PHP < 8.3 (so that the 2nd key is 0, not -3).
I feel like this fix should be done differently.
Because PHPStan has a wrong assumption even on previous PHP versions: https://phpstan.org/r/492b583a-dc4b-4be2-a4c9-2232e039c19c (2nd key in
$ashould be-3on all PHP versions, not just PHP 8.3).Only on PHP 8.3
$bshould bearray{-4: 1, -3: 2}, otherwisearray{-4: 1, 0: 2}.My suggestion: Fix the behaviour in ConstantArrayTypeBuilder regardless of PHP version. And then in
InitializerExprTypeResolver::getArrayType()(DI is available there, hooray!) do the condition for PHP < 8.3 (so that the 2nd key is 0, not -3).
Great - thanks! And nice caught with the direct initialization difference in the old PHP versions!
Anyhow - I can't seem to figure out how to implement the change in the InitializerExprTypeResolver::getArrayType() method. It is still the ConstantArrayTypeBuilder that handles the calculation of which nextAutoIndexes to use, and I can't change this data from within InitializerExprTypeResolver.
If I'm not to change the visibility of the methods of ConstantArrayTypeBuilder, can someone give me a hint of how to do this?
In InitializerExprTypeResolver::getArrayType() you can call a new method on ConstantArrayTypeBuilder after creation that changes how it computes next indexes...
OK, I've added tests for both pre-8.0, 8.0+ and 8.3+ as requested in phpstan/phpstan#10862 (comment).
As requested earlier on, I've also moved the PHP version check to the InitializerExprTypeResolver class, but also had to add a PHP version check in ConstantArrayType to take care of the PHP >=8.0 < 8.3 findings.
Seems like the 7.3 and 7.4 errors test errors were introduced in #3128 ??
This fix ensures that we use correct array indicies in >= PHP8.3. (See https://www.php.net/manual/en/migration83.incompatible.php#migration83.incompatible.core.negative-index-to-empty-array). We ensure, that if - PHP version is 8.3+ - AND the current element to add is the first element - AND the key is less than 0 (it's negative) then we set the newAutoIndexes to the negative value + 1. Added tests to show correctness for both < PHP8.3 and >= PHP8.3
Fixing negative array indexes
544c80f to
2a5d0e1
Compare
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'd prefer if the conditions contained the whole yield from call instead of concatenating the path dynamically like this. It will make the path clickable in IDE.
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.
Changes to the builder certainly need new tests in ConstantArrayTypeBuilderTest.
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'd thought about this method that it should be set once after creation and not before every setOffsetValueType call. It should change the behaviour for all the calls, not just for the next one. Instead of this logic, this method should switch a bool property so that the computation of nextAutoIndexes changes based on that.
Hi @greew, are you still interested in finishing in PR (and doing it on 2.1.x now) ?
@VincentLanglet Uhmm... I'll see if I can get the time within the next weeks :)
I had pretty much forgotten about this PR :)
This fix ensures that we use correct array indicies in >= PHP8.3.
(See https://www.php.net/manual/en/migration83.incompatible.php#migration83.incompatible.core.negative-index-to-empty-array).
We ensure, that if
then we set the newAutoIndexes to the negative value + 1.
Added tests to show correctness for both < PHP8.3 and >= PHP8.3
This should fix bug phpstan/phpstan#10862
I'm not sure that I quite understood the
$optionalpart - also, if you prefer having tests created in another way than each in their separate file (or something else), please let me know! :)