vimeo / psalm

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

Nonsensical error messages such as "expects empty, int(0) provided." when appending to empty template #3441

Open ostrolucky opened 4 years ago

ostrolucky commented 4 years ago

Details in https://github.com/vimeo/psalm/issues/3436, but I'll repost reproducer here https://psalm.dev/r/be559962d4.

This should ideally not be an error, Psalm should retype such empty collection to first non-empty value. Second best scenario, message should be definitely improved, normal user has no chance to understand how to fix code with such message.

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

I found these snippets:

https://psalm.dev/r/be559962d4 ```php */ private $elements; /** * @psalm-param array $elements */ public function __construct(array $elements = []) { $this->elements = $elements; } /** * @return void * * @psalm-param TKey $key * @psalm-param T $value */ public function set($key, $value) { $this->elements[$key] = $value; } } $coll = new ArrayCollection(); $coll->set(0, 'wat'); ``` ``` Psalm output (using commit f5a0622): ERROR: InvalidScalarArgument - 35:12 - Argument 1 of ArrayCollection::set expects empty, int(0) provided ERROR: InvalidScalarArgument - 35:15 - Argument 2 of ArrayCollection::set expects empty, string(wat) provided ```
weirdan commented 4 years ago

This should ideally not be an error, Psalm should retype such empty collection to first non-empty value.

Collections are just a generic classes to Psalm. Altering the type based on calls to some random method seems far-fetched, unless the method has @psalm-assert docblock.

The message could be improved for sure. I have yet to see where inferred empty could be useful in the context of template params, so maybe it may make sense to flag that immediately, not deferring the issue to the first call.

ostrolucky commented 4 years ago

Why do you say random method? Psalm should know which methods are mutating TKey and T.