Support sealed/unsealed array shapes

[jrmajor]

Feature request

Psalm allows array{foo: int, bar: int, ...} notation to specify that this array may also contain other keys. Array shapes without the ... must not contain any other keys.

This is a feature introduced in Psalm 5. It's described in depth in the announcement post. Here are the examples from the post in PHPStan playground: https://phpstan.org/r/40e730d8-e941-4102-9e76-a07f1c0a2e46.

[stof]

The basic support would be to support parsing the ... syntax while still treating all shapes as unsealed (i.e. the current behavior). Fully supporting sealed shapes would require a new major version (and so only be available in bleeding-edge)

[jrmajor]

while still treating all shapes as unsealed (i.e. the current behavior)

I think the way PHPStan treats array shapes currently is something in-between sealed and unsealed.

Fully supporting sealed shapes would require a new major version (and so only be available in bleeding-edge)

Yes, but given that currently the unsealed syntax is an error in PHPStan, treating array shapes with ... differently would not be a BC break.

This means we can change sealed shapes semantics only in bleeding edge, but use the new semantics for unsealed shapes right away.

[stof]

If the current behavior is indeed in-between, the unsealed array shapes might indeed be implemented fully already.

[jrmajor]

Here is an example of PHPStan treating array shape as sealed. If that was fully the case though, there would be an error reported in the first function in my original example.

[VincentLanglet]

Not sure if PHPStan should follow something imposed by Psalm 20 days before the major release without discussion when another syntax strict-array was preferred for months and would have avoid BC breaks.

array{key: string}&array<int, int> is another syntax supported by Psalm which is still not supported by PHPStan, I think by choice.

[stof]

The issue is having the same syntax having a different meaning in both tools, as it means a library writing types with one tool in mind is incompatible with the other tool (and projects using the library may choose to use the other tool)

[jrmajor]

@VincentLanglet Hi, thanks for bringing that up. I wasn't aware of the discussions in vimeo/psalm#4700 and vimeo/psalm#8395.

@ondrejmirtes Regarding your comments in these discussions — I would just like to point out that it's not entirely true that array shapes are currently treated as unsealed. I've provided an example in my previous comment, but ~here is a more entertaining one~. The error reported in this example shows that PHPStan treats this shape as a sealed one, but no error reported for the return type of the function implies that it's unsealed. This inconsistency can be fixed by making array shapes consistently fully sealed or fully unsealed. Both solutions are BC breaking, but one of them makes PHPStan compatible with Psalm, and the other one makes it even more incompatible.

Edit: I've made a mistake in that example, which @VincentLanglet pointed out. Here is a corrected one.

[VincentLanglet]

I'm sad the strategy change wasn't more discussed.

When a method is doThings(Foo $foo): Foo nobody is shocked if we pass SuperFoo or if the method returns SuperFoo. So with doThings(array $foo): array i would have consider natural to accepts bigger arrays or returning bigger arrays.

strict-array or sealed-array seemed more natural to me. And we could have been asking something like sealed-Foo for classes.

but here is a more entertaining one.

With object, you have https://phpstan.org/r/b808b62f-08a6-4a89-805d-7f64f9ce5855 and nobody complains about this error. And it doesn't mean you can remove the line. Code must be updated to https://phpstan.org/r/b6a7e4cc-844b-40b0-8b49-76bbf3810d14.

Maybe error message should be improved ? Maybe https://phpstan.org/r/d228d69a-83a3-46e4-9aed-72ea41ffaba2 should be valid ? I dunno. On this opposite side, we have https://phpstan.org/r/bda38be1-9895-4cf5-bb95-0196159bd462 which should report an error like https://phpstan.org/r/327201df-d8c6-4e02-85d9-6d0006ea33f1 does.

I would just like to point out that it's not entirely true that array shapes are currently treated as unsealed.

I would say that PHPStan tried to take the best of both world (like Hannah Montana 😄).

Most of the time you're only reading the data from a given array shape, so it doesn't require to be sealed.

[stof]

