Type hierarchy with conditional type not recognized correctly

[derrabus]

Bug report

In Doctrine ORM, we conditionally extend an abstract class if it exists. If it doesn't, we directly implement the interface that was previously implemented by that abstract class. This code is part of a compatibility layer and allows us to remain compatible with two major versions of a library. PHPStan is run with a set of dependencies where that abstract class does not exist anymore.

Until PHPStan 1.6, this was fine. PHPStan 1.7 however complains, if we use an instance of that conditionally declared class where an implementation of the interface is expected. I could reduce our scenario to the following snippet.

Code snippet that reproduces the problem

interface SomeContract {}

if (class_exists(LegacyAbstractClass::class)) {
	class MyClass extends LegacyAbstractClass {}
} else {
	class MyClass implements SomeContract {}
}

function foo(SomeContract $param): void {
}

foo(new MyClass);

https://phpstan.org/r/d1be003c-fdb0-40ee-86b9-077dc70163bf

Expected output

The code should pass without errors.

Fun fact: If I flip the if/else condition, the code passes, although both versions should be equivalent.

// […]
if (!class_exists(LegacyAbstractClass::class)) {
	class MyClass implements SomeContract {}
} else {
	class MyClass extends LegacyAbstractClass {}
}
// […]

https://phpstan.org/r/2e69dca7-ffb4-46a8-abc4-b206660b810d

It seems as if PHPStan ignored the condition, picked the first declaration and discard the second one.

Did PHPStan help you today? Did it make you happy in any way?

The new release found some previously undiscovered issues as well. I'm pretty happy about that. Keep up the good work! ❤️

[ondrejmirtes]

PHPStan has no way of modeling that a class might have two different definitions.

It seems as if PHPStan ignored the condition, picked the first declaration and discard the second one.

Yes, that's exactly what's happening :)

It might be possible to fix in the future, because I see this problem very close to "A class might have different definitions across different PHP versions" so we MIGHT fix this in the future, but it'd require significant rewrite of PHPStan internals.

[derrabus]

Got it. And I assume the reason that it worked in 1.6 was because runtime reflection was used and the PHP interpreter picked the correct path automatically. For the time being, is there any way I could write that code differently to help PHPStan understand better, what's going on here?

[dmaicher]

I have a similar issue for class aliases like

https://github.com/googleapis/google-api-php-client-services/blob/main/src/Oauth2.php#L137

So referencing \Google_Service_Oauth2::USERINFO_EMAIL as an alias for \Google\Service\Oauth2::USERINFO_EMAIL fails with phpstan 1.7.1 (undefined constant) and worked fine with 1.6.9.

I guess this happens for the same reason? Or should I open a separate issue?

[ondrejmirtes]

@dmaicher Please see https://github.com/phpstan/phpstan/issues/7296.

[VincentLanglet]

Fun fact: If I flip the if/else condition, the code passes, although both versions should be equivalent.

Is there any drawback to flip the if/else in the ORM code ? If it solve the following static analysis error https://phpstan.org/r/d1be003c-fdb0-40ee-86b9-077dc70163bf, it might be better to flip it then (and it promote the new definition of the class). WDYT @derrabus ?

[derrabus]

In the ORM's CI, we perform multiple PHPStan runs with different sets of dependencies. This means that in our case, both branches of the if/else block are covered in different runs. If I flip the condition, I'd only move the failure into a different PHPStan run.

[VincentLanglet]

In the ORM's CI, we perform multiple PHPStan runs with different sets of dependencies. This means that in our case, both branches of the if/else block are covered in different runs. If I flip the condition, I'd only move the failure into a different PHPStan run.

With

if (class_exists(LegacyAbstractClass::class)) {
	class MyClass extends LegacyAbstractClass {}
} else {
	class MyClass implements SomeContract {}
}

The error occurs if the dependencies are up to date

With

if (!class_exists(LegacyAbstractClass::class)) {
	class MyClass implements SomeContract {}
} else {
	class MyClass extends LegacyAbstractClass {}
}

The error occurs if the dependencies are not up to date

For the user, I would say it's better to not have the phpstan error when using the latest version of the dependencies.

[derrabus]

Yeah maybe it's better to flip that condition in the long run. Right now, the adoption of Persistence 3 is not really high yet and might be blocked by other dependencies.

[phpstan-bot]

@derrabus @VincentLanglet After the latest push in 1.11.x, PHPStan now reports different result with your code snippet:

@@ @@
-14: Parameter #1 $param of function foo expects SomeContract, MyClass given.
+-1: Reflection error: LegacyAbstractClass not found.
+ 6: Reflection error: LegacyAbstractClass not found.
+ 6: Reflection error: LegacyAbstractClass not found.
+ 6: Reflection error: LegacyAbstractClass not found.
+ 6: Reflection error: LegacyAbstractClass not found.
+ 6: Reflection error: LegacyAbstractClass not found.
+ 6: Reflection error: LegacyAbstractClass not found.
+14: Reflection error: LegacyAbstractClass not found.

Full report

Line	Error
-1	Reflection error: LegacyAbstractClass not found.
6	Reflection error: LegacyAbstractClass not found.
6	Reflection error: LegacyAbstractClass not found.
6	Reflection error: LegacyAbstractClass not found.
6	Reflection error: LegacyAbstractClass not found.
6	Reflection error: LegacyAbstractClass not found.
6	Reflection error: LegacyAbstractClass not found.
14	Reflection error: LegacyAbstractClass not found.

[phpstan-bot]

@derrabus After the latest push in 1.11.x, PHPStan now reports different result with your code snippet:

@@ @@
-No errors
+8: Reflection error: LegacyAbstractClass not found.
+8: Reflection error: LegacyAbstractClass not found.
+8: Reflection error: LegacyAbstractClass not found.
+8: Reflection error: LegacyAbstractClass not found.
+8: Reflection error: LegacyAbstractClass not found.

Full report

Line	Error
8	Reflection error: LegacyAbstractClass not found.
8	Reflection error: LegacyAbstractClass not found.
8	Reflection error: LegacyAbstractClass not found.
8	Reflection error: LegacyAbstractClass not found.
8	Reflection error: LegacyAbstractClass not found.

[derrabus]

Thanks, dear bot, but that's still not the output we're looking for. :v: