Repository: google/gson (Java)

Issue:
Don't silently ignore missing type information from `TypeTokens`.
I'm active on Stack Overflow, and often see questions with code more or less like this:

```
public class Main {
		
	public static void main(String[] args) {
            // should work right?
	    MyClass mc = new Gson().fromJson("{}", Main.<MyClass>typeTokenHelper());
	}
	
	static class MyClass {}
	
	public static <T> Type typeTokenHelper() {
		return new TypeToken<T>() {}.getType();
	}

}
```

Of course this fails with:

```
java.lang.ClassCastException: com.google.gson.internal.LinkedTreeMap cannot be cast to test.Main$MyClass
```

Since there is not actually any type information available from the `TypeToken`. The `Type` that you get is the generic type `T`:

```
Type t = Main.<MyClass>typeTokenHelper();
System.out.println(t); // prints 'T'. instead of the naïvely expected 'MyClass'
```

This is confusing. The missing type information should not be silently ignored only to get a `ClassCastException` later. The missing type information is caused by a design time error and should be flagged as early as possible.

At first glance a check like this:

```
// where 'typeOfT' is the type returned by TypeToken::getType
if(typeOfT instanceof TypeVariable) {  // java.lang.reflect.TypeVariable
    throw new RuntimeExcepiton(...);
}
```

Somewhere might fix this issue. Perhaps in [`TypeToken::getSuperclassTypeParameter`](https://github.com/google/gson/blob/0636635cbffa08157bdbd558b1212e4d806474eb/gson/src/main/java/com/google/gson/reflect/TypeToken.java#L81) or in [`Gson::fromJson`](https://github.com/google/gson/blob/0636635cbffa08157bdbd558b1212e4d806474eb/gson/src/main/java/com/google/gson/Gson.java#L878). (I don't know where the use of TypeTokens with TypeVariables might be required though)


Hints:
@eamonnmcmanus, can this please be revisited (maybe using the original code from #2072) in one of the next major versions? The author of this issue is right, people keep running into this issue somewhat frequently, and as pointed out in https://github.com/google/gson/pull/2072#discussion_r807417151 this can also cause issues for serialization.

Here is an incomplete list of Stack Overflow questions and answers where users encountered issues due to this:
- https://stackoverflow.com/q/74321188
- https://stackoverflow.com/q/74357747
- https://stackoverflow.com/q/75567439
- https://stackoverflow.com/a/67921091
  Currently this answer erroneously suggests using `TypeToken<List<T>>() {}`
- https://stackoverflow.com/q/62147697
- https://stackoverflow.com/q/47618965
- https://stackoverflow.com/q/28123978
- https://stackoverflow.com/q/45554148

As I mentioned in the previous discussion, I am concerned that this would invalidate theoretically-incorrect but working code. The scenario I'm imagining is where the original developer is long gone and someone just wants to update the Gson dependency. All of a sudden they get this new exception and they have no idea how to go about fixing it.

As an alternative, could we detect this situation during deserialization and produce a more helpful exception? The exception could even indicate the line where the faulty `TypeToken` was created.
> All of a sudden they get this new exception and they have no idea how to go about fixing it.

We could at least assist the developers by creating an entry in the troubleshooting guide explaining which alternatives exist to solve this (this is [already covered](https://github.com/google/gson/blob/master/Troubleshooting.md#classcastexception-when-using-deserialized-object) to some extent). Maybe we could even directly add the troubleshooting guide URL in the exception message. (I have been thinking about proposing this for multiple Gson exceptions; I might create a separate issue for that.)

We could also add a "Gson unsafe features system property" which allows users to enable unsafe feature again, such as type variable capturing here with `gson-typetoken-allows-type-variable=true`. Though setting system properties can be annoying sometimes, especially if you have to set it before the Gson classes are loaded (we could avoid that by checking the system property every time) or if other unrelated classes clear the value (should hopefully not occur because these feature properties would all be opt-in; we could also document that clearing the system property is discouraged).

> As an alternative, could we detect this situation during deserialization and produce a more helpful exception? The exception could even indicate the line where the faulty `TypeToken` was created.

Maybe, but we would probably have to rewrite adapter retrieval for that and maybe even introduce a wrapper adapter (subclassing `SerializationDelegatingTypeAdapter`) which allows serialization but blocks deserialization.
Will capturing the stack trace be expensive (especially if users don't cache the `TypeToken`)? `java.lang.StackWalker` was only added in Java 9.

Yeah, I think capturing the stack trace is probably not a good idea. I'm hoping it will _usually_ be fairly obvious where the problem arose, especially since this should usually only happen just after someone has written the problematic code and is testing it.

A system property to turn off the new exception could be an alternative. We could mention the system property in the exception message. I'm a bit concerned that we may start to accumulate a whole bunch of these random system properties, which is why I've proposed the notion of a compatibility level in the past. But if we have to check that level in the `TypeToken` constructor then it would still have to be a system property or other static state.

Linking to the troubleshooting guide from exception messages seems like an excellent idea in general.

Patch:
diff --git a/gson/src/main/java/com/google/gson/reflect/TypeToken.java b/gson/src/main/java/com/google/gson/reflect/TypeToken.java
--- 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/shrinker-test/src/main/java/com/example/ClassWithJsonAdapterAnnotation.java b/shrinker-test/src/main/java/com/example/ClassWithJsonAdapterAnnotation.java
--- 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 {


Test patch:
diff --git a/gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java b/gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java
--- 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() {