Skip to content
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

Reindex constant arrays via shuffle #1438

Merged
merged 2 commits into from Jul 1, 2022
Merged

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Jun 17, 2022

Closes phpstan/phpstan#6138

While this is an improvement I'm not a 100% happy with it, because the constant array still dictates the same order of values after shuffle, which is not correct. But generalizing it to a normal array would loose the key order I presume? Better ideas?

@herndlm herndlm marked this pull request as ready for review June 17, 2022 19:17
@ondrejmirtes
Copy link
Member

I'd say the array should be general (ArrayType) after shuffle because the key => value type association is no longer maintained - ConstantArrayType is no longer correct.

@herndlm
Copy link
Contributor Author

herndlm commented Jun 17, 2022

I had the same thought but then we loose the keys I guess. But OK, I'll look into that tomorrow

@herndlm herndlm changed the base branch from 1.7.x to 1.8.x June 30, 2022 12:52
@herndlm
Copy link
Contributor Author

herndlm commented Jun 30, 2022

"tomorrow" = ~2 weeks in the parallel universe I was living :)

should be adapted. shuffle leads now to a generic array and re-indexes the keys. The only thing that was lost is the order of the keys which is always 0, 1, 2, .. but I guess we can't retain them at the moment.

@herndlm
Copy link
Contributor Author

herndlm commented Jun 30, 2022

Not a 100% happy with this, but I can't come up with something better. Basically something like a shuffled list / shuffled constant array would be needed, but that feels very much like an edge case :)

@ondrejmirtes ondrejmirtes merged commit addecc7 into phpstan:1.8.x Jul 1, 2022
@ondrejmirtes
Copy link
Member

Thank you.

@herndlm herndlm deleted the fix-6138 branch July 1, 2022 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants