Reification by lorenzleutgeb · Pull Request #46 · jhalterman/typetools

lorenzleutgeb · 2018-12-24T13:39:14Z

This is another attempt to resolve #8, meant to supersede #12.

Different from what @jhalterman originally proposed, this approach does /not/ use type tokens. Rather it traverses common implementations of Type and replaces type variables in there recursively. The benefit I see doing this, is that it is compatible with the existing type hierarchy. A generic parameterized type is just resolved to look like a concrete parameterized type.

codecov-io · 2018-12-24T13:44:29Z

Codecov Report

Merging #46 into master will decrease coverage by 4.22%.
The diff coverage is 62.65%.

@@             Coverage Diff              @@
##             master      #46      +/-   ##
============================================
- Coverage     78.99%   74.76%   -4.23%     
- Complexity       81       94      +13     
============================================
  Files             1        1              
  Lines           238      321      +83     
  Branches         73       95      +22     
============================================
+ Hits            188      240      +52     
- Misses           17       39      +22     
- Partials         33       42       +9

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/net/jodah/typetools/TypeResolver.java	`74.76% <62.65%> (-4.23%)`	`94 <13> (+13)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc5f6d0...5bafe91. Read the comment docs.

lorenzleutgeb · 2018-12-24T13:46:03Z

    return resolveRawArguments(resolveGenericType(type, subType), subType);
  }

+  public static <T, S extends T> Type resolveType(Class<T> type, Class<S> subType) {


Looking for inspiration on how to document this new public method...

lorenzleutgeb · 2018-12-24T13:46:19Z

+    return substituteGenerics(resolveGenericType(type, subType), getTypeVariableMap(subType, null));
+  }
+
+  public static Type resolveType(Type type, Class<?> context) {


Looking for inspiration on how to document this new public method...

jhalterman · 2018-12-24T19:46:27Z

+    assert typeArguments[0] instanceof ParameterizedType;
+    ParameterizedType firstTypeArgument = (ParameterizedType) typeArguments[0];
+    assert firstTypeArgument.getRawType() == HashSet.class;
+    assert firstTypeArgument.getActualTypeArguments()[0] == Object.class;


This test would be better if the type argument was something other than Object.

Well, in this case there's no explicit upper bound for the wildcard type, but I still wanted to re-use the existing fixture. I would like to keep this test, and making it "better" would require a different fixture.

Maybe we can add another one transports better what you want. So you want one case where a wildcard type is resolved to something below Object? I added this in aa744c9. If that's not what you mean, please give clearer instructions.

So you want one case where a wildcard type is resolved to something below Object?

Yea, I thought that would be more clear.

I would like to keep this test, and making it "better" would require a different fixture.

Good point.

jhalterman · 2018-12-24T19:46:35Z

+    assert typeArguments[1] instanceof ParameterizedType;
+    ParameterizedType secondTypeArgument = (ParameterizedType) typeArguments[1];
+    assert secondTypeArgument.getRawType() == ArrayList.class;
+    assert secondTypeArgument.getActualTypeArguments()[0] == Object.class;


Same here re: Object, and in a few places below.

jhalterman · 2018-12-24T19:51:40Z

+    private final Type[] resolvedTypeArguments;
+
+    private ResolvedParameterizedType(ParameterizedType original, Type[] resolvedTypeArguments) {
+      Objects.requireNonNull(original);


TypeTools is released for Java 1.6, so we can't use this API :)

jhalterman · 2018-12-24T19:51:49Z

+
+    @Override
+    public int hashCode() {
+      int result = Objects.hash(original);


Same here re: the 1.6 API limitation.

jhalterman · 2018-12-24T19:53:31Z

+      if (upperBounds.length == 1 && lowerBounds.length == 0) {
+        return substituteGenerics(upperBounds[0], typeVariableMap);
+      }
+      throw new UnsupportedOperationException(


UnsupportedOperationException is good to throw here, but I don't like embedding the issue URL in the exception. A brief message about why this operation is unsupported is enough.

Resolved in fc7cffb

jhalterman · 2018-12-24T19:53:44Z

+
+      return changed ? new ResolvedParameterizedType(parameterizedType,  resolvedTypeArguments) : parameterizedType;
+    }
+    throw new UnsupportedOperationException(


Same re: the unsupported message.

Resolved in fc7cffb

jhalterman · 2018-12-24T19:54:30Z


+  /**
+   * The same as {@link #substituteGenerics(Type, Class)}, but first resolves the generic type of {@code type}.
+   * This is a convenience method.


We should document a @throws UnsupportedOperationException that describes what is unsupported

Well, this method is just pure convenience for consuming the API, I would like to avoid documenting it with anything more than just stating this and point the reader at the "real" method.

Ah, sure. I meant that both of them should be documented.

jhalterman · 2018-12-24T19:54:38Z

+   * @param context the class that serves as starting point to resolve replacements of type variables
+   * @return a type that is structurally the same as {@code type}, except that type variables have been replaced
+   */
+  public static Type substituteGenerics(Type type, Class<?> context) {


Same re: @throws UnsupportedOperationException

Resolved in fc7cffb

jhalterman · 2018-12-24T20:00:16Z

@lorenzleutgeb This is very nice. Using ParameterizedType is pretty clever, but the API is not as friendly as if we created our own class for resolving concrete types since it requires type checking and casting. If we know that type arguments should always be ParameterizedType instances, then having to cast is not ideal.

The benefit I see doing this, is that it is compatible with the existing type hierarchy

Maybe you can explain this problem a bit more?

In general I'm interested to consider good reasons why we shouldn't just offer something like GenericType that would be a bit more friendly to use.

jhalterman · 2018-12-24T20:04:15Z

+   * The same as {@link #substituteGenerics(Type, Class)}, but first resolves the generic type of {@code type}.
+   * This is a convenience method.
+   */
+  public static <T, S extends T> Type substituteGenerics(Class<T> type, Class<S> subType) {


A name that came to mind for me is "reifyGenerics", which seems appropriate here :)

lorenzleutgeb · 2018-12-24T21:36:19Z

First off, I agree with most comments, and even was expecting some. I will address most of them ASAP.

@lorenzleutgeb This is very nice. Using ParameterizedType is pretty clever, but the API is not as friendly as if we created our own class for resolving concrete types since it requires type checking and casting. If we know that type arguments should always be ParameterizedType instances, then having to cast is not ideal.

Assume T maps to Integer. Then List<T> will be reified to a ParameterizedType. However, T is not a ParameterizedType but a TypeVariable (which might be the generic type of a field, argument, etc.) and be reified to Integer.class, which is not a ParameterizedType. While many test cases will be awkwardly walking through ParameterizedType instances, others don't and I think that this should be supported.

The benefit I see doing this, is that it is compatible with the existing type hierarchy

Maybe you can explain this problem a bit more?

The reason I am even PRing this is that I want to improve some other library. This other library is lacking support for resolution of type variables. If it sees List<T> it is just gonna explode when unpacking this ParameterizedType and trying to make sense of T as the first type parameter.
It is aware of parameterized types, and for example interested in the element type of container classes like lists. Now, to make it work with type variables, my idea is just to substitute them, and "transparently" hand over the substituted (or as you would say, reified) types. The library knows how to handle parameterized types, and if I give it List<Integer> it will do its job.

On the contrary, type tokens introduce a new type like GenericType which would require more integration work in this scenario, as far as I understand. If I were to introduce GenericType like in java-classmate then I fear that typetools would gain more weight. Just look at all the machinery in the other repo. Also, this library that I am patching would have to depend on GenericType and learn how to inspect/traverse it. This is what I would consider "incompatible with the existing type hierarchy".

Maybe I am misunderstanding something, but this is my reasoning for preferring the proposed approach.

In general I'm interested to consider good reasons why we shouldn't just offer something like GenericType that would be a bit more friendly to use.

Probably GenericType would be easier to use as such, but harder to integrate in existing code. I think that the approach I sketched is worth having, and it can coexist with a type-token approach. One might add resolveTypeToken (or something with a better name) that offers the other variant.

jhalterman · 2018-12-24T23:23:33Z

I think that the approach I sketched is worth having, and it can coexist with a type-token approach.

This is a fair enough point.

The last thing I'd like to resolve is naming. How about reifyArguments? This is consistent with resolveRawArguments and the Javadocs could describe how "arguments for the type are reified based on type information from the context".

lorenzleutgeb · 2018-12-25T10:19:38Z

The last thing I'd like to resolve is naming. How about reifyArguments?

Of course, naming is always hardest...

I agree to call it "reify" in some way, but I dislike reifyArguments. For me it is unclear what those "arguments" are. My first thought is that they correspond to the parameters of a parameterized type. However, resolveRawArguments can also resolve a TypeVariable as such in which case there are no parameters that the arguments can correspond to (the testing fixture here is ID id). This confuses me.

I am repeating myself, but the essence of this contribution is that it just takes a type and substitutes all TypeVariables and WildcardTypes with concrete types (where trivially possible). No matter what the input type is (in reality, for a reasonable variety of input types).

To emphasize this, I added a version with just one parameter and no context, see here.

Another viewpoint would be that all type variables and wildcards inside a type and its referenced types are its parameters. Then calling it reifyArguments starts making sense again, but I think that this is quite an exotic view.

What about just calling it reify?

jhalterman · 2018-12-25T19:23:14Z

resolveRawArguments can also resolve a TypeVariable
What about just calling it reify?

Yep, good point. reify sounds good.

lorenzleutgeb · 2018-12-25T22:26:24Z

OK. I think that all your remarks are resolved, please have another look and consider merging. Thank you.

jhalterman · 2018-12-26T03:40:46Z

This was a really thorough PR - thanks @lorenzleutgeb! Will cut a new release shortly...

lorenzleutgeb force-pushed the resolve branch 2 times, most recently from 5e8016c to d1dbf2b Compare December 24, 2018 13:44

lorenzleutgeb commented Dec 24, 2018

View reviewed changes

lorenzleutgeb force-pushed the resolve branch 2 times, most recently from 867c0ca to c78617f Compare December 24, 2018 14:13

jhalterman reviewed Dec 24, 2018

View reviewed changes

lorenzleutgeb mentioned this pull request Dec 25, 2018

Resolve concrete types of type variables crnk-project/crnk-framework#432

Closed

lorenzleutgeb changed the title ~~Implement substitution of generics~~ Reification Dec 25, 2018

Implement reification

5bafe91

lorenzleutgeb force-pushed the resolve branch from f048c58 to 5bafe91 Compare December 25, 2018 22:25

jhalterman merged commit 15cf7a1 into jhalterman:master Dec 26, 2018

lorenzleutgeb mentioned this pull request Nov 17, 2021

Support for just resolving type variables #12

Open

Conversation

lorenzleutgeb commented Dec 24, 2018

Uh oh!

codecov-io commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lorenzleutgeb Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhalterman Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhalterman commented Dec 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorenzleutgeb commented Dec 24, 2018

Uh oh!

jhalterman commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lorenzleutgeb commented Dec 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhalterman commented Dec 25, 2018

Uh oh!

lorenzleutgeb commented Dec 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhalterman commented Dec 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented Dec 24, 2018 •

edited

Loading

lorenzleutgeb Dec 24, 2018 •

edited

Loading

jhalterman Dec 24, 2018 •

edited

Loading

jhalterman commented Dec 24, 2018 •

edited

Loading

lorenzleutgeb commented Dec 25, 2018 •

edited

Loading

lorenzleutgeb commented Dec 25, 2018 •

edited

Loading