Skip to content

Conversation

@dktapps
Copy link
Contributor

@dktapps dktapps commented Nov 14, 2021

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.

@staabm
Copy link
Contributor

staabm commented Nov 15, 2021

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.

@dktapps
Copy link
Contributor Author

dktapps commented Nov 15, 2021

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

@dktapps
Copy link
Contributor Author

dktapps commented Nov 15, 2021

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.

dktapps added a commit to pmmp/PocketMine-MP that referenced this pull request Nov 15, 2021
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.
@johnbillion
Copy link
Contributor

This is indeed true. TIL. https://3v4l.org/o7VWB

@dktapps
Copy link
Contributor Author

dktapps commented Nov 17, 2021

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

@ondrejmirtes
Copy link
Member

This is a hard problem and I don't want to make all of these errors rain on PHPStan's users...

@ZebulanStanphill
Copy link
Contributor

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.

@dktapps
Copy link
Contributor Author

dktapps commented Jan 31, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants