Update ProGuard default shrinking rules (#2448)

* Update ProGuard default shrinking rules to correctly deal with classes without a no-args constructor

* Update test after changing default shrinking rules

* Adjust shrinker tests

* Update rules

* Addressed review comments

* Addressed more review comments

* Addressed more review comments

---------

Co-authored-by: Marcono1234 <Marcono1234@users.noreply.github.com>

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

@@ -15,13 +15,13 @@
 # Note: Cannot perform finer selection here to only cover Gson annotations, see also https://stackoverflow.com/q/47515093
 -keepattributes RuntimeVisibleAnnotations,AnnotationDefault
 
-
 ### 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/main/compatibility-faq.md#r8-full-mode
 
-# Keep class TypeToken (respectively its generic signature)
--keep class com.google.gson.reflect.TypeToken { *; }
+# Keep class TypeToken (respectively its generic signature) if present
+-if class com.google.gson.reflect.TypeToken
+-keep,allowobfuscation class com.google.gson.reflect.TypeToken
 
 # Keep any (anonymous) classes extending TypeToken
 -keep,allowobfuscation class * extends com.google.gson.reflect.TypeToken
@@ -29,11 +29,6 @@
 # Keep classes with @JsonAdapter annotation
 -keep,allowobfuscation,allowoptimization @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
 # Also allow obfuscation, assuming that users will additionally use @SerializedName or
 # other means to preserve the field names
@@ -59,12 +54,19 @@
   <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
+# Keep fields annotated with @SerializedName for classes which are referenced.
+# If classes with fields annotated with @SerializedName have a no-args
+# constructor keep that as well. 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>();
+-keepclasseswithmembers,allowobfuscation class <1> {
   @com.google.gson.annotations.SerializedName <fields>;
 }
+-if class * {
+  @com.google.gson.annotations.SerializedName <fields>;
+}
+-keepclassmembers,allowobfuscation,allowoptimization class <1> {
+  <init>();
+}

shrinker-test/common.pro

@@ -0,0 +1,40 @@
+### Common rules for ProGuard and R8
+### Should only contains rules needed specifically for the integration test;
+### any general rules which are relevant for all users should not be here but in `META-INF/proguard` of Gson
+
+-allowaccessmodification
+
+# On Windows mixed case class names might cause problems
+-dontusemixedcaseclassnames
+
+# Ignore notes about duplicate JDK classes
+-dontnote module-info,jdk.internal.**
+
+
+# Keep test entrypoints
+-keep class com.example.Main {
+  public static void runTests(...);
+}
+-keep class com.example.NoSerializedNameMain {
+  public static java.lang.String runTest();
+  public static java.lang.String runTestNoJdkUnsafe();
+  public static java.lang.String runTestNoDefaultConstructor();
+}
+
+
+### Test data setup
+
+# Keep fields without annotations which should be preserved
+-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>;
+}

shrinker-test/proguard.pro

@@ -1,48 +1,17 @@
-### Common rules for ProGuard and R8
-### Should only contains rules needed specifically for the integration test;
-### any general rules which are relevant for all users should not be here but in `META-INF/proguard` of Gson
+# Include common rules
+-include common.pro
 
--allowaccessmodification
+### ProGuard specific rules
 
