Array shapes support #30

arnaud-lb · 2019-06-03T18:22:02Z

Adds support for parsing array shapes (object-like arrays).

Examples:

array{'foo': int, "bar": string}
array{0: int, 1?: int}
array{int, int}
array{foo: int, bar: string}

Keys can be single quoted strings, double quoted strings, integers, or identifiers (represented as ConstExprStringNode, ConstExprIntegerNode, IdentifierTypeNode).

They can be marked optional with a ? before the key and the :.

They can be omitted entirely.

Values can be any type, including nullable types or nested array shapes.

arnaud-lb · 2019-06-03T18:22:43Z

Related: phpstan/phpstan#2173

ondrejmirtes · 2019-06-03T20:25:36Z

I love it! 😍 But looks like you've forgot to add those new classes in. The build (after my CS fix) fails on:

1) Warning
The data provider specified for PHPStan\PhpDocParser\Parser\TypeParserTest::testParse is invalid.
Class 'PHPStan\PhpDocParser\Ast\Type\ArrayShapeNode' not found

BTW: The optional keys in PHPStan are right now modeled with a union of two constant arrays - with the keys and without the keys. Unfortunately, this leads to some wrongly interpreted code. We can improve that later for example by introducing optional keys to ConstantArrayType but that brings more work. I'd continue represent them with unions of two arrays right now.

JanTvrdik · 2019-06-04T07:13:28Z

src/Parser/TypeParser.php

