Skip to content

Commit a589ef2

Browse files
Marcono1234Marcono1234
authored
Improve creation of ParameterizedType (#2375)
- Reject non-generic raw types for TypeToken.getParameterized - Fix ParameterizedTypeImpl erroneously requiring owner type for types without owner
1 parent 9cf0f8d commit a589ef2

File tree

4 files changed

+132
-119
lines changed

4 files changed

+132
-119
lines changed

‎gson/src/main/java/com/google/gson/internal/$Gson$Types.java‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@
3333
import java.util.HashMap;
3434
import java.util.Map;
3535
import java.util.NoSuchElementException;
36-
import java.util.Properties;
3736
import java.util.Objects;
37+
import java.util.Properties;
3838

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

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

483+
/**
484+
* Whether an {@linkplain ParameterizedType#getOwnerType() owner type} must be specified when
485+
* constructing a {@link ParameterizedType} for {@code rawType}.
486+
*
487+
* <p>Note that this method might not require an owner type for all cases where Java reflection
488+
* would create parameterized types with owner type.
489+
*/
490+
public static boolean requiresOwnerType(Type rawType) {
491+
if (rawType instanceof Class<?>) {
492+
Class<?> rawTypeAsClass = (Class<?>) rawType;
493+
return !Modifier.isStatic(rawTypeAsClass.getModifiers())
494+
&& rawTypeAsClass.getDeclaringClass() != null;
495+
}
496+
return false;
497+
}
498+
484499
private static final class ParameterizedTypeImpl implements ParameterizedType, Serializable {
485500
private final Type ownerType;
486501
private final Type rawType;
487502
private final Type[] typeArguments;
488503

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

499512
this.ownerType = ownerType == null ? null : canonicalize(ownerType);

‎gson/src/main/java/com/google/gson/reflect/TypeToken.java‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -338,8 +338,8 @@ public static <T> TypeToken<T> get(Class<T> type) {
338338
* and care must be taken to pass in the correct number of type arguments.
339339
*
340340
* @throws IllegalArgumentException
341-
* If {@code rawType} is not of type {@code Class}, or if the type arguments are invalid for
342-
* the raw type
341+
* If {@code rawType} is not of type {@code Class}, if it is not a generic type, or if the
342+
* type arguments are invalid for the raw type
343343
*/
344344
public static TypeToken<?> getParameterized(Type rawType, Type... typeArguments) {
345345
Objects.requireNonNull(rawType);
@@ -354,6 +354,18 @@ public static TypeToken<?> getParameterized(Type rawType, Type... typeArguments)
354354
Class<?> rawClass = (Class<?>) rawType;
355355
TypeVariable<?>[] typeVariables = rawClass.getTypeParameters();
356356

357+
// Note: Does not check if owner type of rawType is generic because this factory method
358+
// does not support specifying owner type
359+
if (typeVariables.length == 0) {
360+
throw new IllegalArgumentException(rawClass.getName() + " is not a generic type");
361+
}
362+
363+
// Check for this here to avoid misleading exception thrown by ParameterizedTypeImpl
364+
if ($Gson$Types.requiresOwnerType(rawType)) {
365+
throw new IllegalArgumentException("Raw type " + rawClass.getName() + " is not supported because"
366+
+ " it requires specifying an owner type");
367+
}
368+
357369
int expectedArgsCount = typeVariables.length;
358370
int actualArgsCount = typeArguments.length;
359371
if (actualArgsCount != expectedArgsCount) {
@@ -362,16 +374,16 @@ public static TypeToken<?> getParameterized(Type rawType, Type... typeArguments)
362374
}
363375

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

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

372384
if (!rawBound.isAssignableFrom(rawTypeArgument)) {
373-
throw new IllegalArgumentException("Type argument " + typeArgument + " does not satisfy bounds "
374-
+ "for type variable " + typeVariable + " declared by " + rawType);
385+
throw new IllegalArgumentException("Type argument " + typeArgument + " does not satisfy bounds"
386+
+ " for type variable " + typeVariable + " declared by " + rawType);
375387
}
376388
}
377389
}

‎gson/src/test/java/com/google/gson/internal/GsonTypesTest.java‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
package com.google.gson.internal;
1818

1919
import static com.google.common.truth.Truth.assertThat;
20-
import static org.junit.Assert.fail;
20+
import static org.junit.Assert.assertThrows;
2121

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

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

41+
IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
42+
// NonStaticInner<A> is not allowed without owner
43+
() -> $Gson$Types.newParameterizedTypeWithOwner(null, NonStaticInner.class, A.class));
44+
assertThat(e).hasMessageThat().isEqualTo("Must specify owner type for " + NonStaticInner.class);
45+
46+
type = $Gson$Types.newParameterizedTypeWithOwner(GsonTypesTest.class, NonStaticInner.class, A.class);
47+
assertThat(type.getOwnerType()).isEqualTo(GsonTypesTest.class);
48+
assertThat(type.getRawType()).isEqualTo(NonStaticInner.class);
49+
assertThat(type.getActualTypeArguments()).asList().containsExactly(A.class);
50+
3951
final class D {
4052
}
41-
try {
42-
// D<A> is not allowed since D is not a static inner class
43-
$Gson$Types.newParameterizedTypeWithOwner(null, D.class, A.class);
44-
fail();
45-
} catch (IllegalArgumentException expected) {}
53+
54+
// D<A> is allowed since D has no owner type
55+
type = $Gson$Types.newParameterizedTypeWithOwner(null, D.class, A.class);
56+
assertThat(type.getOwnerType()).isNull();
57+
assertThat(type.getRawType()).isEqualTo(D.class);
58+
assertThat(type.getActualTypeArguments()).asList().containsExactly(A.class);
4659

4760
// A<D> is allowed.
4861
type = $Gson$Types.newParameterizedTypeWithOwner(null, A.class, D.class);
@@ -63,6 +76,9 @@ private static final class B {
6376
}
6477
private static final class C {
6578
}
79+
@SuppressWarnings({"ClassCanBeStatic", "UnusedTypeParameter"})
80+
private final class NonStaticInner<T> {
81+
}
6682

6783
/**
6884
* Given a parameterized type A&lt;B,C&gt;, returns B. If the specified type is not

‎gson/src/test/java/com/google/gson/reflect/TypeTokenTest.java‎

Lines changed: 68 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,10 @@
1717
package com.google.gson.reflect;
1818

1919
import static com.google.common.truth.Truth.assertThat;
20-
import static org.junit.Assert.fail;
20+
import static org.junit.Assert.assertThrows;
2121

2222
import java.lang.reflect.GenericArrayType;
23+
import java.lang.reflect.ParameterizedType;
2324
import java.lang.reflect.Type;
2425
import java.util.ArrayList;
2526
import java.util.List;
@@ -103,11 +104,13 @@ public void testArrayFactory() {
103104
Type listOfString = new TypeToken<List<String>>() {}.getType();
104105
assertThat(TypeToken.getArray(listOfString)).isEqualTo(expectedListOfStringArray);
105106

106-
try {
107-
TypeToken.getArray(null);
108-
fail();
109-
} catch (NullPointerException e) {
110-
}
107+
TypeToken<?> expectedIntArray = new TypeToken<int[]>() {};
108+
assertThat(TypeToken.getArray(int.class)).isEqualTo(expectedIntArray);
109+
110+
assertThrows(NullPointerException.class, () -> TypeToken.getArray(null));
111+
}
112+
113+
static class NestedGeneric<T> {
111114
}
112115

113116
@Test
@@ -131,84 +134,70 @@ public void testParameterizedFactory() {
131134

132135
TypeToken<?> expectedSatisfyingTwoBounds = new TypeToken<GenericWithMultiBound<ClassSatisfyingBounds>>() {};
133136
assertThat(TypeToken.getParameterized(GenericWithMultiBound.class, ClassSatisfyingBounds.class)).isEqualTo(expectedSatisfyingTwoBounds);
137+
138+
TypeToken<?> nestedTypeToken = TypeToken.getParameterized(NestedGeneric.class, Integer.class);
139+
ParameterizedType nestedParameterizedType = (ParameterizedType) nestedTypeToken.getType();
140+
// TODO: This seems to differ from how Java reflection behaves; when using TypeToken<NestedGeneric<Integer>>,
141+
// then NestedGeneric<Integer> does have an owner type
142+
assertThat(nestedParameterizedType.getOwnerType()).isNull();
143+
assertThat(nestedParameterizedType.getRawType()).isEqualTo(NestedGeneric.class);
144+
assertThat(nestedParameterizedType.getActualTypeArguments()).asList().containsExactly(Integer.class);
145+
146+
class LocalGenericClass<T> {}
147+
TypeToken<?> expectedLocalType = new TypeToken<LocalGenericClass<Integer>>() {};
148+
assertThat(TypeToken.getParameterized(LocalGenericClass.class, Integer.class)).isEqualTo(expectedLocalType);
134149
}
135150

136151
@Test
137152
public void testParameterizedFactory_Invalid() {
138-
try {
139-
TypeToken.getParameterized(null, new Type[0]);
140-
fail();
141-
} catch (NullPointerException e) {
142-
}
153+
assertThrows(NullPointerException.class, () -> TypeToken.getParameterized(null, new Type[0]));
154+
assertThrows(NullPointerException.class, () -> TypeToken.getParameterized(List.class, new Type[] { null }));
143155

144156
GenericArrayType arrayType = (GenericArrayType) TypeToken.getArray(String.class).getType();
145-
try {
146-
TypeToken.getParameterized(arrayType, new Type[0]);
147-
fail();
148-
} catch (IllegalArgumentException e) {
149-
assertThat(e).hasMessageThat().isEqualTo("rawType must be of type Class, but was java.lang.String[]");
150-
}
157+
IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(arrayType, new Type[0]));
158+
assertThat(e).hasMessageThat().isEqualTo("rawType must be of type Class, but was java.lang.String[]");
151159

152-
try {
153-
TypeToken.getParameterized(String.class, String.class);
154-
fail();
155-
} catch (IllegalArgumentException e) {
156-
assertThat(e).hasMessageThat().isEqualTo("java.lang.String requires 0 type arguments, but got 1");
157-
}
160+
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(String.class, Number.class));
161+
assertThat(e).hasMessageThat().isEqualTo("java.lang.String is not a generic type");
158162

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

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

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

181-
try {
182-
TypeToken.getParameterized(GenericWithBound.class, Object.class);
183-
fail();
184-
} catch (IllegalArgumentException e) {
185-
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds "
186-
+ "for type variable T declared by " + GenericWithBound.class);
187-
}
174+
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithBound.class, String.class));
175+
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.String does not satisfy bounds"
176+
+ " for type variable T declared by " + GenericWithBound.class);
188177

189-
try {
190-
TypeToken.getParameterized(GenericWithMultiBound.class, Number.class);
191-
fail();
192-
} catch (IllegalArgumentException e) {
193-
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Number does not satisfy bounds "
194-
+ "for type variable T declared by " + GenericWithMultiBound.class);
195-
}
178+
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithBound.class, Object.class));
179+
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds"
180+
+ " for type variable T declared by " + GenericWithBound.class);
196181

197-
try {
198-
TypeToken.getParameterized(GenericWithMultiBound.class, CharSequence.class);
199-
fail();
200-
} catch (IllegalArgumentException e) {
201-
assertThat(e).hasMessageThat().isEqualTo("Type argument interface java.lang.CharSequence does not satisfy bounds "
202-
+ "for type variable T declared by " + GenericWithMultiBound.class);
203-
}
182+
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithMultiBound.class, Number.class));
183+
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Number does not satisfy bounds"
184+
+ " for type variable T declared by " + GenericWithMultiBound.class);
185+
186+
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithMultiBound.class, CharSequence.class));
187+
assertThat(e).hasMessageThat().isEqualTo("Type argument interface java.lang.CharSequence does not satisfy bounds"
188+
+ " for type variable T declared by " + GenericWithMultiBound.class);
189+
190+
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(GenericWithMultiBound.class, Object.class));
191+
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds"
192+
+ " for type variable T declared by " + GenericWithMultiBound.class);
204193

205-
try {
206-
TypeToken.getParameterized(GenericWithMultiBound.class, Object.class);
207-
fail();
208-
} catch (IllegalArgumentException e) {
209-
assertThat(e).hasMessageThat().isEqualTo("Type argument class java.lang.Object does not satisfy bounds "
210-
+ "for type variable T declared by " + GenericWithMultiBound.class);
194+
class Outer {
195+
class NonStaticInner<T> {}
211196
}
197+
198+
e = assertThrows(IllegalArgumentException.class, () -> TypeToken.getParameterized(Outer.NonStaticInner.class, Object.class));
199+
assertThat(e).hasMessageThat().isEqualTo("Raw type " + Outer.NonStaticInner.class.getName()
200+
+ " is not supported because it requires specifying an owner type");
212201
}
213202

214203
private static class CustomTypeToken extends TypeToken<String> {
@@ -231,40 +220,23 @@ class SubTypeToken<T> extends TypeToken<String> {}
231220
class SubSubTypeToken1<T> extends SubTypeToken<T> {}
232221
class SubSubTypeToken2 extends SubTypeToken<Integer> {}
233222

234-
try {
235-
new SubTypeToken<Integer>() {};
236-
fail();
237-
} catch (IllegalStateException expected) {
238-
assertThat(expected).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
239-
}
223+
IllegalStateException e = assertThrows(IllegalStateException.class, () -> new SubTypeToken<Integer>() {});
224+
assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
240225

241-
try {
242-
new SubSubTypeToken1<Integer>();
243-
fail();
244-
} catch (IllegalStateException expected) {
245-
assertThat(expected).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
246-
}
226+
e = assertThrows(IllegalStateException.class, () -> new SubSubTypeToken1<Integer>());
227+
assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
247228

248-
try {
249-
new SubSubTypeToken2();
250-
fail();
251-
} catch (IllegalStateException expected) {
252-
assertThat(expected).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
253-
}
229+
e = assertThrows(IllegalStateException.class, () -> new SubSubTypeToken2());
230+
assertThat(e).hasMessageThat().isEqualTo("Must only create direct subclasses of TypeToken");
254231
}
255232

256233
@SuppressWarnings("rawtypes")
257234
@Test
258235
public void testTypeTokenRaw() {
259-
try {
260-
new TypeToken() {};
261-
fail();
262-
} catch (IllegalStateException expected) {
263-
assertThat(expected).hasMessageThat().isEqualTo("TypeToken must be created with a type argument: new TypeToken<...>() {};"
264-
+ " When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved."
265-
+ "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#type-token-raw"
266-
);
267-
}
236+
IllegalStateException e = assertThrows(IllegalStateException.class, () -> new TypeToken() {});
237+
assertThat(e).hasMessageThat().isEqualTo("TypeToken must be created with a type argument: new TypeToken<...>() {};"
238+
+ " When using code shrinkers (ProGuard, R8, ...) make sure that generic signatures are preserved."
239+
+ "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#type-token-raw");
268240
}
269241
}
270242

0 commit comments

Comments
 (0)