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

Easier to configure ProGuard rules #1721

Merged
merged 11 commits into from Oct 29, 2021
Merged

Easier to configure ProGuard rules #1721

merged 11 commits into from Oct 29, 2021

Conversation

Whathecode
Copy link
Contributor

@Whathecode Whathecode commented Oct 11, 2021

While enabling ProGuard on my Android project, I ran into the (seemingly recently changed) ProGuard example configuration of kotlinx.serialization. Given the way serializer lookup works at runtime, the ProGuard configuration seems unnecessarily complex to me in terms of the number of changes end users need to make. I noticed this by running into some alternatives provided by @ArcticLampyrid, which provided the base inspiration for this PR.

As part of Hacktoberfest, I wanted to both challenge myself learning about ProGuard and contribute to my favorite Kotlin library!

Noticeable changes, more or less in order they appear in the rules:

  • -keepattributes *Annotation* does not seem needed. I don't know of any annotations that are retrieved at runtime in kotlinx.serialization, but maybe I missed something. If this is needed, it can probably be narrowed down to RuntimeVisibleAnnotations or similar.
  • I could not figure out what -dontnote kotlinx.serialization.AnnotationsKt is meant to do. This may be an omission on my part.
  • -keepattributes InnerClasses is only needed in case there are serializable classes with named companion objects.
  • If I'm not mistaken the kotlinx.serialization-json specific rules were to handle @Serializable of JsonElement and the like. These are no longer needed as they are handled by the new all-encompassing ** rules.
  • The new rules for serializable classes with unnamed companion objects don't require changes by the end user by relying on wildcard matches.
  • The new rule for serializable classes with named companion objects is easier to modify. Only a list of these classes needs to be changed, rather than copy/pasting the rule multiple times. This works again by relying on wildcard matches.
  • Custom serializers seem to work out-of-the-box. Don't see why these would need to be kept manually.

These changes were tested in an Android project which contained, and used, the following serializable classes. Maybe it is a good idea to set up a test for this?

@Serializable
class HasSimpleCompanion( val output: String )

@Serializable
object ObjectSerializable { val output: String = "This is an object." }

@Serializable
class HasNamedCompanion( val output: String )
{
    companion object Named
}

@Serializable
class HasNamedCompanion2( val output: String )
{
    companion object Named
}

@Serializable
class HasJsonElement( val output: JsonElement )

@Serializable( CustomSerializer::class )
class HasCustomSerializer( val output: String )

object CustomSerializer : KSerializer<HasCustomSerializer>
{
    override val descriptor: SerialDescriptor =
        PrimitiveSerialDescriptor( "Custom", PrimitiveKind.STRING )

    override fun serialize( encoder: Encoder, value: HasCustomSerializer ) =
        encoder.encodeString( "CUSTOM!" )

    override fun deserialize( decoder: Decoder ): HasCustomSerializer {
        val hardcoded = decoder.decodeString()
        return HasCustomSerializer(hardcoded)
    }
}

In case you approve of these changes, you can slap a "hacktoberfest-accepted" label on this PR to thank me. :)

Copy link
Contributor

@shanshin shanshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
sorry for the delay, and thanks for your contribution.

The main idea behind specifying the application package in the rules was for the user to control which classes really needed to be serialized (not obfuscate them or shrink their members). This can be useful if some dependency contains serializable classes that the application does not serialize for some reason.

On the other hand, such cases are rare and it is more convenient for users to use the rules without changing anything.
It might be worth keeping the old rules, but adding the new ones as an alternative by placing them in an expanding block like this

<details>
<summary>Rules that apply to all serializable classes</summary>
  ...rules...
</details>

This way we can collect more feedback and be able to create rules that are suitable for the majority of users.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
# public static <1>$$serializer INSTANCE;
#}

# Keep `serializer()` on companion objects of serializable classes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please indicate in this comment that this rule applies to default (Companion) and named companions. This makes the rules easier to read.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a small side effect, this rule will keep the serializer function for all nested classes, but this is a very rare case and this rule is the easiest way to support named companions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this rule will keep the serializer function for all nested classes

It actually doesn't, because of the pattern matching. It will pass beyond the if check, but the <3> in -keepclassmembers class <1>$<3> will be the static field name defined in the if condition.

I tried with the following:

@Serializable
class HasSimpleCompanion( val output: String )
{
    class Test
    {
        fun answer() = 42
        fun serializer(): KSerializer<*> = HasSimpleCompanion.serializer()
    }
}

This would translate to -keepclassmembers class HasSimpleCompanion$Companion, not -keepclassmembers class HasSimpleCompanion$Test. There is no static field for normal inner classes.

serializer is still removed (answer() is used so the full class isn't removed) as shown in the usage.txt output:

com.example.myapplication.HasSimpleCompanion$Test:
public final kotlinx.serialization.KSerializer serializer()

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@shanshin shanshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
I believe that the new rules will be more convenient for Android developers.

@shanshin shanshin merged commit f6a3432 into Kotlin:master Oct 29, 2021
ansman pushed a commit to ansman/kotlinx.serialization that referenced this pull request Nov 1, 2021
These rules only require modification when serializable classed with named companion objects are used, and even then, less than the previous rules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants