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

Consider optional keys in ConstantArrayType removeFirst and removeLast #1352

Merged
merged 1 commit into from
May 27, 2022

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented May 26, 2022

... and get rid of getConstantArrays in NodeScopeResolver for array_pop and array_shift.

This seems to be also fixing problems with the optional keys. green test cases from this PR for comparison with the latest 1.7.x state which is broken IMO
array_pop: https://phpstan.org/r/99745329-2125-46a3-8fe6-ea4afdfc754e
array_shift: https://phpstan.org/r/751ff62d-d672-4ef8-9db1-5abbb6cf4a15

The added tests show that apparently ArrayPopFunctionReturnTypeExtension and ArrayShiftFunctionReturnTypeExtension are not working correctly either ("should be" comments), but I'd like to look at that in another PR if that's ok. Maybe a bug somewhere else in ConstantArrayType itself 🤔 But I also have an idea for replacing those getConstantArrays calls as well.

My main motivation is to prepare stuff I can use in the ConstantArrayType::slice adaption. But now, since I learned that I can replace some of the potentially badly-scaling array gather & modify in a loop & unify code parts in general, I want to get rid of what I can :D

@herndlm herndlm force-pushed the remove-first-last branch 3 times, most recently from 2c07251 to e113275 Compare May 26, 2022 19:01
@herndlm
Copy link
Contributor Author

herndlm commented May 26, 2022

should be ready. there were some random unrelated CI errors I never saw before, I'll force-push once more to retry and leave it then, as it is, to not burden the system more in case it is overloaded or whatever
UPDATE: sorry, just renamed something which bothered me :)

@herndlm herndlm marked this pull request as ready for review May 26, 2022 19:05
@herndlm herndlm marked this pull request as draft May 26, 2022 21:10
@herndlm herndlm force-pushed the remove-first-last branch from e113275 to b2ef801 Compare May 26, 2022 21:13
@herndlm herndlm marked this pull request as ready for review May 26, 2022 21:18
@herndlm herndlm force-pushed the remove-first-last branch from b2ef801 to 076604d Compare May 27, 2022 07:48
@herndlm
Copy link
Contributor Author

herndlm commented May 27, 2022

hmm, the new composer integration error looks unrelated. I see that it's using a symfony lib in a version that was released literally minutes ago, which might be related :O

@ondrejmirtes ondrejmirtes merged commit f7be102 into phpstan:1.7.x May 27, 2022
@ondrejmirtes
Copy link
Member

Thank you!

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.

None yet

2 participants