Fix NumericString to array key

[VincentLanglet]

Closes https://github.com/phpstan/phpstan/issues/4163 Fixes https://github.com/phpstan/phpstan/issues/4671 (already closed with wrong result) Closes https://github.com/phpstan/phpstan/issues/8592 Closes https://github.com/phpstan/phpstan/issues/11390

The only "drawback" (as shown by https://github.com/phpstan/phpstan-src/actions/runs/10405045183?pr=3326 for the snippet https://phpstan.org/r/c218f006-1fe3-4891-9ab7-9a550cd911b8) will be that

class HelloWorld
{
    /** @var array<int, string> */
	public array $enum;

    protected function setEnum(): void
    {
		$this->enum[(string) mt_rand()] = mt_rand();
	}

will now report an error

Property HelloWorld::$enum (array<int, string>) does not accept array<int|string, string>.

because

• mt_rand() is int
• (string) mt_rand() is numeric-string (cause int-string doesn't exists)
• (string) mt_rand() as array key is int|numeric-string

[phpstan-bot]

This pull request has been marked as ready for review.

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

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]

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

[VincentLanglet]

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]

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]

Thank you.

[VincentLanglet]

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

[ondrejmirtes]

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

[VincentLanglet]

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]

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