Skip to content
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

ProGuard rules are too strict #1129

Closed
mxalbert1996 opened this issue Oct 9, 2020 · 16 comments · Fixed by #1717
Closed

ProGuard rules are too strict #1129

mxalbert1996 opened this issue Oct 9, 2020 · 16 comments · Fixed by #1717
Assignees
Labels

Comments

@mxalbert1996
Copy link

mxalbert1996 commented Oct 9, 2020

The ProGuard rules in the README are so strict that they prevent json model classes and generated serializer classes from being optimized and obfuscated.

After some tests, these seem enough:

-keepclassmembers,allowoptimization class com.yourcompany.yourpackage.** {
    *** Companion;
}
-keepclassmembers,allowoptimization class com.yourcompany.yourpackage.** {
    kotlinx.serialization.KSerializer serializer(...);
}

The first rule keeps the Companion field and the Companion class of json model classes. The field cannot be renamed because of the reflection usage but its class can. This rule does not prevent the model class itself and its Companion class from being obfuscated.

The second rule keeps the serializer() method of the Companion class of json model classes. This method also cannot be renamed because of this. Since this method will return the serializer instance for this model class, the specific serializer class will also be kept. Serializer classes don't use reflection so they (including their fields and methods) can be obfuscated and this rule does not prevent that.

If this looks good to you, I'm glad to submit a PR.

@JakeWharton
Copy link
Contributor

I'm using

# Kotlin serialization looks up the generated serializer classes through a function on companion
# objects. The companions are looked up reflectively so we need to explicitly keep these functions.
-keepclasseswithmembers class **.*$Companion {
    kotlinx.serialization.KSerializer serializer(...);
}
# If a companion has the serializer function, keep the companion field on the original type so that
# the reflective lookup succeeds.
-if class **.*$Companion {
  kotlinx.serialization.KSerializer serializer(...);
}
-keepclassmembers class <1>.<2> {
  <1>.<2>$Companion Companion;
}

which is a bit more general but potentially not as permissive with regard to obfuscation (as i don't obfuscate).

Perhaps we could combine and embed the rules so that they're applied automatically (similar to what we've done with coroutines, and many other libraries).

@sandwwraith
Copy link
Member

What's the difference from the current proguard configuration? I can see only keepclassmembers vs keepclasseswithmembers and 'allowoptimization' flag.

Also, line
-keep,includedescriptorclasses class com.yourcompany.yourpackage.**$$serializer { *; } is needed in case the class has named companion — in this case, lookup tries to access generated class directly without Companion.serializer() invocation.

@jw Am I right that

-keepclasseswithmembers class **.*$Companion {
    kotlinx.serialization.KSerializer serializer(...);
}

allows to keep all the Companion.serializer() functions regardless of the package? Then we likely can embed this rules in the library — without the need for users to fill in their app's package.

@JakeWharton
Copy link
Contributor

Yes. It assumes the companion is named Companion (I'm not sure if serialization supports named companions) and it also means that you potentially keep more code than is needed (if you are consuming a library with serialization code but it's unused).

The first issue could probably be fixed by replacing Companion with another wildcard (*), and the second issue is just something I live with since in practice it likely doesn't happen much, if at all.

@mxalbert1996
Copy link
Author

@sandwwraith
keepclassmembers keeps the specified members if the class is not shrinked while keepclasseswithmembers always keeps the class and the members. The word keep in ProGuard rules basically means keep from being shrinked and obfuscated so keepclasseswithmembers will prevent the class from being obfuscated. Another difference is that if the user isn't actually using a model class, keepclassmembers will allow the class to be shrinked but keepclasseswithmembers will keep the class anyway.

I didn't realized the support of named companions. In that case the classes cannot be obfuscated. However, I don't think includedescriptorclasses is needed as the generated serializers do not use reflection.

Obfuscation is important to me and is actually one of the reasons I migrated from Moshi as the Moshi codegen generates ProGuard rules that prevent obfuscation. I don't think there is any ways to exclude part of the rules from libraries, so I would prefer not embedding the rules as they are intrusive (they affect user code and cannot be disabled).

@nbransby
Copy link

Was there any decision / progress made on this?

@dustinsummers
Copy link

On a similar vein, what is the purpose/requirement for this rule?

-keepclassmembers class com.yourcompany.yourpackage.** { # <-- change package name to your app's
    *** Companion;
}

This seems like it would be keeping EVERY Companion object's classmembers, even if they aren't using Kotlin Serialization? Is that really necessary?

@ArcticLampyrid
Copy link

ArcticLampyrid commented Apr 3, 2021

val fromNamedCompanion = try {
jClass.declaredClasses.singleOrNull { it.simpleName == ("\$serializer") }
?.getField("INSTANCE")?.get(null) as? KSerializer<T>
} catch (e: NoSuchFieldException) {
null
}

includedescriptorclasses

Specifies that any classes in the type descriptors of the methods and fields that the -keep option keeps should be kept as well. This is typically useful when keeping native method names, to make sure that the parameter types of native methods aren't renamed either. Their signatures then remain completely unchanged and compatible with the native libraries.

Seems that we need only keep the name of serializers without includedescriptorclasses
And then keep the relationship between a class and its inner classes and outer classes. (-keepattributes InnerClasses)

@eygraber
Copy link

@shanshin shanshin self-assigned this Jun 15, 2021
@shanshin
Copy link
Contributor

Hi!
@mxalbert1996, @JakeWharton, are these rules sufficient and valid for your projects?

-keepclassmembers @kotlinx.serialization.Serializable class com.yourcompany.yourpackage.** {
    # lookup for plugin generated serializable classes
    *** Companion;
    # lookup for serializable objects
    *** INSTANCE;
    kotlinx.serialization.KSerializer serializer(...);
}
# lookup for plugin generated serializable classes
-if @kotlinx.serialization.Serializable class com.yourcompany.yourpackage.**
-keepclassmembers class com.yourcompany.yourpackage.<1>$Companion {
    kotlinx.serialization.KSerializer serializer(...);
}

Serialization supports named companions but for such classes it is necessary to add an additional rule.

-keep class com.yourcompany.yourpackage.SerializableClassWithNamedCompanion$$serializer {
    *** INSTANCE;
}

This rule keep serializer and serializable class from obfuscation. Therefore, it is recommended not to use wildcards in it, but to write rules for each such class.

@dustinsummers
Copy link

@shanshin Appreciate the updated rules. These seemed to work for us as well.

Only correction I think that would need to be made is in the if statement that you provided.

-if @kotlinx.serialization.Serializable class com.yourcompany.yourpackage.**
-keepclassmembers class <1>$Companion {
    kotlinx.serialization.KSerializer serializer(...);
}

The class found in the if statement will be held in the <1> variable.

@shanshin
Copy link
Contributor

@dustinsummers, can you recheck this, please?
In my local project only com.yourcompany.yourpackage.<1>$Companion works well.

@shanshin
Copy link
Contributor

shanshin commented Sep 29, 2021

According this <1> returns the value of the first wildcard, not the whole class name.

@mxalbert1996
Copy link
Author

Actually I'm specifying the serializers of all json classes in my project manually so that I don't need any proguard rules.

val json = Json {
    serializersModule = SerializersModule {
        contextual(JsonClass1.serializer())
        contextual(JsonClass2.serializer())
    }
}

shanshin added a commit that referenced this issue Oct 8, 2021
Now the rules apply only for classes marked as @serializable
Resolves #1129
shanshin added a commit that referenced this issue Oct 8, 2021
Now the rules apply only for classes marked as @serializable
Resolves #1129
@Whathecode
Copy link
Contributor

Whathecode commented Oct 8, 2021

@shanshin Was @ArcticLampyrid 's suggestion accidentally overlooked?

I'm not really familiar with ProGuard rules, but at a glance it seems much more intuitive. I was browsing through ProGuard's documentation to figure out why I wouldn't be able to apply this rule to all @Serializable classes, which ...

-keepclassmembers @kotlinx.serialization.Serializable class ** {
    *** Companion;
}

... seems to take care of.

I thus suspect this comment was overlooked and the current rules are still more complicated than they have to be.

@mxalbert1996
Copy link
Author

mxalbert1996 commented Oct 9, 2021

@Whathecode
I think there are two problems here.

  1. You are keeping the Companion static field here but when using named companion object the field name may not be Companion.
  2. Keeping the companion object field doesn't necessarily keep the methods of the companion object class (specifically serilizer() here).

@Whathecode
Copy link
Contributor

Whathecode commented Oct 9, 2021

@mxalbert1996 this was only a partial copy of the full suggestion by @ArcticLampyrid to highlight at least part of the configuration I understood made more sense to me. serializer is also taken care of there; named companions I'm not certain.

Sorry if that wasn't clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants