Fix rewriting of @AnnotationDefault annotations
This fixes an issue where @AnnotationDefault annotations were not correctly rewritten in presence of enum unboxing.
This also extends the annotation remover so that it removes @AnnotationDefault annotation elements corresponding to methods removed by tree shaking.
Fixes: b/448634317
Bug: b/191741002
Change-Id: If54cef675d27657024fa520ad06321bc25d50feb
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java
index 905267d..3154d72 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java
@@ -107,6 +107,7 @@
import com.android.tools.r8.ir.optimize.info.MutableMethodOptimizationInfo;
import com.android.tools.r8.ir.optimize.info.OptimizationFeedback.OptimizationInfoFixer;
import com.android.tools.r8.ir.optimize.info.OptimizationFeedbackDelayed;
+import com.android.tools.r8.shaking.AnnotationFixer;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.android.tools.r8.shaking.KeepInfoCollection;
import com.android.tools.r8.utils.Reporter;
@@ -722,6 +723,11 @@
.fixupTypeReferences(converter, executorService, timing);
EnumUnboxingLens enumUnboxingLens = treeFixerResult.getLens();
+ // Run annotation fixer so that enum field references in @AnnotationDefault annotations are
+ // correctly rewritten.
+ new AnnotationFixer(appView, enumUnboxingLens)
+ .run(appView.appInfo().classes(), executorService);
+
// Enqueue the (lens rewritten) methods that require reprocessing.
//
// Note that the reprocessing set must be rewritten to the new enum unboxing lens before pruning
diff --git a/src/main/java/com/android/tools/r8/shaking/AnnotationFixer.java b/src/main/java/com/android/tools/r8/shaking/AnnotationFixer.java
index 3dea441..69b0f34 100644
--- a/src/main/java/com/android/tools/r8/shaking/AnnotationFixer.java
+++ b/src/main/java/com/android/tools/r8/shaking/AnnotationFixer.java
@@ -20,8 +20,10 @@
import com.android.tools.r8.graph.DexValue.DexValueAnnotation;
import com.android.tools.r8.graph.DexValue.DexValueArray;
import com.android.tools.r8.graph.DexValue.DexValueEnum;
+import com.android.tools.r8.graph.DexValue.DexValueInt;
import com.android.tools.r8.graph.DexValue.DexValueType;
import com.android.tools.r8.graph.lens.GraphLens;
+import com.android.tools.r8.ir.optimize.enums.EnumDataMap;
import com.android.tools.r8.utils.ArrayUtils;
import com.android.tools.r8.utils.ThreadUtils;
import java.util.Collection;
@@ -116,6 +118,12 @@
}
} else if (value.isDexValueEnum()) {
DexField original = value.asDexValueEnum().value;
+ if (lens.isEnumUnboxerLens()) {
+ EnumDataMap unboxedEnums = lens.asEnumUnboxerLens().getUnboxedEnums();
+ if (unboxedEnums.hasUnboxedValueFor(original)) {
+ return DexValueInt.create(unboxedEnums.getUnboxedValue(original));
+ }
+ }
DexField rewritten = lens.lookupField(original, annotationLens);
if (original != rewritten) {
return new DexValueEnum(rewritten);
diff --git a/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java b/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
index 5672c39..53d895b 100644
--- a/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
+++ b/src/main/java/com/android/tools/r8/shaking/AnnotationRemover.java
@@ -13,10 +13,13 @@
import com.android.tools.r8.graph.DexClass;
import com.android.tools.r8.graph.DexDefinition;
import com.android.tools.r8.graph.DexEncodedAnnotation;
+import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexItemFactory;
import com.android.tools.r8.graph.DexProgramClass;
+import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.graph.DexValue;
+import com.android.tools.r8.graph.DexValue.DexValueAnnotation;
import com.android.tools.r8.graph.EnclosingMethodAttribute;
import com.android.tools.r8.graph.InnerClassAttribute;
import com.android.tools.r8.graph.ProgramDefinition;
@@ -25,6 +28,7 @@
import com.android.tools.r8.kotlin.KotlinMemberLevelInfo;
import com.android.tools.r8.kotlin.KotlinPropertyInfo;
import com.android.tools.r8.shaking.Enqueuer.Mode;
+import com.android.tools.r8.utils.ArrayUtils;
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.ThreadUtils;
import com.google.common.collect.ImmutableList;
@@ -274,35 +278,64 @@
// Check if we should keep this annotation first.
if (filterAnnotations(holder, original, kind, keepInfo)) {
// Then, filter out values that refer to dead definitions.
- return original.rewrite(this::rewriteEncodedAnnotation);
+ return original.rewrite(annotation -> rewriteEncodedAnnotation(annotation, holder));
}
return null;
}
- private DexEncodedAnnotation rewriteEncodedAnnotation(DexEncodedAnnotation original) {
+ private DexEncodedAnnotation rewriteEncodedAnnotation(
+ DexEncodedAnnotation original, ProgramDefinition holder) {
GraphLens graphLens = appView.graphLens();
DexType annotationType = original.type.getBaseType();
DexType rewrittenType = graphLens.lookupType(annotationType);
DexEncodedAnnotation rewrite =
original.rewrite(
- graphLens::lookupType, element -> rewriteAnnotationElement(rewrittenType, element));
+ graphLens::lookupType,
+ element -> rewriteAnnotationElement(rewrittenType, element, holder));
assert rewrite != null;
- DexClass annotationClass = appView.appInfo().definitionFor(rewrittenType);
- assert annotationClass == null
+ assert appView.appInfo().definitionFor(rewrittenType) == null
|| appView.appInfo().isNonProgramTypeOrLiveProgramType(rewrittenType);
+ if (rewrite.getType().isIdenticalTo(appView.dexItemFactory().annotationDefault)
+ && rewrite.getNumberOfElements() == 0) {
+ return null;
+ }
return rewrite;
}
- @SuppressWarnings("ReferenceEquality")
private DexAnnotationElement rewriteAnnotationElement(
- DexType annotationType, DexAnnotationElement original) {
+ DexType annotationType, DexAnnotationElement original, ProgramDefinition holder) {
// The dalvik.annotation.AnnotationDefault is typically not on bootclasspath. However, if it
// is present, the definition does not define the 'value' getter but that is the spec:
// https://source.android.com/devices/tech/dalvik/dex-format#dalvik-annotation-default
- // If the annotation matches the structural requirement keep it.
- if (appView.dexItemFactory().annotationDefault.equals(annotationType)
- && appView.dexItemFactory().valueString.equals(original.name)) {
- return original;
+ // If the annotation matches the structural requirement keep it, but prune the default values
+ // that no longer match a method on the annotation class. If no default values remain after
+ // pruning, then remove the AnnotationDefault altogether.
+ DexString name = original.getName();
+ if (appView.dexItemFactory().annotationDefault.isIdenticalTo(annotationType)
+ && appView.dexItemFactory().valueString.isIdenticalTo(name)) {
+ DexEncodedAnnotation annotationValue = original.getValue().asDexValueAnnotation().getValue();
+ DexAnnotationElement[] elements = annotationValue.elements;
+ DexAnnotationElement[] rewrittenElements =
+ ArrayUtils.map(
+ elements,
+ element -> {
+ DexString innerName = element.getName();
+ DexEncodedMethod method =
+ holder
+ .getContextClass()
+ .lookupMethod(m -> m.getName().isIdenticalTo(innerName));
+ return method != null ? element : null;
+ },
+ DexAnnotationElement.EMPTY_ARRAY);
+ if (rewrittenElements == elements) {
+ return original;
+ }
+ return ArrayUtils.isEmpty(rewrittenElements)
+ ? null
+ : new DexAnnotationElement(
+ name,
+ new DexValueAnnotation(
+ new DexEncodedAnnotation(annotationValue.getType(), rewrittenElements)));
}
// We cannot strip annotations where we cannot look up the definition, because this will break
// apps that rely on the annotation to exist. See b/134766810 for more information.
@@ -314,7 +347,7 @@
boolean liveGetter =
definition
.getMethodCollection()
- .hasVirtualMethods(method -> method.getReference().name == original.name);
+ .hasVirtualMethods(method -> method.getName().isIdenticalTo(name));
return liveGetter ? original : null;
}
diff --git a/src/test/java/com/android/tools/r8/shaking/annotations/annotationdefault/DefaultValueForUnusedAnnotationShrinkingTest.java b/src/test/java/com/android/tools/r8/shaking/annotations/annotationdefault/DefaultValueForUnusedAnnotationShrinkingTest.java
index a01b251..fd581f5 100644
--- a/src/test/java/com/android/tools/r8/shaking/annotations/annotationdefault/DefaultValueForUnusedAnnotationShrinkingTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/annotations/annotationdefault/DefaultValueForUnusedAnnotationShrinkingTest.java
@@ -4,6 +4,7 @@
package com.android.tools.r8.shaking.annotations.annotationdefault;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
@@ -50,17 +51,16 @@
compileResult -> {
CodeInspector inspector = compileResult.inspector();
- // MyAnnotation has a @Retention annotation and an @AnnotationDefault annotation.
- // TODO(b/191741002): MyAnnotation.value() is unused, thus there is no need to retain
- // the @AnnotationDefault annotation.
+ // MyAnnotation has a @Retention annotation. The @AnnotationDefault annotation has
+ // been removed since MyAnnotation.value() is unused.
ClassSubject annotationClassSubject = inspector.clazz(MyAnnotation.class);
assertThat(annotationClassSubject, isPresent());
assertThat(
annotationClassSubject.annotation(Retention.class.getTypeName()), isPresent());
assertThat(
annotationClassSubject.annotation("dalvik.annotation.AnnotationDefault"),
- isPresent());
- assertEquals(2, annotationClassSubject.getDexProgramClass().annotations().size());
+ isAbsent());
+ assertEquals(1, annotationClassSubject.annotations().size());
compileResult
.run(parameters.getRuntime(), Main.class)
