vimeo / psalm

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

Cannot silence InvalidReturnType to allow return-less methods via stub files #4330

Open flaviovs opened 4 years ago

flaviovs commented 4 years ago

I'm getting this strange behaviour from Psalm when trying to silence InvalidReturnType via stub file overrides.

The method is Yii2's Widget::run(), and this is how its signature looks like:

    /**
     * @return string
     */
    public function run()
    {
    }

https://github.com/yiisoft/yii2/blob/7ff516063de41a9f75ee7ac01c5c7c364e5b10d4/framework/base/Widget.php#L206-L212

Without going into details of Yii2 widgets, suffice to say that as per the documentation overridden implementations are supposed to return a string to be printed by Yii2, or echo the printed string itself (in which case the method obviously doesn't need to return anything).

Obviously, the method signature in Yii2 is wrong. But now I have this large codebase where some implementations return strings, but many others echo the output directly and don't return anything, and when I run psalm I get InvalidReturnType - The declared return type 'string' for MyWidget::run is incorrect, got 'void' (see https://psalm.dev/011) from the latter.

My first guess to fix this was to override the return type by using @return void|string in a stub file. This change the error to InvalidReturnType - The declared return type 'null|string' for MyWidget::run is incorrect, got 'void'.

Using @return null|string gives the same error.

If I change the stub to just @return void, then the error go away. However, all other classes where the method does return a string now get a The declared return type 'void' for MyOtherWidget::run is incorrect, got 'string'

So it seems that when I use @return void Psalm does the right thing and change the method return type to allow void, but with void|string it changes it to null|string, which I suppose requires a return statement, and is not the same thing.

So I guess my questions are:

Psalm version is 3.17.1@8f211792d813e4dc89f04ed372785ce93b902fd1. requireVoidReturnType is set to false.

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

Hey @flaviovs, can you reproduce the issue on https://psalm.dev ?

flaviovs commented 4 years ago

Sorry - should have mentioned this: I could not reproduce this using a snippet, probably because it involves stub files. Closest approximation is https://psalm.dev/r/9a9e9a8323, but this gives me the error in the original base method, which in Yii2's case is outside of user's control.

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

I found these snippets:

https://psalm.dev/r/9a9e9a8323 ```php
weirdan commented 4 years ago

I think you should override the return type in a stub to @return ?string and then the following should work: https://psalm.dev/r/5c841fcfdd

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

I found these snippets:

https://psalm.dev/r/5c841fcfdd ```php
flaviovs commented 4 years ago

@weirdan, the snippet you sent change the base class provided by the framework (Yii2), which is outside user's control thus, in theory, cannot be changed.

Notice that the snippet I sent does not replicate the issue, because it involves the use of stub files which AFAIK cannot be used in psalm.dev.

Anyway, changing to @return ?string in the stub file didn't work either: derived class still get The declared return type 'null|string' for MyWidget::run is incorrect, got 'void'

It looks like Psalm correctly understand that @return void in stub files allows functions without return statements (or -- untested -- perhaps return with no values), whereas if you use @return void|string it internally translate to null|string which does require a return (at least a return null). I think this might be the bug.