From 7b5629bcfca156051253db7d6f6510db5862a967 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Gjesse?= Date: Tue, 22 Aug 2023 17:40:31 +0200 Subject: [PATCH] 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 --- .../main/resources/META-INF/proguard/gson.pro | 30 ++++++------ shrinker-test/common.pro | 40 +++++++++++++++ shrinker-test/proguard.pro | 49 ++++--------------- shrinker-test/r8.pro | 10 ++-- .../example/ClassWithDefaultConstructor.java | 4 ++ .../ClassWithoutDefaultConstructor.java | 17 +++++++ .../src/main/java/com/example/Main.java | 11 +++++ ...torMain.java => NoSerializedNameMain.java} | 20 ++++---- .../java/com/google/gson/it/ShrinkingIT.java | 26 ++++++---- 9 files changed, 129 insertions(+), 78 deletions(-) create mode 100644 shrinker-test/common.pro create mode 100644 shrinker-test/src/main/java/com/example/ClassWithoutDefaultConstructor.java rename shrinker-test/src/main/java/com/example/{DefaultConstructorMain.java => NoSerializedNameMain.java} (60%) diff --git a/gson/src/main/resources/META-INF/proguard/gson.pro b/gson/src/main/resources/META-INF/proguard/gson.pro index 59d3bb441d..8f5a69b30f 100644 --- a/gson/src/main/resources/META-INF/proguard/gson.pro +++ b/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 ; -} - # 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 @@ (); } -# 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> { - (); +-keepclasseswithmembers,allowobfuscation class <1> { + @com.google.gson.annotations.SerializedName ; +} +-if class * { @com.google.gson.annotations.SerializedName ; } +-keepclassmembers,allowobfuscation,allowoptimization class <1> { + (); +} diff --git a/shrinker-test/common.pro b/shrinker-test/common.pro new file mode 100644 index 0000000000..ab45d2039a --- /dev/null +++ b/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 ; +} + +-keepclassmembernames class com.example.ClassWithExposeAnnotation { + ; +} +-keepclassmembernames class com.example.ClassWithJsonAdapterAnnotation { + ** f; +} +-keepclassmembernames class com.example.ClassWithVersionAnnotations { + ; +} diff --git a/shrinker-test/proguard.pro b/shrinker-test/proguard.pro index 3e8a812041..ee96073372 100644 --- a/shrinker-test/proguard.pro +++ b/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 ; -} - --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 { ; } --keepclassmembernames class com.example.ClassWithJsonAdapterAnnotation { - ** f; -} --keepclassmembernames class com.example.ClassWithVersionAnnotations { - ; -} - - --keepclassmembernames class com.example.DefaultConstructorMain$TestClass { +-keepclassmembernames class com.example.NoSerializedNameMain$TestClassNotAbstract { ; } --keepclassmembernames class com.example.DefaultConstructorMain$TestClassNotAbstract { +-keepclassmembernames class com.example.NoSerializedNameMain$TestClassWithoutDefaultConstructor { ; } diff --git a/shrinker-test/r8.pro b/shrinker-test/r8.pro index 392f0f0d9c..01b8c84ee2 100644 --- a/shrinker-test/r8.pro +++ b/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 ; } diff --git a/shrinker-test/src/main/java/com/example/ClassWithDefaultConstructor.java b/shrinker-test/src/main/java/com/example/ClassWithDefaultConstructor.java index 6296237f66..6cff119011 100644 --- a/shrinker-test/src/main/java/com/example/ClassWithDefaultConstructor.java +++ b/shrinker-test/src/main/java/com/example/ClassWithDefaultConstructor.java @@ -2,6 +2,10 @@ 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; diff --git a/shrinker-test/src/main/java/com/example/ClassWithoutDefaultConstructor.java b/shrinker-test/src/main/java/com/example/ClassWithoutDefaultConstructor.java new file mode 100644 index 0000000000..2af573677a --- /dev/null +++ b/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; + } +} diff --git a/shrinker-test/src/main/java/com/example/Main.java b/shrinker-test/src/main/java/com/example/Main.java index 55bbb6377d..488bc03b82 100644 --- a/shrinker-test/src/main/java/com/example/Main.java +++ b/shrinker-test/src/main/java/com/example/Main.java @@ -32,6 +32,7 @@ public static void runTests(BiConsumer outputConsumer) { testNamedFields(outputConsumer); testSerializedName(outputConsumer); + testNoDefaultConstructor(outputConsumer); testNoJdkUnsafe(outputConsumer); testEnum(outputConsumer); @@ -89,6 +90,16 @@ private static void testSerializedName(BiConsumer outputConsumer }); } + private static void testNoDefaultConstructor(BiConsumer 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 outputConsumer) { Gson gson = new GsonBuilder().disableJdkUnsafe().create(); TestExecutor.run(outputConsumer, "Read: No JDK Unsafe; initial constructor value", () -> { diff --git a/shrinker-test/src/main/java/com/example/DefaultConstructorMain.java b/shrinker-test/src/main/java/com/example/NoSerializedNameMain.java similarity index 60% rename from shrinker-test/src/main/java/com/example/DefaultConstructorMain.java rename to shrinker-test/src/main/java/com/example/NoSerializedNameMain.java index 4f0152f7d1..cd70af34a7 100644 --- a/shrinker-test/src/main/java/com/example/DefaultConstructorMain.java +++ b/shrinker-test/src/main/java/com/example/NoSerializedNameMain.java @@ -4,32 +4,32 @@ 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 static String runTest() { } /** - * 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 static String runTestNoJdkUnsafe() { } /** - * 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)); diff --git a/shrinker-test/src/test/java/com/google/gson/it/ShrinkingIT.java b/shrinker-test/src/test/java/com/google/gson/it/ShrinkingIT.java index 3304570197..d9d8d56cfb 100644 --- a/shrinker-test/src/test/java/com/google/gson/it/ShrinkingIT.java +++ b/shrinker-test/src/test/java/com/google/gson/it/ShrinkingIT.java @@ -127,6 +127,14 @@ public void test() throws Exception { "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 void test() throws Exception { } @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 void testDefaultConstructor() throws Exception { 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 void testDefaultConstructor() throws Exception { } @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 void testDefaultConstructorNoJdkUnsafe() throws Exception { // 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 void testDefaultConstructorNoJdkUnsafe() throws Exception { } @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 void testNoDefaultConstructor() throws Exception { 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" ); }