Add ProGuard / R8 integration tests & add default ProGuard rules (goo…

…gle#2397)

* Add code shrinking tools integration test

* Keep no-args constructor of classes usable with JsonAdapter

* Add library ProGuard rules for Gson

They are automatically applied for all users of Gson, see
https://developer.android.com/build/shrink-code#configuration-files

* Skip japicmp-maven-plugin for shrinker-test

* Add more tests for JsonAdapter, add tests for generic classes

* Extend default constructor test

* Add Troubleshooting Guide entry for TypeToken

Troubleshooting.md

@@ -281,3 +281,49 @@ Class.forName(jsonString, false, getClass().getClassLoader()).asSubclass(MyBaseC
 ```
 
 This will not initialize arbitrary classes, and it will throw a `ClassCastException` if the loaded class is not the same as or a subclass of `MyBaseClass`.
+
+## <a id="type-token-raw"></a> `IllegalStateException`: 'TypeToken must be created with a type argument' <br> `RuntimeException`: 'Missing type parameter'
+
+**Symptom:** An `IllegalStateException` with the message 'TypeToken must be created with a type argument' is thrown.  
+For older Gson versions a `RuntimeException` with message 'Missing type parameter' is thrown.
+
+**Reason:**
+
+- You created a `TypeToken` without type argument, for example `new TypeToken() {}` (note the missing `<...>`). You always have to provide the type argument, for example like this: `new TypeToken<List<String>>() {}`. Normally the compiler will also emit a 'raw types' warning when you forget the `<...>`.
+- You are using a code shrinking tool such as ProGuard or R8 (Android app builds normally have this enabled by default) but have not configured it correctly for usage with Gson.
+
+**Solution:** When you are using a code shrinking tool such as ProGuard or R8 you have to adjust your configuration to include the following rules:
+
+```
+# Keep generic signatures; needed for correct type resolution
+-keepattributes Signature
+
+# Keep class TypeToken (respectively its generic signature)
+-keep class com.google.gson.reflect.TypeToken { *; }
+
+# Keep any (anonymous) classes extending TypeToken
+-keep class * extends com.google.gson.reflect.TypeToken
+```
+
+See also the [Android example](examples/android-proguard-example/README.md) for more information.
+
+Note: For newer Gson versions these rules might be applied automatically; make sure you are using the latest Gson version and the latest version of the code shrinking tool.
+
+## <a id="r8-abstract-class"></a> `JsonIOException`: 'Abstract classes can't be instantiated!' (R8)
+
+**Symptom:** A `JsonIOException` with the message 'Abstract classes can't be instantiated!' is thrown; the class mentioned in the exception message is not actually `abstract` in your source code, and you are using the code shrinking tool R8 (Android app builds normally have this configured by default).
+
+**Reason:** The code shrinking tool R8 performs optimizations where it removes the no-args constructor from a class and makes the class `abstract`. Due to this Gson cannot create an instance of the class.
+
+**Solution:** Make sure the class has a no-args constructor, then adjust your R8 configuration file to keep the constructor of the class. For example:
+
+```
+# Keep the no-args constructor of the deserialized class
+-keepclassmembers class com.example.MyClass {
+  <init>();
+}
+```
+
+For Android you can add this rule to the `proguard-rules.pro` file, see also the [Android documentation](https://developer.android.com/build/shrink-code#keep-code). In case the class name in the exception message is obfuscated, see the Android documentation about [retracing](https://developer.android.com/build/shrink-code#retracing).
+
+Note: If the class which you are trying to deserialize is actually abstract, then this exception is probably unrelated to R8 and you will have to implement a custom [`InstanceCreator`](https://javadoc.io/doc/com.google.code.gson/gson/latest/com.google.gson/com/google/gson/InstanceCreator.html) or [`TypeAdapter`](https://javadoc.io/doc/com.google.code.gson/gson/latest/com.google.gson/com/google/gson/TypeAdapter.html) which creates an instance of a non-abstract subclass of the class.

examples/android-proguard-example/README.md

@@ -6,7 +6,12 @@ or remove them if they appear to be unused. This can cause issues for Gson which
 access the fields of a class. It is necessary to configure ProGuard to make sure that Gson works correctly.
 
 Also have a look at the [ProGuard manual](https://www.guardsquare.com/manual/configuration/usage#keepoverview)
-for more details on how ProGuard can be configured.
+and the [ProGuard Gson examples](https://www.guardsquare.com/manual/configuration/examples#gson) for more
+details on how ProGuard can be configured.
 
 The R8 code shrinker uses the same rule format as ProGuard, but there are differences between these two
 tools. Have a look at R8's Compatibility FAQ, and especially at the [Gson section](https://r8.googlesource.com/r8/+/refs/heads/main/compatibility-faq.md#gson).
+
+Note that newer Gson versions apply some of the rules shown in `proguard.cfg` automatically by default,
+see the file [`gson/META-INF/proguard/gson.pro`](/gson/src/main/resources/META-INF/proguard/gson.pro) for
+the Gson version you are using.

extras/pom.xml

@@ -50,6 +50,7 @@
       <artifactId>jsr250-api</artifactId>
       <version>1.0</version>
     </dependency>
+
     <dependency>
       <groupId>junit</groupId>
       <artifactId>junit</artifactId>

gson/pom.xml

@@ -40,15 +40,16 @@
     <dependency>
       <groupId>junit</groupId>
       <artifactId>junit</artifactId>
+      <scope>test</scope>
     </dependency>
     <dependency>
       <groupId>org.hamcrest</groupId>
       <artifactId>hamcrest</artifactId>
+      <scope>test</scope>
     </dependency>
     <dependency>
       <groupId>com.google.truth</groupId>
       <artifactId>truth</artifactId>
-      <version>1.1.3</version>
       <scope>test</scope>
     </dependency>
     <dependency>
@@ -75,7 +76,7 @@
               <goal>filter-sources</goal>
             </goals>
             <configuration>
-              <sourceDirectory>${basedir}/src/main/java-templates</sourceDirectory>
+              <sourceDirectory>${project.basedir}/src/main/java-templates</sourceDirectory>
               <outputDirectory>${project.build.directory}/generated-sources/java-templates</outputDirectory>
             </configuration>
           </execution>
@@ -178,7 +179,7 @@
           <injar>test-classes-obfuscated-injar</injar>
           <outjar>test-classes-obfuscated-outjar</outjar>
           <inFilter>**/*.class</inFilter>
-          <proguardInclude>${basedir}/src/test/resources/testcases-proguard.conf</proguardInclude>
+          <proguardInclude>${project.basedir}/src/test/resources/testcases-proguard.conf</proguardInclude>
           <libs>
             <lib>${project.build.directory}/classes</lib>
             <lib>${java.home}/jmods/java.base.jmod</lib>

gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java

@@ -73,12 +73,20 @@ public ConstructorConstructor(Map<Type, InstanceCreator<?>> instanceCreators, bo
   static String checkInstantiable(Class<?> c) {
     int modifiers = c.getModifiers();
     if (Modifier.isInterface(modifiers)) {
-      return "Interfaces can't be instantiated! Register an InstanceCreator "
-          + "or a TypeAdapter for this type. Interface name: " + c.getName();
+      return "Interfaces can't be instantiated! Register an InstanceCreator"
+          + " or a TypeAdapter for this type. Interface name: " + c.getName();
     }
     if (Modifier.isAbstract(modifiers)) {
-      return "Abstract classes can't be instantiated! Register an InstanceCreator "
-          + "or a TypeAdapter for this type. Class name: " + c.getName();
+      // R8 performs aggressive optimizations where it removes the default constructor of a class
+      // and makes the class `abstract`; check for that here explicitly
+      if (c.getDeclaredConstructors().length == 0) {
+        return "Abstract classes can't be instantiated! Adjust the R8 configuration or register"
+            + " an InstanceCreator or a TypeAdapter for this type. Class name: " + c.getName()
+            + "\nSee " + TroubleshootingGuide.createUrl("r8-abstract-class");
+      }
+
+      return "Abstract classes can't be instantiated! Register an InstanceCreator"
+          + " or a TypeAdapter for this type. Class name: " + c.getName();
     }
     return null;
   }
@@ -147,9 +155,9 @@ public <T> ObjectConstructor<T> get(TypeToken<T> typeToken) {
       // finally try unsafe
       return newUnsafeAllocator(rawType);
     } else {
-      final String message = "Unable to create instance of " + rawType + "; ReflectionAccessFilter "
-          + "does not permit using reflection or Unsafe. Register an InstanceCreator or a TypeAdapter "
-          + "for this type or adjust the access filter to allow using reflection.";
+      final String message = "Unable to create instance of " + rawType + "; ReflectionAccessFilter"
+          + " does not permit using reflection or Unsafe. Register an InstanceCreator or a TypeAdapter"
+          + " for this type or adjust the access filter to allow using reflection.";
       return new ObjectConstructor<T>() {
         @Override public T construct() {
           throw new JsonIOException(message);
@@ -222,10 +230,10 @@ private static <T> ObjectConstructor<T> newDefaultConstructor(Class<? super T> r
         && (filterResult != FilterResult.BLOCK_ALL || Modifier.isPublic(constructor.getModifiers())));
 
     if (!canAccess) {
-      final String message = "Unable to invoke no-args constructor of " + rawType + "; "
-          + "constructor is not accessible and ReflectionAccessFilter does not permit making "
-          + "it accessible. Register an InstanceCreator or a TypeAdapter for this type, change "
-          + "the visibility of the constructor or adjust the access filter.";
+      final String message = "Unable to invoke no-args constructor of " + rawType + ";"
+          + " constructor is not accessible and ReflectionAccessFilter does not permit making"
+          + " it accessible. Register an InstanceCreator or a TypeAdapter for this type, change"
+          + " the visibility of the constructor or adjust the access filter.";
       return new ObjectConstructor<T>() {
         @Override public T construct() {
           throw new JsonIOException(message);
@@ -373,19 +381,29 @@ private <T> ObjectConstructor<T> newUnsafeAllocator(final Class<? super T> rawTy
             T newInstance = (T) UnsafeAllocator.INSTANCE.newInstance(rawType);
             return newInstance;
           } catch (Exception e) {
-            throw new RuntimeException(("Unable to create instance of " + rawType + ". "
-                + "Registering an InstanceCreator or a TypeAdapter for this type, or adding a no-args "
-                + "constructor may fix this problem."), e);
+            throw new RuntimeException(("Unable to create instance of " + rawType + "."
+                + " Registering an InstanceCreator or a TypeAdapter for this type, or adding a no-args"
+                + " constructor may fix this problem."), e);
           }
         }
       };
     } else {
-      final String exceptionMessage = "Unable to create instance of " + rawType + "; usage of JDK Unsafe "
-          + "is disabled. Registering an InstanceCreator or a TypeAdapter for this type, adding a no-args "
-          + "constructor, or enabling usage of JDK Unsafe may fix this problem.";
+      String exceptionMessage = "Unable to create instance of " + rawType + "; usage of JDK Unsafe"
+          + " is disabled. Registering an InstanceCreator or a TypeAdapter for this type, adding a no-args"
+          + " constructor, or enabling usage of JDK Unsafe may fix this problem.";
+
+      // Check if R8 removed all constructors
+      if (rawType.getDeclaredConstructors().length == 0) {
+        // R8 with Unsafe disabled might not be common enough to warrant a separate Troubleshooting Guide entry
+        exceptionMessage += " Or adjust your R8 configuration to keep the no-args constructor of the class.";
+      }
+
+      // Explicit final variable to allow usage in the anonymous class below
+      final String exceptionMessageF = exceptionMessage;
+
       return new ObjectConstructor<T>() {
         @Override public T construct() {
-          throw new JsonIOException(exceptionMessage);
+          throw new JsonIOException(exceptionMessageF);
         }
       };
     }

gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java

@@ -51,6 +51,8 @@ public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> targetType) {
 
   TypeAdapter<?> getTypeAdapter(ConstructorConstructor constructorConstructor, Gson gson,
       TypeToken<?> type, JsonAdapter annotation) {
+    // TODO: The exception messages created by ConstructorConstructor are currently written in the context of
+    // deserialization and for example suggest usage of TypeAdapter, which would not work for @JsonAdapter usage
     Object instance = constructorConstructor.get(TypeToken.get(annotation.value())).construct();
 
     TypeAdapter<?> typeAdapter;

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

@@ -21,6 +21,7 @@
 
 import com.google.gson.internal.$Gson$Types;
 import com.google.gson.internal.InvalidStateException;
+import com.google.gson.internal.TroubleshootingGuide;
 import java.lang.reflect.GenericArrayType;
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
@@ -101,8 +102,10 @@ 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."
+          + "\nSee " + TroubleshootingGuide.createUrl("type-token-raw")
+      );
     }
 
     // User created subclass of subclass of TypeToken

gson/src/main/resources/META-INF/proguard/gson.pro

@@ -0,0 +1,58 @@
+### Gson ProGuard and R8 rules which are relevant for all users
+### This file is automatically recognized by ProGuard and R8, see https://developer.android.com/build/shrink-code#configuration-files
+###
+### IMPORTANT:
+### - These rules are additive; don't include anything here which is not specific to Gson (such as completely
+###   disabling obfuscation for all classes); the user would be unable to disable that then
+### - These rules are not complete; users will most likely have to add additional rules for their specific
+###   classes, for example to disable obfuscation for certain fields or to keep no-args constructors
+###
+
+# Keep generic signatures; needed for correct type resolution
+-keepattributes Signature
+
+# Keep Gson annotations
+# Note: Cannot perform finer selection here to only cover Gson annotations, see also https://stackoverflow.com/q/47515093
+-keepattributes *Annotation*
+
+
+### The following rules are needed for R8 in "full mode" which only adheres to `-keepattribtues` if
+### the corresponding class or field is matches by a `-keep` rule as well, see
+### https://r8.googlesource.com/r8/+/refs/heads/master/compatibility-faq.md#r8-full-mode
+
+# Keep class TypeToken (respectively its generic signature)
+-keep class com.google.gson.reflect.TypeToken { *; }
+
+# Keep any (anonymous) classes extending TypeToken
+-keep class * extends com.google.gson.reflect.TypeToken
+
+# Keep classes with @JsonAdapter annotation
+-keep @com.google.gson.annotations.JsonAdapter class *
+
+# Keep fields with @SerializedName annotation, but allow obfuscation of their names
+-keepclassmembers,allowobfuscation class * {
+  @com.google.gson.annotations.SerializedName <fields>;
+}
+
+# Keep fields with any other Gson annotation
+-keepclassmembers class * {
+  @com.google.gson.annotations.Expose <fields>;
+  @com.google.gson.annotations.JsonAdapter <fields>;
+  @com.google.gson.annotations.Since <fields>;
+  @com.google.gson.annotations.Until <fields>;
+}
+
+# Keep no-args constructor of classes which can be used with @JsonAdapter
+# By default their no-args constructor is invoked to create an adapter instance
+-keep class * extends com.google.gson.TypeAdapter {
+  <init>();
+}
+-keep class * implements com.google.gson.TypeAdapterFactory {
+  <init>();
+}
+-keep class * implements com.google.gson.JsonSerializer {
+  <init>();
+}
+-keep class * implements com.google.gson.JsonDeserializer {
+  <init>();
+}

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

@@ -260,8 +260,10 @@ public void testTypeTokenRaw() {
       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.");
+      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."
+          + "\nSee https://github.com/google/gson/blob/master/Troubleshooting.md#type-token-raw"
+      );
     }
   }
 }

metrics/pom.xml

@@ -63,7 +63,6 @@
         <plugin>
           <groupId>com.github.siom79.japicmp</groupId>
           <artifactId>japicmp-maven-plugin</artifactId>
-          <version>0.17.1</version>
           <configuration>
             <!-- This module is not supposed to be consumed as library, so no need to check API -->
             <skip>true</skip>

pom.xml

@@ -32,6 +32,7 @@
 
   <modules>
     <module>gson</module>
+    <module>shrinker-test</module>
     <module>extras</module>
     <module>metrics</module>
     <module>proto</module>
@@ -83,7 +84,6 @@
         <groupId>junit</groupId>
         <artifactId>junit</artifactId>
         <version>4.13.2</version>
-        <scope>test</scope>
         <exclusions>
           <exclusion>
             <groupId>org.hamcrest</groupId>
@@ -95,7 +95,11 @@
         <groupId>org.hamcrest</groupId>
         <artifactId>hamcrest</artifactId>
         <version>2.2</version>
-        <scope>test</scope>
+      </dependency>
+      <dependency>
+        <groupId>com.google.truth</groupId>
+        <artifactId>truth</artifactId>
+        <version>1.1.3</version>
       </dependency>
     </dependencies>
   </dependencyManagement>

proto/pom.xml

@@ -61,12 +61,12 @@
     <dependency>
       <groupId>org.hamcrest</groupId>
       <artifactId>hamcrest</artifactId>
+      <scope>test</scope>
     </dependency>
 
     <dependency>
       <groupId>com.google.truth</groupId>
       <artifactId>truth</artifactId>
-      <version>1.1.3</version>
       <scope>test</scope>
     </dependency>
   </dependencies>

shrinker-test/README.md

@@ -0,0 +1,9 @@
+# shrinker-test
+
+This Maven module contains integration tests which check the behavior of Gson when used in combination with code shrinking and obfuscation tools, such as ProGuard or R8.
+
+The code which is shrunken is under `src/main/java`; it should not contain any important assertions in case the code shrinking tools affect these assertions in any way. The test code under `src/test/java` executes the shrunken and obfuscated JAR and verifies that it behaves as expected.
+
+The tests might be a bit brittle, especially the R8 test setup. Future ProGuard and R8 versions might cause the tests to behave differently. In case tests fail the ProGuard and R8 mapping files created in the `target` directory can help with debugging. If necessary rewrite tests or even remove them if they cannot be implemented anymore for newer ProGuard or R8 versions.
+
+**Important:** Because execution of the code shrinking tools is performed during the Maven build, trying to directly run the integration tests from the IDE might not work, or might use stale results if you changed the configuration in between. Run `mvn clean verify` before trying to run the integration tests from the IDE.