Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent TypeToken from capturing type variables #2376

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Apr 16, 2023

⚠️ Depends on #2375

Purpose

Fixes #1219

Description

Throws an exception when the user tries to capture a type variable using new TypeToken<...>() {}, see #1219. This is extremely error-prone because at compile time such code looks safe, but at runtime it leads to confusing ClassCastExceptions due to type erasure; and users keep running into this issue (see #1219 (comment)).

The exception message which is thrown refers to a new troubleshooting guide section which describes possible workarounds. Additionally for now an opt-in system property is added which allows capturing type variables again.

Checklist

  • New code follows the Google Java Style Guide
  • If necessary, new public API validates arguments, for example rejects null
  • New public API has Javadoc
    • Javadoc uses @since $next-version$
      ($next-version$ is a special placeholder which is automatically replaced during release)
  • If necessary, new unit tests have been added
    • Assertions in unit tests use Truth, see existing tests
    • No JUnit 3 features are used (such as extending class TestCase)
    • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
  • mvn clean verify javadoc:jar passes without errors

@Marcono1234 Marcono1234 force-pushed the marcono1234/no-typetoken-type-var-capture branch from a8934ed to eebd97a Compare April 16, 2023 14:35
Comment on lines +155 to +159
// 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");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite an interesting bug, which even occurred even on CI when running with JDK 11, so in the corresponding unit tests it checks for this exception message as well.

See also eclipse-jdt/eclipse.jdt.core#975

@@ -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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refers to https://dev.java here and below because that page contains a good explanation, and also this seems to be the official Java website, even though it is probably not that well known yet because it is still quite new.


- 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).
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Apr 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not completely sure to what extent the Guava TypeToken actually wants to support capturing type variables. For example new TypeToken<T>() {} throws an exception. However creating TypeToken<List<T>> and similar works. Similarly the wiki says that TypeToken "supports non-reified types such as T, List<T>", so support for type variables seems to be at least to some extent intended.

@Marcono1234
Copy link
Collaborator Author

@eamonnmcmanus, what is your position on this? Your comment #1219 (comment) sounded to me as if you are generally open to adding this restriction eventually, but maybe I misunderstood it. The solution proposed here does not differentiate between serialization and deserialization, it blocks usage of type variables for TypeToken in general.

Depending on that I will try to resolve the merge conflicts in the near future.

@eamonnmcmanus
Copy link
Member

@eamonnmcmanus, what is your position on this? Your comment #1219 (comment) sounded to me as if you are generally open to adding this restriction eventually, but maybe I misunderstood it. The solution proposed here does not differentiate between serialization and deserialization, it blocks usage of type variables for TypeToken in general.

OK, a few things. I commented on #2375 separately and I think we can probably make that work with the small change I suggested.

I ran this change against all of Google's internal tests and encountered one failure. The class looks like this in outline:

public abstract class AbstractAction<D extends Dto, T extends Service<D>> implements Action {
  ...
  private final Type listType = new TypeToken<List<D>>() {}.getType();
  ...
   return new SafeJsonResponse(GSON, result, listType);

So, even though Gson is in the picture, here TypeToken is just being used to get a Type for List<D>. However, that Type does later get used for gson.toJson(object, type) inside SafeJsonResponse.

I naively tried to fix the new exception by writing new TypeToken<List<? extends Dto>> but that didn't work. A test failure suggests that the JSON serialization was different. I didn't track down why. new TypeToken<List<Dto>> does work. I mention that because it suggests that fixing code to avoid the new exception may not be completely trivial. (In fact I see that I already changed this code to ? extends Dto last year but that change was reverted. The test failure I saw didn't exist then but presumably the serialization difference did.)

Just one problem out of tens of thousands of files referencing Gson is a pretty low rate, and needs to be balanced against the many issues you found on StackOverflow. So I think we probably could control this with a system property. For example the class I cited could have a static intializer that sets the property.

I think this would be the first time we introduce a system property into the Gson code, so we would need to decide on a naming convention. Unfortunately system property names are all over the place, but using dashes does seem to be somewhat unusual. I've never been very happy with the likes of com.google.gson.words.separated.by.dots either. I think maybe the least-bad option is to use something that looks like a class name, for example com.google.gson.AllowCapturingTypeVariables or gson.AllowCapturingTypeVariables.

@Marcono1234
Copy link
Collaborator Author

Thanks for taking a look and thanks for the feedback on this!

I naively tried to fix the new exception by writing new TypeToken<List<? extends Dto>> but that didn't work.

Might be #1870; for WildcardType the runtime type is not used so that could make a difference in the JSON output. Or the reason could be that the Collection and Map type resolution logic does not use the upper bounds for wildcards, the Map resolution logic has a tentative TODO comment for that.

I mention that because it suggests that fixing code to avoid the new exception may not be completely trivial

In this specific case, if you have Class<D> available within AbstractAction, maybe you could use TypeToken.getParameterized(List.class, classOfD) to create listType?

I think this would be the first time we introduce a system property into the Gson code, so we would need to decide on a naming convention. Unfortunately system property names are all over the place, but using dashes does seem to be somewhat unusual. I've never been very happy with the likes of com.google.gson.words.separated.by.dots either. I think maybe the least-bad option is to use something that looks like a class name, for example com.google.gson.AllowCapturingTypeVariables or gson.AllowCapturingTypeVariables.

Good point! If we use a naming which looks like a package or class name, then we could also use the qualified class name of TypeToken and append a 'fake' field name, for example something like com.google.gson.reflect.TypeToken.allowCapturingTypeVariables. Though that might be a bit verbose. What do you think?

Your shorter suggestions also sound good, but I think it would be better to lowercase the first a of AllowCa.... Otherwise it looking like a class name might rather cause confusion, because if I recall correctly sometimes system properties do actually have the name of a class for example when the user can specify a concrete implementation for that class.

Not completely sure if that is grammatically correct, but it might make the
text a bit easier to understand.
@Marcono1234 Marcono1234 force-pushed the marcono1234/no-typetoken-type-var-capture branch from eebd97a to 9bc4255 Compare June 23, 2023 23:31
@Marcono1234
Copy link
Collaborator Author

Rebased the changes onto main, now after #2375 has been merged (thanks for that!). But to avoid any misunderstanding, I think your question about naming the system property is still open.

@Marcono1234
Copy link
Collaborator Author

Would using gson.allowCapturingTypeVariables (with lowercase a) as system property name be fine for you?

@eamonnmcmanus
Copy link
Member

Would using gson.allowCapturingTypeVariables (with lowercase a) as system property name be fine for you?

Yes, I think the convention that that establishes looks good.

@Marcono1234
Copy link
Collaborator Author

@eamonnmcmanus, do you think this can be merged?

In the worst case, if this is really causing more issues than it helps preventing bugs, we could revert it again in a future release. Since this only adds validation but does not introduce any new feature or alters Gson behavior in any other way, reverting the changes should be safe.

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. I think we can go ahead with this. Just one small change requested.

gson/src/main/java/com/google/gson/reflect/TypeToken.java Outdated Show resolved Hide resolved
@eamonnmcmanus eamonnmcmanus merged commit 7b8ce2b into google:main Aug 14, 2023
@Marcono1234 Marcono1234 deleted the marcono1234/no-typetoken-type-var-capture branch August 15, 2023 20:11
tibor-universe pushed a commit to getuniverse/gson that referenced this pull request Sep 14, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't silently ignore missing type information from TypeTokens.
2 participants