R8 full mode removes constructors kept by Proguard [149729626]

Fixed

Bug

Status Update

No update yet.

Description

mc...@snapchat.com

created issue #1

Feb 18, 2020 07:33PM

When R8 full mode is enabled, it removes a constructor that is normally kept by Proguard. R8 without full mode keeps the constructor as expected. See Proguard rules and sample code below.

Proguard rules:
-keepclassmembers,allowobfuscation @com.example.reflectionerror.JobId class * {
<init>(...);
}

Sample Application:
package com.example.reflectionerror

import android.app.Application

@Target(AnnotationTarget.ANNOTATION_CLASS, AnnotationTarget.CLASS)
@Retention(AnnotationRetention.RUNTIME)
annotation class JobId

@JobId
class Job

class App: Application() {
override fun onCreate() {
super.onCreate()
try {
Job::class.java.getConstructor()
} catch (e: NoSuchMethodException) {
throw IllegalStateException("No constructor")
}
}
}

Comments

mc...@snapchat.com <mc...@snapchat.com> #2Feb 19, 2020 12:13AM

The error was found using the following versions:
Gradle 6.1.1
Android Gradle Plugin 4.0.0-alpha09
R8 2.0.13-dev

ze...@google.com <ze...@google.com> #3Feb 19, 2020 07:31AM

Assigned to ch...@google.com.

Christoffer: feel free to triage elsewhere (including me). This looks like an error as the rule is specific to keeping <init> and not a compat-mode derived rule. I recall another issue I filed with odd behavior when using @annotation in the rules. I'll see if I can locate that.

ze...@google.com <ze...@google.com> #4Feb 19, 2020 07:39AM

Related issue

b/137890409

. The discussion there is that the -keepclassmembers rule does not indicate that any such instances exist, so if no instantiations are known and no keep rule is given for the class, then the rule does not apply. That does match the sample where class Job is not kept and it is not instantiated in the program.

I think we can close this as intended behavior as that follows the R8 interpretation of -keepclassmembers.

ch...@google.com <ch...@google.com> #5Feb 19, 2020 09:37AM

Thanks for the report!

R8 in Proguard compatibility mode

This program works with R8 in Proguard compatibility mode (even without the keep rule!), because we automatically synthesize a keep rule -keep,allowobfuscation com.example.reflectionerror.Job { <init>(); } in response to the Job::class.java expression.

R8 in full mode

In full mode, we try not to synthesize any keep rules under the hood, since they have a negative impact on the resulting code size.

It turns out that three things are currently required in order for the example in comment #1 to work with R8 in full mode:

The rule -keepattributes RuntimeVisibleAnnotations must be included.
The annotation @JobId must be used somewhere in the program, e.g., print(JobId::class.java) -- or it must be kept by a keep rule.
The class Job must be instantiated in order for the -keepclassmembers directive to have any impact on instance members (including constructors). Note that this can be "fixed" by using the following keep rule instead of the -keepclassmembers rule:

-if @com.example.reflectionerror.JobId class *
-keep,allowobfuscation class <1> { <init>(...); }

We discussed that (1) and (2) are not really intended behavior. We have previously discussed that it is questionable to "ignore" -keepclassmembers rules for constructors when the enclosing class is not instantiated (instead, the rule would have to mark the enclosing class as instantiated).

I will keep you posted on any changes.

ap...@google.com <ap...@google.com> #6Feb 19, 2020 11:06AM

Project: r8
Branch: master

commit 5fdbdc8c9ddd955802dcdd6a8cfe5b3f92bf3ed0
Author: Christoffer Quist Adamsen <christofferqa@google.com>
Date: Wed Feb 19 10:51:49 2020

Add a test for keeping annotated items

Change-Id: I3e03372cf0a98ce8c6cc74c910c13aa1fd820c11
Bug: 149729626

