Improve TypeToken creation validation (#2072)

* Add comments regarding multiple bounds of wildcard

* Remove WildcardType check in getCollectionElementType

The returned Type is never a wildcard due to the changes made to getSupertype
by commit b1fb9ca.

* Remove redundant getRawType call from MapTypeAdapterFactory

getRawType(TypeToken.getType()) is the same as calling TypeToken.getRawType().

* Make TypeToken members private

* Remove incorrect statement about TypeToken wildcards

It is possible to use wildcards as part of the type argument, e.g.:
`new TypeToken<List<? extends CharSequence>>() {}`

* Only allow direct subclasses of TypeToken

Previously subclasses of subclasses (...) of TypeToken were allowed which
can behave incorrectly when retrieving the type argument, e.g.:

  class SubTypeToken<T> extends TypeToken<Integer> {}
  new SubTypeToken<String>() {}.getType()

This returned `String` despite the class extending TypeToken<Integer>.

* Throw exception when TypeToken captures type variable

Due to type erasure the runtime type argument for a type variable is not
available. Therefore there is no point in capturing a type variable and it
might even give a false sense of type-safety.

* Make $Gson$Types members private

* Rename $Gson$Types.getGenericSupertype parameter

Rename the method parameter to match the documentation of the method and
to be similar to getSupertype(...).

* Improve tests and handle raw TypeToken supertype better

* Make some $Gson$Types members package-private again to prevent synthetic accessors

* Remove TypeToken check for type variable

As mentioned in review comments, there are cases during serialization where
usage of the type variable is not so problematic (but still not ideal).

Author: Marcono1234

gson/src/main/java/com/google/gson/internal/$Gson$Types.java

@@ -154,7 +154,10 @@ public static Class<?> getRawType(Type type) {
       return Object.class;
 
     } else if (type instanceof WildcardType) {
-      return getRawType(((WildcardType) type).getUpperBounds()[0]);
+      Type[] bounds = ((WildcardType) type).getUpperBounds();
+      // Currently the JLS only permits one bound for wildcards so using first bound is safe
+      assert bounds.length == 1;
+      return getRawType(bounds[0]);
 
     } else {
       String className = type == null ? "null" : type.getClass().getName();
@@ -163,7 +166,7 @@ public static Class<?> getRawType(Type type) {
     }
   }
 
-  static boolean equal(Object a, Object b) {
+  private static boolean equal(Object a, Object b) {
     return a == b || (a != null && a.equals(b));
   }
 
@@ -225,10 +228,6 @@ public static boolean equals(Type a, Type b) {
     }
   }
 
-  static int hashCodeOrZero(Object o) {
-    return o != null ? o.hashCode() : 0;
-  }
-
   public static String typeToString(Type type) {
     return type instanceof Class ? ((Class<?>) type).getName() : type.toString();
   }
@@ -238,19 +237,19 @@ public static String typeToString(Type type) {
    * IntegerSet}, the result for when supertype is {@code Set.class} is {@code Set<Integer>} and the
    * result when the supertype is {@code Collection.class} is {@code Collection<Integer>}.
    */
-  static Type getGenericSupertype(Type context, Class<?> rawType, Class<?> toResolve) {
-    if (toResolve == rawType) {
+  private static Type getGenericSupertype(Type context, Class<?> rawType, Class<?> supertype) {
+    if (supertype == rawType) {
       return context;
     }
 
     // we skip searching through interfaces if unknown is an interface
-    if (toResolve.isInterface()) {
+    if (supertype.isInterface()) {
       Class<?>[] interfaces = rawType.getInterfaces();
       for (int i = 0, length = interfaces.length; i < length; i++) {
-        if (interfaces[i] == toResolve) {
+        if (interfaces[i] == supertype) {
           return rawType.getGenericInterfaces()[i];
-        } else if (toResolve.isAssignableFrom(interfaces[i])) {
-          return getGenericSupertype(rawType.getGenericInterfaces()[i], interfaces[i], toResolve);
+        } else if (supertype.isAssignableFrom(interfaces[i])) {
+          return getGenericSupertype(rawType.getGenericInterfaces()[i], interfaces[i], supertype);
         }
       }
     }
@@ -259,17 +258,17 @@ static Type getGenericSupertype(Type context, Class<?> rawType, Class<?> toResol
     if (!rawType.isInterface()) {
       while (rawType != Object.class) {
         Class<?> rawSupertype = rawType.getSuperclass();
-        if (rawSupertype == toResolve) {
+        if (rawSupertype == supertype) {
           return rawType.getGenericSuperclass();
-        } else if (toResolve.isAssignableFrom(rawSupertype)) {
-          return getGenericSupertype(rawType.getGenericSuperclass(), rawSupertype, toResolve);
+        } else if (supertype.isAssignableFrom(rawSupertype)) {
+          return getGenericSupertype(rawType.getGenericSuperclass(), rawSupertype, supertype);
         }
         rawType = rawSupertype;
       }
     }
 
     // we can't resolve this further
-    return toResolve;
+    return supertype;
   }
 
   /**
@@ -279,10 +278,13 @@ static Type getGenericSupertype(Type context, Class<?> rawType, Class<?> toResol
    *
    * @param supertype a superclass of, or interface implemented by, this.
    */
-  static Type getSupertype(Type context, Class<?> contextRawType, Class<?> supertype) {
+  private static Type getSupertype(Type context, Class<?> contextRawType, Class<?> supertype) {
     if (context instanceof WildcardType) {
       // wildcards are useless for resolving supertypes. As the upper bound has the same raw type, use it instead
-      context = ((WildcardType)context).getUpperBounds()[0];
+      Type[] bounds = ((WildcardType)context).getUpperBounds();
+      // Currently the JLS only permits one bound for wildcards so using first bound is safe
+      assert bounds.length == 1;
+      context = bounds[0];
     }
     checkArgument(supertype.isAssignableFrom(contextRawType));
     return resolve(context, contextRawType,
@@ -306,9 +308,6 @@ public static Type getArrayComponentType(Type array) {
   public static Type getCollectionElementType(Type context, Class<?> contextRawType) {
     Type collectionType = getSupertype(context, contextRawType, Collection.class);
 
-    if (collectionType instanceof WildcardType) {
-      collectionType = ((WildcardType)collectionType).getUpperBounds()[0];
-    }
     if (collectionType instanceof ParameterizedType) {
       return ((ParameterizedType) collectionType).getActualTypeArguments()[0];
     }
@@ -440,7 +439,7 @@ private static Type resolve(Type context, Class<?> contextRawType, Type toResolv
     return toResolve;
   }
 
-  static Type resolveTypeVariable(Type context, Class<?> contextRawType, TypeVariable<?> unknown) {
+  private static Type resolveTypeVariable(Type context, Class<?> contextRawType, TypeVariable<?> unknown) {
     Class<?> declaredByRaw = declaringClassOf(unknown);
 
     // we can't reduce this further
@@ -522,6 +521,10 @@ public ParameterizedTypeImpl(Type ownerType, Type rawType, Type... typeArguments
           && $Gson$Types.equals(this, (ParameterizedType) other);
     }
 
+    private static int hashCodeOrZero(Object o) {
+      return o != null ? o.hashCode() : 0;
+    }
+
     @Override public int hashCode() {
       return Arrays.hashCode(typeArguments)
           ^ rawType.hashCode()

gson/src/main/java/com/google/gson/internal/bind/MapTypeAdapterFactory.java

@@ -120,8 +120,7 @@ public MapTypeAdapterFactory(ConstructorConstructor constructorConstructor,
       return null;
     }
 
-    Class<?> rawTypeOfSrc = $Gson$Types.getRawType(type);
-    Type[] keyAndValueTypes = $Gson$Types.getMapKeyAndValueTypes(type, rawTypeOfSrc);
+    Type[] keyAndValueTypes = $Gson$Types.getMapKeyAndValueTypes(type, rawType);
     TypeAdapter<?> keyAdapter = getKeyAdapter(gson, keyAndValueTypes[0]);
     TypeAdapter<?> valueAdapter = gson.getAdapter(TypeToken.get(keyAndValueTypes[1]));
     ObjectConstructor<T> constructor = constructorConstructor.get(typeToken);

gson/src/main/java/com/google/gson/reflect/TypeToken.java

@@ -37,17 +37,20 @@
  * <p>
  * {@code TypeToken<List<String>> list = new TypeToken<List<String>>() {};}
  *
- * <p>This syntax cannot be used to create type literals that have wildcard
- * parameters, such as {@code Class<?>} or {@code List<? extends CharSequence>}.
+ * <p>Capturing a type variable as type argument of a {@code TypeToken} should
+ * be avoided. Due to type erasure the runtime type of a type variable is not
+ * available to Gson and therefore it cannot provide the functionality one
+ * might expect, which gives a false sense of type-safety at compilation time
+ * and can lead to an unexpected {@code ClassCastException} at runtime.
  *
  * @author Bob Lee
  * @author Sven Mawson
  * @author Jesse Wilson
  */
 public class TypeToken<T> {
-  final Class<? super T> rawType;
-  final Type type;
-  final int hashCode;
+  private final Class<? super T> rawType;
+  private final Type type;
+  private final int hashCode;
 
   /**
    * Constructs a new type literal. Derives represented class from type
@@ -59,7 +62,7 @@ public class TypeToken<T> {
    */
   @SuppressWarnings("unchecked")
   protected TypeToken() {
-    this.type = getSuperclassTypeParameter(getClass());
+    this.type = getTypeTokenTypeArgument();
     this.rawType = (Class<? super T>) $Gson$Types.getRawType(type);
     this.hashCode = type.hashCode();
   }
@@ -68,23 +71,33 @@ protected TypeToken() {
    * Unsafe. Constructs a type literal manually.
    */
   @SuppressWarnings("unchecked")
-  TypeToken(Type type) {
+  private TypeToken(Type type) {
     this.type = $Gson$Types.canonicalize($Gson$Preconditions.checkNotNull(type));
     this.rawType = (Class<? super T>) $Gson$Types.getRawType(this.type);
     this.hashCode = this.type.hashCode();
   }
 
   /**
-   * Returns the type from super class's type parameter in {@link $Gson$Types#canonicalize
+   * Verifies that {@code this} is an instance of a direct subclass of TypeToken and
+   * returns the type argument for {@code T} in {@link $Gson$Types#canonicalize
    * canonical form}.
    */
-  static Type getSuperclassTypeParameter(Class<?> subclass) {
-    Type superclass = subclass.getGenericSuperclass();
-    if (superclass instanceof Class) {
-      throw new RuntimeException("Missing type parameter.");
+  private Type getTypeTokenTypeArgument() {
+    Type superclass = getClass().getGenericSuperclass();
+    if (superclass instanceof ParameterizedType) {
+      ParameterizedType parameterized = (ParameterizedType) superclass;
+      if (parameterized.getRawType() == TypeToken.class) {
+        return $Gson$Types.canonicalize(parameterized.getActualTypeArguments()[0]);
+      }
+    }
+    // Check for raw TypeToken as superclass
+    else if (superclass == TypeToken.class) {
+      throw new IllegalStateException("TypeToken must be created with a type argument: new TypeToken<...>() {}; "
+          + "When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved.");
     }
-    ParameterizedType parameterized = (ParameterizedType) superclass;
-    return $Gson$Types.canonicalize(parameterized.getActualTypeArguments()[0]);
+
+    // User created subclass of subclass of TypeToken
+    throw new IllegalStateException("Must only create direct subclasses of TypeToken");
   }
 
   /**

gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java

@@ -27,23 +27,24 @@
 /**
  * @author Jesse Wilson
  */
-@SuppressWarnings({"deprecation"})
 public final class TypeTokenTest extends TestCase {
-
+  // These fields are accessed using reflection by the tests below
   List<Integer> listOfInteger = null;
   List<Number> listOfNumber = null;
   List<String> listOfString = null;
   List<?> listOfUnknown = null;
   List<Set<String>> listOfSetOfString = null;
   List<Set<?>> listOfSetOfUnknown = null;
 
+  @SuppressWarnings({"deprecation"})
   public void testIsAssignableFromRawTypes() {
     assertTrue(TypeToken.get(Object.class).isAssignableFrom(String.class));
     assertFalse(TypeToken.get(String.class).isAssignableFrom(Object.class));
     assertTrue(TypeToken.get(RandomAccess.class).isAssignableFrom(ArrayList.class));
     assertFalse(TypeToken.get(ArrayList.class).isAssignableFrom(RandomAccess.class));
   }
 
+  @SuppressWarnings({"deprecation"})
   public void testIsAssignableFromWithTypeParameters() throws Exception {
     Type a = getClass().getDeclaredField("listOfInteger").getGenericType();
     Type b = getClass().getDeclaredField("listOfNumber").getGenericType();
@@ -56,6 +57,7 @@ public void testIsAssignableFromWithTypeParameters() throws Exception {
     assertFalse(TypeToken.get(b).isAssignableFrom(a));
   }
 
+  @SuppressWarnings({"deprecation"})
   public void testIsAssignableFromWithBasicWildcards() throws Exception {
     Type a = getClass().getDeclaredField("listOfString").getGenericType();
     Type b = getClass().getDeclaredField("listOfUnknown").getGenericType();
@@ -69,6 +71,7 @@ public void testIsAssignableFromWithBasicWildcards() throws Exception {
     // assertTrue(TypeToken.get(b).isAssignableFrom(a));
   }
 
+  @SuppressWarnings({"deprecation"})
   public void testIsAssignableFromWithNestedWildcards() throws Exception {
     Type a = getClass().getDeclaredField("listOfSetOfString").getGenericType();
     Type b = getClass().getDeclaredField("listOfSetOfUnknown").getGenericType();
@@ -102,4 +105,56 @@ public void testParameterizedFactory() {
     Type listOfListOfString = TypeToken.getParameterized(List.class, listOfString).getType();
     assertEquals(expectedListOfListOfListOfString, TypeToken.getParameterized(List.class, listOfListOfString));
   }
+
+  private static class CustomTypeToken extends TypeToken<String> {
+  }
+
+  public void testTypeTokenNonAnonymousSubclass() {
+    TypeToken<?> typeToken = new CustomTypeToken();
+    assertEquals(String.class, typeToken.getRawType());
+    assertEquals(String.class, typeToken.getType());
+  }
+
+  /**
+   * User must only create direct subclasses of TypeToken, but not subclasses
+   * of subclasses (...) of TypeToken.
+   */
+  public void testTypeTokenSubSubClass() {
+    class SubTypeToken<T> extends TypeToken<String> {}
+    class SubSubTypeToken1<T> extends SubTypeToken<T> {}
+    class SubSubTypeToken2 extends SubTypeToken<Integer> {}
+
+    try {
+      new SubTypeToken<Integer>() {};
+      fail();
+    } catch (IllegalStateException expected) {
+      assertEquals("Must only create direct subclasses of TypeToken", expected.getMessage());
+    }
+
+    try {
+      new SubSubTypeToken1<Integer>();
+      fail();
+    } catch (IllegalStateException expected) {
+      assertEquals("Must only create direct subclasses of TypeToken", expected.getMessage());
+    }
+
+    try {
+      new SubSubTypeToken2();
+      fail();
+    } catch (IllegalStateException expected) {
+      assertEquals("Must only create direct subclasses of TypeToken", expected.getMessage());
+    }
+  }
+
+  @SuppressWarnings("rawtypes")
+  public void testTypeTokenRaw() {
+    try {
+      new TypeToken() {};
+      fail();
+    } catch (IllegalStateException expected) {
+      assertEquals("TypeToken must be created with a type argument: new TypeToken<...>() {}; "
+          + "When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved.",
+          expected.getMessage());
+    }
+  }
 }