@@ -53,6 +55,9 @@ private function parseAtomic(TokenIterator $tokens): Ast\Type\TypeNode

 			} elseif ($tokens->isCurrentTokenType(Lexer::TOKEN_OPEN_SQUARE_BRACKET)) {
 				$type = $this->tryParseArray($tokens, $type);
+
+			} elseif ($type->name === 'array' && $tokens->isCurrentTokenType(Lexer::TOKEN_OPEN_CURLY_BRACKET)) {
+				$type = $this->parseArrayShape($tokens, $type);


this should be maybe tryParseArrayShape, so that /** @return array {curly description} */ is not suddenly invalid

The problem with tryParse is that it silently ignores errors. An array shape specification will be silently ignored if it has a minor typo in it.

Alternatively, if we don't allow spaces between array and {, this would prevent the problem you mention. We could do that by adding an array{ token. What do you think ?

The problem with tryParse is that it silently ignores errors.

That's not a problem of tryParse but inherent problem of phpDoc which is designed to be human friendly and not computer friendly (same problem as markdown).

without tryParse it's a BC break.

So what do you think of not allowing a whitespace between array and { ? This would prevent the BC break, while also avoiding silenced errors (which are not user friendly).

Yes, that should be fine.

src/Parser/TypeParser.php

JanTvrdik · 2019-06-04T07:32:37Z

src/Parser/TypeParser.php

@@ -53,6 +55,9 @@ private function parseAtomic(TokenIterator $tokens): Ast\Type\TypeNode

 			} elseif ($tokens->isCurrentTokenType(Lexer::TOKEN_OPEN_SQUARE_BRACKET)) {
 				$type = $this->tryParseArray($tokens, $type);
+
+			} elseif ($type->name === 'array' && $tokens->isCurrentTokenType(Lexer::TOKEN_OPEN_CURLY_BRACKET)) {
+				$type = $this->parseArrayShape($tokens, $type);


The problem with tryParse is that it silently ignores errors.

That's not a problem of tryParse but inherent problem of phpDoc which is designed to be human friendly and not computer friendly (same problem as markdown).

JanTvrdik · 2019-06-04T07:33:21Z

src/Parser/TypeParser.php

@@ -53,6 +55,9 @@ private function parseAtomic(TokenIterator $tokens): Ast\Type\TypeNode

 			} elseif ($tokens->isCurrentTokenType(Lexer::TOKEN_OPEN_SQUARE_BRACKET)) {
 				$type = $this->tryParseArray($tokens, $type);
+
+			} elseif ($type->name === 'array' && $tokens->isCurrentTokenType(Lexer::TOKEN_OPEN_CURLY_BRACKET)) {
+				$type = $this->parseArrayShape($tokens, $type);


without tryParse it's a BC break.

src/Parser/TypeParser.php

src/Ast/Type/ArrayItemNode.php

src/Ast/Type/ArrayShapeNode.php

doc/grammars/type.abnf

JanTvrdik · 2019-06-04T07:59:27Z

I wonder if we should make array{ byte sequence a new token instead of having array and { as two tokens. Mostly depends on whether allowing array {string, int} is desired or not.

Also it should be noted that allowing quoted string (which psalm does not allow) introduces grammar inconsistency (because it creates a place which looks like it expects constants expression but identifiers are not treated as constants) so I'm not sure it's a good idea.

JanTvrdik · 2019-06-04T08:06:21Z

cc @muglug

arnaud-lb · 2019-06-04T08:44:07Z

I wonder if we should make array{ byte sequence a new token instead of having array and { as two tokens. Mostly depends on whether allowing array {string, int} is desired or not.

I'm in favor of not allowing whitespaces between array and { (by introducing a new array{ token, or by taking profit of the fact that the tokeniser reports whitespace tokens).

Pros:

We can report syntax errors in a array{ definition without BC break
No silent errors

Cons:

We can not write array { (with a whitespace)

Also it should be noted that allowing quoted string (which psalm does not allow) introduces grammar inconsistency (because it creates a place which looks like it expects constants expression but identifiers are not treated as constants) so I'm not sure it's a good idea.

While looking at Psalm I though it was supported, but you are right: it's not. In Pslam, array{'a': int} will expect an array with the key '\'a\'', and not 'a'. I think that there is a risk of generating confusing errors here: expected array{'a': int}, array{a: int} provided.

So I would be in favor of either allowing quoted strings (so that array{'a': int} expects a key 'a'), or to explicitly disallow them (such that array{'a': int} is a syntax error).

ondrejmirtes · 2019-06-04T09:27:04Z

Thank you both for improving this!

@arnaud-lb I don't see a testcase for something wrong inside the array{...} so that we know you're not using tryParse....

arnaud-lb · 2019-06-04T10:28:39Z

Added :)

More about keys:

If we allow only identifiers, we limit the set of keys we can express with this syntax to the keys that are valid identifiers. Psalm would support our syntax entirely.
If we allow identifiers and quoted strings at the same time, there are grammar inconsistencies, as noted by @JanTvrdik.
If we allow only quoted strings, the grammar inconsistencies disappears, and we can express all possible keys, however we lose any compatibility with Psalm on array shapes.
Psalm allows anything as key, and treats the whole thing as a string: if there are quotes, they are part of the key's value. All characters appear to be allowed. I think that this is dangerous, as this prevents any backwards-compatible evolution of this syntax in the future. This is also confusing when keys appears to be quoted strings, but are not really.

Thoughts ?

JanTvrdik · 2019-06-04T11:26:54Z

If we allow only identifiers, we limit the set of keys we can express with this syntax to the keys that are valid identifiers.

I think this is the best option for now. Edit: identifiers + integers

muglug · 2019-06-04T12:11:08Z

Array key escaping is an open issue vimeo/psalm#1518

Psalm should support it, I just haven’t got around to it yet

ondrejmirtes · 2019-06-04T18:14:47Z

We should have a test that verifies that quoted string as a key parsing fails :) Otherwise it's ready to merge, right, everyone?

arnaud-lb · 2019-06-04T18:23:49Z

👍 added a test

ondrejmirtes · 2019-06-07T19:13:47Z

Thank you very much! I'll start poking it with ConstantArrayType right away :)

ondrejmirtes · 2019-06-07T22:05:08Z

And done: phpstan/phpstan@c4ed3c7 Feel free to review :)

arnaud-lb force-pushed the array-shapes branch from 7f977db to 1da2721 Compare June 3, 2019 18:42

JanTvrdik reviewed Jun 4, 2019

View reviewed changes

src/Parser/TypeParser.php Outdated Show resolved Hide resolved

JanTvrdik reviewed Jun 4, 2019

View reviewed changes

src/Parser/TypeParser.php Show resolved Hide resolved

arnaud-lb force-pushed the array-shapes branch 2 times, most recently from f93ce08 to d5c7122 Compare June 4, 2019 07:31

JanTvrdik requested changes Jun 4, 2019

View reviewed changes

Array shapes support

e8fa232

arnaud-lb force-pushed the array-shapes branch from d5c7122 to e8fa232 Compare June 4, 2019 08:24

Disallow whitespaces between 'array' and '{'

0d2d902

arnaud-lb force-pushed the array-shapes branch from 03768b2 to 0d2d902 Compare June 4, 2019 08:58

Add parse error test

db466fd

Allow only identifiers and integers in keys

eeb4a02

Add parse error test

c188001

ondrejmirtes merged commit 8c4ef2a into phpstan:master Jun 7, 2019

shmax mentioned this pull request Oct 27, 2019

Phpstan Level 3 webonyx/graphql-php#566

Merged

ondrejmirtes mentioned this pull request Dec 28, 2019

Space in generic type definition. phpstan/phpstan#2779

Closed

mfn mentioned this pull request Jan 1, 2020

Is there a supported mechanism to describe the contents of an array? phpstan/phpstan#2789

Closed

kukulich mentioned this pull request Apr 9, 2020

Add Forbidden Classes sniff slevomat/coding-standard#966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Array shapes support #30

Array shapes support #30

arnaud-lb commented Jun 3, 2019 •

edited

Loading

arnaud-lb commented Jun 3, 2019

ondrejmirtes commented Jun 3, 2019

JanTvrdik Jun 4, 2019

arnaud-lb Jun 4, 2019

JanTvrdik Jun 4, 2019

JanTvrdik Jun 4, 2019

arnaud-lb Jun 4, 2019

JanTvrdik Jun 4, 2019

arnaud-lb Jun 4, 2019

JanTvrdik Jun 4, 2019

JanTvrdik Jun 4, 2019

JanTvrdik commented Jun 4, 2019

JanTvrdik commented Jun 4, 2019

arnaud-lb commented Jun 4, 2019 •

edited

Loading

ondrejmirtes commented Jun 4, 2019

arnaud-lb commented Jun 4, 2019

JanTvrdik commented Jun 4, 2019 •

edited

Loading

muglug commented Jun 4, 2019

ondrejmirtes commented Jun 4, 2019

arnaud-lb commented Jun 4, 2019

ondrejmirtes commented Jun 7, 2019

ondrejmirtes commented Jun 7, 2019

Array shapes support #30

Array shapes support #30

Conversation

arnaud-lb commented Jun 3, 2019 • edited Loading

arnaud-lb commented Jun 3, 2019

ondrejmirtes commented Jun 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanTvrdik commented Jun 4, 2019

JanTvrdik commented Jun 4, 2019

arnaud-lb commented Jun 4, 2019 • edited Loading

ondrejmirtes commented Jun 4, 2019

arnaud-lb commented Jun 4, 2019

JanTvrdik commented Jun 4, 2019 • edited Loading

muglug commented Jun 4, 2019

ondrejmirtes commented Jun 4, 2019

arnaud-lb commented Jun 4, 2019

ondrejmirtes commented Jun 7, 2019

ondrejmirtes commented Jun 7, 2019

arnaud-lb commented Jun 3, 2019 •

edited

Loading

arnaud-lb commented Jun 4, 2019 •

edited

Loading

JanTvrdik commented Jun 4, 2019 •

edited

Loading