M src/main/java/com/android/tools/r8/R8.java
M src/main/java/com/android/tools/r8/shaking/Enqueuer.java
M src/test/java/com/android/tools/r8/TestShrinkerBuilder.java
A src/test/java/com/android/tools/r8/shaking/annotations/B149729626.java

https://r8-review.googlesource.com/48964

mc...@snapchat.com <mc...@snapchat.com> #7Feb 20, 2020 11:19PM

Thanks for the tip. Will add the extra proguard rules to keep the constructors. Related, I also had to keep Gson constructors explicitly:
-keep,allowobfuscation class com.google.gson.Gson { <init>(...); }

ap...@google.com <ap...@google.com> #8Feb 25, 2020 01:34PM

Project: r8
Branch: master

commit f379c05704e3e724d99b58b7b2ca404a700f907a
Author: Christoffer Quist Adamsen <christofferqa@google.com>
Date: Tue Feb 25 11:29:23 2020

Mark types with dependent instance constructor as instantiated

Bug: 149729626, 147972078
Change-Id: Ia030c8c90715fc73ac7ab4c6201bd316725b156b

M src/main/java/com/android/tools/r8/shaking/Enqueuer.java
M src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
M src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTestRunner.java
M src/test/java/com/android/tools/r8/shaking/annotations/B149729626.java
M src/test/java/com/android/tools/r8/shaking/ifrule/ConditionalKeepIfKeptTest.java

https://r8-review.googlesource.com/49085

ap...@google.com <ap...@google.com> #9Feb 25, 2020 02:10PM

Project: r8
Branch: master

commit bd1f8688c56767b84cdff336ffbce4cc7ab2957d
Author: Christoffer Quist Adamsen <christofferqa@google.com>
Date: Tue Feb 25 15:07:42 2020

Fix test for keeping annotated items

Change-Id: I175173789272f2ebf2c827099756c67b4b82381f
Bug: 149729626

M src/main/java/com/android/tools/r8/R8.java
M src/main/java/com/android/tools/r8/graph/DexDefinition.java
M src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/LambdaMerger.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/kotlin/JStyleLambdaGroup.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/kotlin/JStyleLambdaGroupIdFactory.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/kotlin/KStyleLambdaGroup.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/kotlin/KStyleLambdaGroupIdFactory.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/kotlin/KotlinLambdaGroupId.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/kotlin/KotlinLambdaGroupIdFactory.java
A src/main/java/com/android/tools/r8/shaking/AnnotationMatchResult.java
M src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
A src/main/java/com/android/tools/r8/shaking/ConsequentRootSetBuilder.java
M src/main/java/com/android/tools/r8/shaking/Enqueuer.java
M src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java
M src/main/java/com/android/tools/r8/shaking/ProguardMemberRule.java
M src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
M src/test/java/com/android/tools/r8/shaking/annotations/B149729626.java

https://r8-review.googlesource.com/49024

ch...@google.com <ch...@google.com> #10Feb 26, 2020 07:38AM

All issues are now fixed on ToT and will go into 2.1.9-dev. I will keep this issue open until the relevant fixes have been merged to the 2.0 branch.

ap...@google.com <ap...@google.com> #11Feb 27, 2020 05:42PM

Project: r8
Branch: 2.0

commit 87645e540f2f79dc06013fdd441a14417a1eef6a
Author: Christoffer Quist Adamsen <christofferqa@google.com>
Date: Thu Feb 27 12:59:53 2020

Version 2.0.40

Cherry pick: Fix test for keeping annotated items
CL:

https://r8-review.googlesource.com/c/r8/+/49024

Cherry pick: Add a test for keeping annotated items
CL:

https://r8-review.googlesource.com/c/r8/+/48964

Cherry pick: Unify ways to relax assertion/warning for inner class name.
CL:

https://r8-review.googlesource.com/c/r8/+/49060

Bug: 149729626
Change-Id: I87169164acde50b43567a6e2e8e0d5cae60885a2

