Fix bug #10862 #3065

greew · 2024-05-12T10:19:19Z

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

This should fix bug phpstan/phpstan#10862

I'm not sure that I quite understood the $optional part - also, if you prefer having tests created in another way than each in their separate file (or something else), please let me know! :)

staabm · 2024-05-12T11:15:19Z

src/Type/Constant/ConstantArrayTypeBuilder.php

@@ -181,6 +182,14 @@ public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $opt
 							$this->nextAutoIndexes[] = $newAutoIndex;
 						}
 					}
+					if (PHP_VERSION_ID >= 80300 && count($this->keyTypes) === 1 && $offsetType->getValue() < 0) {


Usually php feature detects are implemented in PhpVersion class

Great!! It's exactly these kinds of comments, I need!

I'll try and figure out how that's working! 😊

greew · 2024-05-12T12:43:13Z

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.

herndlm · 2024-05-12T12:44:40Z

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.

$phpVersion = PhpVersionStaticAccessor::getInstance();

just directly where you need it apparently. It's already used somewhere in ConstantArrayType

greew · 2024-05-12T12:46:22Z

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.
$phpVersion = PhpVersionStaticAccessor::getInstance();
just directly where you need it apparently. It's already used somewhere in ConstantArrayType

Greeeeat - thank you very much!! 😃

ondrejmirtes · 2024-05-30T14:12:40Z

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).

greew · 2024-06-02T19:44:32Z

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).

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?

ondrejmirtes · 2024-06-03T07:40:46Z

In InitializerExprTypeResolver::getArrayType() you can call a new method on ConstantArrayTypeBuilder after creation that changes how it computes next indexes...

greew · 2024-06-09T15:15:11Z

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.

greew · 2024-06-09T17:48:20Z

Seems like the 7.3 and 7.4 errors test errors were introduced in #3128 ??

staabm · 2024-06-10T06:54:01Z

Seems like the 7.3 and 7.4 errors test errors were introduced in #3128 ??

fixed the build error with #3137

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

ondrejmirtes · 2024-06-10T08:05:23Z

tests/PHPStan/Analyser/Bug10862Test.php

+		if (PHP_VERSION_ID < 80000) {
+			$path = 'bug-10862';
+		}
+		yield from self::gatherAssertTypes(__DIR__ . '/data/' . $path . '.php');


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.

ondrejmirtes · 2024-06-10T08:06:18Z

src/Type/Constant/ConstantArrayTypeBuilder.php

@@ -181,6 +181,14 @@ public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $opt
 							$this->nextAutoIndexes[] = $newAutoIndex;
 						}
 					}
+					if (count($this->keyTypes) === 1 && $offsetType->getValue() < 0) {


Changes to the builder certainly need new tests in ConstantArrayTypeBuilderTest.

ondrejmirtes · 2024-06-10T08:09:50Z

src/Type/Constant/ConstantArrayTypeBuilder.php

@@ -314,4 +322,12 @@ public function isList(): bool
 		return $this->isList->yes();
 	}

+	public function resetNextAutoIndexToZeroIfNegative(): void


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.

staabm reviewed May 12, 2024

View reviewed changes

greew added 8 commits June 10, 2024 10:00

Use PhpVersion class instead

8207377

Replace PhpVersionFactory with PhpVersionStaticAccessor

ad9f716

Fix bug bug-10862

7d590e7

Fixing negative array indexes

CSFixer

1f60275

PHPStan, wrong method and CS Fixer

fce8b15

Fix <PHP8 tests

5e9f5b0

Bugger... Missed one...

2a5d0e1

ondrejmirtes force-pushed the bug-10862 branch from 544c80f to 2a5d0e1 Compare June 10, 2024 08:01

ondrejmirtes requested changes Jun 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug #10862 #3065

Fix bug #10862 #3065

greew commented May 12, 2024

staabm May 12, 2024 •

edited

Loading

greew May 12, 2024

greew commented May 12, 2024

herndlm commented May 12, 2024 •

edited

Loading

greew commented May 12, 2024

ondrejmirtes commented May 30, 2024

greew commented Jun 2, 2024

ondrejmirtes commented Jun 3, 2024

greew commented Jun 9, 2024

greew commented Jun 9, 2024

staabm commented Jun 10, 2024

ondrejmirtes Jun 10, 2024

ondrejmirtes Jun 10, 2024

ondrejmirtes Jun 10, 2024

Fix bug #10862 #3065

Are you sure you want to change the base?

Fix bug #10862 #3065

Conversation

greew commented May 12, 2024

staabm May 12, 2024 • edited Loading

Choose a reason for hiding this comment

greew May 12, 2024

Choose a reason for hiding this comment

greew commented May 12, 2024

herndlm commented May 12, 2024 • edited Loading

greew commented May 12, 2024

ondrejmirtes commented May 30, 2024

greew commented Jun 2, 2024

ondrejmirtes commented Jun 3, 2024

greew commented Jun 9, 2024

greew commented Jun 9, 2024

staabm commented Jun 10, 2024

ondrejmirtes Jun 10, 2024

Choose a reason for hiding this comment

ondrejmirtes Jun 10, 2024

Choose a reason for hiding this comment

ondrejmirtes Jun 10, 2024

Choose a reason for hiding this comment

staabm May 12, 2024 •

edited

Loading

herndlm commented May 12, 2024 •

edited

Loading