google
Make TypeVisitor public
API(s)
com.google.common.reflect.TypeVisitor
How do you want it to be improved?
Make it public
Why do we need it to be improved?
This is a very useful class for e.g. computing class distances in the class-hierarchy (we use this to do runtime factory matching), checking if a type is concrete and several other use cases. However, currently it's not accessible AND because it's relatively simple it's awkward to make an alternative implementation without triggering copyright scanners.
Example
In our case we use OSGI to find factories that generate GUI components based on types. In order to find the best possible match we compute the distance of the type-to-render to the supported type by the factory (equal = 0, superclass = 1,...). I'd like extend this from Classes to Types so we can use wildcards. However, the code needed for this is almost exactly what TypeVisitor does which causes our copyright scanners to trigger.
Current Behavior
It's currently package private
Desired Behavior
I'd like to make it publicly available
Concrete Use Cases
In our case we use OSGI to find factories that generate GUI components based on types. In order to find the best possible match we compute the distance of the type-to-render to the supported type by the factory (equal = 0, superclass = 1,...). I'd like extend this from Classes to Types so we can use wildcards. However, the code needed for this is almost exactly what TypeVisitor does which causes our copyright scanners to trigger.
Additionally in the new system I'd like to reject typevariables in the factory declarations (I use the typetoken trick to define their supported input types as the class parameter) as they would not make sense.
Checklist
-
[x] I agree to follow the code of conduct.
-
[x] I have read and understood the contribution guidelines.
-
[x] I have read and understood Guava's philosophy, and I strongly believe that this proposal aligns with it.
Hi maintainers 👋
This PR addresses issue #7838 by making the TypeVisitor class public to allow reuse externally.
All CLA checks have passed ✅ Could someone with write access kindly review and approve this PR? Also, approval is needed to trigger workflows. Thank you!
@cpovirk @netdpb @parthea
Hi maintainers 👋
This PR addresses issue #7838 by making the
TypeVisitorclass public to allow reuse externally.All CLA checks have passed ✅ Could someone with write access kindly review and approve this PR? Also, approval is needed to trigger workflows. Thank you!
That points to the issue I think you meant this #7845
Yes. The challenge is not with OSGI though. The fact that the factories are only known at runtime (and can be changed by end users in my case) means I cannot predetermine an ideal ranking. So I have to use some metrics based on how specific these are to the request (more specific means higher rank, ultimately we select the highest rank and only use that factory).
I've been working on this in past few days and I did find a way to do all this with just TypeToken and TypeSet which would reduce the 'ubiquity' a bit (according to guava guidelines). However, if TypeVisitor would be available I'd still use it in at least one location as it allows much finer control over Type parsing than TypeToken and would allow me to disallow TypeVariables.
PS The class distance metric in my original issue description is now a bit misleading. Since Types are more complicated than classes, as they can have multiple bounds which each can have generic arguments themselves the simple distance metric didnt suffice and the system got a bit more complicated. However, they general idea remains: find the best matching factory from a list of factories that is only known at runtime.
TypeVisitor as it is is pretty primitive. It's pretty much a switch-case with a visited Set to avoid visiting a type more than once.
To do meaningful type traversal, the javadoc requires explicit and manual code, like:
new TypeVisitor() {
@Override void visitGenericArrayType(GenercArrayType arrayType) {
visit(arrayType.getComponentType());
}
@Override void visitParameterizedType(ParameterizedType parameterizedType) {
if (parameterizedType.getOwnerType() != null) {
visit(parameterizedType.getOwnerType());
}
visit(parameterizedType.getRawType());
for (Type param : parameterizedType.getTypeParameters()) {
visit(param);
}
}
...
}
In a sense, this is a breadth-first graph traversal, that using Java 21 switch-case and pattern matching, can be done equivalently as in:
Traverser<Type> traverser = Traverser.forGraph(
type -> switch (type) {
case GenericArrayType arrayType -> List.of(arrayType.getComponentType());
case ParameterizedType parameterizedType -> Stream.concat(
Stream.nullable(parameterizedType.getOwnerType()),
Stream.of(parameterizedType.getRawType()),
Arrays.stream(parameterizedType.getTypeParameters())).toList();
...
});
for (Type type : traverser.breadthFirst(fromType)) {
...
}
Compared to visitor pattern, Java 21's pattern matching syntax is usually more concise.
IIUC the use case, this traverser still needs work to support distance (which seems like an unweighted shortest path?).
If that's the case, I think I might be able to create a wrapper class that wraps Type with a custom equals() to handle this. Plus, as alluded to, it's not merely about distance. There are other logic to be involved, which seems like a wrapper object would be useful anyways.
But let me stop here to make sure I'm not deep in the weeds already.
TypeVisitor as it is is pretty primitive. It's pretty much a switch-case with a visited Set to avoid visiting a type more than once.
To do meaningful type traversal, the javadoc requires explicit and manual code, like:
new TypeVisitor() { @Override void visitGenericArrayType(GenercArrayType arrayType) { visit(arrayType.getComponentType()); }
@Override void visitParameterizedType(ParameterizedType parameterizedType) { if (parameterizedType.getOwnerType() != null) { visit(parameterizedType.getOwnerType()); } visit(parameterizedType.getRawType()); for (Type param : parameterizedType.getTypeParameters()) { visit(param); } }
... }
All the if null checks and the for-loop are not necessary with TypeVisitor, it handles them for you, also it isn't breadth first(?) Which makes it nice and simple to work with. If you implement something yourself like you propose you have to be more aware of all the null cases and for-loop necessities. Its simplicity is what I like, but I acknowledge.... it's implementable in other ways.
When I was implementing a TypeVariable refuser... my own implementation was way longer than the typevisitor one:
result = false;
new TypeVisitor() {
@Override void visitGenericArrayType(GenercArrayType arrayType) {
visit(arrayType.getComponentType());
}
@Override void visitParameterizedType(ParameterizedType parameterizedType) {
visit(parameterizedType.getOwnerType());
visit(parameterizedType.getTypeParameters());
}
@Override void visitTypeVariableType(TypeVariableType vartype) {
result = true;
}
...
}.visit(input);
return result;
Maybe it's an idea to enable recursion by default... then it would actually be quite concise
Yes. I think I overlooked the null check currently in visit(@Nullable). It was for internal convenience when we knew it's only used a few places.
I don't know what others think, but I suspect in the consistent null-hostility spirit in Guava's public API, even if we publicize the API, we might even want to take away the @Nullable?
Regardless, yeah, it'll require a null check but that's pretty much it.
In this TypeVariable refuser, with Java 21, I might implement it this way:
Traverser<Type> traverser = Traverser.forGraph(
t -> switch (t) {
case GenericArrayType arrayType -> List.of(arrayType.getComponentType());
case ParameterizedType parameterizedType -> Stream.concat(
Stream.ofNullable(parameterizedType.getOwnerType()),
Arrays.stream(parameterizedType.getTypeParameters()))
.toList();
...
});
return Streams.stream(traverser.breadthFirst(type))
.anyMatch(TypeVariable::isInstance);
Regarding breadth first, you are absolutely right. I mis-spoke. Current implementation of TypeVisitor is depth-first (which is also supported in Traverser).
But I guess for finding the shortest distance, breadth first could be more useful?
Although I consider the stream creation a tad heavy for the purpose I think your example implementation is a perfectly fine alternative. (in other words, I wont be offended if this request gets refused ;-) )
But I guess for finding the shortest distance, breadth first could be more useful?
Yes, definitely. Although, now it's defined differently due to the possibility of multiple bounds, (digressing a lot here):
I wanted something that would behave intuitively for 'objective' cases while in other cases it could consider things 'equally specific'. For a given requested type e.g. ArrayList<? extends Integer> 'objective' cases of supported types are:
- ArrayList should be more specific than List
- List<? extends Integer> should be more specific than List<? extends Number>
- (not applicable in the example) A<String, ? extends Integer> is more specific than A<String, ? extends Number>
In order to achieve this I do the following to rank factories:
- whether or not typetoken considers the requested type a subtype of the supported type of the factory (after this step we know the requested type is a subtype of all bounds in the supported type)
- if the supported type implements more classes and interfaces it's considered more specific (subclasses always implement at least 1 type more than their supers)
- Then recurse on all TypeArguments; if there is a consensus between all TypeArgumetns (e.g. they are all equal or more specific) than a decision is based on that
- otherwise the supported types are considered equal
for step 3 I upgrade raw classes to their Parametric counterpart with the highest possible bounds based on the class definition.
The type refuser is used to force users to create proper implementations of the factories so that the supported types are fully defined.
Yep. I'd be intrigued to learn if you can make Traverser work for your use case.
It by default returns Iterable, so if Stream is not your thing, there is that. :-)