The example given in the psalm doc or release notes (I don't remember which one it is) shows an issue with the unsealed shapes (or the partially unsealed ones): it is not safe to consider array{foo: string, bar: string} as a type accepted by the type array<string, string> (or even string[]) even though all values in the shape are strings because you accept any other key that can have other types. And this does not involve writing into the shape. Wanting to make array shape sound in in the type system is a worthy goal, and that cannot be achieved with a single type of shapes that is partially sealed (having only sealed shapes would be sound as well, but annoying for some use cases).

[jrmajor]

With object, you have phpstan.org/r/b808b62f-08a6-4a89-805d-7f64f9ce5855 and nobody complains about this error. And it doesn't mean you can remove the line. Code must be updated to phpstan.org/r/b6a7e4cc-844b-40b0-8b49-76bbf3810d14.

Maybe https://phpstan.org/r/d228d69a-83a3-46e4-9aed-72ea41ffaba2 should be valid ?

@VincentLanglet You're correct, I completely missed a point with that example. What I was intending to show is that checking for key existence in this case will be reported as always false (meaning that the shape is treated as sealed). I've added a corrected example to my original comment.

I would say that PHPStan tried to take the best of both world (like Hannah Montana 😄).

It causes real problems though. When adding type annotations to PHP-CS-Fixer, I've come across something like that and wasn't able to find a solution other than @phpstan-ignore.

[VincentLanglet]

The example given in the psalm doc or release notes (I don't remember which one it is) shows an issue with the unsealed shapes (or the partially unsealed ones): it is not safe to consider array{foo: string, bar: string} as a type accepted by the type array<string, string> (or even string[]) even though all values in the shape are strings because you accept any other key that can have other types.

I didn't said we didn't need to differentiate sealed and unsealed array. And the syntax ... is interesting.

The Psalm syntax

array{key: string}&array<int, int>

which wasn't supported by PHPStan could be changed to

array{key: string, ...<int, int>}

I was just wondering in which side we had more BC-break. Psalm can easily introduce BC break with the v5 release, but it forces PHPStan a quick support of the syntax, and a possible behavior change (in bleeding edge) for people which uses both tools. A psalm option to change the behavior, removed in v6, could have introduce less frictions maybe.

Edit: just found another topic about it https://github.com/vimeo/psalm/issues/8744

[stof]

for people which uses both tools

It is not just for people that use both tools. It is for phpstan users relying on a dependency where the maintainer uses psalm (and so uses the psalm semantic of shapes) or the opposite.

[stof]

It causes real problems though. When adding type annotations to PHP-CS-Fixer, I've come across something like that and wasn't able to find a solution other than @phpstan-ignore.

This particular case would be fixed if the abstract method in IntegrationTest was documented as @return array{php?: int, os?: string} to show the full list of supported requirements. Sealed vs unsealed has nothing to do with your case here IMO (the runner has specific requirements for the os key when it exists) And note that sealed array shapes would then detect case where a subclass returns an array with a wrong key (operating_system vs os), which would not be detected with the current phpstan shapes or unsealed shapes (missing the os key won't be an error as the key is optional)

[VincentLanglet]

for people which uses both tools

It is not just for people that use both tools. It is for phpstan users relying on a dependency where the maintainer uses psalm (and so uses the psalm semantic of shapes) or the opposite.

Yeah, I fully agree. But the issue already exists with only Psalm. If your project works with Psalm 5 and you're using a library witch still use Psalm 4, you'll get some trouble.

Phpdoc BC break was not a good idea IMHO. And it's not because Psalm take a bad decision that it must be followed. Otherwise it will create the same issue between bleeding-edge user (or v2 user) and v1 user of PHPStan.

[jrmajor]

This particular case would be fixed if the abstract method in IntegrationTest was documented as @return array{php?: int, os?: string} to show the full list of supported requirements. Sealed vs unsealed has nothing to do with your case here IMO (the runner has specific requirements for the os key when it exists)

@stof It had something to do with test cases being extended either by PHP-CS-Fixer tests (which can have additional keys) or by PHP-CS-Fixer plugin tests (which couldn't), and there was an extended runner for PHP-CS-Fixer tests. I believe you can still find that in PHP-CS-Fixer repo. OFC this could have been solved in many different ways, but types couldn't have been properly annotated without changing the code, because I was running into this inconsistency (which would have been solved by changing the return type to @return array{php?: int, ...}).

[VincentLanglet]

The Psalm syntax

array{key: string}&array<int, int>

which wasn't supported by PHPStan could be changed to

array{key: string, ...<int, int>}

For the record @jrmajor, since you're working on unsealed syntax https://github.com/phpstan/phpdoc-parser/pull/169 (from https://github.com/vimeo/psalm/issues/8804)

What I'd prefer is something like array{foo: 1, bar: 2, [string: string]}. That's in line how this looks in TypeScript. Or maybe even array{foo: 1, bar: 2, ...[string: string]}

[MidnightDesign]

(I hope this isn't too off-topic)

This entire discussion re-affirms my belief that the in-docblock type system should be a separate repository that's shared between tools like PHPStan, Psalm and PhpStorm. With RFCs for new features like sealed arrays where all the stakeholders can have their say.

[johnbillion]

Related: #5141

[phpstan-bot]

@jrmajor After the latest push in 1.9.x, PHPStan now reports different result with your code snippet:

@@ @@
-20: Function unsealed() has parameter $user with no value type specified in iterable type array.
-20: Function unsealed() return type has no value type specified in iterable type array.
-20: PHPDoc tag @param has invalid value (array{id: string, name: string, ...} $user): Unexpected token "...", expected type at offset 105
-20: PHPDoc tag @return has invalid value (array{id: string, name: string, ...}): Unexpected token "...", expected type at offset 159
+No errors

[mvorisek]

https://phpstan.org/r/85c3f676-87e9-44ca-be14-279e3b731840 unsealed testcase (from #8919) which should be tested once this feature is implemented

[mvorisek]

https://phpstan.org/r/57575f99-3267-4ea0-a41b-a5b401761ae7 list{xxx} testcase (from #9352), discusssion with proposed fix: https://github.com/phpstan/phpstan/issues/9352#issuecomment-1560557836

[mvorisek]

https://phpstan.org/r/0de7914a-3578-47af-b316-046071037fee list{} testcase (from #9317), array{} should allow any array to be consistent with list{x: y} (non-empty array shape).

[phpstan-bot]

@VincentLanglet @jrmajor After the latest push in 1.11.x, PHPStan now reports different result with your code snippet:

@@ @@
+PHP 8.0 – 8.2 (1 error)
+==========
+
+16: Call to an undefined method Foo::super().
+
+PHP 7.1 – 7.4 (2 errors)
+==========
+
+ 5: Constructor of class Foo has a return type.
 16: Call to an undefined method Foo::super().

Full report

PHP 8.0 – 8.2 (1 error) ----------- | Line | Error | |---|---| | 16 | `Call to an undefined method Foo::super().` | PHP 7.1 – 7.4 (2 errors) ----------- | Line | Error | |---|---| | 5 | `Constructor of class Foo has a return type.` | | 16 | `Call to an undefined method Foo::super().` |

[phpstan-bot]

@VincentLanglet @jrmajor After the latest push in 1.11.x, PHPStan now reports different result with your code snippet:

@@ @@
-No errors
+PHP 8.0 – 8.2
+==========
+
+No errors
+
+PHP 7.1 – 7.4 (1 error)
+==========
+
+5: Constructor of class Foo has a return type.

Full report

PHP 8.0 – 8.2 ----------- No errors PHP 7.1 – 7.4 (1 error) ----------- | Line | Error | |---|---| | 5 | `Constructor of class Foo has a return type.` |

[phpstan-bot]

@VincentLanglet After the latest push in 1.11.x, PHPStan now reports different result with your code snippet:

@@ @@
-No errors
+PHP 8.0 – 8.2
+==========
+
+No errors
+
+PHP 7.1 – 7.4 (1 error)
+==========
+
+5: Constructor of class Foo has a return type.

Full report

PHP 8.0 – 8.2 ----------- No errors PHP 7.1 – 7.4 (1 error) ----------- | Line | Error | |---|---| | 5 | `Constructor of class Foo has a return type.` |

[Ocramius]

Cross-linking here: https://github.com/vimeo/psalm/issues/10288

Similar feature, just not 100% working there either :)_

[ondrejmirtes]

We should implement this syntax: array{a: mixed, ...<array-key, mixed>} as it's now supported in Psalm: https://github.com/vimeo/psalm/issues/8804#issuecomment-1936451877

[ondrejmirtes]

I'm thinking of these steps to follow when implementing this (each as a separate PR, possibly multiple PRs):

1) Support for unsealed array shapes in phpdoc-parser, both array{string, int, ...} and array{string, int, ...<K, V>} syntax. 2) Basic implementation in TypeNodeResolver + modeling of unsealed array shapes in ConstantArrayType, including rightfully answering in methods like accepts() and isSuperTypeOf. 3) Fixing all the array interactions throughout the codebase to account for sealed/unsealed array shapes - NodeScopeResolver, MutatingScope, narrowing in TypeSpecifier, all dynamic return type extensions for array functions... 4) Changing the meaning of array{string, int} to a sealed array shape, probably under the bleeding edge flag

/cc @tscni

[ondrejmirtes]

Also after point 2) we can fix current inconsistencies of ConstantArrayType (un)sealedness. For example currently ConstantArrayType behaves as unsealed and sometimes as sealed.

• ConstantArrayType::accepts() accepts unsealed array shapes with extra keys
• count() behaves as sealed, returning the exact number of keys the array has
• ConstantArrayType::hasOffsetValueType() behaves as sealed