leangen / geantyref

Advanced generic type reflection library with support for working with AnnotatedTypes (for Java 8+)
Apache License 2.0
99 stars 15 forks source link

mapTypeParameters: consider supertypes' type parameters #5

Closed stevenschlansker closed 5 years ago

stevenschlansker commented 5 years ago

Hello from jdbi. We are looking into replacing Guava's type handling with geantyref, for a number of reasons. However we noticed a difference in how it handles type resolution and think it might be considered a shortcoming.

Currently, resolveTypeMapping only considers the current class's type variables. It is possible for some of those, however, to be inherited from the super class.

Guava handles this: https://github.com/google/guava/blob/master/guava/src/com/google/common/reflect/TypeResolver.java#L388-L392 but geantyref does not.

Is this an intentional limitation? If not, can we extend the current algorithm to include supertypes? Thanks!

kaqqao commented 5 years ago

Hey! Good to hear you're finding GeAnTyRef useful :) And thanks for the interest in contributing!

As for mapTypeParameters, it is only accidentally public (should have been private), and its behavior is intentional. You can see from the other methods using it (e.g. getExactReturnType) how the typeAndParams is first fully resolved to the exact declaring type of the method before calling mapTypeParameters.

As for your example:

public void testConcreteSubtypeResolve() throws NoSuchMethodException {
        Method m = R.class.getMethod("z");
        assertEquals(String.class, GenericTypeReflector.erase(
                GenericTypeReflector.mapTypeParameters(
                        GenericTypeReflector.annotate(m.getGenericReturnType()),
                        GenericTypeReflector.annotate(R.class)).getType()));
}

It should really be written as:

public void testConcreteSubtypeResolve() throws NoSuchMethodException {
        Method m = R.class.getMethod("z");
        AnnotatedType returnType = GenericTypeReflector.getExactReturnType(m, GenericTypeReflector.annotate(R.class));
        assertEquals(String.class, returnType.getType());
}

That would give you the correct result.

Is there a use-case for mapTypeParameters that's not already covered by the other more direct methods exposed by GenericTypeReflector (getTypeParameter, getExactReturnType, getExactFieldType etc)? If so, I'd be open to adding a new method to accommodate it.

stevenschlansker commented 5 years ago

The use case I'm trying to cover is a method we've implemented and exposed as public API,

Type resolveType(Type typeToResolve, Type inContext);

class A<T> {
    T get();
}

class B extends A<String> {}

resolveType(A.class.getMethod("get"), B.class); // returns String.class

If we were writing our code anew we would just use the "correct" approach you outlined above. But I'm trying to shoehorn this into the old API so we don't make a breaking change. And the existing API does not discriminate between return type, parameter type, etc... it just takes a Type.

The current implementation with Guava: https://github.com/jdbi/jdbi/blob/master/core/src/main/java/org/jdbi/v3/core/generic/GenericTypes.java#L113-L117

and an example usage: https://github.com/jdbi/jdbi/blob/master/sqlobject/src/main/java/org/jdbi/v3/sqlobject/statement/internal/ResultReturner.java#L60

so I'm essentially just looking to replicate what Guava does.

Of course if we always had the TypeVariable, we could use getTypeParameter(inContext, typeToResolve) but sometimes we've got a parameterized type with possibly multiple type variables like Map.Entry<X, Y>

stevenschlansker commented 5 years ago

Hi @kaqqao do you have any suggestions as to how I might implement such a resolveType method as I outlined above, using GeAnTyRef? Thanks!

kaqqao commented 5 years ago

@stevenschlansker Hey, sorry for not replying! There doesn't seem to be a way at the moment, but I'll put your code into a new public method and release a new version over the weekend. Does that work for you?

stevenschlansker commented 5 years ago

Yes, thank you very much! 😸 I will test it as soon as it's available.

stevenschlansker commented 5 years ago

Any chance we could give this a spin soon? ;) Happy new year!

stevenschlansker commented 5 years ago

hi @kaqqao if you don't have time to do this but could give me some hints and pointers on how to do it myself, I could give it a try.

kaqqao commented 5 years ago

@stevenschlansker Hey! Terribly sorry for the radio silence! Was on a trip, and then got bogged down with various things upon return. I think about how to approach this, though... and it's not exactly obvious. I'm trying to wrap my mind around the implications captures might have on type resolution in this case. Anyway, I promise to finally deal with it this weekend. Again, sorry I went MIA.

kaqqao commented 5 years ago

I think I implemented the feature you need. The relevant methods are GenericTypeReflector.resolveExactType (which throws an exception if an unresolvable variable is encountered) and GenericTypeReflector.resolveType (which allows partial resolution). If you have a chance please try it out and verify it satisfies your use-case. I will release a new version with this feature in a couple of days, or as soon as you confirm that's what you needed.

kaqqao commented 5 years ago

Version 1.3.7 is released.

stevenschlansker commented 5 years ago

Thanks! I'll give it a try and report back!