vimeo / psalm

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

Psalm doesn't understand `{@inheritDoc}`, extending library classes leads to erased generic types #6202

Open fluffycondor opened 3 years ago

fluffycondor commented 3 years ago

This is a simplified snippet of Doctrine's ArrayCollection: https://psalm.dev/r/562ef4433e When you try to extend ArrayCollection, you got erased generic types in all methods that documented as {@inheritDoc}. If you replace /** {@inheritDoc} */ on line 27 with /** @return array<TKey, T> */, everything works as expected.

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

I found these snippets:

https://psalm.dev/r/562ef4433e ```php */ public function toArray(); } /** * @template TKey of array-key * @template T * @implements Collection */ class ArrayCollection implements Collection { /** @var array */ private array $elements; /** @param array $elements */ public function __construct(array $elements) { $this->elements = $elements; } /** {@inheritDoc} */ public function toArray() { return $this->elements; } } /** * @template TKey of array-key * @template T * @extends ArrayCollection */ class MyArrayCollection extends ArrayCollection {} interface Foo {} class Bar implements Foo {} class Baz { /** @var MyArrayCollection */ private MyArrayCollection $bars; /** * @param list $bars */ public function __construct(array $bars) { $this->bars = new MyArrayCollection($bars); } /** * @return array */ public function getBars(): array { return $this->bars->toArray(); } } ``` ``` Psalm output (using commit 89ee1f1): INFO: MixedReturnTypeCoercion - 61:16 - The type 'array' is more general than the declared return type 'array' for Baz::getBars INFO: MixedReturnTypeCoercion - 57:16 - The declared return type 'array' for Baz::getBars is more specific than the inferred return type 'array' ```
orklah commented 3 years ago

I vaguely remember reading that docblock with templates are not inherited by design but I couldn't find the source.

However, I think it would be really dangerous to do.

T in Collection::toArray docblock may not have the same signification than T in ArrayCollection::toArray because they're just letters. What about https://psalm.dev/r/5bde6e0da5? Should it be an error? Should it realize T was overrided by V in @implements and replace it by V automatically?

I really think you're better of repeating the docblock in the child

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

I found these snippets:

https://psalm.dev/r/5bde6e0da5 ```php */ public function toArray(); } /** * @template TKey of array-key * @template V * @implements Collection */ class ArrayCollection implements Collection { /** @var array */ private array $elements; /** @param array $elements */ public function __construct(array $elements) { $this->elements = $elements; } /** {@inheritDoc} */ public function toArray() { return $this->elements; } } /** * @template TKey of array-key * @template T * @extends ArrayCollection */ class MyArrayCollection extends ArrayCollection {} interface Foo {} class Bar implements Foo {} class Baz { /** @var MyArrayCollection */ private MyArrayCollection $bars; /** * @param list $bars */ public function __construct(array $bars) { $this->bars = new MyArrayCollection($bars); } /** * @return array */ public function getBars(): array { return $this->bars->toArray(); } } ``` ``` Psalm output (using commit 89ee1f1): INFO: MixedReturnTypeCoercion - 61:16 - The type 'array' is more general than the declared return type 'array' for Baz::getBars INFO: MixedReturnTypeCoercion - 57:16 - The declared return type 'array' for Baz::getBars is more specific than the inferred return type 'array' ```
fluffycondor commented 3 years ago

Should it be an error? Should it realize T was overrided by V in @implements and replace it by V automatically?

It's a complicated question. From my point of view, if you pass V to @implements, then it should be mapped to parent's T. What if you implement two interfaces like this: https://psalm.dev/r/7da72473ce You have no choice but to rename one template.

But if you consider {@inheritDoc} as a dangerous behavior that shouldn't work, it works right now: https://psalm.dev/r/26c8d82b95 It is broken in the example in the first post, when you extends a class that have {@inheritDoc}, but the class itself works fine, and generic annotations are inherited.

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

I found these snippets:

https://psalm.dev/r/7da72473ce ```php * @implements Bar */ class Baz implements Foo, Bar {} ``` ``` Psalm output (using commit 6c475e0): No issues! ```
https://psalm.dev/r/26c8d82b95 ```php */ public function toArray(); } /** * @template TKey of array-key * @template T * @implements Collection */ class ArrayCollection implements Collection { /** @var array */ private array $elements; /** @param array $elements */ public function __construct(array $elements) { $this->elements = $elements; } /** {@inheritDoc} */ public function toArray() { return $this->elements; } } interface Foo {} class Bar implements Foo {} class Baz { /** @var ArrayCollection */ private ArrayCollection $bars; /** * @param list $bars */ public function __construct(array $bars) { $this->bars = new ArrayCollection($bars); } /** * @return array */ public function getBars(): array { return $this->bars->toArray(); } } ``` ``` Psalm output (using commit 6c475e0): No issues! ```