Add ProGuard / R8 integration tests & add default ProGuard rules

[Marcono1234]

Purpose

Adds integration tests using ProGuard and R8 with Gson, and adds default ProGuard rules as META-INF/proguard/gson.pro, see also https://developer.android.com/build/shrink-code#configuration-files

Resolves #1945
Supersedes #1929

Description

Adds a new shrinker-test Maven module which has both ProGuard and R8 configured to process a JAR file and then run integration tests on the JAR. These tests are (hopefully) more extensive than the the existing EnumWithObfuscatedTest (I have still kept it though).

Additionally the file META-INF/proguard/gson.pro is added which contains common ProGuard and R8 rules relevant for all users. This file is recognized by R8 (see https://developer.android.com/build/shrink-code#configuration-files) and apparently also by the ProGuard Android plugin (haven't verified this myself yet).
However, for user specific code (such as model classes) it might still be necessary that the users specify additional custom rules themselves, such as disabling obfuscation for specific fields.

This pull request also adds a new troubleshooting entry for the "Abstract classes can't be instantiated!" exception which recently occurred pretty frequently (see #2379) due to R8 "full mode" being enabled by default. The integration test covers a case where that exception is thrown as well.

⚠️ Unlike the original pull request #1929, this only adds one library rules file gson.pro instead of 4. I hope that this should work fine nonetheless for recent R8 versions (integration test aims to verify this) and for ProGuard (automatic detection not checked by integration test due to Guardsquare/proguard#337).
I am however not that familiar with ProGuard and R8, so I might have overlooked or failed to consider something. Any feedback is appreciated.

Checklist

• New code follows the Google Java Style Guide
• If necessary, new public API validates arguments, for example rejects null
• New public API has Javadoc
  • Javadoc uses @since $next-version$
    ($next-version$ is a special placeholder which is automatically replaced during release)
• If necessary, new unit tests have been added
  • Assertions in unit tests use Truth, see existing tests
  • No JUnit 3 features are used (such as extending class TestCase)
  • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
• mvn clean verify javadoc:jar passes without errors

[Marcono1234]

The Maven build currently produces the following warning for the shrinker-test module:

Warning:  Missing POM for com.android.tools:r8:jar:8.0.40

I haven't really figured out why that is the case; the artifact seems to have a POM file. Even adding the Google Maven repository (containing R8) as regular repository in the shrinker-test pom.xml does not seem to have solved this. But I guess the warning can be ignored.

Also R8 logs the following:

Proguard configuration rule does not match anything: `-keep class * implements com.google.gson.JsonSerializer {
  <init>();
}`

(same also for JsonDeserializer)

I guess these can be ignored since the shrinker-test project simply does not have a test for JsonSerializer and JsonDeserializer at the moment. I am a bit surprised though that R8 logs this for a library rules file (gson.pro) in the first place; maybe that is an issue with R8?
Edit: Have adjusted the tests to also cover JsonSerializer and JsonDeserializer so now the warnings above are not shown anymore for the tests. They might still occur for users, but they should be safe to ignore I guess.

Also note that this new shrinker-test module causes the build for the complete Maven project to take a bit longer but I hope that is acceptable for the advantages it (hopefully) provides.

[Marcono1234]

Not completely sure if this is actually needed, but for now I just kept it to be safe. I think removing the static toJson and fromJson methods (and implicitly the same(...) calls) from Main made a difference; but I am not completely sure anymore.

[Marcono1234]

Removed this here from the <dependencyManagement> because specifying the scope there seems to be discouraged, see https://stackoverflow.com/q/15221299

All the modules of this project using JUnit as test dependency already set the scope anyways apparently

[Marcono1234]

@christofferqa, I hope it is ok to ask you, since you suggested some improvements to the existing ProGuard rules file in the past (#1930), does the META-INF/proguard/gson.pro file added by this pull request look reasonable to you? And would it indeed suffice (at least for R8) to have only one file under META-INF/proguard, or is it necessary to have multiple files for the current rules as initially proposed by #1929?

Also, slightly off-topic, but to what extent does R8 support the -addconfigurationdebugging option (see also Google bug tracker issue)? If I add -addconfigurationdebugging to the shrinker-test/r8.pro file of this PR, and then run mvn --projects shrinker-test --also-make clean verify in the root directory, R8 fails with a CompilationFailedException. Maybe this R8 setup here is too contrived though (?).

[eamonnmcmanus]

Thanks very much for doing this!
Like you I am not hugely familiar with ProGuard and R8, so I'll give @christofferqa a chance to weigh in before I merge it.

[Marcono1234]

I have extended the tests a bit more and added a new section to the troubleshooting guide. But I think this pull request is ready now.

As disclaimer: The code shrinking tests are a bit brittle, so if they fail in the future when adjusting the default Gson ProGuard rules or when upgrading the ProGuard or R8 version used by the test, it might be necessary to rewrite them or maybe even remove some of them if rewriting them is not easily possible.
But for now I hope that they are useful.

[eamonnmcmanus]

OK, if this is ready for merging then I'll merge it. :-) We can always adjust later.

[christofferqa]

@Marcono1234 It is fine to add only META-INF/proguard/gson.pro. It is only if you start adding rules in META-INF/com.android.tools/ that META-INF/proguard/gson.pro will not be included by R8.

There has not been much work on supporting -addconfigurationdebugging in R8. While compilation should definitely not fail, I don't think there is much value in using the directive at this point.

Regarding the rules added by this CL, there are a few issues that would be good to address:

• The rule -keepattributes *Annotation* will keep all runtime invisible annotations for all dependencies. You probably want to change this to just -keepattributes RuntimeVisibleAnnotations (see also https://www.guardsquare.com/manual/configuration/attributes).

• There are several unconditional -keep rules that should preferably be changed to -if rules, -keep,allowshrinking rules, or -keepclassmembers rules. Ideally if you add a dependency on gson to an existing app, and does not use gson at all, no classes from com.google.gson should be included in the app after shrinking.

• There have been a few issues reported on the R8 issuetracker due to inadequate rules related to gson. See, for example, https://issuetracker.google.com/150189783 and https://issuetracker.google.com/280658459. The key issue is that the following rule:

  -keepclassmembers,allowobfuscation class * {
    @com.google.gson.annotations.SerializedName <fields>;
  }
  

  ... does not specify that the classes with @SerializeName fields are instantiated by reflection. As suggested in https://issuetracker.google.com/280658459#comment4, this can be addressed by the following rule, which also keeps the class using -keepclasseswithmembers:

  -if class *
  -keepclasseswithmembers,allowshrinking,allowobfuscation class <1> {
    @com.google.gson.annotations.SerializedName <fields>;
  }
  

[sgjesse]

I agree with @christofferqa on the comments above. -keepattributes *Annotation* should definitely be replaced with -keepattributes RuntimeVisibleAnnotations or perhaps -keepattributes AnnotationDefault,RuntimeVisibleAnnotations if any of the GSON annotations has default values. As rules are global matching RuntimeInvisible*Annotations attributes should be avoided.

For the rules for @Since and @Until would it be possible to add allowobfuscation? If not then any fields annotated with both @SerailizedName and @Since or @Until will not get renamed after all. Same for @Expose and @JsonAdapter on a field.

Finally is -keep @com.google.gson.annotations.JsonAdapter class * needed? Doesn't the adapter implementation reference the class (and members)?

[Marcono1234]

Thanks to you both for your answers!

---

• The rule -keepattributes *Annotation* will keep all runtime invisible annotations for all dependencies. You probably want to change this to just -keepattributes RuntimeVisibleAnnotations (see also https://www.guardsquare.com/manual/configuration/attributes).

It appears RuntimeVisibleAnnotations and AnnotationDefault are necessary, otherwise an IncompleteAnnotationException occurs for the integration tests.

---

• There are several unconditional -keep rules that should preferably be changed to -if rules, -keep,allowshrinking rules, or -keepclassmembers rules. Ideally if you add a dependency on gson to an existing app, and does not use gson at all, no classes from com.google.gson should be included in the app after shrinking.

I assume you mean the following:

# 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>();
}

Yes looks like -keepclassmembers will work here as well.

---

• this can be addressed by the following rule, which also keeps the class using -keepclasseswithmembers:

  -if class *
  -keepclasseswithmembers,allowshrinking,allowobfuscation class <1> {
    @com.google.gson.annotations.SerializedName <fields>;
  }
  

So the following rule from https://issuetracker.google.com/issues/150189783#comment11 should then also solve the issues with the constructors, right?

-if class *
-keepclasseswithmembers class <1> {
  <init>(...);
  @com.google.gson.annotations.SerializedName <fields>;
}

Or is explicitly specifying <init>(...); in that case not needed?

And to clarify, what is the purpose of the -if class * here? I don't think this is documented officially anywhere, but I assume this means "if the class is somehow referenced in the first place", right? Because just reading it literally it seem this should have no special effect and could be omitted.

---

As rules are global matching RuntimeInvisible*Annotations attributes should be avoided.

I would also prefer explicitly matching only Gson annotations instead, but restricting this only to specific annotation types is apparently not possible for -keepattributes? Also, is this a typo in your sentence and it should say the following?

matching RuntimeInvisible*Annotations*Annotation* attributes

---

For the rules for @Since and @Until would it be possible to add allowobfuscation? If not then any fields annotated with both @SerailizedName and @Since or @Until will not get renamed after all. Same for @Expose and @JsonAdapter on a field.

Yes I guess that makes sense. Except for @Expose it is unlikely that all fields will have @Since, @Until or @JsonAdapter so the user will have to use @SerializedName anyway for some fields.

---

Finally is -keep @com.google.gson.annotations.JsonAdapter class * needed? Doesn't the adapter implementation reference the class (and members)?

If I remove it then the integration test fails because R8 seems to remove the annotation here:

gson/shrinker-test/src/main/java/com/example/ClassWithAdapter.java

Lines 9 to 10 in 2d7cc2e

	@JsonAdapter(ClassWithAdapter.Adapter.class)
	public class ClassWithAdapter {

gson/shrinker-test/src/main/java/com/example/GenericClasses.java

Lines 41 to 42 in 2d7cc2e

	@JsonAdapter(DummyClass.Adapter.class)
	static class DummyClass {

I can try using this 'magic' -if class * in case that is any better (see other question above in this comment), but that only seems to solve the issue for the ClassWithAdapter from the integration test, but not for the GenericClasses$DummyClass.

---

I will try to create a pull request then which addresses this and the other points I wrote down in #2401.

[Marcono1234]

@christofferqa and @sgjesse, I have created #2420 as follow-up. If you have the time, could you please have a look at my comment above and the ProGuard rule changes in that pull request and see if they are reasonable?

[sgjesse]

Thank you for working on this! I have added a few comments in #2420, but it looks good, and great to have the right rules be part of the GSON library!