Revised the ProGuard rules and added tests on R8 #3041
Conversation
…e version (#3046) ^KT-76136 Co-authored-by: Nikolay Lunyak <[email protected]>
It was decided to deprecate and phase out the concept (see KT-77721).
| serializer(typeOf<SealedInterface>()).descriptor.toString() | ||
| ) | ||
| } | ||
| assertSame<KSerializer<*>>(PlainSerializer, serializer(typeOf<PlainWithCustom>())) |
There was a problem hiding this comment.
Is this change related? I suggest extracting it to separate commit
Also change base branch to dev btw
There was a problem hiding this comment.
Not fully related but I think we should add these changes somewhere
rules/build.gradle.kts
Outdated
| } | ||
|
|
||
| tasks.compileTestKotlin { | ||
| configureCompilation(true) |
There was a problem hiding this comment.
I think it is better to call function with explicit parameter name: configureCompilation(r8FullMode = true)
| return R8CheckerImpl(parseR8Output(mapFile, usageFile)) | ||
| } | ||
|
|
||
| interface R8Checker { |
There was a problem hiding this comment.
I don't think we should do interface/Impl split in a 250-lines-long utils file for a small test
| private fun <T : Any> Class<T>.findNamedCompanionByAnnotation(): Any? { | ||
| // search static field with type marked by kotlinx.serialization.internal.NamedCompanion - it's the companion | ||
| val field = declaredFields.firstOrNull { field -> | ||
| Modifier.isStatic(field.modifiers) && field.type.getAnnotation(NamedCompanion::class.java) != null |
There was a problem hiding this comment.
Mention in the comment that declaredClasses unavailable after R8
rules/build.gradle.kts
Outdated
|
|
||
| sourceSets { | ||
| // create the source set for storing sources and using dependency configuration | ||
| val common by creating |
There was a problem hiding this comment.
common name can be misleading because of the strong associations with MPP. Also, test/testCompatible names themselves are quite vague. WDYT of renaming everything to:
- '(test)Shared'
- 'testR8Full'
- 'testPGCompatible'
Or something similar.
Also, you can just reuse regular main sourceset?
| } | ||
| } | ||
| } else { | ||
| current.fields.put(name, FieldEntry(name, returnType, obfuscated)) |
There was a problem hiding this comment.
How is it different from fieldRegex?
| // process usage.txt | ||
| var currentUsageClass: ClassEntry? = null | ||
|
|
||
| usageFile.forEachLine { raw -> |
There was a problem hiding this comment.
If this file contains methods that were removed due to being unused, then it better be named unused.txt
There was a problem hiding this comment.
I have the same question :)
But it's common name for this files in Android.
| val unusedClass = checker.findClass("kotlinx.serialization.r8.UnusedClass") | ||
| assertTrue(unusedClass.isShrunk) | ||
|
|
||
| ObfuscatedClass("World").used() |
There was a problem hiding this comment.
Removing this line causes test to fail, so it's technically a test data. Maybe put this call to some top-level function in Model.kt?
There was a problem hiding this comment.
If we move this call to Model.kt it will be shrinked as only *Tests classes are alwayes kept. So I move it to separate method in R8Tests.
| } | ||
|
|
||
|
|
||
|
|
settings.gradle.kts
Outdated
| @@ -82,6 +82,9 @@ project(":benchmark").projectDir = file("./benchmark") | |||
| include(":guide") | |||
| project(":guide").projectDir = file("./guide") | |||
|
|
|||
| include(":proguard-rules") | |||
| project(":proguard-rules").projectDir = file("./rules") | |||
There was a problem hiding this comment.
Maybe it should be named proguard-rules-test, because rules on their own are just a bunch of .pro files and do not need Gradle module
- The warning message about unusual symbols in field type is eliminated. - Added correct rules for named companions - Changed code for the locating name companion by annotation. Since the list of nested classes is not saved by R8, static fields are analyzed - added tests on the rules using R8 in full mode and ProGuard compatibility mode Fixes #3033
Fixes #3033