Phan sometimes fails to infer the inherited method's return type from definition's return statements

[Daimona]

Preamble: I saw several LSB-related issues, but I can't tell if this is a duplicate of any of them. I also can't assess how many different bugs are involved below, so I'm just creating a single issue; feel free to split it if it needs to be.

This is a bug I came across using phan APIs for the security check plugin. Take the following code:

class A {
	public function __construct() {
		echo "A constructor";
	}
	public static function getStaticInstance() {
		return new static;
	}
}
class B extends A {
	public function __construct() {
		echo "B constructor";
	}
}

$b = B::getStaticInstance();

function test( string $arg ) {
	echo $arg;
}

test( $b );

$b is an instance of B, so calling test( $b ) should issue a warning, but Phan stays silent.
For this specific case, I guess phan is not inferring any type for $b (since inferring A should still result in a warning). Maybe some known limitation?
If I add a docblock to getStaticInstance with @return static, I get the wanted PhanTypeMismatchArgument issue, where $b is reported to be \A|\B. Which starts to make sense, although in this specific case it can only be instanceof B. Also, maybe phan should be able to infer the return type even without the docblock, given this is a simple case.

---

Looking at it from another perspective (i.e. the plugin), inside visitNew I have the following code:

$ctxNode = new ContextNode(
	$this->code_base,
	$this->context,
	$node
);

$constructor = $ctxNode->getMethod(
	'__construct',
	false,
	false,
	true
);

This code works fine for simpler cases (e.g. explicit class name, self, and parent), but running on the code snippet above results in $constructor being A's constructor. This happens whether I add the docblock to getStaticInstance or not. Now, I'm unsure if it's the same problem as above or I'm just doing it the wrong way, but I thought it'd be worth mentioning.

[TysonAndre]

I'm guessing that it might be that phan isn't recursively determining the type (i.e. opposite of --quick) when a method is inherited (from A to B). It warns about A::getStaticInstance()

$b = B::getStaticInstance();
$a = A::getStaticInstance();

function test( string $arg ) {
	echo $arg;
}

test( $b );  // This fails to warn
test( $a );  // This correctly warns

I get the wanted PhanTypeMismatchArgument issue, where $b is reported to be \A|\B

That's deliberate, it's printing the expanded type with all parent types to show that no part of the type is compatible

[TysonAndre]

This is a consequence of how phan infers the types for methods.

1. It sets the types of the analyzed method when the return statements are analyzed
2. Phan only does this analysis for the defining method to avoid duplicate error messages, etc

One solution might be to make the methods in subclasses inherit the types of the defining method unless something else gets called (e.g. via plugin) to explicitly set the return type of B::getStaticInstance()

https://github.com/phan/phan/blob/1.2.3/src/Phan/Analysis/PostOrderAnalysisVisitor.php#L1174-L1219

[Daimona]

I get the wanted PhanTypeMismatchArgument issue, where $b is reported to be \A|\B

That's deliberate, it's printing the expanded type with all parent types to show that no part of the type is compatible

Alright, that makes sense.

This is a consequence of how phan infers the types for methods.

1. It sets the types of the analyzed method when the return statements are analyzed
2. Phan only does this analysis for the defining method to avoid duplicate error messages, etc

Thanks for the explanation! So IIUC, the PR above will make Phan complain for the initial snippet even without the docblock, while the behaviour of ContextNode::getMethod will remain unchanged, right?

[TysonAndre]

the PR above will make Phan complain for the initial snippet

Yes, it will (expected string but got A for B::getStaticInstance()). Unfortunately, that would make it infer A because that's what was inferred analyzing A::getStaticInstance(), but you can fix it by adding the /** @return static */ comment.

while the behaviour of ContextNode::getMethod will remain unchanged, right?

yes