M src/main/java/com/android/tools/r8/R8.java
M src/main/java/com/android/tools/r8/Version.java
M src/main/java/com/android/tools/r8/graph/DexClass.java
M src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/LambdaMerger.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/kotlin/JStyleLambdaGroup.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/kotlin/JStyleLambdaGroupIdFactory.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/kotlin/KStyleLambdaGroup.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/kotlin/KStyleLambdaGroupIdFactory.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/kotlin/KotlinLambdaGroupId.java
M src/main/java/com/android/tools/r8/ir/optimize/lambda/kotlin/KotlinLambdaGroupIdFactory.java
M src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
M src/main/java/com/android/tools/r8/naming/MinifiedRenaming.java
M src/main/java/com/android/tools/r8/naming/signature/GenericSignatureRewriter.java
A src/main/java/com/android/tools/r8/shaking/AnnotationMatchResult.java
M src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
A src/main/java/com/android/tools/r8/shaking/ConsequentRootSetBuilder.java
M src/main/java/com/android/tools/r8/shaking/Enqueuer.java
M src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java
M src/main/java/com/android/tools/r8/shaking/ProguardMemberRule.java
M src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
M src/main/java/com/android/tools/r8/utils/InternalOptions.java
M src/test/java/com/android/tools/r8/shaking/annotations/B149729626.java

https://r8-review.googlesource.com/49200

ap...@google.com <ap...@google.com> #12Feb 27, 2020 05:42PM

Project: r8
Branch: 2.0

commit 7e8955152326cf60d772eca65803326eff56eb02
Author: Christoffer Quist Adamsen <christofferqa@google.com>
Date: Thu Feb 27 08:16:56 2020

Version 2.0.39

Cherry pick: Mark types with dependent instance constructor as instantiated
CL:

https://r8-review.googlesource.com/c/r8/+/49085

Cherry pick: Also consider referenced fields in if rule evaluator
CL:

https://r8-review.googlesource.com/c/r8/+/49089

Bug: 150189783, 149729626, 147972078
Change-Id: I52855f85275708631a765f2c806cda7db82fd9d7

M src/main/java/com/android/tools/r8/Version.java
M src/main/java/com/android/tools/r8/shaking/Enqueuer.java
M src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java
M src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
M src/test/java/com/android/tools/r8/TestShrinkerBuilder.java
M src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTestRunner.java
A src/test/java/com/android/tools/r8/shaking/annotations/B149729626.java
M src/test/java/com/android/tools/r8/shaking/ifrule/ConditionalKeepIfKeptTest.java
A src/test/java/com/android/tools/r8/shaking/ifrule/KeepClassesWithAnnotatedFieldsTest.java

https://r8-review.googlesource.com/49180

ch...@google.com <ch...@google.com> #13Feb 27, 2020 05:43PM

Marked as fixed.

This is fixed in 2.0.40 and will also go into 2.1.9-dev.

ap...@google.com <ap...@google.com> #14Mar 2, 2020 08:05AM

Project: r8
Branch: 1.6

commit d3f778aefbac3b9b82f4cb7cb663b0971a402931
Author: Christoffer Quist Adamsen <christofferqa@google.com>
Date: Sun Mar 01 12:20:38 2020

Version 1.6.79

Cherry pick: Mark types with dependent instance constructor as instantiated
CL:

https://r8-review.googlesource.com/c/r8/+/49085

Change-Id: Ieab0d65eff4bf2e2d161c0598ecc013a738ccac5
Bug: 149729626, 147972078

M src/main/java/com/android/tools/r8/Version.java
M src/main/java/com/android/tools/r8/shaking/Enqueuer.java
M src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
M src/test/java/com/android/tools/r8/compatproguard/CompatKeepClassMemberNamesTestRunner.java
M src/test/java/com/android/tools/r8/shaking/ifrule/ConditionalKeepIfKeptTest.java

https://r8-review.googlesource.com/49323