From d8d2471737ba830c115c45d10333a1f6a78b4376 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 17 Jun 2023 16:18:16 +0200 Subject: [PATCH 1/5] Adjust ProGuard default rules and shrinking tests --- Troubleshooting.md | 6 +++- examples/android-proguard-example/README.md | 3 ++ .../gson/internal/ConstructorConstructor.java | 17 +++++------ .../main/resources/META-INF/proguard/gson.pro | 28 +++++++++++++------ .../gson/functional/Java17RecordTest.java | 27 +++++++++--------- .../internal/ConstructorConstructorTest.java | 22 +++++++-------- shrinker-test/pom.xml | 2 ++ shrinker-test/proguard.pro | 18 ++++++++++++ shrinker-test/r8.pro | 16 +---------- .../ClassWithJsonAdapterAnnotation.java | 1 + .../com/example/DefaultConstructorMain.java | 3 -- 11 files changed, 83 insertions(+), 60 deletions(-) diff --git a/Troubleshooting.md b/Troubleshooting.md index 57e781cbb9..b0cb9c0758 100644 --- a/Troubleshooting.md +++ b/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,8 @@ Note: For newer Gson versions these rules might be applied automatically; make s } ``` +You can also use `(...);` to keep all constructors of that class, but then you might actually rely on JDK Unsafe 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. diff --git a/examples/android-proguard-example/README.md b/examples/android-proguard-example/README.md index 942477d71a..53ab818d26 100644 --- a/examples/android-proguard-example/README.md +++ b/examples/android-proguard-example/README.md @@ -15,3 +15,6 @@ tools. Have a look at R8's Compatibility FAQ, and especially at the [Gson sectio 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. + +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). diff --git a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java b/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java index 7d2dc9b622..0f488a9bb9 100644 --- a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java +++ b/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java @@ -76,14 +76,15 @@ static String checkInstantiable(Class c) { 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; } diff --git a/gson/src/main/resources/META-INF/proguard/gson.pro b/gson/src/main/resources/META-INF/proguard/gson.pro index c9f235e95b..884d7048e9 100644 --- a/gson/src/main/resources/META-INF/proguard/gson.pro +++ b/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 ; @com.google.gson.annotations.JsonAdapter ; @com.google.gson.annotations.Since ; @@ -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 { (); } --keep class * implements com.google.gson.TypeAdapterFactory { +-keepclassmembers class * implements com.google.gson.TypeAdapterFactory { (); } --keep class * implements com.google.gson.JsonSerializer { +-keepclassmembers class * implements com.google.gson.JsonSerializer { (); } --keep class * implements com.google.gson.JsonDeserializer { +-keepclassmembers class * implements com.google.gson.JsonDeserializer { (); } + +# If a class still exists after shrinking, and has fields annotated with @SerializedName, +# keep the constructors +# Note: This behavior is not officially documented somewhere; based on https://issuetracker.google.com/issues/150189783#comment11 +# and discussion in https://github.com/google/gson/pull/2397 +-if class * +-keepclasseswithmembers,allowobfuscation,allowoptimization class <1> { + (); + @com.google.gson.annotations.SerializedName ; +} diff --git a/gson/src/test/java/com/google/gson/functional/Java17RecordTest.java b/gson/src/test/java/com/google/gson/functional/Java17RecordTest.java index 0b9a6c078c..bc9e8f06b6 100644 --- a/gson/src/test/java/com/google/gson/functional/Java17RecordTest.java +++ b/gson/src/test/java/com/google/gson/functional/Java17RecordTest.java @@ -89,7 +89,7 @@ public int i() { 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 @@ record LocalRecord(String s) { // 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 void testPrimitiveJsonNullValue() { 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 @@ record Blocked(int b) {} 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 void testReflectionFilterBlockInaccessible() { 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 @@ record LocalRecord(int i) {} 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"); } } diff --git a/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java b/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java index 602ba074e9..e582ad08ac 100644 --- a/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java +++ b/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java @@ -19,17 +19,14 @@ 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.>emptyMap(), true, - Collections.emptyList() + Collections.emptyMap(), true, + Collections.emptyList() ); private abstract static class AbstractClass { @@ -39,7 +36,7 @@ public AbstractClass() { } 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 void testGet_AbstractClassNoArgConstructor() { 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 void testGet_Interface() { 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"); } } } diff --git a/shrinker-test/pom.xml b/shrinker-test/pom.xml index dc4b01c91b..b292300cc5 100644 --- a/shrinker-test/pom.xml +++ b/shrinker-test/pom.xml @@ -191,6 +191,8 @@ + com.android.tools r8 8.0.40 diff --git a/shrinker-test/proguard.pro b/shrinker-test/proguard.pro index 0cb35048c9..072cbb2d6e 100644 --- a/shrinker-test/proguard.pro +++ b/shrinker-test/proguard.pro @@ -27,3 +27,21 @@ -keepclassmembers class com.example.ClassWithNamedFields { !transient ; } + +-keepclassmembernames class com.example.ClassWithExposeAnnotation { + ; +} +-keepclassmembernames class com.example.ClassWithJsonAdapterAnnotation { + ** f; +} +-keepclassmembernames class com.example.ClassWithVersionAnnotations { + ; +} + + +-keepclassmembernames class com.example.DefaultConstructorMain$TestClass { + ; +} +-keepclassmembernames class com.example.DefaultConstructorMain$TestClassNotAbstract { + ; +} diff --git a/shrinker-test/r8.pro b/shrinker-test/r8.pro index 690fe339b5..c7959112d6 100644 --- a/shrinker-test/r8.pro +++ b/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 { - (); -} --keepclassmembers class com.example.GenericClasses$GenericClass { - (); -} --keepclassmembers class com.example.GenericClasses$UsingGenericClass { - (); -} --keepclassmembers class com.example.GenericClasses$GenericUsingGenericClass { - (); -} - # 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 @@ -31,6 +17,6 @@ } # Keep enum constants which are not explicitly used in code --keep class com.example.EnumClass { +-keepclassmembers class com.example.EnumClass { ** SECOND; } diff --git a/shrinker-test/src/main/java/com/example/ClassWithJsonAdapterAnnotation.java b/shrinker-test/src/main/java/com/example/ClassWithJsonAdapterAnnotation.java index 238ee1818e..0cb05a25a6 100644 --- a/shrinker-test/src/main/java/com/example/ClassWithJsonAdapterAnnotation.java +++ b/shrinker-test/src/main/java/com/example/ClassWithJsonAdapterAnnotation.java @@ -23,6 +23,7 @@ */ 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; diff --git a/shrinker-test/src/main/java/com/example/DefaultConstructorMain.java b/shrinker-test/src/main/java/com/example/DefaultConstructorMain.java index e570866bec..e523f8b75c 100644 --- a/shrinker-test/src/main/java/com/example/DefaultConstructorMain.java +++ b/shrinker-test/src/main/java/com/example/DefaultConstructorMain.java @@ -4,17 +4,14 @@ import com.google.gson.Gson; import com.google.gson.GsonBuilder; -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 { - @SerializedName("s") public String s; } From b932a6545d6b57ff9f601cbb2503c9123706b001 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 17 Jun 2023 16:46:13 +0200 Subject: [PATCH 2/5] Adjust comment --- gson/src/main/resources/META-INF/proguard/gson.pro | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gson/src/main/resources/META-INF/proguard/gson.pro b/gson/src/main/resources/META-INF/proguard/gson.pro index 884d7048e9..ff5772da2d 100644 --- a/gson/src/main/resources/META-INF/proguard/gson.pro +++ b/gson/src/main/resources/META-INF/proguard/gson.pro @@ -60,7 +60,7 @@ } # If a class still exists after shrinking, and has fields annotated with @SerializedName, -# keep the constructors +# keep the no-args constructor # Note: This behavior is not officially documented somewhere; based on https://issuetracker.google.com/issues/150189783#comment11 # and discussion in https://github.com/google/gson/pull/2397 -if class * From a206d906c1f0666daf9906fc6fb58ffc9479bc22 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Thu, 22 Jun 2023 17:17:37 +0200 Subject: [PATCH 3/5] Add shrinking test for class without no-args constructor; improve docs --- Troubleshooting.md | 2 ++ examples/android-proguard-example/README.md | 4 ++-- shrinker-test/proguard.pro | 1 + shrinker-test/r8.pro | 2 ++ .../com/example/DefaultConstructorMain.java | 21 +++++++++++++++++++ .../java/com/google/gson/it/ShrinkingIT.java | 20 ++++++++++++++++++ 6 files changed, 48 insertions(+), 2 deletions(-) diff --git a/Troubleshooting.md b/Troubleshooting.md index b0cb9c0758..b6d1e6caa9 100644 --- a/Troubleshooting.md +++ b/Troubleshooting.md @@ -331,3 +331,5 @@ You can also use `(...);` to keep all constructors of that class, but then 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). 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. diff --git a/examples/android-proguard-example/README.md b/examples/android-proguard-example/README.md index 53ab818d26..902960fdfa 100644 --- a/examples/android-proguard-example/README.md +++ b/examples/android-proguard-example/README.md @@ -12,9 +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). diff --git a/shrinker-test/proguard.pro b/shrinker-test/proguard.pro index 072cbb2d6e..3e8a812041 100644 --- a/shrinker-test/proguard.pro +++ b/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(); } diff --git a/shrinker-test/r8.pro b/shrinker-test/r8.pro index c7959112d6..392f0f0d9c 100644 --- a/shrinker-test/r8.pro +++ b/shrinker-test/r8.pro @@ -11,6 +11,8 @@ # 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 ; diff --git a/shrinker-test/src/main/java/com/example/DefaultConstructorMain.java b/shrinker-test/src/main/java/com/example/DefaultConstructorMain.java index e523f8b75c..4f0152f7d1 100644 --- a/shrinker-test/src/main/java/com/example/DefaultConstructorMain.java +++ b/shrinker-test/src/main/java/com/example/DefaultConstructorMain.java @@ -4,6 +4,7 @@ import com.google.gson.Gson; import com.google.gson.GsonBuilder; +import com.google.gson.annotations.SerializedName; public class DefaultConstructorMain { static class TestClass { @@ -15,6 +16,18 @@ 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; + } + } + /** * Main entrypoint, called by {@code ShrinkingIT.testDefaultConstructor()}. */ @@ -31,4 +44,12 @@ public static String runTestNoJdkUnsafe() { 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; + } } 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 9c32f1aa57..3304570197 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 @@ -220,4 +220,24 @@ public void testDefaultConstructorNoJdkUnsafe() throws Exception { } }); } + + @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" + ); + } + }); + } } From 91772ee6094defb05f6c1adfb8d9eb4a95b6f9cc Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Thu, 22 Jun 2023 17:31:38 +0200 Subject: [PATCH 4/5] Improve Unsafe mention in Troubleshooting Guide --- Troubleshooting.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Troubleshooting.md b/Troubleshooting.md index b6d1e6caa9..184f19166e 100644 --- a/Troubleshooting.md +++ b/Troubleshooting.md @@ -326,7 +326,7 @@ Note: If the class which you are trying to deserialize is actually abstract, the } ``` -You can also use `(...);` to keep all constructors of that class, but then you might actually rely on JDK Unsafe 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. +You can also use `(...);` 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). From 6feba4c0880aa182b4cfe670f66f164ea92fef69 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Tue, 4 Jul 2023 22:41:41 +0200 Subject: [PATCH 5/5] Improve comment for `-if class *` --- gson/src/main/resources/META-INF/proguard/gson.pro | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/gson/src/main/resources/META-INF/proguard/gson.pro b/gson/src/main/resources/META-INF/proguard/gson.pro index ff5772da2d..59d3bb441d 100644 --- a/gson/src/main/resources/META-INF/proguard/gson.pro +++ b/gson/src/main/resources/META-INF/proguard/gson.pro @@ -59,10 +59,10 @@ (); } -# If a class still exists after shrinking, and has fields annotated with @SerializedName, -# keep the no-args constructor -# Note: This behavior is not officially documented somewhere; based on https://issuetracker.google.com/issues/150189783#comment11 -# and discussion in https://github.com/google/gson/pull/2397 +# 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> { ();