vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.54k stars 660 forks source link

Type declaration inconsistency with rest parameters. #7957

Open rzvc opened 2 years ago

rzvc commented 2 years ago

The following two declarations result in the same type:

/** @param string $params */
function test1(...$params) : void
{
    /** @psalm-trace $params */
    print_r($params);
}

/** @param string[] $params */
function test2(...$params) : void
{
    /** @psalm-trace $params */
    print_r($params);
}

https://psalm.dev/r/738418eb4d

Edit: I suggest that only the second one to be considered valid, while the first one should issue an error.

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/738418eb4d ```php INFO: Trace - 14:5 - $params: array ```
AndrolGenhald commented 2 years ago

I've noticed this too and thought it was intentional, but it does get confusing if the type is an array: https://psalm.dev/r/6b3ada442e

We also support ...$params which I think is much clearer: https://psalm.dev/r/70fdeadbc2

I suppose we could change it like this for 5.0, but I'm not sure how much of an impact it would have on existing projects. If it's going to break a lot of stuff it doesn't seem worth it. Does the phpdoc documentation have anything to say about variadic arguments?

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/6b3ada442e ```php > $params */ function test1(...$params): void { /** @psalm-trace $params */; } ``` ``` Psalm output (using commit f960d71): INFO: Trace - 6:32 - $params: array> INFO: UnusedParam - 4:19 - Param $params is never referenced in this method ```
https://psalm.dev/r/70fdeadbc2 ```php ...$params */ function test1(...$params): void { /** @psalm-trace $params */; } ``` ``` Psalm output (using commit f960d71): INFO: Trace - 6:32 - $params: array> INFO: UnusedParam - 4:19 - Param $params is never referenced in this method ```
https://psalm.dev/r/ebbaeca8ba ```php $params */ function allowedButNotPreferredDueToAmbiguity(...$params): void { /** @psalm-trace $params */; } /** @param string $params */ function noLongerAllowed(...$params): void { /** @psalm-trace $params */; } ``` ``` Psalm output (using commit f960d71): INFO: Trace - 6:32 - $params: array INFO: Trace - 12:32 - $params: array INFO: Trace - 18:32 - $params: array ```
rzvc commented 2 years ago

Regarding your allowedButNotPreferredDueToAmbiguity, I would like to stress how important it is that it continues to be allowed, because it makes things like this possible:

/**
* @param list<string> | array{ 0: PossiblyOtherShapeOrArrayType } $params
*/

This will also come in handy in the future when method overloading is implemented.

I think that just disallowing the /** @param string $params */ notation, will remove all the ambiguity and future confusion, because /** @param string[] $params */ correctly specifies the resulting type of the $params variable, while /** @param string ...$params */ specifies the type of each individual item in the resulting $param variable. It works.

Edit:

If it's going to break a lot of stuff it doesn't seem worth it.

Perhaps an opt-in into the old (broken) behavriour could be added for those cases.

zlodes commented 1 year ago

Another one example: https://psalm.dev/r/8f74c8dc4e

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/8f74c8dc4e ```php $values */ private array $values = []; /** * @param list $strings * * @return list */ public function fill(string ...$strings): array { array_push($this->values, ...$strings); return $this->values; } } ``` ``` Psalm output (using commit 96cb44b): ERROR: PropertyTypeCoercion - 17:20 - $this->values expects 'list', parent type 'array' provided INFO: LessSpecificReturnStatement - 19:16 - The type 'array' is more general than the declared return type 'list' for Foo::fill INFO: MoreSpecificReturnType - 13:13 - The declared return type 'list' for Foo::fill is more specific than the inferred return type 'array' ```
danog commented 1 year ago

@zlodes this is a separate issue, and can be fixed by adding a @no-named-arguments annotation: https://psalm.dev/r/17620f76e6

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/17620f76e6 ```php $values */ private array $values = []; /** * @no-named-arguments * * @param list $strings * * @return list */ public function fill(string ...$strings): array { array_push($this->values, ...$strings); return $this->values; } } ``` ``` Psalm output (using commit 96cb44b): No issues! ```