Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix getAllInvolvedRawTypes() recursing infinitely #1276

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

codecholeric
Copy link
Collaborator

@codecholeric codecholeric commented Mar 24, 2024

This fixes the problem of recursing infinitely when calling JavaType.getAllInvolvedRawTypes()
for a recursive type parameter declaration (e.g. class Example<T extends Example<T>>).
It also adds a new public API to conveniently traverse JavaType signatures
without the need to add chains of instanceof checks and manually handle how to traverse further.

Resolves: #1237

@codecholeric codecholeric changed the title Fix get all involved raw types recursing infinitely fix <get all involved raw types recursing infinitely Mar 24, 2024
@codecholeric codecholeric changed the title fix <get all involved raw types recursing infinitely fix getAllInvolvedRawTypes() recursing infinitely Mar 24, 2024
@codecholeric codecholeric requested a review from hankem March 26, 2024 20:51
Copy link
Member

@hankem hankem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

}

default Result visitArrayType(JavaClass type) {
return CONTINUE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether the default implementation should be

Suggested change
return CONTINUE;
return visitClass(type);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but that would seem strange to me. Because the contract should be that visitClass is never called with an array type, right? (Otherwise, why even have visitClass and visitArrayType separately in the visitor? If you can't rely on isArray() to return false whenever visitClass is called?)
The traversal behavior will just traverse down to the component type by default, right? So implementing CONTINUE will finally call visitClass with the base component type, and that seems a reasonable behavior if I e.g. only implement visitClass but not visitArrayType 🤷

Copy link
Member

@hankem hankem Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the contract should be that visitClass is never called with an array type, right?

Let's document the contract. 😉

I was checking whether we could make more use of the new SignatureVisitor internally, and found JavaClassDependencies.dependenciesOfType(JavaType). SignatureVisitor won't be a nice solution there, but I realized that there might be more use cases where one previously had

        if (javaType instanceof JavaClass) {
            // do something with `(JavaClass) javaType`
        } else if (javaType instanceof JavaParameterizedType) {
            // do something with `(JavaParameterizedType) javaType`
        } else if (javaType instanceof JavaWildcardType) {
            // do something with `(JavaWildcardType) javaType`
        }

and it might not immediately obvious that refactoring this to the SignatureVisitor pattern requires overwriting both visitClass and visitArrayType for the do something with `(JavaClass) javaType` part.

So yes, the question is actually whether SignatureVisitor should have individual methods for array and non-array JavaClasses. Having only one might be more consistent (why just distinguish arrays, and not, e.g. interfaces?) and therefore less confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see your point, I just figured it would be kind of nice that the default behavior would traverse up the component types and automatically only give you the element type (as I found the JLS calls our "base component type" 🤪). But maybe that in the end creates a more confusing API instead of helping 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One argument for the separate visitArrayType method might be that it's somewhat consistent with visitGenericArrayType 🤔 But I guess there the class hierarchy really is also distinct 🤷
Guess I'm gonna remove the array type method then...

docs/Dockerfile Show resolved Hide resolved
@codecholeric codecholeric force-pushed the fix-getAllInvolvedRawTypes-recursing-infinitely branch 2 times, most recently from 75a50b5 to 07bf2cd Compare April 7, 2024 22:48
After switching my job the old email address is now dead.

Signed-off-by: Peter Gafert <peter.gafert@archunit.org>
The Reflection API considers the visibility of an array type class object
to be equal to the visibility of the element type class object.
ArchUnit so far considered every array type class object as public.
Since we usually strive for being as similar to the Reflection API as possible,
we fix this now by deriving the visibility from the element type as well.

Signed-off-by: Peter Gafert <peter.gafert@archunit.org>
At the moment, handling `JavaType`s is not convenient,
because there are many different subtypes.
And effectively the only way to handle those is a chain of `instanceof`
checks with individual handling for each type.
We can make this more convenient by adding a visitor pattern API
that allows to simply define what to do on every partial type encountered
in the signature (i.e. what to do when a parameterized type is encountered,
what to do when each actual type argument of the parameterized type is encountered,
and so on).
As a benefit, we can use this API in the next step to fix the infinite
recursion problem we have for the `getAllRawTypes()` method at the moment.
Because, there we haven't handled the case where type variables are defined
recursively. Adding this visitor pattern API we can solve this problem
in a generic way once at the infrastructure level, i.e. implement the
traversal correctly once and utilize it in more specific use cases.

Signed-off-by: Peter Gafert <peter.gafert@archunit.org>
For recursive type parameter definitions like `class Example<T extends Comparable<T>>`
the method `JavaClass.getAllInvolvedRawTypes()` causes a `StackOverflowError`
at the moment. This is because we have a simple recursion without any short circuit
condition if we encounter a type we've already seen before.
We can utilize `JavaType.traverseSignature(..)` to solve this,
because there the problem is already solved in the visitor pattern implementation.

Signed-off-by: Peter Gafert <peter.gafert@archunit.org>
@codecholeric codecholeric force-pushed the fix-getAllInvolvedRawTypes-recursing-infinitely branch from 07bf2cd to cc5fec5 Compare April 9, 2024 21:21
@codecholeric codecholeric merged commit c644d0c into main Apr 9, 2024
21 checks passed
@codecholeric codecholeric deleted the fix-getAllInvolvedRawTypes-recursing-infinitely branch April 9, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JavaType#getAllInvolvedRawTypes() throws StackOverflowError with self-referential generic types
2 participants