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

Make array shapes strict by default #8701

Merged
merged 2 commits into from Nov 12, 2022

Conversation

muglug
Copy link
Collaborator

@muglug muglug commented Nov 11, 2022

This PR removes strict-array and strict-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 type Foo, but which may have more arbitrary list elements of any type.

In practical terms I see array{a: Foo, ...} and list{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).

@muglug muglug force-pushed the muglug-strict-by-default-arrays branch from 9da26de to f326971 Compare November 11, 2022 14:54
@muglug muglug added the release:fix The PR will be included in 'Fixes' section of the release notes label Nov 11, 2022
@muglug muglug marked this pull request as ready for review November 11, 2022 15:04
@@ -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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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)

@weirdan
Copy link
Collaborator

weirdan commented Nov 11, 2022

LGTM. @danog @romm will this require some changes in https://github.com/CuyZ/Valinor ?

@romm
Copy link

romm commented Nov 11, 2022

Hi, thanks for the ping. 🙂

This will require Valinor to be able to understand the ... operator, which shouldn't be a big deal but certainly requires a bit of work. I created CuyZ/Valinor#252 to keep a trace.

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 strict-array type.

@danog
Copy link
Collaborator

danog commented Nov 11, 2022

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...

@muglug muglug merged commit 8d36bdc into vimeo:master Nov 12, 2022
@muglug muglug deleted the muglug-strict-by-default-arrays branch November 12, 2022 01:14
romm added a commit to romm/Valinor that referenced this pull request Nov 21, 2022
Psalm went backward on the introduction of the new `strict-array` type, see
vimeo/psalm#8701 for more information.
romm added a commit to CuyZ/Valinor that referenced this pull request Nov 21, 2022
Psalm went backward on the introduction of the new `strict-array` type, see
vimeo/psalm#8701 for more information.
@VincentLanglet
Copy link
Contributor

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 strict-array type.

It would be interesting indeed when I see
#4700 (comment)
#8395 (comment)
#4700 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants