Related: phpstan/phpstan#2173

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.

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.

cc @muglug

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

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

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 ?

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

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

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

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

👍 added a test

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

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