Improve creation of ParameterizedType (#2375)

- Reject non-generic raw types for TypeToken.getParameterized - Fix ParameterizedTypeImpl erroneously requiring owner type for types without owner
google · Jun 23, 2023 · a589ef2 · a589ef2
1 parent 9cf0f8d
commit a589ef2
Show file tree

Hide file tree

Showing 4 changed files with 132 additions and 119 deletions.
diff --git a/gson/src/main/java/com/google/gson/internal/$Gson$Types.java b/gson/src/main/java/com/google/gson/internal/$Gson$Types.java
@@ -33,8 +33,8 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.NoSuchElementException;
-import java.util.Properties;
 import java.util.Objects;
+import java.util.Properties;
 
 /**
  * Static methods for working with types.
@@ -138,9 +138,8 @@ public static Class<?> getRawType(Type type) {
     } else if (type instanceof ParameterizedType) {
       ParameterizedType parameterizedType = (ParameterizedType) type;
 
-      // I'm not exactly sure why getRawType() returns Type instead of Class.
-      // Neal isn't either but suspects some pathological case related
-      // to nested classes exists.
+      // getRawType() returns Type instead of Class; that seems to be an API mistake,
+      // see https://bugs.openjdk.org/browse/JDK-8250659
       Type rawType = parameterizedType.getRawType();
       checkArgument(rawType instanceof Class);
       return (Class<?>) rawType;
@@ -481,19 +480,33 @@ static void checkNotPrimitive(Type type) {
     checkArgument(!(type instanceof Class<?>) || !((Class<?>) type).isPrimitive());
   }
 
+  /**
+   * Whether an {@linkplain ParameterizedType#getOwnerType() owner type} must be specified when
+   * constructing a {@link ParameterizedType} for {@code rawType}.
+   *
+   * <p>Note that this method might not require an owner type for all cases where Java reflection
+   * would create parameterized types with owner type.
+   */
+  public static boolean requiresOwnerType(Type rawType) {
+    if (rawType instanceof Class<?>) {
+      Class<?> rawTypeAsClass = (Class<?>) rawType;
+      return !Modifier.isStatic(rawTypeAsClass.getModifiers())
+          && rawTypeAsClass.getDeclaringClass() != null;
+    }
+    return false;
+  }
+
   private static final class ParameterizedTypeImpl implements ParameterizedType, Serializable {
     private final Type ownerType;
     private final Type rawType;
     private final Type[] typeArguments;
 
     public ParameterizedTypeImpl(Type ownerType, Type rawType, Type... typeArguments) {
+      // TODO: Should this enforce that rawType is a Class? See JDK implementation of
+      // the ParameterizedType interface and https://bugs.openjdk.org/browse/JDK-8250659
       requireNonNull(rawType);
-      // require an owner type if the raw type needs it
-      if (rawType instanceof Class<?>) {
-        Class<?> rawTypeAsClass = (Class<?>) rawType;
-        boolean isStaticOrTopLevelClass = Modifier.isStatic(rawTypeAsClass.getModifiers())
-            || rawTypeAsClass.getEnclosingClass() == null;
-        checkArgument(ownerType != null || isStaticOrTopLevelClass);
+      if (ownerType == null && requiresOwnerType(rawType)) {
+        throw new IllegalArgumentException("Must specify owner type for " + rawType);
       }
 
       this.ownerType = ownerType == null ? null : canonicalize(ownerType);

diff --git a/gson/src/main/java/com/google/gson/reflect/TypeToken.java b/gson/src/main/java/com/google/gson/reflect/TypeToken.java
@@ -338,8 +338,8 @@ public static <T> TypeToken<T> get(Class<T> type) {
    * and care must be taken to pass in the correct number of type arguments.
    *
    * @throws IllegalArgumentException
-   *   If {@code rawType} is not of type {@code Class}, or if the type arguments are invalid for
-   *   the raw type
+   *   If {@code rawType} is not of type {@code Class}, if it is not a generic type, or if the
+   *   type arguments are invalid for the raw type
    */
   public static TypeToken<?> getParameterized(Type rawType, Type... typeArguments) {
     Objects.requireNonNull(rawType);
@@ -354,6 +354,18 @@ public static TypeToken<?> getParameterized(Type rawType, Type... typeArguments)
     Class<?> rawClass = (Class<?>) rawType;
     TypeVariable<?>[] typeVariables = rawClass.getTypeParameters();
 
+    // Note: Does not check if owner type of rawType is generic because this factory method
+    // does not support specifying owner type
+    if (typeVariables.length == 0) {
+      throw new IllegalArgumentException(rawClass.getName() + " is not a generic type");
+    }
+
+    // Check for this here to avoid misleading exception thrown by ParameterizedTypeImpl
+    if ($Gson$Types.requiresOwnerType(rawType)) {
+      throw new IllegalArgumentException("Raw type " + rawClass.getName() + " is not supported because"
+          + " it requires specifying an owner type");
+    }
+
     int expectedArgsCount = typeVariables.length;
     int actualArgsCount = typeArguments.length;
     if (actualArgsCount != expectedArgsCount) {
@@ -362,16 +374,16 @@ public static TypeToken<?> getParameterized(Type rawType, Type... typeArguments)
     }
 
     for (int i = 0; i < expectedArgsCount; i++) {
-      Type typeArgument = typeArguments[i];
+      Type typeArgument = Objects.requireNonNull(typeArguments[i], "Type argument must not be null");
       Class<?> rawTypeArgument = $Gson$Types.getRawType(typeArgument);
       TypeVariable<?> typeVariable = typeVariables[i];
 
       for (Type bound : typeVariable.getBounds()) {
         Class<?> rawBound = $Gson$Types.getRawType(bound);
 
         if (!rawBound.isAssignableFrom(rawTypeArgument)) {
-          throw new IllegalArgumentException("Type argument " + typeArgument + " does not satisfy bounds "
-              + "for type variable " + typeVariable + " declared by " + rawType);
+          throw new IllegalArgumentException("Type argument " + typeArgument + " does not satisfy bounds"
+              + " for type variable " + typeVariable + " declared by " + rawType);
         }
       }
     }

diff --git a/gson/src/test/java/com/google/gson/internal/GsonTypesTest.java b/gson/src/test/java/com/google/gson/internal/GsonTypesTest.java
@@ -17,7 +17,7 @@
 package com.google.gson.internal;
 
 import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.assertThrows;
 
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
@@ -29,20 +29,33 @@ public final class GsonTypesTest {
   @Test
   public void testNewParameterizedTypeWithoutOwner() throws Exception {
     // List<A>. List is a top-level class
-    Type type = $Gson$Types.newParameterizedTypeWithOwner(null, List.class, A.class);
-    assertThat(getFirstTypeArgument(type)).isEqualTo(A.class);
+    ParameterizedType type = $Gson$Types.newParameterizedTypeWithOwner(null, List.class, A.class);
+    assertThat(type.getOwnerType()).isNull();
+    assertThat(type.getRawType()).isEqualTo(List.class);
+    assertThat(type.getActualTypeArguments()).asList().containsExactly(A.class);
 
     // A<B>. A is a static inner class.
     type = $Gson$Types.newParameterizedTypeWithOwner(null, A.class, B.class);
     assertThat(getFirstTypeArgument(type)).isEqualTo(B.class);
 
+    IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
+        // NonStaticInner<A> is not allowed without owner
+        () -> $Gson$Types.newParameterizedTypeWithOwner(null, NonStaticInner.class, A.class));
+    assertThat(e).hasMessageThat().isEqualTo("Must specify owner type for " + NonStaticInner.class);
+
+    type = $Gson$Types.newParameterizedTypeWithOwner(GsonTypesTest.class, NonStaticInner.class, A.class);
+    assertThat(type.getOwnerType()).isEqualTo(GsonTypesTest.class);
+    assertThat(type.getRawType()).isEqualTo(NonStaticInner.class);
+    assertThat(type.getActualTypeArguments()).asList().containsExactly(A.class);
+
     final class D {
     }
-    try {
-      // D<A> is not allowed since D is not a static inner class
-      $Gson$Types.newParameterizedTypeWithOwner(null, D.class, A.class);
-      fail();
-    } catch (IllegalArgumentException expected) {}
+
+    // D<A> is allowed since D has no owner type
+    type = $Gson$Types.newParameterizedTypeWithOwner(null, D.class, A.class);
+    assertThat(type.getOwnerType()).isNull();
+    assertThat(type.getRawType()).isEqualTo(D.class);
+    assertThat(type.getActualTypeArguments()).asList().containsExactly(A.class);
 
     // A<D> is allowed.
     type = $Gson$Types.newParameterizedTypeWithOwner(null, A.class, D.class);
@@ -63,6 +76,9 @@ private static final class B {
   }
   private static final class C {
   }
+  @SuppressWarnings({"ClassCanBeStatic", "UnusedTypeParameter"})
+  private final class NonStaticInner<T> {
+  }
 
   /**
    * Given a parameterized type A&lt;B,C&gt;, returns B. If the specified type is not

diff --git a/gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java b/gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java
@@ -17,9 +17,10 @@
 package com.google.gson.reflect;
 
 import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.assertThrows;
 
 import java.lang.reflect.GenericArrayType;
+import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
 import java.util.ArrayList;
 import java.util.List;
@@ -103,11 +104,13 @@ public void testArrayFactory() {
     Type listOfString = new TypeToken<List<String>>() {}.getType();
     assertThat(TypeToken.getArray(listOfString)).isEqualTo(expectedListOfStringArray);
 
-    try {
-      TypeToken.getArray(null);
-      fail();
-    } catch (NullPointerException e) {
-    }
+    TypeToken<?> expectedIntArray = new TypeToken<int[]>() {};
+    assertThat(TypeToken.getArray(int.class)).isEqualTo(expectedIntArray);
+
+    assertThrows(NullPointerException.class, () -> TypeToken.getArray(null));
+  }
+
+  static class NestedGeneric<T> {
   }
 
   @Test
@@ -131,84 +134,70 @@ public void testParameterizedFactory() {
 
     TypeToken<?> expectedSatisfyingTwoBounds = new TypeToken<GenericWithMultiBound<ClassSatisfyingBounds>>() {};
     assertThat(TypeToken.getParameterized(GenericWithMultiBound.class, ClassSatisfyingBounds.class)).isEqualTo(expectedSatisfyingTwoBounds);
+
+    TypeToken<?> nestedTypeToken = TypeToken.getParameterized(NestedGeneric.class, Integer.class);
+    ParameterizedType nestedParameterizedType = (ParameterizedType) nestedTypeToken.getType();
+    // TODO: This seems to differ from how Java reflection behaves; when using TypeToken<NestedGeneric<Integer>>,
+    // then NestedGeneric<Integer> does have an owner type
+    assertThat(nestedParameterizedType.getOwnerType()).isNull();
+    assertThat(nestedParameterizedType.getRawType()).isEqualTo(NestedGeneric.class);
+    assertThat(nestedParameterizedType.getActualTypeArguments()).asList().containsExactly(Integer.class);
+
+    class LocalGenericClass<T> {}
+    TypeToken<?> expectedLocalType = new TypeToken<LocalGenericClass<Integer>>() {};
+    assertThat(TypeToken.getParameterized(LocalGenericClass.class, Integer.class)).isEqualTo(expectedLocalType);
   }
 
   @Test
   public void testParameterizedFactory_Invalid() {
-    try {
-      TypeToken.getParameterized(null, new Type[0]);
-      fail();
-    } catch (NullPointerException e) {
-    }
+    assertThrows(NullPointerException.class, () -> TypeToken.getParameterized(null, new Type[0]));
+    assertThrows(NullPointerException.class, () -> TypeToken.getParameterized(List.class, new Type[] { null }));
 
     GenericArrayType arrayType = (GenericArrayType) TypeToken.getArray(String.class).getType();
-    try {
-      TypeToken.getParameterized(arrayType, new Type[0]);
-      fail();
-    } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("rawType must be of type Class, but was java.lang.String[]");
-    }
+    IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(arrayType, new Type[0]));
+    assertThat(e).hasMessageThat().isEqualTo("rawType must be of type Class, but was java.lang.String[]");
 
-    try {
-      TypeToken.getParameterized(String.class, String.class);
-      fail();
-    } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("java.lang.String requires 0 type arguments, but got 1");
-    }
+    e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(String.class, Number.class));
+    assertThat(e).hasMessageThat().isEqualTo("java.lang.String is not a generic type");
 
-    try {
-      TypeToken.getParameterized(List.class, new Type[0]);
-      fail();
-    } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("java.util.List requires 1 type arguments, but got 0");
-    }
+    e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(List.class, new Type[0]));
+    assertThat(e).hasMessageThat().isEqualTo("java.util.List requires 1 type arguments, but got 0");
 
-    try {
-      TypeToken.getParameterized(List.class, String.class, String.class);
-      fail();
-    } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("java.util.List requires 1 type arguments, but got 2");
-    }
+    e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(List.class, String.class, String.class));
+    assertThat(e).hasMessageThat().isEqualTo("java.util.List requires 1 type arguments, but got 2");
 
-    try {
-      TypeToken.getParameterized(GenericWithBound.class, String.class);
-      fail();
-    } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.String does not satisfy bounds "
-          + "for type variable T declared by " + GenericWithBound.class);
-    }
+    // Primitive types must not be used as type argument
+    e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(List.class, int.class));
+    assertThat(e).hasMessageThat().isEqualTo("Type argument int does not satisfy bounds"
+        + " for type variable E declared by " + List.class);
 
-    try {
-      TypeToken.getParameterized(GenericWithBound.class, Object.class);
-      fail();
-    } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds "
-          + "for type variable T declared by " + GenericWithBound.class);
-    }
+    e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithBound.class, String.class));
+    assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.String does not satisfy bounds"
+        + " for type variable T declared by " + GenericWithBound.class);
 
-    try {
-      TypeToken.getParameterized(GenericWithMultiBound.class, Number.class);
-      fail();
-    } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Number does not satisfy bounds "
-          + "for type variable T declared by " + GenericWithMultiBound.class);
-    }
+    e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithBound.class, Object.class));
+    assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds"
+        + " for type variable T declared by " + GenericWithBound.class);
 
-    try {
-      TypeToken.getParameterized(GenericWithMultiBound.class, CharSequence.class);
-      fail();
-    } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("Type argument interface java.lang.CharSequence does not satisfy bounds "
-          + "for type variable T declared by " + GenericWithMultiBound.class);
-    }
+    e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithMultiBound.class, Number.class));
+    assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Number does not satisfy bounds"
+        + " for type variable T declared by " + GenericWithMultiBound.class);
+
+    e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithMultiBound.class, CharSequence.class));
+    assertThat(e).hasMessageThat().isEqualTo("Type argument interface java.lang.CharSequence does not satisfy bounds"
+        + " for type variable T declared by " + GenericWithMultiBound.class);
+
+    e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithMultiBound.class, Object.class));
+    assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds"
+        + " for type variable T declared by " + GenericWithMultiBound.class);
 
-    try {
-      TypeToken.getParameterized(GenericWithMultiBound.class, Object.class);
-      fail();
-    } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds "
-          + "for type variable T declared by " + GenericWithMultiBound.class);
+    class Outer {
+      class NonStaticInner<T> {}
     }
+
+    e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(Outer.NonStaticInner.class, Object.class));
+    assertThat(e).hasMessageThat().isEqualTo("Raw type " + Outer.NonStaticInner.class.getName()
+        + " is not supported because it requires specifying an owner type");
   }
 
   private static class CustomTypeToken extends TypeToken<String> {
@@ -231,40 +220,23 @@ 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) {
-      assertThat(expected).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
-    }
+    IllegalStateException e = assertThrows(IllegalStateException.class, () -> new SubTypeToken<Integer>() {});
+    assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
 
-    try {
-      new SubSubTypeToken1<Integer>();
-      fail();
-    } catch (IllegalStateException expected) {
-      assertThat(expected).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
-    }
+    e = assertThrows(IllegalStateException.class, () -> new SubSubTypeToken1<Integer>());
+    assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
 
-    try {
-      new SubSubTypeToken2();
-      fail();
-    } catch (IllegalStateException expected) {
-      assertThat(expected).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
-    }
+    e = assertThrows(IllegalStateException.class, () -> new SubSubTypeToken2());
+    assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
   }
 
   @SuppressWarnings("rawtypes")
   @Test
   public void testTypeTokenRaw() {
-    try {
-      new TypeToken() {};
-      fail();
-    } catch (IllegalStateException expected) {
-      assertThat(expected).hasMessageThat().isEqualTo("TypeToken must be created with a type argument: new TypeToken<...>() {};"
-          + " When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved."
-          + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#type-token-raw"
-      );
-    }
+    IllegalStateException e = assertThrows(IllegalStateException.class, () -> new TypeToken() {});
+    assertThat(e).hasMessageThat().isEqualTo("TypeToken must be created with a type argument: new TypeToken<...>() {};"
+        + " When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved."
+        + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#type-token-raw");
   }
 }