Better detection of circular references

[herndlm]

Hi,

indirectly, via phpstan user report, I found out that there is valid PHP code that triggers endless recursion in Better Reflection.
E.g. circular class or interface extends statements like

class TestClass extends TestClass {}

class Foo extends Bar {}
class Bar extends Foo {}

This code will parse just fine, but lead to errors like Class "TestClass" not found, see https://3v4l.org/YIQNl

There are currently 2 cases for that specific example that I found that are problematic

1. ReflectionClass::getParentClass() will never return null and consumers can recurse forever. This is kind of expected IMO and fixing it screams BC break or am I wrong?
2. Everything that internally uses ReflectionClass::getInheritanceClassHierarchy(), which in term uses getParentClass to find the root parent, leads to endless recursion. E.g. ReflectionClass::getInterfaces(), IMO this could be already "fixed" without a BC break? But if getParentClass would be able to deal with it then nothing needs to be done here additionally I assume.

The second one would be easy to prevent in Better Reflection directly, I did not dig deep enough yet to have an idea for the first one. And by preventing I mean something as throwing a CircularReferenceException, or do you have any better ideas?

Ondrej also mentioned potential other problematic things via phpstan/phpstan#7787 (comment) and phpstan/phpstan#7787 (comment)

What do you think? Looking forward to your opinions. Also, I'm happy to work on anything related to this.

[herndlm]

The handling of this kind of feels related to #69 (finding more than one unique symbol). If e.g. exceptions are decided to be used I could try to implement that too.

[Ocramius]

Unsure about how this one should be handled. #69 is about "locating" sources, while this one is about circular references in general.

What happens if you have A -> B -> C -> A, and in separate files?

I don't think we should complicate this for code that is invalid upfront. Note that BetterReflection already crashes on code that cannot be parsed.

[herndlm]

Right, those 2 issues are not related much. I only thought both could lead to exceptions thrown.

With A -> B -> C -> A in separate files the same thing happens. E.g. getParentClass() can be called forever, getInterfaceNames leads to a segfault / endless recursion.

The argument about invalid code is understandable. I thought Better Reflection should be able to somehow handle all code that is valid in terms of it can be parsed. With having static code analysis using Better Reflection in mind that made sense to me. But yeah, this code will lead to a fatal error if being executed and trying to handle this might add complexity here.
Avoiding the endless recursion via e.g. getInterfaceNames would still be quite simple though, even if it's not a "handle circular refs" everywhere approach I guess.
Hard one 🤔

[herndlm]

Any new ideas here? I didn't come up with anything so far.. The question is: should BR detect this or not?
If you say it shouldn't, I'm fine with closing this, I don't want to increase the issue counter for no reason :) I'm sure this can be somehow detected in consumer code as well.

[Ocramius]

IMO put it in a failing test PR first.

Then discuss from there.