Fix NumericString to array key #3326

VincentLanglet · 2024-08-15T12:15:21Z

Closes phpstan/phpstan#4163
Fixes phpstan/phpstan#4671 (already closed with wrong result)
Closes phpstan/phpstan#8592
Closes phpstan/phpstan#11390
Closes phpstan/phpstan#12413

src/Type/Constant/ConstantArrayType.php

VincentLanglet · 2024-08-15T12:27:16Z

tests/PHPStan/Rules/Variables/IssetRuleTest.php

@@ -233,10 +233,7 @@ public function testBug4671(): void
 	{
 		$this->treatPhpDocTypesAsCertain = true;
 		$this->strictUnnecessaryNullsafePropertyFetch = false;
-		$this->analyse([__DIR__ . '/data/bug-4671.php'], [[
-			'Offset numeric-string on array<string, string> in isset() does not exist.',


This was wrong

VincentLanglet · 2024-08-15T13:31:04Z

src/Type/Constant/ConstantArrayType.php

 		if ($offsetType instanceof UnionType) {
 			$results = [];
 			foreach ($offsetType->getTypes() as $innerType) {
-				$results[] = $this->hasOffsetValueType($innerType);
+				$results[] = $this->recursiveHasOffsetValueType($innerType);


With the previous logic, toArrayKey was again called, creating a new UnionType for numeric string and an infinite loop.

phpstan-bot · 2024-08-15T13:31:34Z

This pull request has been marked as ready for review.

phpstan-bot · 2024-08-15T15:06:25Z

This pull request has been marked as ready for review.

ondrejmirtes · 2024-08-15T15:50:33Z

Hey, this area is really tricky. This isn't the right fix, I'd tackle this completely differently. Your change would break too many scenarios.

My idea is: Introduce non-int-string type, and with an optional flag being false by default, I'd interpret array<string, X> as array<non-int-string, X>. Which would make the analysis stricter and correct. That would be a step in the right direction of fixing arrays-with-string-keys issues.

We can't fix numeric-string. Asking is_numeric() does not tell us anything about what's going to happen inside array keys. That's currently broken design. Because only "integer strings" are cast to integers in arrays, but numeric strings also include "float strings"

ondrejmirtes · 2024-08-15T15:57:40Z

I'll give these changes a bit more thought, stay tuned and thanks, I appreciate it 😊

VincentLanglet · 2024-08-15T16:06:40Z

Hey, this area is really tricky.

I agree

This isn't the right fix, I'd tackle this completely differently. Your change would break too many scenarios.

Which one ? Looking at tests and issues, it seems to only solve issues without breaking tests.

I was worried too that it would open a rabbit hole, but according to the CI it seems not.

I agree it would be the case if I wanted to change the behavior for string but here it's only numeric-string.
And it seems weird that currently the intersection of string->toArrayKey and numeric-string->toArrayKey is NEVER.

My idea is: Introduce non-int-string type, and with an optional flag being false by default, I'd interpret array<string, X> as array<non-int-string, X>. Which would make the analysis stricter and correct. That would be a step in the right direction of fixing arrays-with-string-keys issues.

I definitely agree on this non-int-string type, but here I think it's different since we're on numeric-string.

Currently string->toArrayKey is considered as string but numeric-string->toArrayKey is considered as int.
It seems better to me to consider that numeric-string->toArrayKey might still be a string,
I especially didn't touch to the string->toArrayKey behavior to avoid the tricky part.

We can't fix numeric-string. Asking is_numeric() does not tell us anything about what's going to happen inside array keys. That's currently broken design. Because only "integer strings" are cast to integers in arrays, but numeric strings also include "float strings"

That especially because numeric-string can include float string that the arrayKey shouldn't be considered as only int.

Imho, 'numeric-string' should be considered as int|numeric-string as array key,
while int-string and non-int-string will be considered respectively as int and string as array key the day a native php is_int_string will be introduced (I just tried an RFC on php internals about such functions to know if it could be introduced in next php version).

I'll give these changes a bit more thought, stay tuned and thanks, I appreciate it 😊

Sure, I'm not in hurry.

VincentLanglet · 2025-01-26T11:22:32Z

Hi, I rebased the branch and really would like to move forward this PR if possible @ondrejmirtes. Can we talk again about it ?

I do agree that one day we will need non-int-string. But I feel like it’s not the same topic. 
Currently PHPStan consider that numeric-string is the same than int-string, but it’s not. Numeric-string should be considered as int-string|float-string and while int-string are casted to int when used as array key, float-string are still float-string when used as array-key.  

So NumericString->toArrayKey should be Int|Numeric-string, and not just Int.

At first sight, with all the existing non-regression tests, this change seems to fix a lot of existing issues without introducing a regresion.
Do you prefer this PR to target 2.1.x ?

ondrejmirtes

I'm willing to try this again and see what the impact is on real-world projects.

ondrejmirtes · 2025-05-05T15:32:15Z

src/Type/IntersectionType.php

@@ -1029,7 +1029,10 @@ public function toArray(): Type
 	public function toArrayKey(): Type
 	{
 		if ($this->isNumericString()->yes()) {
-			return new IntegerType();
+			return new UnionType([


This should be TypeCombinator::union() instead of new UnionType to ensure type normalization.

ondrejmirtes · 2025-05-05T16:00:30Z

Thank you.

VincentLanglet · 2025-05-05T16:05:15Z

You're welcome, please feel free to ping me after your tests if you need extra work on this and I can help.

ondrejmirtes · 2025-05-06T12:58:44Z

This is causing infinite recursion on one of the test projects. I'm investigating details.

VincentLanglet · 2025-05-06T13:03:39Z

This is causing infinite recursion on one of the test projects. I'm investigating details.

I had one infinite loop with PHPStan codebase/tests which I fixed this way https://github.com/phpstan/phpstan-src/pull/3326/files#r1718409129 if it can help for the other one you found.

ondrejmirtes · 2025-05-14T07:07:51Z

I'm sorry, it wasn't fault of this PR but a different one.

VincentLanglet commented Aug 15, 2024

View reviewed changes

src/Type/Constant/ConstantArrayType.php Outdated Show resolved Hide resolved

VincentLanglet force-pushed the numeric-string-array-key branch from e170a71 to c42d877 Compare August 15, 2024 12:24

VincentLanglet commented Aug 15, 2024

View reviewed changes

VincentLanglet force-pushed the numeric-string-array-key branch 2 times, most recently from 41feb8b to 3ed0134 Compare August 15, 2024 13:10

VincentLanglet commented Aug 15, 2024

View reviewed changes

VincentLanglet marked this pull request as ready for review August 15, 2024 13:31

VincentLanglet marked this pull request as draft August 15, 2024 13:35

VincentLanglet marked this pull request as ready for review August 15, 2024 15:06

VincentLanglet changed the base branch from 1.11.x to 1.12.x September 19, 2024 15:37

VincentLanglet force-pushed the numeric-string-array-key branch from 01b2d1a to 4d7af2a Compare September 19, 2024 15:40

VincentLanglet mentioned this pull request Sep 22, 2024

Type casted string array key is recognized as a int array key phpstan/phpstan#11680

Open

devnix mentioned this pull request Jan 13, 2025

Inconsistent array key phpstan/phpstan#12413

Closed

VincentLanglet force-pushed the numeric-string-array-key branch from 2957e0a to 205d0c3 Compare January 26, 2025 11:00

VincentLanglet added 4 commits March 18, 2025 08:55

Fix NumericString to array key

c93e80c

Add non-regression test

b0bfea1

Fix old php versions

1b58c57

Fix rebase

27a9b38

VincentLanglet force-pushed the numeric-string-array-key branch from 205d0c3 to 27a9b38 Compare March 18, 2025 07:55

ondrejmirtes requested changes May 5, 2025

View reviewed changes

Review

b24c50f

ondrejmirtes merged commit 36b3925 into phpstan:1.12.x May 5, 2025
136 checks passed

ondrejmirtes mentioned this pull request May 5, 2025

numeric-string as array-key is int|string and not int only phpstan/phpstan#4163

Closed

This was referenced May 5, 2025

numeric-string becomes int when used as array key phpstan/phpstan#8592

Closed

Array key not reconized as int|string phpstan/phpstan#11390

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix NumericString to array key #3326

Fix NumericString to array key #3326

VincentLanglet commented Aug 15, 2024 •

edited

Loading

VincentLanglet Aug 15, 2024

VincentLanglet Aug 15, 2024

phpstan-bot commented Aug 15, 2024

phpstan-bot commented Aug 15, 2024

ondrejmirtes commented Aug 15, 2024

ondrejmirtes commented Aug 15, 2024

VincentLanglet commented Aug 15, 2024

VincentLanglet commented Jan 26, 2025

ondrejmirtes left a comment

ondrejmirtes May 5, 2025

ondrejmirtes commented May 5, 2025

VincentLanglet commented May 5, 2025

ondrejmirtes commented May 6, 2025

VincentLanglet commented May 6, 2025

ondrejmirtes commented May 14, 2025

Fix NumericString to array key #3326

Fix NumericString to array key #3326

Conversation

VincentLanglet commented Aug 15, 2024 • edited Loading

VincentLanglet Aug 15, 2024

Choose a reason for hiding this comment

VincentLanglet Aug 15, 2024

Choose a reason for hiding this comment

phpstan-bot commented Aug 15, 2024

phpstan-bot commented Aug 15, 2024

ondrejmirtes commented Aug 15, 2024

ondrejmirtes commented Aug 15, 2024

VincentLanglet commented Aug 15, 2024

VincentLanglet commented Jan 26, 2025

ondrejmirtes left a comment

Choose a reason for hiding this comment

ondrejmirtes May 5, 2025

Choose a reason for hiding this comment

ondrejmirtes commented May 5, 2025

VincentLanglet commented May 5, 2025

ondrejmirtes commented May 6, 2025

VincentLanglet commented May 6, 2025

ondrejmirtes commented May 14, 2025

VincentLanglet commented Aug 15, 2024 •

edited

Loading