Skip to content
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

Add support for array{foo: string, ...<int>} and array{foo: string, ...<int, int>} syntax #8804

Closed
VincentLanglet opened this issue Dec 1, 2022 · 13 comments

Comments

@VincentLanglet
Copy link
Contributor

In the example https://psalm.dev/r/d9b4b19a18,
the type is described as array{a: mixed, ...<array-key, mixed>} by psalm.

But if we use the same syntax in the phpdoc, there is a syntax error
https://psalm.dev/r/6d3a83fd93

I would say it could be great to move from

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

to

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

And in the same way we could have array{foo: string, ...<int>} instead of array{foo: string}&array<int>

@AndrolGenhald @orklah I saw you were taking in #8744 (comment)
about the syntax array{foo: string, ...mixed}, but I would say it's not supported/correct since
array{a: mixed, ...int} is still array{a: mixed, ...<array-key, mixed>}, cf https://psalm.dev/r/2a3b3d25a3.

array{foo: string, ...<int>} might be more natural than array{foo: string, ...int} since

  • it would be consistent with array{foo: string, ...<int, int>}
  • it's consistent with the spread operator (which only works on an array, and <int> means array<int>)
@AndrolGenhald
Copy link
Collaborator

@muglug decided on the array{foo: string, ...} syntax based on hack, but it doesn't look like hack supports open shapes while specifying something other than an implicit mixed for the extra elements, so I suppose if we want to add that we'll have to decide on a syntax ourselves.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Dec 1, 2022

I suppose if we want to add that we'll have to decide on a syntax ourselves.

I feel like it's already kinda decided since the psalm-trace is described as array{a: mixed, ...<array-key, mixed>}.
WDYT about the proposal ? It seems natural to me.

@VincentLanglet
Copy link
Contributor Author

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

to

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

And in the same way we could have array{foo: string, ...<int>} instead of array{foo: string}&array<int>

Friendly ping @ondrejmirtes to know if this is a syntax PHPStan would be ok with ?
Since #4700 (comment)

@ondrejmirtes
Copy link

...<int, int> is weird. It looks like one of the array items could be array<int, int> or iterable<int, int> or Foo<int, int> for example...

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Dec 1, 2022

...<int, int> is weird. It looks like one of the array items could be array<int, int> or iterable<int, int> or Foo<int, int> for example...

Oh, I didn't see it like this... I thought this could have been understood like

// array<int, int>
$a = [1 => 1, 2 => 2];
// array{foo: string, ...<int, int>}
$b = ['foo' => 'bar', ...$a];

and it was shorter than array{foo: string, ...array<int, int>}

@ondrejmirtes
Copy link

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

@ondrejmirtes
Copy link

Or maybe even array{foo: 1, bar: 2, ...[string: string]}

@jrmajor
Copy link
Contributor

jrmajor commented Dec 1, 2022

Are we sure that we need a separate syntax for this?

array{foo: string}&array<int, int> indeed makes no sense, as it should be just simplified to never, but also I can't imagine a situation where one would actually need this type. Why would you end up with array<int, int> and one foo: string key?

All realistic examples I can think of go something like array{id: int, username: string, ...}&array<string, mixed>. This actually makes sense as an intersection, because of ... added and types of array{} being subtypes of the ones in array<>.

And for edge cases like array{foo: string}&array<int, int>, you can instead use array{foo: string, ...}&array<int|string, int|string>. This type is a bit less specific than array{foo: string, ...<int, int>}, but is still correct, and IMO this is such a corner case that there's no need for a dedicated new syntax.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Dec 1, 2022

Are we sure that we need a separate syntax for this?

array{foo: string}&array<int, int> indeed makes no sense, as it should be just simplified to never, but also I can't imagine a situation where one would actually need this type. Why would you end up with array<int, int> and one foo: string key?

Psalm added array{foo: string}&array<int, int> and it's used.
I already had to use it multiple times.
It exists in some other languages.
So yes, I would say that it would be useful.

Sure, there are better way to write those arrays. For example: array{label: array<string>}&array<int, int>
would be better this way: array{label: array<string>, ids: array<int, int>}
But you cannot always change the way those array have been build (for BC or because it's from a library), so it would allow to represent them correctly with a PHPdoc

@kiler129
Copy link

kiler129 commented Jan 6, 2023

I'm coming here from PhpStan side at phpstan/phpstan#4703.

Just to add to examples where this is very useful: in my code I need to serialize object identity in a way that it will always unserialize if needed. Most objects will have a serialized form of array{#class: string, id: scalar} however, some may have their ID field named differently or have a composite identifier, ending up with something like array{#class: string, uid: string, tenant: int}. Effectively this means array{#class: string} is guaranteed with at least one other field

While I like the @ondrejmirtes syntax suggestion of array{#class: string, ...[string: scalar]} (if I understood it correctly), the .../expansion notation may be confusing. It is not the same as array{#class: string}&array<string: scalar>. In general PHPs semantics ... implies 0 or more: https://3v4l.org/eBCIC

@weirdan
Copy link
Collaborator

weirdan commented Feb 9, 2024

The syntax described in the issue description now works: https://psalm.dev/r/6d3a83fd93

@weirdan weirdan closed this as completed Feb 9, 2024
Copy link

I found these snippets:

https://psalm.dev/r/6d3a83fd93
<?php


/** @return array{a: mixed, ...<array-key, mixed>} */
function test(): array {
    return ['a' => 1, 'b' => 'b'];
}

/** @psalm-trace $a */
$a = test();
Psalm output (using commit 4b2c698):

INFO: Trace - 10:1 - $a: array{a: mixed, ...<array-key, mixed>}

INFO: UnusedVariable - 10:1 - $a is never referenced or the value is not used

@ondrejmirtes
Copy link

Awesome, PHPStan will eventually also add syntax for array{a: mixed, ...} and array{a: mixed, ...<array-key, mixed>}.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants