Support typing extra items in unsealed array shapes

[tscni]

Part of phpstan/phpstan#8438
Resolves #245

[tscni]

This is the first iteration with some open questions. I would mark it as a draft, but somehow I feel that it'd get less feedback that way.

[tscni]

I thought about adjusting the grammar, but there currently doesn't seem to be a big focus on keeping the grammar maintained. There's e.g. no representation for TOKEN_PHPDOC_EOL at all, ArrayShape is already out of date etc.
I could work on all of that, but again, if there's not much interest in keeping it maintained that doesn't quite seem worthwhile.

Ultimately this leads to the question what the point of the grammar is.
Is it mainly for documentation / easier understanding of the parser logic? Or is there a goal to generate more documentation based on it?
Is the end goal to move to a generated parser?
Should it serve for third party consumers?

I suppose ideally the tests would somehow verify that the parse results match the current grammar.

[ondrejmirtes]

Hey, I'd rather have a different design:

/** @var TypeNode|null */
public $extraKeyType;

/** @var TypeNode|null */
public $extraValueType;

// ...

/**
 * @phpstan-assert-if-false !null $this->extraKeyType
 * @phpstan-assert-if-false !null $this->extraValueType
 */
public function isSealed(): bool
{
}

/**
 * @phpstan-assert-if-true !null $this->extraKeyType
 * @phpstan-assert-if-true !null $this->extraValueType
 */
public function isUnsealed(): bool
{
}

The GenericTypeNode here is too broad, it allows for invalid AST with types count that isn't 2.

[tscni]

I mainly went for GenericTypeNode to keep consistency with regular array definitions (e.g. array<string, string, string> works fine on a doc parser level), but I'll happily make it more restrictive.

The assertions on the other hand seem slightly concerning, specifically for unsealed shapes without extra key/value types.

• Given array{...}
• We can internally represent that as array{...<array-key, mixed>}
• And for printing / string conversions output that as array{...} again (also to keep backward compatibility)
• But this also means that an explicit array{...<array-key, mixed>} would also result in an output of array{...}, unless we add a distinction for "not set"
• Also, list{...} would then internally be represented as list{...<mixed>} (internally with an irrelvant int key type)

I don't mind doing that, but is the complexity really more suitable in the parser vs. handling it entirely on the type resolution side? (I suppose there's a fine line between the two)

[ondrejmirtes]

I don't want to allow array{...} without any items, we should throw a parse error for that. People should use a general array instead.

[tscni]

It's currently allowed. Is that breaking change ok? (see the PR that introduced it #169)

Note that my concerns also apply for e.g. array{foo: string, ...} and list{string, ...}

[tscni]

Of note: Psalm allows array{...} & list{...} and, like array{} & list{}, resolves them to array<never, never>: https://psalm.dev/r/7840e76832
Basically meaning that array{...} is still considered sealed by Psalm. (feels like a bug)

In PHPStan they're similarly all resolved to array{} at the moment: https://phpstan.org/r/69df8759-e426-4d5d-a8d3-83d26f973624

[ondrejmirtes]

Alright, I get it. As for the AST in phpdoc-parser, I guess we need to throw away my idea for the methods with @phpstan-assert. We'll keep the public $sealed which will still mean "are there ... in the array shape?". And then we should have a new AST object which will contain both key type nad value type.

So there will be some kind of "ArrayShapeUnsealedTypes|null" property in ArrayShapeNode with $keyType and $valueType. Which will make it impossible to represent impossible states in the object structure.

The only impossible state to represent would be sealed=true and unsealedType !== null but I'm fine to live with that.

As for the PHPStan typesystem representation, we can do the same thing we're doing to array<X> vs. array<array-key, X>.

array{foo: X, ...} would be represented with benevolent union type in the place of unsealed key type, and implicit mixed type in place of the value. So we'd still know what to do in toPhpDocNode().

array<foo: X, ...<array-key, mixed> would still have the benevolent union type, but the mixed would be explicit, so we'd know what to do in toPhpDocNode().

Thanks!

[tscni]

The only impossible state to represent would be sealed=true and unsealedType !== null but I'm fine to live with that.

There's also the impossible state of $kind === 'list' && $unsealedType->keyType !== null, but that's it.

I've made the necessary adjustments.

[ondrejmirtes]

FYI I pushed a commit with a few tests I was missing :)

[ondrejmirtes]

This should be just a Node, not TypeNode. It doesn't make sense standalone.

[tscni]

Makes sense. ArrayShapeItemNode shouldn't have been one either then I guess, oh well.
Done.

[tscni]

FYI I pushed a commit with a few tests I was missing :)

Thanks :)
I added some more cases for list shapes

[ondrejmirtes]

Awesome, thank you!