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
Support sealed/unsealed array shapes #8438
Comments
The basic support would be to support parsing the |
I think the way PHPStan treats array shapes currently is something in-between sealed and unsealed.
Yes, but given that currently the unsealed syntax is an error in PHPStan, treating array shapes with This means we can change sealed shapes semantics only in bleeding edge, but use the new semantics for unsealed shapes right away. |
If the current behavior is indeed in-between, the unsealed array shapes might indeed be implemented fully already. |
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. |
Not sure if PHPStan should follow something imposed by Psalm 20 days before the major release without discussion when another syntax
|
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) |
@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 Edit: I've made a mistake in that example, which @VincentLanglet pointed out. Here is a corrected one. |
I'm sad the strategy change wasn't more discussed. When a method is
With object, you have https://phpstan.org/r/b808b62f-08a6-4a89-805d-7f64f9ce5855 Maybe error message should be improved ?
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. |
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 |
@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.
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 |
I didn't said we didn't need to differentiate sealed and unsealed array. The Psalm syntax
which wasn't supported by PHPStan could be changed to
I was just wondering in which side we had more BC-break. Edit: just found another topic about it vimeo/psalm#8744 |
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. |
This particular case would be fixed if the abstract method in IntegrationTest was documented as |
Yeah, I fully agree. But the issue already exists with only Psalm. Phpdoc BC break was not a good idea IMHO. And it's not because Psalm take a bad decision that it must be followed. |
@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 |
For the record @jrmajor, since you're working on unsealed syntax phpstan/phpdoc-parser#169
|
(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. |
Related: #5141 |
@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 |
https://phpstan.org/r/85c3f676-87e9-44ca-be14-279e3b731840 unsealed testcase (from #8919) which should be tested once this feature is implemented |
https://phpstan.org/r/57575f99-3267-4ea0-a41b-a5b401761ae7 |
https://phpstan.org/r/0de7914a-3578-47af-b316-046071037fee |
@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 reportPHP 8.0 – 8.2 (1 error)
PHP 7.1 – 7.4 (2 errors)
|
@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 reportPHP 8.0 – 8.2No errors PHP 7.1 – 7.4 (1 error)
|
@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 reportPHP 8.0 – 8.2No errors PHP 7.1 – 7.4 (1 error)
|
Cross-linking here: vimeo/psalm#10288 Similar feature, just not 100% working there either :)_ |
We should implement this syntax: |
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.
The text was updated successfully, but these errors were encountered: