-
Notifications
You must be signed in to change notification settings - Fork 545
Acknowledge that array<string, something> may still have integer keys #769
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
Conversation
could you create a snippet on phpstan.org to show a example when a array<string, ...> can contain an int?
Opening this to get CI feedback since it's a pain to run the tests on Windows.
agree have similar issues on my end.
@staabm it's really easy to reproduce. Use a numeric string as an array key and it gets casted to int.
I limited the change to getIterableKeyType() since iteration seems to be the only place where it's actually a problem. The rest of the time the key casting doesn't matter, but for iteration it's a major pain in the ass and the cause of a significant percentage of PM crashes.
On local, I changed this to use a non-benevolent union type, and discovered 25 places in PM where this problem could legitimately occur. I really think this should be treated with non-benevolence.
also, class-string->isNumericString() is maybe? wtf?
this is not as good as phpstan/phpstan-src#769 (e.g. array_key_first()/array_key_last() aren't covered by this, nor is array_rand()) but it does eliminate the most infuriating cases where this usually crops up.
This is indeed true. TIL. https://3v4l.org/o7VWB
If you use strict_types=1 it will bite you on the ass at some point. It's very annoying to me that PHPStan doesn't report the problem. I ended up writing a custom rule to ban direct iteration on arrays with string keys (see pmmp/PocketMine-MP@269231c) but it doesn't completely solve the problem (e.g. array_key_first/last and array_rand() will still bite me at some point).
97c2f21 to
a03a78f
Compare
This is a hard problem and I don't want to make all of these errors rain on PHPStan's users...
It would be good to at least acknowledge this shortcoming in the docs somewhere though, right?
@dktapps Would you consider making your custom rule available as a separate extension? I might want to use it in my own project.
It would be good to at least acknowledge this shortcoming in the docs somewhere though, right?
@dktapps Would you consider making your custom rule available as a separate extension? I might want to use it in my own project.
I guess I can do that.
This "feature" is extremely annoying to me, more so because PHPStan will complain if I cast a key that came from iterating over an
array<string, something>.Personally, I think this should not be benevolent at all - it's bit me on the ass more times than I can count, and I would much appreciate if PHPStan reported it properly.
But in keeping with the rest of the code, and to just shut it up about useless casts, I opted for the BUT.
Opening this to get CI feedback since it's a pain to run the tests on Windows.