-
Notifications
You must be signed in to change notification settings - Fork 668
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
Make array shapes strict by default #8701
Conversation
9da26de
to
f326971
Compare
@@ -638,8 +638,8 @@ private static function initProviders(array $options, Config $config, string $cu | |||
} | |||
|
|||
/** | |||
* @param array{"set-baseline": string} $options | |||
* @return array<string,array<string,strict-array{o:int, s: list<string>}>> | |||
* @param array{"set-baseline": string}&array $options |
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.
Shouldn't it use the new array{"set-baseline": string, ...}
syntax?
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'll fix this separately, and remove all remaining uses of this (while keeping it for BC)
LGTM. @danog @romm will this require some changes in https://github.com/CuyZ/Valinor ? |
Hi, thanks for the ping. 🙂 This will require Valinor to be able to understand the Have you also considered pinging Ondřej from PHPStan concerning this change? I know he's been participating to the discussion when @danog introduced the |
LGTM! @romm AFAIK muglug has already spoken with Ondrej about this change (more of a fix of a very old bug really). Actually, now I've been wondering whether I should send in a PR that also enforces strict ordering of arrays and lists, I've seen a few comments complaining about Psalm missing this feature in PSL... |
Psalm went backward on the introduction of the new `strict-array` type, see vimeo/psalm#8701 for more information.
Psalm went backward on the introduction of the new `strict-array` type, see vimeo/psalm#8701 for more information.
It would be interesting indeed when I see |
This PR removes
strict-array
andstrict-list
(added last week) from the set of allowed types.It makes all array shapes sealed/strict by default.
My belief is that this will affect the least amount of code (compared to the hassle of having everyone update their array shapes to add
strict-array
, which I believe would be a barrier to adoption.The practical ramifications of this change are pretty minor: in a few places those updating to Psalm 5 will have to specify their array can contain more than just the elements provided. This PR provides a new way to do that which mirrors Hack array shapes — the
...
operator.With this change
array{a: Foo}
is an array that contains just one element, indexed at'a'
.array{a: Foo, ...}
is an array that contains at least one element, but may contain many more of any type.We also now have
list{Foo, ...}
which denotes a list whose first element is of typeFoo
, but which may have more arbitrary list elements of any type.In practical terms I see
array{a: Foo, ...}
andlist{Foo, ...}
being used very little in codebases — in almost all circumstances a strict array is a much better match for what people want to do. In Slack's codebase, 86% of our array shapes are strict (we massively overuse array shapes, and we're slowly migrating many of them to objects).