Type check conditions should override out-of-scope DocBlock

AnthonyANI

The code analyzer does determine variable type using type check conditions such as $myVar instanceof MyClass. And while it would make sense for a DocBlock to be able to override this inside the scope of a block with such a condition, currently the analyzer is overriding the the condition with DocBlocks outside of the scope of the block.

/** @var AbstractMyObj */
private $myVar;

function performAction()
{
    if ($this->myVar instanceof MyExtendedObj) {
        $this->handleMyExtendedObj($this->myVar); // Argument '1' passed to handleMyExtendedObj() is expected to be of type MyExtendedObj, AbstractMyObj given
    }
}

function handleMyExtendedObj(MyExtendedObj $myExtendedObj) { ... }

JakubMisek

AnthonyANI good point!

I'm adding this to our to-to list. It's causing a lot of false positives.

AnthonyANI

Wanted to follow up on this since behavior appears to have changed. It appears the PHP code analyzer is now handling instanceof properly, but does not enforce a type if the type is the parent of a child. In this case, AbstractMyObj is the parent being passed as an argument which should be a child which extends it called MyExtendedObj. Shouldn't $this->myVar be required to be type MyExtendedObj (or child of) or have an instanceof MyExtendedObj (or child of) check?

JakubMisek

AnthonyANI Thank you for the test case; you are absolutely right, but -

We're - by purpose - less strict in this case because it is a common practice to poorly annotate types (sadly even in large PHP frameworks).

In this case, the type inferring engine deliberately checks for type inheritance and suppresses warnings in case the type is a subclass or superclass of the target type.

I'm keen to improve this behavior - I'm thinking about a more specific setting, something like analysis-strictness or a separate diagnostic code for this kind of diagnostic (?).

What do you think, @AnthonyANI?

AnthonyANI

JakubMisek That makes sense. A way to enable a stricter setting like that would be fantastic to have for our project, since we do often mean that we want a child class rather than a parent for certain things. This would help us catch and eliminate logic bugs. Perhaps the setting could respect an ignore file/setting so that we don't have such errors for vendor folders for example if we are doing code analysis on the entire project? Thanks for your flexibility!

reio

JakubMisek It would be wonderful if analysis strictness could be aligned with say phpstan levels (or psalm, or both).