psalm
psalm copied to clipboard
vimeo
Keyed array refactoring
This PR introduces new syntax to define list-like keyed arrays: list{} which replaces the previously undocumented (and frankly confusing) array{}.
Also introducing syntax for callable-list{}.
Finally, introducing sealed-*{} for sealed variants of array types.
right now, sealed arrays can only be created by explicitly creating an array in php. The sealed flag is used for several things (like being sure of how many elements there is in an array or checking if the array can be inferred to be empty, things like that).
If you want to be able to document a sealed-array, there are a few new rules that will need to be enforced. For example, a method expecting a sealead-array param now need to check that the provided array is sealed. Likewise, trying to fetch an undocumented key from a sealed array should return TNever (EDIT: this seems handled by InvalidArrayOffset already)
There will be a bunch of things like that to handle that were not previously implemented because the sealed flag couldn't be used directly by people.
I think list{} is ultra confusing, since a list currently is a 0-monotonous index array.
I think list{} is ultra confusing, since a list currently is a 0-monotonous index array.
Hmm, I don't think this is confusing, because the {} syntax was always used for arrays whose exact elements are known, and list{} is simply an enhancement over the array{} type that specifies that elements are also a list (= array_is_list will always return true).
Hmm, I've been considering actually inverting the logic here, by marking as sealed all phpdoc-originated types, and making an unsealed- variant as an escape hatch (given that we already mark as sealed all list-like keyed arrays ie array{"a", "b", "c"})
@orklah I'd love to get this merged into v5 as it introduces huge improvements in all kinds of assertions (and also a lot of bugfixes for pre-existing bugs in get_object_vars, properties-of, array_merge, array_replace and array_map), do you think we could wait a few more days before releasing v5? I'm still fixing the unit tests, but right now the design of this PR is finalized.
For example, a method expecting a sealead-array param now need to check that the provided array is sealed
Already implemented this, too.
Wew the reconciliation code was tricky, probably because the variable names didn't help much ;)
Refactored a bit to make everything cleaner fixing count bugs on sealed arrays, I'm tempted to switch from $did_remove_type => $redundant in all reconcilers in a future PR :)
I'm tempted to switch from $did_remove_type => $redundant in all reconcilers in a future PR :)
That's fine by me :p It took me a while to figure out exactly what was going on in there too
Pinging @romm: after this PR is merged, Psalm will default to the equivalent of Valinor's strict mode when working with shaped arrays.
When working in flexible mode, Valinor should always return unsealed-array types, regardless of whether the input shape is sealed or not.
Also, I'm pretty sure you could just remove the flexible/strict distinction at the library level in a future major and let users specify the strictness of arrays directly in the shape.
All shaped arrays are now sealed by default
Sorry, we had this discussion before, including with Ondrej from PHPStan and the common consensus was that the current syntax must remain not sealed.
There is too much impact in term of compatibility with both tool and also with codebases annotated with one interpretation of arrays that won't be compatible with another interpretation to be a valid path for upgrading.
I'm afraid all array{...} notation must stay unsealed indefinitely (much to my dismay, as I initially argued otherwise (discussion started from here: https://github.com/vimeo/psalm/issues/4700#issuecomment-857372020)
Hmm, the arguments presented in the linked issue almost convinced me to keep arrays unsealed: I knew that this change would require a lot of changes in codebases, and while I hope against it, I realize most codebases will just do a mass-replace of array{ => unsealed-array{ to fix issues in the short term.
But on the other hand, isn't this for the best? Even if the fix is as simple as a mass-replace of array{ => unsealed-array{, won't this just explicitly tell developers encountering those typehints, that hey, they're doing something unsafe?
It's a BC-breaking inconvenience that can be easily hotfixed with a string mass-replace in the IDE, and future code will only be safer because of it.
@ondrejmirtes I just submitted a PR @ https://github.com/phpstan/phpdoc-parser/pull/161, do tell if you'd be willing to merge it and possibly accept future PRs implementing the same strict-by-default logic in phpstan :)
I realize most codebases will just do a mass-replace of array{ => unsealed-array{ to fix issues in the short term.
Unfortunately, they won't. Because unsealed-array won't be understood by PHP CS Fixer, nor by PHPStorm, nor by PHP-Documentor, nor by any tool in usage currently. And even if you do make the early work for all of those, you'll have unmaintained libraries, old versions of libraries that must stay on a project that will still use array{} the old fashioned way that make Psalm report thousand of false positive outside of user reach.
Let's see what Ondrej has to say about that, but the more I think about it, the more it seems unfeasible
You can't just take something that's been used for years and give it a different meaning. That's sure to cause some pain, exactly for reasons @orklah listed.
Ideally to support all use-cases, I'd need something like 4 "constant arrays" implementations in PHPStan:
- Current (unsealed) array shapes in PHPDocs - allows extra keys, order doesn't matter
- Strict (sealed) array shapes in PHPDocs - doesn't allow extra keys, order doesn't matter
- Literal arrays created with
array(...)or[...]in code - order does matter when comparing - Array shapes in PHPDocs with known value of extra keys, sometimes it's called "array shape intersection" but that's incorrect
And also to figure out all interactions between them ("supertype-of" and "subtype-of" relationships). Which is a lot of complexity that currently I don't want to deal with.
Literal arrays created with array(...) or [...] in code - order does matter when comparing
Order is an interesting issue I did briefly consider, but given that there was (I think) no pre-existing support for that in psalm I postponed it.
Ah well, I tried :)
Reverting to strict- syntax, keeping the old array{} syntax unchanged.
there was (I think) no pre-existing support for that in psalm
We have a is_list property in TKeyedArray that will change a standard keyed array like array{0: int, 1: string} into an array{int, string}. The second one enforces the order. That only works with list-like keys though
That only works with list-like keys though
Ah that makes sense, I suppose it would be nice to have an ordered-strict-array variant, but that's for a future PR ;)
This PR should be now good to merge, the phpcs comments can be removed once @ondrejmirtes merges https://github.com/phpstan/phpdoc-parser/pull/161 and tags, which adds basic strict-array, strict-list and list syntax support to phpstan and phpcs, and @romm merges https://github.com/CuyZ/Valinor/pull/246 so at least strict-array syntax is supported in valinor :)
Sorry, the 4.x is in feature freeze, no more PRs are accepted for it, but you can already upgrade to the latest rc1 of Psalm v5 right now, the release itself is imminent! :D
@danog I tried, but it is the Symfony plugin that depends on V4.
Never mind, I will see what I can do. Thank you for this awesome feature anyway.
It makes sense that Symfony still depends on Psalm 4 given the release is yet to come! One of our task after releasing will be to make sure there are no big plugins left behind for too long :)