-
Notifications
You must be signed in to change notification settings - Fork 545
Implement array list type #1751
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
Conversation
Just casually drop this bomb on a Friday afternoon 😀 I had some ideas about this myself, gonna dive right into the code.
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.
This looks great so far :) It's really tedious to go through all array-related functions and other operations and decide:
- Does this create a list out of an array that wasn't a list before?
- Does this preserve a list when it was already a list?
- Does this make list stop being a list?
I'd like some tests around:
- Setting an offset to an array. If we overwrite an existing offset (constant array), the list is preserved, if we add a new offset (doesn't matter if it's through
[]or[5]), the list should also be preserved. We should also work with some confidence around the value - different tests for the offset value being5vs.int. - Unsetting offsets. I'd argue this should always stop being a list. Because if we have
[1, 2, 3]and remove the last element, the array still looks like a list but we can't append a new offset to it: https://3v4l.org/JUWKE - Functions hardcoded in NodeScopeResolver - array_push, array_unshift, array_pop, array_shift.
- What happens in array_walk
src/Type/Accessory/ListArrayType.php
Outdated
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.
Non-zero assignment might still preserve a list? https://3v4l.org/pWCs0
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.
Yes, but because we don't have knowledge about other keys it's not safe to assume it remains a list for non-zero offsets.
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.
Does this need a new property? Can't we always figure it out from keyTypes and nextAutoIndexes?
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.
I tried that at first but it was a bit buggy. I've reworked some other parts so it might be doable now.
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.
The property is necessary, or at the very least I don't know how to resolve issues that arise otherwise:
getAllArrays()should only return lists if the array is a list, which is unknowable otherwise- other code sometimes relies on
ConstantArrayTypeexisting on its own and produces worse type inference with the accessory type separately
I just remembered an idea from phpstan/phpstan#3499 - if we have $a of list<string> and we check count($a) === 3, $a should be narrowed to array{string, string, string} :) This should be done somewhere in TypeSpecifier around the current count() handling.
I noticed that the webmozart-assert run was also green, but I would have expected that https://github.com/phpstan/phpstan-webmozart-assert/blob/1.2.x/tests/Type/WebMozartAssert/data/array.php#L104 starts failing.
Internally this is done with an expression using array_filter, see https://github.com/phpstan/phpstan-webmozart-assert/blob/1.2.x/src/Type/WebMozartAssert/AssertTypeSpecifyingExtension.php#L347
Maybe a list can be inferred for that as well :)
f5ca50a to
e308711
Compare
e308711 to
01056dd
Compare
What I want to try here (I can do that by myself in a few days): PHPStan without bleeding edge will behave exactly as on 1.8.x, PHPStan with bleeding edge will fully support the list type. Because this is too disruptive to enable for everyone right now, but I want to merge it, and I want to gather feedback from bleeding edge users.
01056dd to
fe82fef
Compare
Unfortunately some global state is needed, but this should make it completely disabled for non-bleeding-edge.
Nice :) Did you observe the failures in the integration projects when the list type was enabled for everyone? Were they legit bugs, or false positives caused by PHPStan changes?
266375d to
c9b678b
Compare
I can't get these tests to fail locally. No idea what is going on :/
Nice :) Did you observe the failures in the integration projects when the list type was enabled for everyone? Were they legit bugs, or false positives caused by PHPStan changes?
I looked at them when they failed and as far as I recall they were all legit failures.
I can't get these tests to fail locally. No idea what is going on :/
I know, probably. Because there's global state, it's easy to switch it off and make tests fail. Some tests don't use bleedingEdge (RuleTestCase::getAdditionalConfigFiles() - depends on if the child calls parent and merges the value), so the ContainerFactory switches it off, and because the containers are cached, the factory isn't called again to switch it on again.
I think the solution would be to additionally switch it on/off in PHPStanTestCase::getContainer() after the caching if statement. That should make it deterministic.
Also feel free to fix the getAdditionalConfigFiles() in rule tests that don't explicitly have "NoBleedingEdge" in their name - those should call the parent and merge the value :)
c9b678b to
9832147
Compare
...ecause of named args
Thank you 🎉 Please expect some follow-up work as I test this in the real world and come up with new ideas :) For example, we could update functionMap.php with list types from here: https://github.com/vimeo/psalm/tree/4.x/dictionaries
The functionMap.php file in PHPStan is roughly for PHP 7.3, so the delta files for that version and before should go to functionMap.php, the delta files after 7.3 should go to respective PHPStan functionMap delta files.
Oh, we should actually take the list types from the master branch because that's going to be more up to date: https://github.com/vimeo/psalm/tree/master/dictionaries
non-empty-array<0|1|2|3, literal-string&non-falsy-string>&list should be non-empty-list<0|1|2|3, literal-string&non-falsy-string>. Regexing type description isn't probably a good idea :) phpstan/phpstan#7856 (comment)
Variadic parameter array should be list<string> instead of array<int, string> here: phpstan/phpstan#5968
It should work in these situations:
- Always before PHP 8
- With PHP 8.0+ and
@no-named-arguments
It shouldn't work:
- On PHP 8.0+ without
@no-named-arguments(because that'sarray<int|string, string>)
Nice! I'll see what I can do to infer lists via the webmozart/assert extension soon :)
An idea - is this how we want to collapse list<mixed>|array<string, mixed>? https://phpstan.org/r/b7ffe028-2aca-472b-a61f-8d7c00b825c1
non-empty-array<0|1|2|3, literal-string&non-falsy-string>&listshould benon-empty-list<0|1|2|3, literal-string&non-falsy-string>.
This was on purpose, to always have list<...> equal array<int<0, max>, ...>. To change this the input syntax should also be extended, and I'm not really convinced that that is useful.
An idea - is this how we want to collapse
list<mixed>|array<string, mixed>? https://phpstan.org/r/b7ffe028-2aca-472b-a61f-8d7c00b825c1
I think that is the best we can do in general. Trying to keep list separate introduces a lot of complexity in the code that handles unions of constant arrays, because array{} can be seen as both a list and a regular array. I don't think it's worth the complexity.
Oh, I get and agree with your arguments, thanks for letting me know!
No description provided.