Javaparser: Recursion in JavaParserClassDeclaration canBeAssignedTo if java.lang.Object is parsed from source

[clippka]

I assume this part of the merge removed a previously integrated fix by accident?

https://github.com/javaparser/javaparser/commit/f1a4ec769d104ad6e8bf15356f001d451ad7ad76#diff-14715e250b8f60634e3d67c23aeb907dL245

With javaparser-core-3.15.18 I again see an issue where canBeAssignedTo is endlessly called

[MysterAitch]

Hi, I'm a little confused here so please do help me along a little! :)

I can see that #2494 is the PR where this particular block of code was removed (fixing #2387), and in master I can see that the testcases introduced are still present.

If I'm getting the wrong end of the stick where my understanding is wrong and it's a different thing that has come back, please do give a bit more context. Maybe a failing testcase or a link to the original issue perhaps?

Cheers

[clippka]

Hi @MysterAitch,
thanks for looking into this issue. I will try to create a testcase to reproduce this but may take some time as I'm running javaparser against a large proprietary source set and not sure yet what is the trigger for this issue.

The thing I can see in the debugger is that at one point i'm inside JavaParserClassDeclaration canBeAssignedTo and this is a JavaParserClassDeclaration for "java.lang.Boolean" and other is one for "java.lang.String". Calling "getSuperClass().getTypeDeclaration()" then goes to a JavaParserClassDeclaration for "java.lang.Object" and calling "getSuperClass().getTypeDeclaration()" on that result in the same instance, meaning superclass == this

Maybe I setup something wrong but I can't figure my head around how I could lead to a runtime class beeing its own superclass

Thanks

[clippka]

Ok I see the issue now. JavaParserClassDecleration getSuperClass() always return a new ReferenceTypeImpl for java.lang.Object if "wrappedNode.getExtendedTypes().isEmpty()". So any recursive calling loop to getSuperClass() will end in an endless loop if there is no other condition to end the loop. So maybe the correct fix here is that getSuperClass() for a "java.lang.Object" should return null

[clippka]

I looked at the JavaParserClassDeclarationTest and debuged into testCanBeAssignedTo()
The difference I see is that in this test Case, "java.lang.Object" is resolved in a ReflectionClassDeclaration which canBeAssignedTo() implementation fails the "this.clazz.getSuperClass() != null" condition that breaks the loop at the correct instance.

Now my project includes the java runtime objects from source, resulting in having a JavaParserClassDeclaration for "java.lang.Object" which implementation of canBeAssignedTo() does not have an exit condition for reaching "java.lang.Object".

Not sure how to write a test case for this scenario without having the java sources at hand in the test case...

[clippka]

I have made a pull request containing a new test case and a fix for the issue
https://github.com/javaparser/javaparser/pull/2608

I hope the unit test is not to sketchy but I'm not sure how to reproduce this issue better. At least it replicates the problem.

Initially I wanted to fix JavaParserClassDeclaration getSuperClass() to return null if self is already representing java.lang.Object. IMHO this is the correct fix to be consistent with the behaviour of ReflectionClassDeclaration. But there are many "getSuperClass().getTypeDeclaration()" calls in the source that will throw exceptions if getSuperClass() ever returns null. I assume this is something for someone with a better understanding of the overall code to decide if this is an issue (that only happens if you happen to parse java.lang.Object from source, which may be an edge case)

Let me know if there is any problem with the pr

[MysterAitch]

Really appreciate you following up on this and including your thoughts along the way -- it makes it easy to follow!

While I can't yet comment specifically on the PR (having a fly-by read/comment and not yet had a proper read through but prima facie it seems good) I think my preference based on my understanding of the coding guidelines is to avoid returning null wherever possible, and instead return an optional where a value might be absent.

Thinking out loud: Would implementing this help to highlight this case more clearly, by making it explicit that not every class will have a superclass?

If that's something you're happy to run with then great please do so, but if not imo it's better to fix the bug now with your PR and we can worry about the architecture in a separate issue :)

[clippka]

Personally I have no preference if getSuperClass() should return null if there is no superclass or an optional. As I said if it would return null it then would be consistent to ReflectionClassDeclaration which already returns null in that case. Personally I see no reason why not to return an Optional here but as said before there are multiple places in the code that currently rely on getSuperClass() never to return null (just grep for "getSuperClass().getTypeDeclaration()") and as I'm only a consumer of this code base I would not dare to suggest changes of that magnitude :)

I prefer using my fix to solve the issue at hand and have a different issue tracking the inconsistency between getSuperClass() implementation of JavaParserClassDeclaration and ReflectionClassDeclaration

[MysterAitch]

if I understand you correctly, agreed - let's keep it bug-free and consistent for now, then iterate on it in another issue :)

unless there's anything glaring in the PR that I come across later, I don't see any reason this won't be included in the next release! _(after which you'll be a contributor, not just a consumer! :wink: )_