Incorrect assumption with static and inheritance

[iquito]

Bug report

I think PHPStan does not correctly process the return type "static" in some circumstances. This bug appeared in v1.4.3 and still occurs with the latest version, v1.4.6:

Code snippet that reproduces the problem

The problem only occurs when using inheritance and if a base class returns "static" (as far as I can tell), this is an example (as simple as I managed to get it to, similar to how it was generated in my code):

https://phpstan.org/r/ce9ee7ac-0301-49fa-9c33-80f68e9d8a64

Expected output

There should be no errors, as PHPStan knows $entry has the type Account, not object. With PHPStan v1.4.2 and before it worked as expected.

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

Thanks for your work, I do appreciate PHPStan and how it improves code quality!

[ondrejmirtes]

Hi, you have multiple @implements \IteratorAggregate<..., ...> in your hierarchy so it can happen that the other one gets picked up.

The right solution here is to make your parent classes generic: https://phpstan.org/r/ddf12918-9b8a-45d2-b825-9fe128085580

[iquito]

Your solution leads to two new errors though:

Method BaseIterator::current() should return T of object but returns stdClass.
Method BaseEntries::getIterator() should return BaseIterator<T of object> but returns BaseIterator<object>.

[ondrejmirtes]

Yes, because I don't know how your implementation actually looks like and how to fix it. With this code these iterators are basically infinite.

[iquito]

My code does look about like that, just with more methods and functionality - both the base classes and the specific classes are used. Why would they be infinite? Either the only guarantee is that you get an object (BaseEntries, BaseIterator), or otherwise you get a specific type (Entries, SpecificIterator).

[ondrejmirtes]

You'd be able to get rid of the errors by having a constructor in the class which isn't unrealistic. PHPStan needs some way to know that you'll really provide T that you're promising.

Generics want you to provide as specific type as possible. More info here: https://phpstan.org/blog/generics-in-php-using-phpdocs

[iquito]

I guess I will try it, thanks for the feedback. A pity though that you do not recognize multiple IteratorAggregate definitions and PHPStan picking the wrong one as a bug, as in my opinion this is an easier syntax for people reading the code to understand (and can be used with existing libraries where you cannot change base classes and their PHPDocs). Both PHPStan and Psalm have had no problem with the existing definitions, and PHPStan only since v1.4.3 - it worked for years before that.

[ondrejmirtes]

where you cannot change base classes and their PHPDocs

You can: https://phpstan.org/user-guide/stub-files

had no problem with the existing definitions

This is probably what changed:

• Fix iterable key and value type - prefer generic type variables instead of key()/current() typehints (phpstan/phpstan-src@a2acf64), PHPStan thinks loop element can be false when looping custom Iterator implementation #5817

So the generics were always read in a different order, but your problem was hidden thanks to the typehints in current().

TBH I consider having @implements IteratorAggregate<..., ...> multiple times in a hierarchy with different types to come with an undefined behaviour, that's why I offered a fix with a code that I consider correct.

[iquito]

Why undefined behavior though? To me this is like any covariance type according to LSP, in that narrowing the type in a subclass from object to a specific type like Account is a reasonable thing to do - you can do that with return types too.

[ondrejmirtes]

Yeah but when class A implements B, C and B has IteratorAggregate<int, Lorem>, and C has IteratorAggregate<int, Ipsum>, it's not defined which one will be chosen. PHPStan currently doesn't have any prioritization/sorting of that.

[iquito]

With IteratorAggregate PHPDocs in interfaces you could add multiple interfaces to a class and have ambiguity, but that is not my example and in that case it would be reasonable to force class A to define its own IteratorAggregate definition to clarify or disregard conflicting IteratorAggregate PHPDocs. With actual classes implementing and documenting IteratorAggregate (as in my example) you don't have any ambiguity, as each class has to live up to its own definition and satisfy it, and each class can only extend one other class. There having the basic principles of inheritance is enough, as covariance already defines on how it should behave and makes sufficient guarantees.

[ondrejmirtes]

But it's not always covariance. You can also have T in a parameter, and there it's contravariance.

[iquito]

But not with just Iterator and IteratorAggregate - there you only have return types, and that is my example. If then you add more custom methods with parameters and the same type, then it would be a completely different example.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.