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

Unsatisfactory handling of type variables #1741

Open
TaylanUB opened this issue Jul 21, 2020 · 1 comment
Open

Unsatisfactory handling of type variables #1741

TaylanUB opened this issue Jul 21, 2020 · 1 comment

Comments

@TaylanUB
Copy link

GSON Version: 2.8.6

When $Gson$Types.getRawType(Type) is passed a TypeVariable instance, it simply gives up and returns Object.class:

    ...
    } else if (type instanceof TypeVariable) {
      // we could use the variable's bounds, but that won't work if there are multiple.
      // having a raw type that's more general than necessary is okay
      return Object.class;
    } ...

That's not very good, and giving up just because there may be several bounds seems unnecessary. Since the behavior causes issues with my custom type adapter, I've had to resort to the following hack. (The TypedField class is a component of my custom type adapter. If you want to see the full code, here's the whole shebang.)

    private static class TypedField {
        private final Field field;
        private final Type type;

        private TypedField(Type staticContext, Class<?> actualClass, Field field) {
            this.field = field;
            Type type = $Gson$Types.resolve(staticContext, actualClass, field.getGenericType());
            // As of Gson version 2.8.6, there is absolutely no way of making it use something
            // other than ObjectTypeAdapter for a Type that is a TypeVariable.  For that reason
            // we change the type here to the upper bound of the type variable.
            if (type instanceof TypeVariable) {
                this.type = Util.ifNull(getUpperBound(type), Object.class);
            } else {
                this.type = type;
            }
        }

        ...

        private static Type getUpperBound(Type type) {
            if (isNormalClass(type)) {
                return type;
            } else if (type instanceof ParameterizedType) {
                return ((ParameterizedType) type).getRawType();
            } else if (type instanceof TypeVariable) {
                for (Type bound : ((TypeVariable<?>) type).getBounds()) {
                    Type upperBound = getUpperBound(bound);
                    if (upperBound != null) {
                        return upperBound;
                    }
                }
            } else if (type instanceof WildcardType) {
                for (Type bound : ((WildcardType) type).getUpperBounds()) {
                    Type upperBound = getUpperBound(bound);
                    if (upperBound != null) {
                        return upperBound;
                    }
                }
            }
            return null;
        }

        private static boolean isNormalClass(Type type) {
            return type instanceof Class && ((Class<?>) type).isAssignableFrom(Object.class);
        }
    }

As you see, the way I handle type variables is to traverse their upper bounds and return the first type that's an actual class, i.e. subtype of Object. Incidentally, type variables can only have one upper bound that's an actual class, so there can be no confusion here. Only if no upper bound is an actual class, then the fallback is once again Object.class. (Even better might be to return the one upper bound if there is only one, and only fall back to Object.class in case of ambiguity, but I personally don't need that.)

I think GSON might want to adopt this behavior, but I first wanted some feedback before I make a patch. I'm also not sure whether I understand the relevant parts of GSON's internals correctly.

Another issue I encountered today because of GSON's current behavior, which I'm not able to fix with my custom type adapter, is the following:

Let's say I have a class PencilBox<PencilType extends Pencil> with a field List<PencilType> myPencils:

public class PencilBox<PencilType extends Pencil> {
    private List<PencilType> myPencils;

    public PencilBox(Reader reader) {
        TypeToken<List<PencilType>> token = new TypeToken<List<PencilType>(){};
        myPencils = getGsonWithBarAdapter().fromJson(reader, token.getType());
    }
}

And let's say I have a custom type adapter for the class hierarchy starting from Pencil.

When new PencilBox<ColoringPencil>(reader) is called, I expect that GSON will use my custom type adapter which I've registered for the Pencil type hierarchy. But no such thing happens, because GSON gives up on trying to figure out the upper bound of the PencilType type variable. I get a list of LinkedTreeMap instances which then causes a ClassCastException as I'm trying to stuff them in a List that's meant to hold objects of type Pencil.

I guess the current solution here would be to use TypeToken<List<Pencil>>. I'm not sure if that might cause any headaches during refactoring, but it certainly breaks intuition. GSON should realize that List<PencilType> is effectively List<Pencil> in that situation.

@Marcono1234
Copy link
Collaborator

I think Gson should only resolve type variables for raw types. For every other case I can think of the user is expecting behavior which Gson does not provide and an exception should be thrown (though this is currently not the case). Please let me know if you can think of any other legit use cases for resolving type variables.

Your PencilBox example shows one case where a type variable is misused. The use of the type variable PencilType suggests that Gson is aware of the type argument for that variable, which is however not the case due to Java's type erasure. Even your proposed fix does not solve this.
Consider this example: The user of your class creates a new PencilBox<RedPencil>(json), however the JSON data actually represents a PencilBox<BluePencil>. This would deserialize fine but cause runtime exceptions. The use of the type variable creates the illusion of type-safety here. A more correct implementation would be to have a method static PencilBox<?> deserialize(Reader reader) instead.

And Gson could not even support all type variable usages for raw types:

  • Type variables with multiple bounds cannot be supported because TypeToken cannot represent them. Only using the first bound is not an option because the deserialized type has to satisfy both bounds. E.g. T extends Number & CharSequence could not be deserialized as Number because the created object might not be a CharSequence.
    Note that there are some cases for which multiple bounds could be supported but they are likely rather rare which does not justify the required effort:
    • One bound includes every other bound: E.g. X extends String & CharSequence could be simplified as X extends String since String implements CharSequence. This becomes more complex if the other bounds are parameterized types.
    • One bound is a type variable and a type argument for it satisfies all other bounds: E.g. X extends Y & CharSequence where Y has the type argument String. In this case the type could be resolved as String since it implements CharSequence.
  • Type variables with cyclic bounds cannot be supported because TypeToken cannot represent them. For example X extends Comparable<X> or <X extends Y, Y extends Comparable<X>> cannot be represented using TypeToken. Both can be satisfied if a type is comparable with itself, e.g. Integer implements Comparable<Integer>. However, TypeToken could only represent it as Comparable<Object> respectively Comparable<Comparable<Object>> (or similar) which would allow types which are not comparable with objects of the same type only.

Note that I am not a member of this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants