vimeo / psalm

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

False positive InvalidReturnType for method that always throws #9604

Open derrabus opened 1 year ago

derrabus commented 1 year ago

Given the following code:

class Logger
{
    public static function logCall(string $method): void
    {
    }
}

abstract class MyAbstractClass
{    
    /**
     * @return string
     */
    public function doSomething()
    {
        Logger::logCall(__METHOD__);

        throw new Exception('Not supported.');
    }
}

https://psalm.dev/r/b8245d5f63

Psalm reports:

ERROR: InvalidReturnType - 13:16 - The declared return type 'string' for MyAbstractClass::doSomething is incorrect, got 'never'

I would argue that the return type is still correct, it's just that the return type inferred by Psalm is more specific. The problem here is that this method is overridden by child classes and those implementations actually return a string. If I would document @return never here, the child implementation would illegally widen the return type from never to string.

Note that the error goes away if I…

Both should be pretty much irrelevant for the issue. 🤔

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

I found these snippets:

https://psalm.dev/r/b8245d5f63 ```php
orklah commented 1 year ago

ping @kkmuffme I believe this is linked to your changes around never

kkmuffme commented 10 months ago

Similar to/Inverse of https://github.com/vimeo/psalm/issues/10377

The underlying problem is, that for @return we use explicit never to indicate a function exits, as there is no other way to annotate this. Therefore here the inverse happens - it reports it, as the function always exits. I'm aware of the problem with extending classes, but there isn't a way around it Fyi if you only had the throw https://psalm.dev/r/70d79e581f I already implemented that it won't report an error.

I suggest to just suppress the error, as this cannot be fixed. Fixing this, would mean that in case you have a function where you actually always want to return never, would not get reported in extending classes - which would be a bigger problem.

psalm-github-bot[bot] commented 10 months ago

I found these snippets:

https://psalm.dev/r/70d79e581f ```php
derrabus commented 10 months ago

Thank you for your answer. That's of course not very satisfying, but I think I can live with ignoring this error.