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

Improve count() narrowing of constant arrays #3709

Open
wants to merge 6 commits into
base: 2.1.x
Choose a base branch
from

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Dec 4, 2024

Generalizes the already existing solution (wow, this is way more complicated than I thought!) and uses it outside of union types as well. I kind of had to make the "isNormalCount" determination a bit more dry, which unfortunately made the diff worse..

I did do a performance comparison with hyperfine running make phpstan and didn't notice any changes, but my system is not the best.

Closes phpstan/phpstan#12190
Closes phpstan/phpstan#3631

@herndlm
Copy link
Contributor Author

herndlm commented Dec 4, 2024

I know, I changed that narrow* method to return Type instead of SpecifiesTypes. Maybe I'll change this back again tomorrow.

@herndlm herndlm force-pushed the constant-array-count-narrowing branch 2 times, most recently from 891412d to 22ef97b Compare December 5, 2024 09:47
@herndlm
Copy link
Contributor Author

herndlm commented Dec 5, 2024

I know, I changed that narrow* method to return Type instead of SpecifiesTypes. Maybe I'll change this back again tomorrow.

adapted. looks better now IMO. The master plan is to move more common things for both count() == xx and count > xx specification into the now renamed specifyTypesForCountFuncCall(). Would also be great if turnListIntoConstantArray() goes away completely or is only used internally once... But for here this is more than enough IMO.

@staabm what do you think about this?

@staabm
Copy link
Contributor

staabm commented Dec 5, 2024

what do you think about this?

this PR looks great, thanks for cleaning up after me :)

@herndlm herndlm force-pushed the constant-array-count-narrowing branch 2 times, most recently from 5964211 to 75991d1 Compare December 5, 2024 20:19
@herndlm
Copy link
Contributor Author

herndlm commented Dec 23, 2024

Should stuff like this be based on 1.12.x or latest dev branch? I sometimes have issues deciding 😅

@ondrejmirtes
Copy link
Member

Big changes to TypeSpecifier I prefer on 2.1.x, to avoid conflicts.

@herndlm herndlm changed the base branch from 1.12.x to 2.1.x December 24, 2024 17:25
@herndlm herndlm force-pushed the constant-array-count-narrowing branch 2 times, most recently from a84121e to 8092d6b Compare December 24, 2024 18:08
@herndlm herndlm force-pushed the constant-array-count-narrowing branch from 8092d6b to f340663 Compare December 24, 2024 18:20
@herndlm
Copy link
Contributor Author

herndlm commented Dec 24, 2024

Thx, makes sense. And good call, there was a conflict.. Rebased this and also other of my PRs on 2.1.x and dealt with conflicts 😊

@herndlm herndlm force-pushed the constant-array-count-narrowing branch from c8b87e5 to c0e6269 Compare December 25, 2024 21:42
@herndlm herndlm force-pushed the constant-array-count-narrowing branch from fc54113 to d92b6f0 Compare December 26, 2024 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants