-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update ProGuard default shrinking rules #2448
Changes from all commits
7fa9ef5
e384d4d
f60e17d
eea08d0
4c40a91
c34126b
af98d32
ac6e590
8d14f1a
743d529
3468700
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,25 +15,20 @@ | |
# 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 | ||
|
||
# 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 * | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least for R8 it does seem to make a difference: Unfortunately this is not covered by the tests yet; have created #2455 to add a test for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks also for the other points you raised, but unless sgjesse wants to address them here as well could you please create a separate GitHub issue or pull request? Otherwise the discussion on this pull request here might drift too far away from the original topic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed the rules to be two
|
||
-keepclasseswithmembers,allowobfuscation,allowoptimization class <1> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having |
||
<init>(); | ||
-keepclasseswithmembers,allowobfuscation class <1> { | ||
@com.google.gson.annotations.SerializedName <fields>; | ||
} | ||
-if class * { | ||
@com.google.gson.annotations.SerializedName <fields>; | ||
} | ||
-keepclassmembers,allowobfuscation,allowoptimization class <1> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought in #2448 (comment) you just explained that you intentionally removed (Or am I misunderstanding this?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This rule is only covering the no-args constructor ( |
||
<init>(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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>; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 { | ||
<fields>; | ||
} | ||
|
||
|
||
-keepclassmembernames class com.example.DefaultConstructorMain$TestClass { | ||
-keepclassmembernames class com.example.NoSerializedNameMain$TestClassNotAbstract { | ||
<fields>; | ||
} | ||
-keepclassmembernames class com.example.DefaultConstructorMain$TestClassNotAbstract { | ||
-keepclassmembernames class com.example.NoSerializedNameMain$TestClassWithoutDefaultConstructor { | ||
<fields>; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why did you remove this here and add it below guarded by a
-if class *
? Does that make a difference for the end result?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in R8 full mode we have a distinction between referenced and instantiated classes. For classes which are not seen by R8 as instantiated (e.g. if only referenced statically using
.class
as one will do infromJson
then-keepclassmembers
will only keep static members and not instance members. For ProGuard and R8 in compatability mode the two will do the same.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm thanks, ok that sounds like the previous rules would not have worked properly then because
-keepclassmembers
would not have kepts the non-static@SerializedName
fields. Though I think the shrinker-test showed that it did work as expected?I am bit confused, or am I misunderstanding your comment or are the shrinker-tests flawed and not close enough to real code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way to see the difference is to have an instance field annotated with
@SerializedName
which is not used in the code at all, then the field will be removed with the previous rule in R8 full mode (which is default from AGP 8.0.0). Then in a pass through fromfromJson
totoJson
the field value will be lost. Of course it is a matter of taste if unused fields should be kept this way.