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.

Sign up for GitHub

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

Improve creation of ParameterizedType #2375

Merged
merged 2 commits into from
Jun 23, 2023
Merged

Improve creation of ParameterizedType #2375

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f0de5e
Improve creation of `ParameterizedType`
Marcono1234 Apr 15, 2023
fdc0d67
Merge branch 'main' into marcono1234/parameterized-type-improvements
Marcono1234 Jun 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions gson/src/main/java/com/google/gson/internal/$Gson$Types.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@
import java.util.HashMap;
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Properties;
import java.util.Objects;
import java.util.Properties;

/**
* Static methods for working with types.
Expand Down Expand Up @@ -138,9 +138,8 @@ public static Class<?> getRawType(Type type) {
} else if (type instanceof ParameterizedType) {
ParameterizedType parameterizedType = (ParameterizedType) type;

// I'm not exactly sure why getRawType() returns Type instead of Class.
// Neal isn't either but suspects some pathological case related
// to nested classes exists.
// getRawType() returns Type instead of Class; that seems to be an API mistake,
// see https://bugs.openjdk.org/browse/JDK-8250659
Type rawType = parameterizedType.getRawType();
checkArgument(rawType instanceof Class);
return (Class<?>) rawType;
Expand Down Expand Up @@ -481,19 +480,33 @@ static void checkNotPrimitive(Type type) {
checkArgument(!(type instanceof Class<?>) || !((Class<?>) type).isPrimitive());
}

/**
* Whether an {@linkplain ParameterizedType#getOwnerType() owner type} must be specified when
* constructing a {@link ParameterizedType} for {@code rawType}.
*
* <p>Note that this method might not require an owner type for all cases where Java reflection
* would create parameterized types with owner type.
*/
public static boolean requiresOwnerType(Type rawType) {
if (rawType instanceof Class<?>) {
Class<?> rawTypeAsClass = (Class<?>) rawType;
return !Modifier.isStatic(rawTypeAsClass.getModifiers())
&& rawTypeAsClass.getDeclaringClass() != null;
}
return false;
}

private static final class ParameterizedTypeImpl implements ParameterizedType, Serializable {
private final Type ownerType;
private final Type rawType;
private final Type[] typeArguments;

public ParameterizedTypeImpl(Type ownerType, Type rawType, Type... typeArguments) {
// TODO: Should this enforce that rawType is a Class? See JDK implementation of
// the ParameterizedType interface and https://bugs.openjdk.org/browse/JDK-8250659
requireNonNull(rawType);
// require an owner type if the raw type needs it
if (rawType instanceof Class<?>) {
Class<?> rawTypeAsClass = (Class<?>) rawType;
boolean isStaticOrTopLevelClass = Modifier.isStatic(rawTypeAsClass.getModifiers())
|| rawTypeAsClass.getEnclosingClass() == null;
checkArgument(ownerType != null || isStaticOrTopLevelClass);
if (ownerType == null && requiresOwnerType(rawType)) {
throw new IllegalArgumentException("Must specify owner type for " + rawType);
}

this.ownerType = ownerType == null ? null : canonicalize(ownerType);
Expand Down
26 changes: 19 additions & 7 deletions gson/src/main/java/com/google/gson/reflect/TypeToken.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ private Type getTypeTokenTypeArgument() {
}
// Check for raw TypeToken as superclass
else if (superclass == TypeToken.class) {
throw new IllegalStateException("TypeToken must be created with a type argument: new TypeToken<...>() {}; "
+ "When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved.");
throw new IllegalStateException("TypeToken must be created with a type argument: new TypeToken<...>() {};"
+ " When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved.");
}

// User created subclass of subclass of TypeToken
Expand Down Expand Up @@ -335,8 +335,8 @@ public static <T> TypeToken<T> get(Class<T> type) {
* and care must be taken to pass in the correct number of type arguments.
*
* @throws IllegalArgumentException
* If {@code rawType} is not of type {@code Class}, or if the type arguments are invalid for
* the raw type
* If {@code rawType} is not of type {@code Class}, if it is not a generic type, or if the
* type arguments are invalid for the raw type
*/
public static TypeToken<?> getParameterized(Type rawType, Type... typeArguments) {
Objects.requireNonNull(rawType);
Expand All @@ -351,6 +351,18 @@ public static TypeToken<?> getParameterized(Type rawType, Type... typeArguments)
Class<?> rawClass = (Class<?>) rawType;
TypeVariable<?>[] typeVariables = rawClass.getTypeParameters();

// Note: Does not check if owner type of rawType is generic because this factory method
// does not support specifying owner type
if (typeVariables.length == 0) {
Copy link
Member

@eamonnmcmanus eamonnmcmanus Jun 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I ran #2376 against all of Google's internal tests and found one failure where someone was calling this method with a non-generic type and an empty typeArguments array. Of course then it is just the same as calling get(NonGeneric.class), but I think this case should continue to work. It's something of a historical quirk that the reflection API represents classes with no type parameters so differently from classes with some.

There were no other failures, so I think the other changes are probably safe.

This change would imply updating the @throws text.

Copy link
Collaborator Author

@Marcono1234 Marcono1234 Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for testing this!

found one failure where someone was calling this method with a non-generic type and an empty typeArguments array

Was this actually a bug in the code or is there some reason why the code was doing this? The only situation I can imagine is if the typeArguments is created dynamically and the direct caller of getParameterized does not know whether the raw class it provides is generic; but even then I think it is a bit questionable requesting creation of a ParameterizedType when it is not actually known (and not verified) that the given raw type is generic.
Are you able to share a bit more information about the code where this failed, maybe in some redacted or simplified way?

Of course then it is just the same as calling get(NonGeneric.class), but I think this case should continue to work.

Except that getParameterized actually gives you a TypeToken(ParameterizedType) where the ParameterizedType is bogus, instead of a TypeToken(Class). So I am really not sure if we should continue to support this.

We could change TypeToken.getParameterized to return a TypeToken(Class) in that case, but that could on the other hand also cause backward compatibility issues; though supporting TypeToken.getParameterized to create bogus TypeToken(ParameterizedType) is probably also not something we want to support.

Copy link
Member

Choose a reason for hiding this comment

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

The code I was talking about is in a test, and looks something like this:

    Type type = TypeToken.getParameterized(FooResponse.class).getType();
    return new GsonBuilder()
        .registerTypeAdapterFactory(AutoValueAdapterFactory.create())
        .registerTypeAdapter(ImmutableList.class, new ImmutableListDeserializer())
        .create()
        .fromJson(json, type);

I had not realized that TypeToken.getParameterized returns a ParameterizedType with no type arguments in this case. That is indeed a bizarre sort of beast, and I can't help feeling that this code works mostly by accident. Since it's test code, rather than production, and apparently nothing else in all of Google's code is doing this, I think it's probably OK to introduce this exception. We should make sure to mention it in the next release notes, though.

Copy link
Member

Choose a reason for hiding this comment

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

For the record, I did find another instance, in non-test code this time. I think the code in question happened to be broken for unrelated reasons when I did my earlier test, which is why I didn't see the failure then. I think it might be worth catching this case and returning TypeToken.get(rawType), assuming the type-argument list is also empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, have created #2447 for that. I hope that is what you had in mind.

throw new IllegalArgumentException(rawClass.getName() + " is not a generic type");
}

// Check for this here to avoid misleading exception thrown by ParameterizedTypeImpl
if ($Gson$Types.requiresOwnerType(rawType)) {
throw new IllegalArgumentException("Raw type " + rawClass.getName() + " is not supported because"
+ " it requires specifying an owner type");
}

int expectedArgsCount = typeVariables.length;
int actualArgsCount = typeArguments.length;
if (actualArgsCount != expectedArgsCount) {
Expand All @@ -359,16 +371,16 @@ public static TypeToken<?> getParameterized(Type rawType, Type... typeArguments)
}

for (int i = 0; i < expectedArgsCount; i++) {
Type typeArgument = typeArguments[i];
Type typeArgument = Objects.requireNonNull(typeArguments[i], "Type argument must not be null");
Class<?> rawTypeArgument = $Gson$Types.getRawType(typeArgument);
TypeVariable<?> typeVariable = typeVariables[i];

for (Type bound : typeVariable.getBounds()) {
Class<?> rawBound = $Gson$Types.getRawType(bound);

if (!rawBound.isAssignableFrom(rawTypeArgument)) {
throw new IllegalArgumentException("Type argument " + typeArgument + " does not satisfy bounds "
+ "for type variable " + typeVariable + " declared by " + rawType);
throw new IllegalArgumentException("Type argument " + typeArgument + " does not satisfy bounds"
+ " for type variable " + typeVariable + " declared by " + rawType);
}
}
}
Expand Down
32 changes: 24 additions & 8 deletions gson/src/test/java/com/google/gson/internal/GsonTypesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
package com.google.gson.internal;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.fail;
import static org.junit.Assert.assertThrows;

import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
Expand All @@ -29,20 +29,33 @@ public final class GsonTypesTest {
@Test
public void testNewParameterizedTypeWithoutOwner() throws Exception {
// List<A>. List is a top-level class
Type type = $Gson$Types.newParameterizedTypeWithOwner(null, List.class, A.class);
assertThat(getFirstTypeArgument(type)).isEqualTo(A.class);
ParameterizedType type = $Gson$Types.newParameterizedTypeWithOwner(null, List.class, A.class);
assertThat(type.getOwnerType()).isNull();
assertThat(type.getRawType()).isEqualTo(List.class);
assertThat(type.getActualTypeArguments()).asList().containsExactly(A.class);

// A<B>. A is a static inner class.
type = $Gson$Types.newParameterizedTypeWithOwner(null, A.class, B.class);
assertThat(getFirstTypeArgument(type)).isEqualTo(B.class);

IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
// NonStaticInner<A> is not allowed without owner
() -> $Gson$Types.newParameterizedTypeWithOwner(null, NonStaticInner.class, A.class));
assertThat(e).hasMessageThat().isEqualTo("Must specify owner type for " + NonStaticInner.class);

type = $Gson$Types.newParameterizedTypeWithOwner(GsonTypesTest.class, NonStaticInner.class, A.class);
assertThat(type.getOwnerType()).isEqualTo(GsonTypesTest.class);
assertThat(type.getRawType()).isEqualTo(NonStaticInner.class);
assertThat(type.getActualTypeArguments()).asList().containsExactly(A.class);

final class D {
}
try {
// D<A> is not allowed since D is not a static inner class
$Gson$Types.newParameterizedTypeWithOwner(null, D.class, A.class);
fail();
} catch (IllegalArgumentException expected) {}

// D<A> is allowed since D has no owner type
type = $Gson$Types.newParameterizedTypeWithOwner(null, D.class, A.class);
assertThat(type.getOwnerType()).isNull();
assertThat(type.getRawType()).isEqualTo(D.class);
assertThat(type.getActualTypeArguments()).asList().containsExactly(A.class);

// A<D> is allowed.
type = $Gson$Types.newParameterizedTypeWithOwner(null, A.class, D.class);
Expand All @@ -63,6 +76,9 @@ private static final class B {
}
private static final class C {
}
@SuppressWarnings({"ClassCanBeStatic", "UnusedTypeParameter"})
private final class NonStaticInner<T> {
}

/**
* Given a parameterized type A&lt;B,C&gt;, returns B. If the specified type is not
Expand Down
161 changes: 67 additions & 94 deletions gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@
package com.google.gson.reflect;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.fail;
import static org.junit.Assert.assertThrows;

import java.lang.reflect.GenericArrayType;
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -103,11 +104,13 @@ public void testArrayFactory() {
Type listOfString = new TypeToken<List<String>>() {}.getType();
assertThat(TypeToken.getArray(listOfString)).isEqualTo(expectedListOfStringArray);

try {
TypeToken.getArray(null);
fail();
} catch (NullPointerException e) {
}
TypeToken<?> expectedIntArray = new TypeToken<int[]>() {};
assertThat(TypeToken.getArray(int.class)).isEqualTo(expectedIntArray);

assertThrows(NullPointerException.class, () -> TypeToken.getArray(null));
}

static class NestedGeneric<T> {
}

@Test
Expand All @@ -131,84 +134,70 @@ public void testParameterizedFactory() {

TypeToken<?> expectedSatisfyingTwoBounds = new TypeToken<GenericWithMultiBound<ClassSatisfyingBounds>>() {};
assertThat(TypeToken.getParameterized(GenericWithMultiBound.class, ClassSatisfyingBounds.class)).isEqualTo(expectedSatisfyingTwoBounds);

TypeToken<?> nestedTypeToken = TypeToken.getParameterized(NestedGeneric.class, Integer.class);
ParameterizedType nestedParameterizedType = (ParameterizedType) nestedTypeToken.getType();
// TODO: This seems to differ from how Java reflection behaves; when using TypeToken<NestedGeneric<Integer>>,
// then NestedGeneric<Integer> does have an owner type
assertThat(nestedParameterizedType.getOwnerType()).isNull();
assertThat(nestedParameterizedType.getRawType()).isEqualTo(NestedGeneric.class);
assertThat(nestedParameterizedType.getActualTypeArguments()).asList().containsExactly(Integer.class);

class LocalGenericClass<T> {}
TypeToken<?> expectedLocalType = new TypeToken<LocalGenericClass<Integer>>() {};
assertThat(TypeToken.getParameterized(LocalGenericClass.class, Integer.class)).isEqualTo(expectedLocalType);
}

@Test
public void testParameterizedFactory_Invalid() {
try {
TypeToken.getParameterized(null, new Type[0]);
fail();
} catch (NullPointerException e) {
}
assertThrows(NullPointerException.class, () -> TypeToken.getParameterized(null, new Type[0]));
assertThrows(NullPointerException.class, () -> TypeToken.getParameterized(List.class, new Type[] { null }));

GenericArrayType arrayType = (GenericArrayType) TypeToken.getArray(String.class).getType();
try {
TypeToken.getParameterized(arrayType, new Type[0]);
fail();
} catch (IllegalArgumentException e) {
assertThat(e).hasMessageThat().isEqualTo("rawType must be of type Class, but was java.lang.String[]");
}
IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(arrayType, new Type[0]));
assertThat(e).hasMessageThat().isEqualTo("rawType must be of type Class, but was java.lang.String[]");

try {
TypeToken.getParameterized(String.class, String.class);
fail();
} catch (IllegalArgumentException e) {
assertThat(e).hasMessageThat().isEqualTo("java.lang.String requires 0 type arguments, but got 1");
}
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(String.class, Number.class));
assertThat(e).hasMessageThat().isEqualTo("java.lang.String is not a generic type");

try {
TypeToken.getParameterized(List.class, new Type[0]);
fail();
} catch (IllegalArgumentException e) {
assertThat(e).hasMessageThat().isEqualTo("java.util.List requires 1 type arguments, but got 0");
}
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(List.class, new Type[0]));
assertThat(e).hasMessageThat().isEqualTo("java.util.List requires 1 type arguments, but got 0");

try {
TypeToken.getParameterized(List.class, String.class, String.class);
fail();
} catch (IllegalArgumentException e) {
assertThat(e).hasMessageThat().isEqualTo("java.util.List requires 1 type arguments, but got 2");
}
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(List.class, String.class, String.class));
assertThat(e).hasMessageThat().isEqualTo("java.util.List requires 1 type arguments, but got 2");

try {
TypeToken.getParameterized(GenericWithBound.class, String.class);
fail();
} catch (IllegalArgumentException e) {
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.String does not satisfy bounds "
+ "for type variable T declared by " + GenericWithBound.class);
}
// Primitive types must not be used as type argument
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(List.class, int.class));
assertThat(e).hasMessageThat().isEqualTo("Type argument int does not satisfy bounds"
+ " for type variable E declared by " + List.class);

try {
TypeToken.getParameterized(GenericWithBound.class, Object.class);
fail();
} catch (IllegalArgumentException e) {
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds "
+ "for type variable T declared by " + GenericWithBound.class);
}
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithBound.class, String.class));
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.String does not satisfy bounds"
+ " for type variable T declared by " + GenericWithBound.class);

try {
TypeToken.getParameterized(GenericWithMultiBound.class, Number.class);
fail();
} catch (IllegalArgumentException e) {
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Number does not satisfy bounds "
+ "for type variable T declared by " + GenericWithMultiBound.class);
}
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithBound.class, Object.class));
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds"
+ " for type variable T declared by " + GenericWithBound.class);

try {
TypeToken.getParameterized(GenericWithMultiBound.class, CharSequence.class);
fail();
} catch (IllegalArgumentException e) {
assertThat(e).hasMessageThat().isEqualTo("Type argument interface java.lang.CharSequence does not satisfy bounds "
+ "for type variable T declared by " + GenericWithMultiBound.class);
}
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithMultiBound.class, Number.class));
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Number does not satisfy bounds"
+ " for type variable T declared by " + GenericWithMultiBound.class);

e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithMultiBound.class, CharSequence.class));
assertThat(e).hasMessageThat().isEqualTo("Type argument interface java.lang.CharSequence does not satisfy bounds"
+ " for type variable T declared by " + GenericWithMultiBound.class);

e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithMultiBound.class, Object.class));
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds"
+ " for type variable T declared by " + GenericWithMultiBound.class);

try {
TypeToken.getParameterized(GenericWithMultiBound.class, Object.class);
fail();
} catch (IllegalArgumentException e) {
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds "
+ "for type variable T declared by " + GenericWithMultiBound.class);
class Outer {
class NonStaticInner<T> {}
}

e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(Outer.NonStaticInner.class, Object.class));
assertThat(e).hasMessageThat().isEqualTo("Raw type " + Outer.NonStaticInner.class.getName()
+ " is not supported because it requires specifying an owner type");
}

private static class CustomTypeToken extends TypeToken<String> {
Expand All @@ -231,38 +220,22 @@ class SubTypeToken<T> extends TypeToken<String> {}
class SubSubTypeToken1<T> extends SubTypeToken<T> {}
class SubSubTypeToken2 extends SubTypeToken<Integer> {}

try {
new SubTypeToken<Integer>() {};
fail();
} catch (IllegalStateException expected) {
assertThat(expected).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
}
IllegalStateException e = assertThrows(IllegalStateException.class, () -> new SubTypeToken<Integer>() {});
assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");

try {
new SubSubTypeToken1<Integer>();
fail();
} catch (IllegalStateException expected) {
assertThat(expected).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
}
e = assertThrows(IllegalStateException.class, () -> new SubSubTypeToken1<Integer>());
assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");

try {
new SubSubTypeToken2();
fail();
} catch (IllegalStateException expected) {
assertThat(expected).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
}
e = assertThrows(IllegalStateException.class, () -> new SubSubTypeToken2());
assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
}

@SuppressWarnings("rawtypes")
@Test
public void testTypeTokenRaw() {
try {
new TypeToken() {};
fail();
} catch (IllegalStateException expected) {
assertThat(expected).hasMessageThat().isEqualTo("TypeToken must be created with a type argument: new TypeToken<...>() {}; "
+ "When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved.");
}
IllegalStateException e = assertThrows(IllegalStateException.class, () -> new TypeToken() {});
assertThat(e).hasMessageThat().isEqualTo("TypeToken must be created with a type argument: new TypeToken<...>() {};"
+ " When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved.");
}
}

Expand Down