-# On Windows mixed case class names might cause problems
--dontusemixedcaseclassnames
-
-# Ignore notes about duplicate JDK classes
--dontnote module-info,jdk.internal.**
-
-
-# Keep test entrypoints
--keep class com.example.Main {
-  public static void runTests(...);
-}
--keep class com.example.DefaultConstructorMain {
-  public static java.lang.String runTest();
-  public static java.lang.String runTestNoJdkUnsafe();
-  public static java.lang.String runTestNoDefaultConstructor();
-}
-
-
-### Test data setup
-
-# Keep fields without annotations which should be preserved
--keepclassmembers class com.example.ClassWithNamedFields {
-  !transient <fields>;
-}
-
--keepclassmembernames class com.example.ClassWithExposeAnnotation {
+# Unlike R8, ProGuard does not perform aggressive optimization which makes classes abstract,
+# therefore for ProGuard can successfully perform deserialization, and for that need to
+# preserve the field names
+-keepclassmembernames class com.example.NoSerializedNameMain$TestClass {
   <fields>;
 }
--keepclassmembernames class com.example.ClassWithJsonAdapterAnnotation {
-  ** f;
-}
--keepclassmembernames class com.example.ClassWithVersionAnnotations {
+-keepclassmembernames class com.example.NoSerializedNameMain$TestClassNotAbstract {
   <fields>;
 }
-
-
--keepclassmembernames class com.example.DefaultConstructorMain$TestClass {
-  <fields>;
-}
--keepclassmembernames class com.example.DefaultConstructorMain$TestClassNotAbstract {
+-keepclassmembernames class com.example.NoSerializedNameMain$TestClassWithoutDefaultConstructor {
   <fields>;
 }

shrinker-test/r8.pro

@@ -1,5 +1,5 @@
-# Extend the ProGuard rules
--include proguard.pro
+# Include common rules
+-include common.pro
 
 ### 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
@@ -10,11 +10,11 @@
 -keep,allowshrinking,allowoptimization,allowobfuscation,allowaccessmodification class com.example.GenericClasses$GenericUsingGenericClass
 
 # 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
+-keep,allowshrinking,allowoptimization class com.example.NoSerializedNameMain$TestClass
+-keep,allowshrinking,allowoptimization class com.example.NoSerializedNameMain$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 {
+-keep class com.example.NoSerializedNameMain$TestClassNotAbstract {
   @com.google.gson.annotations.SerializedName <fields>;
 }
 

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

@@ -2,6 +2,10 @@ package com.example;
 
 import com.google.gson.annotations.SerializedName;
 
+/**
+ * Class with no-args default constructor and with field annotated with
+ * {@link SerializedName}.
+ */
 public class ClassWithDefaultConstructor {
   @SerializedName("myField")
   public int i;

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

@@ -0,0 +1,17 @@
+package com.example;
+
+import com.google.gson.annotations.SerializedName;
+
+/**
+ * Class without no-args default constructor, but with field annotated with
+ * {@link SerializedName}.
+ */
+public class ClassWithoutDefaultConstructor {
+  @SerializedName("myField")
+  public int i;
+
+  // Specify explicit constructor with args to remove implicit no-args default constructor
+  public ClassWithoutDefaultConstructor(int i) {
+    this.i = i;
+  }
+}

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

@@ -32,6 +32,7 @@ public class Main {
     testNamedFields(outputConsumer);
     testSerializedName(outputConsumer);
 
+    testNoDefaultConstructor(outputConsumer);
     testNoJdkUnsafe(outputConsumer);
 
     testEnum(outputConsumer);
@@ -89,6 +90,16 @@ public class Main {
     });
   }
 
+  private static void testNoDefaultConstructor(BiConsumer<String, String> outputConsumer) {
+    Gson gson = new GsonBuilder().setPrettyPrinting().create();
+    TestExecutor.run(outputConsumer, "Write: No default constructor", () -> toJson(gson, new ClassWithoutDefaultConstructor(2)));
+    // This most likely relies on JDK Unsafe (unless the shrinker rewrites the constructor in some way)
+    TestExecutor.run(outputConsumer, "Read: No default constructor", () -> {
+      ClassWithoutDefaultConstructor deserialized = fromJson(gson, "{\"myField\": 3}", ClassWithoutDefaultConstructor.class);
+      return Integer.toString(deserialized.i);
+    });
+  }
+
   private static void testNoJdkUnsafe(BiConsumer<String, String> outputConsumer) {
     Gson gson = new GsonBuilder().disableJdkUnsafe().create();
     TestExecutor.run(outputConsumer, "Read: No JDK Unsafe; initial constructor value", () -> {

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

@@ -4,32 +4,32 @@ import static com.example.TestExecutor.same;
 
 import com.google.gson.Gson;
 import com.google.gson.GsonBuilder;
-import com.google.gson.annotations.SerializedName;
 
-public class DefaultConstructorMain {
+/**
+ * Covers cases of classes which don't use {@code @SerializedName} on their fields, and are
+ * therefore not matched by the default {@code gson.pro} rules.
+ */
+public class NoSerializedNameMain {
   static class TestClass {
     public String s;
   }
 
-  // R8 rule for this class still removes no-args constructor, but doesn't make class abstract
+  // R8 test rule in r8.pro 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;
 
+    // Specify explicit constructor with args to remove implicit no-args default constructor
     public TestClassWithoutDefaultConstructor(String s) {
       this.s = s;
     }
   }
 
   /**
-   * Main entrypoint, called by {@code ShrinkingIT.testDefaultConstructor()}.
+   * Main entrypoint, called by {@code ShrinkingIT.testNoSerializedName_DefaultConstructor()}.
    */
   public static String runTest() {
     TestClass deserialized = new Gson().fromJson("{\"s\":\"value\"}", same(TestClass.class));
@@ -37,7 +37,7 @@ public class DefaultConstructorMain {
   }
 
   /**
-   * Main entrypoint, called by {@code ShrinkingIT.testDefaultConstructorNoJdkUnsafe()}.
+   * Main entrypoint, called by {@code ShrinkingIT.testNoSerializedName_DefaultConstructorNoJdkUnsafe()}.
    */
   public static String runTestNoJdkUnsafe() {
     Gson gson = new GsonBuilder().disableJdkUnsafe().create();
@@ -46,7 +46,7 @@ public class DefaultConstructorMain {
   }
 
   /**
-   * Main entrypoint, called by {@code ShrinkingIT.testNoDefaultConstructor()}.
+   * Main entrypoint, called by {@code ShrinkingIT.testNoSerializedName_NoDefaultConstructor()}.
    */
   public static String runTestNoDefaultConstructor() {
     TestClassWithoutDefaultConstructor deserialized = new Gson().fromJson("{\"s\":\"value\"}", same(TestClassWithoutDefaultConstructor.class));

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

@@ -127,6 +127,14 @@ public class ShrinkingIT {
         "Read: SerializedName",
         "3",
         "===",
+        "Write: No default constructor",
+        "{",
+        "  \"myField\": 2",
+        "}",
+        "===",
+        "Read: No default constructor",
+        "3",
+        "===",
         "Read: No JDK Unsafe; initial constructor value",
         "-3",
         "===",
@@ -181,8 +189,8 @@ public class ShrinkingIT {
   }
 
   @Test
-  public void testDefaultConstructor() throws Exception {
-    runTest("com.example.DefaultConstructorMain", c -> {
+  public void testNoSerializedName_DefaultConstructor() throws Exception {
+    runTest("com.example.NoSerializedNameMain", c -> {
       Method m = c.getMethod("runTest");
 
       if (jarToTest.equals(PROGUARD_RESULT_PATH)) {
@@ -193,7 +201,7 @@ public class ShrinkingIT {
         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$TestClass"
+            + " or a TypeAdapter for this type. Class name: com.example.NoSerializedNameMain$TestClass"
             + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#r8-abstract-class"
         );
       }
@@ -201,8 +209,8 @@ public class ShrinkingIT {
   }
 
   @Test
-  public void testDefaultConstructorNoJdkUnsafe() throws Exception {
-    runTest("com.example.DefaultConstructorMain", c -> {
+  public void testNoSerializedName_DefaultConstructorNoJdkUnsafe() throws Exception {
+    runTest("com.example.NoSerializedNameMain", c -> {
       Method m = c.getMethod("runTestNoJdkUnsafe");
 
       if (jarToTest.equals(PROGUARD_RESULT_PATH)) {
@@ -212,7 +220,7 @@ public class ShrinkingIT {
         // R8 performs more aggressive optimizations
         Exception e = assertThrows(InvocationTargetException.class, () -> m.invoke(null));
         assertThat(e).hasCauseThat().hasMessageThat().isEqualTo(
-            "Unable to create instance of class com.example.DefaultConstructorMain$TestClassNotAbstract;"
+            "Unable to create instance of class com.example.NoSerializedNameMain$TestClassNotAbstract;"
             + " 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. Or adjust"
             + " your R8 configuration to keep the no-args constructor of the class."
@@ -222,8 +230,8 @@ public class ShrinkingIT {
   }
 
   @Test
-  public void testNoDefaultConstructor() throws Exception {
-    runTest("com.example.DefaultConstructorMain", c -> {
+  public void testNoSerializedName_NoDefaultConstructor() throws Exception {
+    runTest("com.example.NoSerializedNameMain", c -> {
       Method m = c.getMethod("runTestNoDefaultConstructor");
 
       if (jarToTest.equals(PROGUARD_RESULT_PATH)) {
@@ -234,7 +242,7 @@ public class ShrinkingIT {
         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"
+            + " or a TypeAdapter for this type. Class name: com.example.NoSerializedNameMain$TestClassWithoutDefaultConstructor"
             + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#r8-abstract-class"
         );
       }