optimize TypeParameterResolver

[guanchengang]

optimize TypeParameterResolver

1. The correct ownerType has been identified
2. canonicalize type (In the original implementation, there would be a mix of JDK implementations such as sun.reflect.generics.reflectiveObjects.ParameterizedTypeImpl ...)
3. optimized toString()
4. when the specific generic is not declared, erase to the most appropriate upper bound

class A<T extends Serializable> {
  public T foo(T t) {
    return t;
  }
}

class B<T extends Number> extends A<T> {}

class C<T extends Integer> extends B<T> {}

Method m = A.class.getMethod("foo", Serializable.class);

// In old code, it's Object.class. Actually, Integer
System.out.println(TypeParameterResolver.resolveReturnType(m, C.class)); 

[harawata]

Hello @guanchengang ,

T extends Integer is meaningless because Integer is final [1] [2].
And it is incorrect to expect Integer as the return type of C#foo() because "generic type invocation" is not performed at that point [3].

You will get Integer if you declare C with generic type invocation:

class C extends B<Integer> {}

[1] There should be a compiler warning: The type parameter T should not be bounded by the final type Integer. Final types cannot be further extended
[2] Explanation for why it is still compilable : https://stackoverflow.com/a/12475147/1261766
[3] https://docs.oracle.com/javase/tutorial/java/generics/types.html#instantiation

[guanchengang]

I'm sorry I gave an inappropriate example @harawata.
I understand that classes modified with final cannot be extended. That is just an example. Nevertheless, I truly appreciate your help in looking up the information. I’m really grateful!
"generic type invocation" is not performed at this point，but we can still obtain the bounds information of generics
Let’s refocus on this issue:

class L0 {}
class L1 extends L0 {}

class A<AT extends L0> {
  public AT foo() {
    return null;
  }
}

class B0 extends A {
}

class B1<BT1 extends L0> extends A<BT1> {
}

class B2<BT2 extends L1> extends A<BT2> {
}

public static void main(String[] args) throws Exception {
  Method foo = A.class.getMethod("foo");
  System.out.println(TypeParameterResolver.resolveReturnType(foo, A.class)); // class com.example.Test$L0
  System.out.println(TypeParameterResolver.resolveReturnType(foo, B0.class)); // class com.example.Test$L0
  System.out.println(TypeParameterResolver.resolveReturnType(foo, B1.class)); // class java.lang.Object
  System.out.println(TypeParameterResolver.resolveReturnType(foo, B2.class)); // class java.lang.Object
}

Subclass loses more information after extending from A.
The bound of a subclass cannot exceed that of its parent class.
In the case of B1 and B2，the return type is parsed as Object.class. Obviously, their bounds cannot exceed L0, regardless of the specific type is declared or not.
IMHO, even if they are not erased as the most appropriate bound, they should not exceed L0.

[harawata]

@guanchengang ,

Assuming you need this enhancement for some mappings in your MyBatis application (let me know if this is not the case), please create a small demo project that demonstrates the actual usage (including DB data, mapper, Java classes, etc.) and share it on your GitHub repo.

Here are some project templates and examples.
https://github.com/harawata/mybatis-issues

I couldn't think of a good example that benefit from this in the context of MyBatis mappings.

[guanchengang]

@harawata ，
I just looked at the implementation of this class and felt that the return was slightly unreasonable, without encountering any actual problems.A more precise boundary should not bring more problems.
If you think this is unnecessary, I will roll back the implementation of this part. Can other optimization parts be considered for merging（ownerType， canonicalize type and toString）

[harawata]

Thank you , @guanchengang !

I will look into it, but it might take a while as it's not an urgent matter.

[harawata]

2. canonicalize type (In the original implementation, there would be a mix of JDK implementations such as sun.reflect.generics.reflectiveObjects.ParameterizedTypeImpl ...)

Mixing ParameterizedType implementations should not be a problem.
What makes you think this is necessary? i.e. What is the problem?

[guanchengang]

There may be some issues when comparing ParameterizedType with different implementations using equals (when they represent the same type, the implementations are different), although this situation will not occur now and we will not compare them (but we cannot guarantee in the future) Moreover, there are some differences in the behavior between our implementation and the standard implementation to some extent(although they are all minor). So I believe that still have some role to play.WDYT

[harawata]

Hi @guanchengang ,

I'm sorry about the slow response.

The change to the toString() looks mostly OK, but there should be some tests covering the new logic.
I added one test, but could you add another that covers this line?

There may be some issues when comparing ParameterizedType with different implementations using equals

If two ParameterizedType implementations represent the same type, equals() must return true.
If it returns false, it is a bug.

And I can see that it could return false currently because ownerType is ignored and you fixed it 👍️
I added a new test that covers the corner case.

I also understand that there could be other issues caused by the implementation differences, however, users can get ParameterizedType instances, etc. without using TypeParameterResolver, so re-creating custom instances inside TypeParameterResolver may not be an effective solution [1].

And let's skip the "more precise boundary" part as well.
Adding extra code with no clear purpose is actually a bad thing.
If you find a usage that requires this change, please let us know and we will re-evaluate.

To summarize:

1. Please add a test case for this line
2. Please remove the "canonicalization" and "more precise boundary" part.

---

[1] I found an open ticket discussing hashCode() implementation and the latest comment suggests adding an official way to instantiate ParameterizedType. It could be the solution to our problem if it becomes reality.

[guanchengang]

hello @harawata，
I have removed the "canonicalization" and "more precise boundary" part.
Regarding the this line you mentioned, after reviewing it again, I think this part has the same effect as the rawType.getSimpleName() below, so I removed it.
And I discovered another issue about toString() when using inner classes. I fixed it and added a test.
I also understand your thoughts on "canonization" and agree with this sign.

Additionally, there is another issue in equals() .
As shown below:

class A {
      public List<String>[] foo() {
        return null;
      }
    }
    TypeParameterResolver.GenericArrayTypeImpl mybatisGA =
      new TypeParameterResolver.GenericArrayTypeImpl(new TypeParameterResolver.ParameterizedTypeImpl(List.class, null, new Type[]{String.class}));

    Method foo = A.class.getMethod("foo");
    Type jdkGA = A.class.getMethod("foo").getGenericReturnType();
    System.out.println(jdkGA.equals(mybatisGA)); // true
    System.out.println(mybatisGA.equals(jdkGA)); // false

a.equals(b) and b.equals(a) have different results.
As you said users can also get WildcardType and GenericArrayType instances without using TypeParameterResolver.
So I think this is a bug.
I have fixed it and added a test.

[harawata]

A quick question:
This assertion passes when 1) both "expected" and "actual" are true OR 2) both "expected" and "actual" are false.
I understand 1), but why is 2) valid?

[guanchengang]

I believe that the result of a.equals(b) and b.equals(a) should be symmetric, meaning that if one evaluates to true, so should the other. Therefore, a not equals b and b not equals a can logically be considered valid.
Comparing the following two forms:

assertEquals(typeMybatis.equals(typeJdk), typeJdk.equals(typeMybatis));

and

assertTrue(typeMybatis.equals(typeJdk));
assertTrue(typeJdk.equals(typeMybatis));

Personally, I prefer the first option, as it not only emphasizes the symmetry of the object comparison but also avoids the redundancy of repeated assertions.