Prevent TypeToken from capturing type variables (#2376)

* Prevent `TypeToken` from capturing type variables * Use hyphen for term "type-safe" Not completely sure if that is grammatically correct, but it might make the text a bit easier to understand. * Update Troubleshooting Guide URLs in tests from 'master' to 'main' * Rename system property * Simplify system property check
google · Aug 14, 2023 · 7b8ce2b · 7b8ce2b
1 parent db4a58a
commit 7b8ce2b
Show file tree

Hide file tree

Showing 4 changed files with 177 additions and 10 deletions.
diff --git a/Troubleshooting.md b/Troubleshooting.md
@@ -17,7 +17,7 @@ This guide describes how to troubleshoot common issues when using Gson.
   See the [user guide](UserGuide.md#collections-examples) for more information.
 - When using `TypeToken` prefer the `Gson.fromJson` overloads with `TypeToken` parameter such as [`fromJson(Reader, TypeToken)`](https://www.javadoc.io/doc/com.google.code.gson/gson/latest/com.google.gson/com/google/gson/Gson.html#fromJson(java.io.Reader,com.google.gson.reflect.TypeToken)).
   The overloads with `Type` parameter do not provide any type-safety guarantees.
-- When using `TypeToken` make sure you don't capture a type variable. For example avoid something like `new TypeToken<List<T>>()` (where `T` is a type variable). Due to Java type erasure the actual type of `T` is not available at runtime. Refactor your code to pass around `TypeToken` instances or use [`TypeToken.getParameterized(...)`](https://www.javadoc.io/doc/com.google.code.gson/gson/latest/com.google.gson/com/google/gson/reflect/TypeToken.html#getParameterized(java.lang.reflect.Type,java.lang.reflect.Type...)), for example `TypeToken.getParameterized(List.class, elementClass)`.
+- When using `TypeToken` make sure you don't capture a type variable. For example avoid something like `new TypeToken<List<T>>()` (where `T` is a type variable). Due to Java [type erasure](https://dev.java/learn/generics/type-erasure/) the actual type of `T` is not available at runtime. Refactor your code to pass around `TypeToken` instances or use [`TypeToken.getParameterized(...)`](https://www.javadoc.io/doc/com.google.code.gson/gson/latest/com.google.gson/com/google/gson/reflect/TypeToken.html#getParameterized(java.lang.reflect.Type,java.lang.reflect.Type...)), for example `TypeToken.getParameterized(List.class, elementType)` where `elementType` is a type you have to provide separately.
 
 ## <a id="reflection-inaccessible"></a> `InaccessibleObjectException`: 'module ... does not "opens ..." to unnamed module'
 
@@ -336,3 +336,22 @@ For Android you can add this rule to the `proguard-rules.pro` file, see also the
 For Android you can alternatively use the [`@Keep` annotation](https://developer.android.com/studio/write/annotations#keep) on the class or constructor you want to keep. That might be easier than having to maintain a custom R8 configuration.
 
 Note that the latest Gson versions (> 2.10.1) specify a default R8 configuration. If your class is a top-level class or is `static`, has a no-args constructor and its fields are annotated with Gson's [`@SerializedName`](https://www.javadoc.io/doc/com.google.code.gson/gson/latest/com.google.gson/com/google/gson/annotations/SerializedName.html), you might not have to perform any additional R8 configuration.
+
+## <a id="typetoken-type-variable"></a> `IllegalArgumentException`: 'TypeToken type argument must not contain a type variable'
+
+**Symptom:** An exception with the message 'TypeToken type argument must not contain a type variable' is thrown
+
+**Reason:** This exception is thrown when you create an anonymous `TypeToken` subclass which captures a type variable, for example `new TypeToken<List<T>>() {}` (where `T` is a type variable). At compile time such code looks safe and you can use the type `List<T>` without any warnings. However, this code is not actually type-safe because at runtime due to [type erasure](https://dev.java/learn/generics/type-erasure/) only the upper bound of the type variable is available. For the previous example that would be `List<Object>`. When using such a `TypeToken` with any Gson methods performing deserialization this would lead to confusing and difficult to debug `ClassCastException`s. For serialization it can in some cases also lead to undesired results.
+
+Note: Earlier version of Gson unfortunately did not prevent capturing type variables, which caused many users to unwittingly write type-unsafe code.
+
+**Solution:**
+
+- Use [`TypeToken.getParameterized(...)`](https://www.javadoc.io/doc/com.google.code.gson/gson/latest/com.google.gson/com/google/gson/reflect/TypeToken.html#getParameterized(java.lang.reflect.Type,java.lang.reflect.Type...)), for example `TypeToken.getParameterized(List.class, elementType)` where `elementType` is a type you have to provide separately.
+- For Kotlin users: Use [`reified` type parameters](https://kotlinlang.org/docs/inline-functions.html#reified-type-parameters), that means change `<T>` to `<reified T>`, if possible. If you have a chain of functions with type parameters you will probably have to make all of them `reified`.
+- If you don't actually use Gson's `TypeToken` for any Gson method, use a general purpose 'type token' implementation provided by a different library instead, for example Guava's [`com.google.common.reflect.TypeToken`](https://javadoc.io/doc/com.google.guava/guava/latest/com/google/common/reflect/TypeToken.html).
+
+For backward compatibility it is possible to restore Gson's old behavior of allowing `TypeToken` to capture type variables by setting the [system property](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/System.html#setProperty(java.lang.String,java.lang.String)) `gson.allowCapturingTypeVariables` to `"true"`, **however**:
+
+- This does not solve any of the type-safety problems mentioned above; in the long term you should prefer one of the other solutions listed above. This system property might be removed in future Gson versions.
+- You should only ever set the property to `"true"`, but never to any other value or manually clear it. Otherwise this might counteract any libraries you are using which might have deliberately set the system property because they rely on its behavior.
diff --git a/gson/src/main/java/com/google/gson/reflect/TypeToken.java b/gson/src/main/java/com/google/gson/reflect/TypeToken.java
@@ -22,6 +22,7 @@
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
 import java.lang.reflect.TypeVariable;
+import java.lang.reflect.WildcardType;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
@@ -38,11 +39,12 @@
  * <p>
  * {@code TypeToken<List<String>> list = new TypeToken<List<String>>() {};}
  *
- * <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.
+ * <p>Capturing a type variable as type argument of an anonymous {@code TypeToken}
+ * subclass is not allowed, for example {@code TypeToken<List<T>>}.
+ * 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.
+ * This would give a false sense of type-safety at compile time and could
+ * lead to an unexpected {@code ClassCastException} at runtime.
  *
  * <p>If the type arguments of the parameterized type are only available at
  * runtime, for example when you want to create a {@code List<E>} based on
@@ -64,7 +66,14 @@ public class TypeToken<T> {
    *
    * <p>Clients create an empty anonymous subclass. Doing so embeds the type
    * parameter in the anonymous class's type hierarchy so we can reconstitute it
-   * at runtime despite erasure.
+   * at runtime despite erasure, for example:
+   * <p>
+   * {@code new TypeToken<List<String>>() {}}
+   *
+   * @throws IllegalArgumentException
+   *   If the anonymous {@code TypeToken} subclass captures a type variable,
+   *   for example {@code TypeToken<List<T>>}. See the {@code TypeToken}
+   *   class documentation for more details.
    */
   @SuppressWarnings("unchecked")
   protected TypeToken() {
@@ -83,6 +92,10 @@ private TypeToken(Type type) {
     this.hashCode = this.type.hashCode();
   }
 
+  private static boolean isCapturingTypeVariablesForbidden() {
+    return !Objects.equals(System.getProperty("gson.allowCapturingTypeVariables"), "true");
+  }
+
   /**
    * 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
@@ -93,7 +106,12 @@ private Type getTypeTokenTypeArgument() {
     if (superclass instanceof ParameterizedType) {
       ParameterizedType parameterized = (ParameterizedType) superclass;
       if (parameterized.getRawType() == TypeToken.class) {
-        return $Gson$Types.canonicalize(parameterized.getActualTypeArguments()[0]);
+        Type typeArgument = $Gson$Types.canonicalize(parameterized.getActualTypeArguments()[0]);
+
+        if (isCapturingTypeVariablesForbidden()) {
+          verifyNoTypeVariable(typeArgument);
+        }
+        return typeArgument;
       }
     }
     // Check for raw TypeToken as superclass
@@ -108,6 +126,39 @@ else if (superclass == TypeToken.class) {
     throw new IllegalStateException("Must only create direct subclasses of TypeToken");
   }
 
+  private static void verifyNoTypeVariable(Type type) {
+    if (type instanceof TypeVariable) {
+      TypeVariable<?> typeVariable = (TypeVariable<?>) type;
+      throw new IllegalArgumentException("TypeToken type argument must not contain a type variable; captured type variable "
+          + typeVariable.getName() + " declared by " + typeVariable.getGenericDeclaration()
+          + "\nSee " + TroubleshootingGuide.createUrl("typetoken-type-variable"));
+    } else if (type instanceof GenericArrayType) {
+      verifyNoTypeVariable(((GenericArrayType) type).getGenericComponentType());
+    } else if (type instanceof ParameterizedType) {
+      ParameterizedType parameterizedType = (ParameterizedType) type;
+      Type ownerType = parameterizedType.getOwnerType();
+      if (ownerType != null) {
+        verifyNoTypeVariable(ownerType);
+      }
+
+      for (Type typeArgument : parameterizedType.getActualTypeArguments()) {
+        verifyNoTypeVariable(typeArgument);
+      }
+    } else if (type instanceof WildcardType) {
+      WildcardType wildcardType = (WildcardType) type;
+      for (Type bound : wildcardType.getLowerBounds()) {
+        verifyNoTypeVariable(bound);
+      }
+      for (Type bound : wildcardType.getUpperBounds()) {
+        verifyNoTypeVariable(bound);
+      }
+    } else if (type == null) {
+      // Occurs in Eclipse IDE and certain Java versions (e.g. Java 11.0.18) when capturing type variable
+      // declared by method of local class, see https://github.com/eclipse-jdt/eclipse.jdt.core/issues/975
+      throw new IllegalArgumentException("TypeToken captured `null` as type argument; probably a compiler / runtime bug");
+    }
+  }
+
   /**
    * Returns the raw (non-generic) type for this type.
    */
@@ -334,7 +385,7 @@ public static <T> TypeToken<T> get(Class<T> type) {
    * Class<V> valueClass = ...;
    * TypeToken<?> mapTypeToken = TypeToken.getParameterized(Map.class, keyClass, valueClass);
    * }</pre>
-   * As seen here the result is a {@code TypeToken<?>}; this method cannot provide any type safety,
+   * As seen here the result is a {@code TypeToken<?>}; this method cannot provide any type-safety,
    * and care must be taken to pass in the correct number of type arguments.
    *
    * <p>If {@code rawType} is a non-generic class and no type arguments are provided, this method

diff --git a/gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java b/gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java
@@ -20,8 +20,10 @@
 import static org.junit.Assert.assertThrows;
 
 import java.lang.reflect.GenericArrayType;
+import java.lang.reflect.Method;
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
+import java.lang.reflect.TypeVariable;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -233,6 +235,101 @@ class SubSubTypeToken2 extends SubTypeToken<Integer> {}
     assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
   }
 
+  private static <M> void createTypeTokenTypeVariable() {
+    new TypeToken<M>() {};
+  }
+
+  /**
+   * TypeToken type argument must not contain a type variable because, due to
+   * type erasure, at runtime only the bound of the type variable is available
+   * which is likely not what the user wanted.
+   *
+   * <p>Note that type variables are allowed for the {@code TypeToken} factory
+   * methods calling {@code TypeToken(Type)} because for them the return type is
+   * {@code TypeToken<?>} which does not give a false sense of type-safety.
+   */
+  @Test
+  public void testTypeTokenTypeVariable() throws Exception {
+    // Put the test code inside generic class to be able to access `T`
+    class Enclosing<T> {
+      class Inner {}
+
+      void test() {
+        String expectedMessage = "TypeToken type argument must not contain a type variable;"
+            + " captured type variable T declared by " + Enclosing.class
+            + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#typetoken-type-variable";
+        IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> new TypeToken<T>() {});
+        assertThat(e).hasMessageThat().isEqualTo(expectedMessage);
+
+        e = assertThrows(IllegalArgumentException.class, () -> new TypeToken<List<List<T>>>() {});
+        assertThat(e).hasMessageThat().isEqualTo(expectedMessage);
+
+        e = assertThrows(IllegalArgumentException.class, () -> new TypeToken<List<? extends List<T>>>() {});
+        assertThat(e).hasMessageThat().isEqualTo(expectedMessage);
+
+        e = assertThrows(IllegalArgumentException.class, () -> new TypeToken<List<? super List<T>>>() {});
+        assertThat(e).hasMessageThat().isEqualTo(expectedMessage);
+
+        e = assertThrows(IllegalArgumentException.class, () -> new TypeToken<List<T>[]>() {});
+        assertThat(e).hasMessageThat().isEqualTo(expectedMessage);
+
+        e = assertThrows(IllegalArgumentException.class, () -> new TypeToken<Enclosing<T>.Inner>() {});
+        assertThat(e).hasMessageThat().isEqualTo(expectedMessage);
+
+        String systemProperty = "gson.allowCapturingTypeVariables";
+        try {
+          // Any value other than 'true' should be ignored
+          System.setProperty(systemProperty, "some-value");
+
+          e = assertThrows(IllegalArgumentException.class, () -> new TypeToken<T>() {});
+          assertThat(e).hasMessageThat().isEqualTo(expectedMessage);
+        } finally {
+          System.clearProperty(systemProperty);
+        }
+
+        try {
+          System.setProperty(systemProperty, "true");
+
+          TypeToken<?> typeToken = new TypeToken<T>() {};
+          assertThat(typeToken.getType()).isEqualTo(Enclosing.class.getTypeParameters()[0]);
+        } finally {
+          System.clearProperty(systemProperty);
+        }
+      }
+
+      <M> void testMethodTypeVariable() throws Exception {
+        Method testMethod = Enclosing.class.getDeclaredMethod("testMethodTypeVariable");
+        IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> new TypeToken<M>() {});
+        assertThat(e).hasMessageThat().isAnyOf("TypeToken type argument must not contain a type variable;"
+            + " captured type variable M declared by " + testMethod
+            + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#typetoken-type-variable",
+            // Note: When running this test in Eclipse IDE or with certain Java versions it seems to capture `null`
+            // instead of the type variable, see https://github.com/eclipse-jdt/eclipse.jdt.core/issues/975
+            "TypeToken captured `null` as type argument; probably a compiler / runtime bug");
+      }
+    }
+
+    new Enclosing<>().test();
+    new Enclosing<>().testMethodTypeVariable();
+
+    Method testMethod = TypeTokenTest.class.getDeclaredMethod("createTypeTokenTypeVariable");
+    IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> createTypeTokenTypeVariable());
+    assertThat(e).hasMessageThat().isEqualTo("TypeToken type argument must not contain a type variable;"
+        + " captured type variable M declared by " + testMethod
+        + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#typetoken-type-variable");
+
+    // Using type variable as argument for factory methods should be allowed; this is not a type-safety
+    // problem because the user would have to perform unsafe casts
+    TypeVariable<?> typeVar = Enclosing.class.getTypeParameters()[0];
+    TypeToken<?> typeToken = TypeToken.get(typeVar);
+    assertThat(typeToken.getType()).isEqualTo(typeVar);
+
+    TypeToken<?> parameterizedTypeToken = TypeToken.getParameterized(List.class, typeVar);
+    ParameterizedType parameterizedType = (ParameterizedType) parameterizedTypeToken.getType();
+    assertThat(parameterizedType.getRawType()).isEqualTo(List.class);
+    assertThat(parameterizedType.getActualTypeArguments()).asList().containsExactly(typeVar);
+  }
+
   @SuppressWarnings("rawtypes")
   @Test
   public void testTypeTokenRaw() {

diff --git a/shrinker-test/src/main/java/com/example/ClassWithJsonAdapterAnnotation.java b/shrinker-test/src/main/java/com/example/ClassWithJsonAdapterAnnotation.java
@@ -78,7 +78,7 @@ public void write(JsonWriter out, DummyClass value) throws IOException {
   static class Factory implements TypeAdapterFactory {
     @Override
     public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) {
-      @SuppressWarnings("unchecked") // the code below is not type safe, but does not matter for this test
+      @SuppressWarnings("unchecked") // the code below is not type-safe, but does not matter for this test
       TypeAdapter<T> r = (TypeAdapter<T>) new TypeAdapter<DummyClass>() {
         @Override
         public DummyClass read(JsonReader in) throws IOException {