Wrong 'deadCode' message on possibly overriden function

[vbifial]

Code Example

<?php
class Foo
{
    /**
     * @return string
     */
    public function test()
    {
        throw new \Exception();
    }
    /**
     * @return self
     */
    public static function getInstance()
    {
        // some code like
        // return new static();
        // commented for type to be determined by annotation
    }
}

class Bar extends Foo
{
    public function test()
    {
        return 'x';
    }
    public function run()
    {
        // Linter assume type as 'Foo', but it could be 'Bar' as well
        $object = static::getInstance();
        echo $object->test();
        echo 'end';
    }
}

Actual Behavior

<critical> INFO    deadCode: Unreachable code at Sample.php:30
        echo 'end';
        ^^^^^^^^^^^

Expected Behavior

No deadCode message

[YuriyNasretdinov]

Does it work correctly if you specify "@return static" instead of "@return self"? Because in my understanding "self" always refers to only one class, Foo, so without the actual type information (e.g. without code that does "return new static();" in the body) I think the behaviour is actually correct.

[vbifial]

Maybe it is bad example. It's about case, where expected type is Foo, but actual type is one of Foo's child classes. With presence of method 'test' implementations, which not interrupt caller's code execution, assumption "method 'test' always throw exception" looks wrong to me.

[YuriyNasretdinov]

Could you please provide a real-world example where this happens? The exit flags should correspond to the method that was found. I think I understand what you mean though but having a better example would help to find a good solution. Thanks

[vbifial]

I tried to make example closer to code I've seen in real closed source project. Classname for Builder usually stored in config file. I wouldn't write code like this myself. Ofcource we can do make Service and test() abstract. But in some cases in existing projects it cannot be done easily, somewhere class Service could been directly instantiated.

<?php

class Service
{
    /**
     * @return string
     */
    public function test()
    {
        throw new Exception();
    }
}

class ServiceA
{
    public function test()
    {
        return 'stringA';
    }
}

class ServiceB
{
    public function test()
    {
        return 'stringB';
    }
}

class Builder
{
    public $className = ServiceA::class;

    /**
     * @return Service
     */
    public static function getService()
    {
        return new $className();
    }
}

class Runner
{
    public function run()
    {
        $service = Builder::getService();
        echo $service->test();
        echo 'test';
    }
}

In this code line echo 'test'; is marked as 'unreacheble' by noverify.

[YuriyNasretdinov]

Ok, in this case you still specified in the PHPDoc for Builder->getService() that it returns Service and not ServiceA|ServiceB, and because it is not (yet?) possible for noverify to analyse all possible values of the variables it can't figure out which classes are actually returned.

So, basically there isn't enough information for noverify to come to any other conclusion but to say that $service will always be instance of Service and it always exits, so it says that the code below it is unreachable.

So if phpdoc supported specifying something like '@return ancestorOf(Service)', that maybe at least allowed it to handle this case somehow, but I don't think there is a standard way to do this, so...

I can suggest two possible options how to make noverify stop complaining, but those are the changes that you need to make in your codebase, as I don't think we can do much from the noverify side in this case sadly:

1. Annotate that getService() returns "ServiceA|ServiceB" instead of just Service.
2. Or wrap "throw new Exception ()" in "if(true) { throw new Exception ();". I don't think noverify evaluates constant expressions yet so it won't be able to tell that this branch is always executed. This may change, however, so it is not a reliable solution at all.