Adjust ProGuard default rules and shrinking tests (#2420)

* Adjust ProGuard default rules and shrinking tests

* Adjust comment

* Add shrinking test for class without no-args constructor; improve docs

* Improve Unsafe mention in Troubleshooting Guide

* Improve comment for `-if class *`

Troubleshooting.md

@@ -313,6 +313,8 @@ Note: For newer Gson versions these rules might be applied automatically; make s
 
 **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).
 
+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.
+
 **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:
@@ -324,6 +326,10 @@ Note: For newer Gson versions these rules might be applied automatically; make s
 }
 ```
 
+You can also use `<init>(...);` to keep all constructors of that class, but then you might actually rely on `sun.misc.Unsafe` on both JDK and Android to create classes without no-args constructor, see [`GsonBuilder.disableJdkUnsafe()`](https://javadoc.io/doc/com.google.code.gson/gson/latest/com.google.gson/com/google/gson/GsonBuilder.html#disableJdkUnsafe()) for more information.
+
 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.
+For Android you can alternatively use the [`@Keep` annotation](https://developer.android.com/studio/write/annotations#keep) on the class or constructor you want to keep. That might be easier than having to maintain a custom R8 configuration.
+
+Note that the latest Gson versions (> 2.10.1) specify a default R8 configuration. If your class is a top-level class or is `static`, has a no-args constructor and its fields are annotated with Gson's [`@SerializedName`](https://www.javadoc.io/doc/com.google.code.gson/gson/latest/com.google.gson/com/google/gson/annotations/SerializedName.html), you might not have to perform any additional R8 configuration.

examples/android-proguard-example/README.md

@@ -12,6 +12,9 @@ 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,
+Note that the latest Gson versions (> 2.10.1) 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.
+the Gson version you are using. In general if your classes are top-level classes or are `static`, have a no-args constructor and their fields are annotated with Gson's [`@SerializedName`](https://www.javadoc.io/doc/com.google.code.gson/gson/latest/com.google.gson/com/google/gson/annotations/SerializedName.html), you might not have to perform any additional ProGuard or R8 configuration.
+
+An alternative to writing custom keep rules for your classes in the ProGuard configuration can be to use
+Android's [`@Keep` annotation](https://developer.android.com/studio/write/annotations#keep).

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

@@ -76,14 +76,15 @@ public final class ConstructorConstructor {
     if (Modifier.isAbstract(modifiers)) {
       // 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();
+      /*
+       * Note: Ideally should only show this R8-specific message when it is clear that R8 was
+       * used (e.g. when `c.getDeclaredConstructors().length == 0`), but on Android where this
+       * issue with R8 occurs most, R8 seems to keep some constructors for some reason while
+       * still making the class abstract
+       */
+      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 null;
   }

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

@@ -13,7 +13,7 @@
 
 # Keep Gson annotations
 # Note: Cannot perform finer selection here to only cover Gson annotations, see also https://stackoverflow.com/q/47515093
--keepattributes *Annotation*
+-keepattributes RuntimeVisibleAnnotations,AnnotationDefault
 
 
 ### The following rules are needed for R8 in "full mode" which only adheres to `-keepattribtues` if
@@ -24,10 +24,10 @@
 -keep class com.google.gson.reflect.TypeToken { *; }
 
 # Keep any (anonymous) classes extending TypeToken
--keep class * extends com.google.gson.reflect.TypeToken
+-keep,allowobfuscation class * extends com.google.gson.reflect.TypeToken
 
 # Keep classes with @JsonAdapter annotation
--keep @com.google.gson.annotations.JsonAdapter class *
+-keep,allowobfuscation,allowoptimization @com.google.gson.annotations.JsonAdapter class *
 
 # Keep fields with @SerializedName annotation, but allow obfuscation of their names
 -keepclassmembers,allowobfuscation class * {
@@ -35,7 +35,9 @@
 }
 
 # Keep fields with any other Gson annotation
--keepclassmembers class * {
+# Also allow obfuscation, assuming that users will additionally use @SerializedName or
+# other means to preserve the field names
+-keepclassmembers,allowobfuscation class * {
   @com.google.gson.annotations.Expose <fields>;
   @com.google.gson.annotations.JsonAdapter <fields>;
   @com.google.gson.annotations.Since <fields>;
@@ -44,15 +46,25 @@
 
 # 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 {
+-keepclassmembers class * extends com.google.gson.TypeAdapter {
   <init>();
 }
--keep class * implements com.google.gson.TypeAdapterFactory {
+-keepclassmembers class * implements com.google.gson.TypeAdapterFactory {
   <init>();
 }
--keep class * implements com.google.gson.JsonSerializer {
+-keepclassmembers class * implements com.google.gson.JsonSerializer {
   <init>();
 }
--keep class * implements com.google.gson.JsonDeserializer {
+-keepclassmembers class * implements com.google.gson.JsonDeserializer {
   <init>();
 }
+
+# If a class is used in some way by the application, and has fields annotated with @SerializedName
+# and a no-args constructor, keep those fields and the constructor
+# Based on https://issuetracker.google.com/issues/150189783#comment11
+# See also https://github.com/google/gson/pull/2420#discussion_r1241813541 for a more detailed explanation
+-if class *
+-keepclasseswithmembers,allowobfuscation,allowoptimization class <1> {
+  <init>();
+  @com.google.gson.annotations.SerializedName <fields>;
+}

gson/src/test/java/com/google/gson/functional/Java17RecordTest.java

@@ -89,7 +89,7 @@ public final class Java17RecordTest {
 
     var exception = assertThrows(JsonIOException.class, () -> gson.getAdapter(LocalRecord.class));
     assertThat(exception).hasMessageThat()
-            .isEqualTo("@SerializedName on method '" + LocalRecord.class.getName() + "#i()' is not supported");
+        .isEqualTo("@SerializedName on method '" + LocalRecord.class.getName() + "#i()' is not supported");
   }
 
   @Test
@@ -154,7 +154,7 @@ public final class Java17RecordTest {
     // TODO: Adjust this once Gson throws more specific exception type
     catch (RuntimeException e) {
       assertThat(e).hasMessageThat()
-              .isEqualTo("Failed to invoke constructor '" + LocalRecord.class.getName() + "(String)' with args [value]");
+          .isEqualTo("Failed to invoke constructor '" + LocalRecord.class.getName() + "(String)' with args [value]");
       assertThat(e).hasCauseThat().isSameInstanceAs(LocalRecord.thrownException);
     }
   }
@@ -227,7 +227,7 @@ public final class Java17RecordTest {
     String s = "{'aString': 's', 'aByte': null, 'aShort': 0}";
     var e = assertThrows(JsonParseException.class, () -> gson.fromJson(s, RecordWithPrimitives.class));
     assertThat(e).hasMessageThat()
-            .isEqualTo("null is not allowed as value for record component 'aByte' of primitive type; at path $.aByte");
+        .isEqualTo("null is not allowed as value for record component 'aByte' of primitive type; at path $.aByte");
   }
 
   /**
@@ -384,8 +384,8 @@ public final class Java17RecordTest {
 
     var exception = assertThrows(JsonIOException.class, () -> gson.toJson(new Blocked(1)));
     assertThat(exception).hasMessageThat()
-            .isEqualTo("ReflectionAccessFilter does not permit using reflection for class " + Blocked.class.getName() +
-                ". Register a TypeAdapter for this type or adjust the access filter.");
+        .isEqualTo("ReflectionAccessFilter does not permit using reflection for class " + Blocked.class.getName() +
+            ". Register a TypeAdapter for this type or adjust the access filter.");
   }
 
   @Test
@@ -396,15 +396,15 @@ public final class Java17RecordTest {
 
     var exception = assertThrows(JsonIOException.class, () -> gson.toJson(new PrivateRecord(1)));
     assertThat(exception).hasMessageThat()
-            .isEqualTo("Constructor 'com.google.gson.functional.Java17RecordTest$PrivateRecord(int)' is not accessible and"
-                + " ReflectionAccessFilter does not permit making it accessible. Register a TypeAdapter for the declaring"
-                + " type, adjust the access filter or increase the visibility of the element and its declaring type.");
+        .isEqualTo("Constructor 'com.google.gson.functional.Java17RecordTest$PrivateRecord(int)' is not accessible and"
+            + " ReflectionAccessFilter does not permit making it accessible. Register a TypeAdapter for the declaring"
+            + " type, adjust the access filter or increase the visibility of the element and its declaring type.");
 
     exception = assertThrows(JsonIOException.class, () -> gson.fromJson("{}", PrivateRecord.class));
     assertThat(exception).hasMessageThat()
-            .isEqualTo("Constructor 'com.google.gson.functional.Java17RecordTest$PrivateRecord(int)' is not accessible and"
-                + " ReflectionAccessFilter does not permit making it accessible. Register a TypeAdapter for the declaring"
-                + " type, adjust the access filter or increase the visibility of the element and its declaring type.");
+        .isEqualTo("Constructor 'com.google.gson.functional.Java17RecordTest$PrivateRecord(int)' is not accessible and"
+            + " ReflectionAccessFilter does not permit making it accessible. Register a TypeAdapter for the declaring"
+            + " type, adjust the access filter or increase the visibility of the element and its declaring type.");
 
     assertThat(gson.toJson(new PublicRecord(1))).isEqualTo("{\"i\":1}");
     assertThat(gson.fromJson("{\"i\":2}", PublicRecord.class)).isEqualTo(new PublicRecord(2));
@@ -427,7 +427,8 @@ public final class Java17RecordTest {
 
     var exception = assertThrows(JsonIOException.class, () -> gson.fromJson("{}", Record.class));
     assertThat(exception).hasMessageThat()
-            .isEqualTo("Abstract classes can't be instantiated! Register an InstanceCreator or a TypeAdapter for"
-                + " this type. Class name: java.lang.Record");
+        .isEqualTo("Abstract classes can't be instantiated! Adjust the R8 configuration or register an InstanceCreator"
+            + " or a TypeAdapter for this type. Class name: java.lang.Record"
+            + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#r8-abstract-class");
   }
 }

gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java

@@ -19,17 +19,14 @@ package com.google.gson.internal;
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.fail;
 
-import com.google.gson.InstanceCreator;
-import com.google.gson.ReflectionAccessFilter;
 import com.google.gson.reflect.TypeToken;
-import java.lang.reflect.Type;
 import java.util.Collections;
 import org.junit.Test;
 
 public class ConstructorConstructorTest {
   private ConstructorConstructor constructorConstructor = new ConstructorConstructor(
-      Collections.<Type, InstanceCreator<?>>emptyMap(), true,
-      Collections.<ReflectionAccessFilter>emptyList()
+      Collections.emptyMap(), true,
+      Collections.emptyList()
   );
 
   private abstract static class AbstractClass {
@@ -39,7 +36,7 @@ public class ConstructorConstructorTest {
   private interface Interface { }
 
   /**
-   * Verify that ConstructorConstructor does not try to invoke no-arg constructor
+   * Verify that ConstructorConstructor does not try to invoke no-args constructor
    * of abstract class.
    */
   @Test
@@ -49,9 +46,10 @@ public class ConstructorConstructorTest {
       constructor.construct();
       fail("Expected exception");
     } catch (RuntimeException exception) {
-      assertThat(exception).hasMessageThat().isEqualTo("Abstract classes can't be instantiated! "
-          + "Register an InstanceCreator or a TypeAdapter for this type. "
-          + "Class name: com.google.gson.internal.ConstructorConstructorTest$AbstractClass");
+      assertThat(exception).hasMessageThat().isEqualTo("Abstract classes can't be instantiated!"
+          + " Adjust the R8 configuration or register an InstanceCreator or a TypeAdapter for this type."
+          + " Class name: com.google.gson.internal.ConstructorConstructorTest$AbstractClass"
+          + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#r8-abstract-class");
     }
   }
 
@@ -62,9 +60,9 @@ public class ConstructorConstructorTest {
       constructor.construct();
       fail("Expected exception");
     } catch (RuntimeException exception) {
-      assertThat(exception).hasMessageThat().isEqualTo("Interfaces can't be instantiated! "
-          + "Register an InstanceCreator or a TypeAdapter for this type. "
-          + "Interface name: com.google.gson.internal.ConstructorConstructorTest$Interface");
+      assertThat(exception).hasMessageThat().isEqualTo("Interfaces can't be instantiated!"
+          + " Register an InstanceCreator or a TypeAdapter for this type."
+          + " Interface name: com.google.gson.internal.ConstructorConstructorTest$Interface");
     }
   }
 }

shrinker-test/pom.xml

@@ -199,6 +199,8 @@
         <dependencies>
           <dependency>
             <!-- R8 dependency used above -->
+            <!-- Note: For some reason Maven shows the warning "Missing POM for com.android.tools:r8:jar",
+              but it appears that can be ignored -->
             <groupId>com.android.tools</groupId>
             <artifactId>r8</artifactId>
             <version>8.0.40</version>

shrinker-test/proguard.pro

@@ -18,6 +18,7 @@
 -keep class com.example.DefaultConstructorMain {
   public static java.lang.String runTest();
   public static java.lang.String runTestNoJdkUnsafe();
+  public static java.lang.String runTestNoDefaultConstructor();
 }
 
 
@@ -27,3 +28,21 @@
 -keepclassmembers class com.example.ClassWithNamedFields {
   !transient <fields>;
 }
+
+-keepclassmembernames class com.example.ClassWithExposeAnnotation {
+  <fields>;
+}
+-keepclassmembernames class com.example.ClassWithJsonAdapterAnnotation {
+  ** f;
+}
+-keepclassmembernames class com.example.ClassWithVersionAnnotations {
+  <fields>;
+}
+
+
+-keepclassmembernames class com.example.DefaultConstructorMain$TestClass {
+  <fields>;
+}
+-keepclassmembernames class com.example.DefaultConstructorMain$TestClassNotAbstract {
+  <fields>;
+}

shrinker-test/r8.pro

@@ -4,20 +4,6 @@
 ### The following rules are needed for R8 in "full mode", which performs more aggressive optimizations than ProGuard
 ### See https://r8.googlesource.com/r8/+/refs/heads/main/compatibility-faq.md#r8-full-mode
 
-# Keep the no-args constructor of deserialized classes
--keepclassmembers class com.example.ClassWithDefaultConstructor {
-  <init>();
-}
--keepclassmembers class com.example.GenericClasses$GenericClass {
-  <init>();
-}
--keepclassmembers class com.example.GenericClasses$UsingGenericClass {
-  <init>();
-}
--keepclassmembers class com.example.GenericClasses$GenericUsingGenericClass {
-  <init>();
-}
-
 # For classes with generic type parameter R8 in "full mode" requires to have a keep rule to
 # preserve the generic signature
 -keep,allowshrinking,allowoptimization,allowobfuscation,allowaccessmodification class com.example.GenericClasses$GenericClass
@@ -25,12 +11,14 @@
 
 # Don't obfuscate class name, to check it in exception message
 -keep,allowshrinking,allowoptimization class com.example.DefaultConstructorMain$TestClass
+-keep,allowshrinking,allowoptimization class com.example.DefaultConstructorMain$TestClassWithoutDefaultConstructor
+
 # This rule has the side-effect that R8 still removes the no-args constructor, but does not make the class abstract
 -keep class com.example.DefaultConstructorMain$TestClassNotAbstract {
   @com.google.gson.annotations.SerializedName <fields>;
 }
 
 # Keep enum constants which are not explicitly used in code
--keep class com.example.EnumClass {
+-keepclassmembers class com.example.EnumClass {
   ** SECOND;
 }

shrinker-test/src/main/java/com/example/ClassWithJsonAdapterAnnotation.java

@@ -23,6 +23,7 @@ import java.lang.reflect.Type;
  */
 public class ClassWithJsonAdapterAnnotation {
   // For this field don't use @SerializedName and ignore it for deserialization
+  // Has custom ProGuard rule to keep the field name
   @JsonAdapter(value = Adapter.class, nullSafe = false)
   DummyClass f;
 

shrinker-test/src/main/java/com/example/DefaultConstructorMain.java

@@ -8,14 +8,24 @@ import com.google.gson.annotations.SerializedName;
 
 public class DefaultConstructorMain {
   static class TestClass {
-    @SerializedName("s")
     public String s;
   }
 
   // R8 rule for this class still removes no-args constructor, but doesn't make class abstract
   static class TestClassNotAbstract {
+    public String s;
+  }
+
+  // Current Gson ProGuard rules only keep default constructor (and only then prevent R8 from
+  // making class abstract); other constructors are ignored to suggest to user adding default
+  // constructor instead of implicitly relying on JDK Unsafe
+  static class TestClassWithoutDefaultConstructor {
     @SerializedName("s")
     public String s;
+
+    public TestClassWithoutDefaultConstructor(String s) {
+      this.s = s;
+    }
   }
 
   /**
@@ -34,4 +44,12 @@ public class DefaultConstructorMain {
     TestClassNotAbstract deserialized = gson.fromJson("{\"s\": \"value\"}", same(TestClassNotAbstract.class));
     return deserialized.s;
   }
+
+  /**
+   * Main entrypoint, called by {@code ShrinkingIT.testNoDefaultConstructor()}.
+   */
+  public static String runTestNoDefaultConstructor() {
+    TestClassWithoutDefaultConstructor deserialized = new Gson().fromJson("{\"s\":\"value\"}", same(TestClassWithoutDefaultConstructor.class));
+    return deserialized.s;
+  }
 }

shrinker-test/src/test/java/com/google/gson/it/ShrinkingIT.java

@@ -220,4 +220,24 @@ public class ShrinkingIT {
       }
     });
   }
+
+  @Test
+  public void testNoDefaultConstructor() throws Exception {
+    runTest("com.example.DefaultConstructorMain", c -> {
+      Method m = c.getMethod("runTestNoDefaultConstructor");
+
+      if (jarToTest.equals(PROGUARD_RESULT_PATH)) {
+        Object result = m.invoke(null);
+        assertThat(result).isEqualTo("value");
+      } else {
+        // R8 performs more aggressive optimizations
+        Exception e = assertThrows(InvocationTargetException.class, () -> m.invoke(null));
+        assertThat(e).hasCauseThat().hasMessageThat().isEqualTo(
+            "Abstract classes can't be instantiated! Adjust the R8 configuration or register an InstanceCreator"
+            + " or a TypeAdapter for this type. Class name: com.example.DefaultConstructorMain$TestClassWithoutDefaultConstructor"
+            + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#r8-abstract-class"
+        );
+      }
+    });
+  }
 }