google / gson

A Java serialization/deserialization library to convert Java Objects into JSON and back
Apache License 2.0
23.4k stars 4.29k forks source link

Unsatisfactory handling of type variables #1741

Open TaylanUB opened 4 years ago

TaylanUB commented 4 years ago

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 commented 4 years ago

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:


Note that I am not a member of this project.