-
Notifications
You must be signed in to change notification settings - Fork 506
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
Narrow explode return to constant arrays for small positive limits #3708
Conversation
afe6896
to
1b46208
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to cover the initial request of the discussion, I think it would be useful to assert the result of
explode(',', $date, 2) + [1 => null]
in the tests
I guess you saw it already yourself, but just in case: seems you found a case in which we miss to narrow a constant arrays optional offsets :-) |
Yes, was creating a similar example in parallel (https://phpstan.org/r/d6b8098a-282b-47cd-be91-38e42401d1eb) :) let's fix that first.. |
1b46208
to
d83ff0c
Compare
d83ff0c
to
a8b5d49
Compare
This pull request has been marked as ready for review. |
currently: https://phpstan.org/r/a1181543-1503-4108-bcf4-ee8e13abda66 so most of it are things like
not sure if this PR even adds much value.. |
a8b5d49
to
b441442
Compare
oh interesting, the 3 failing tests show that the narrower type can cause new
which I think is expected and fine. |
eeab815
to
d474491
Compare
d474491
to
d795909
Compare
What to do about this one? Is it OK or e.g. bleeding edge toggle maybe? |
The problem with this one is that it promotes errors that would previously be only reported with Does this have an actual benefit? Isn't |
I was also not sure. If in doubt, maybe it's better not do this. At least the preparation work for this uncovered other count related improvements 😊 |
Related to phpstan/phpstan#12187
kept this simple and only handle a single constant limit on purpose for now
Needs #3709 first