Collect dependencies of all kept annotations and add with reason
All references for all annotations that are kept should be added to
live types, otherwise we will see errors when combined with
-keepattributes Exceptions.
There should be no need to keep library annotations and mark
references as live types for library methods, classes and fields.
It is a bit weird that throwing is treated as an annotation in our
shared representation - refactoring this have a feature request here:
b/124218160.
Further, when marking a type as live, we should give a reason for
keeping it such that we can investigate live-type relationships in the
graph inspector: b/124217402.
Bug: 124019003
Change-Id: I1b877eb5c4adedd662c4f629bfe17b484e3fa1f5
diff --git a/src/main/java/com/android/tools/r8/experimental/graphinfo/GraphEdgeInfo.java b/src/main/java/com/android/tools/r8/experimental/graphinfo/GraphEdgeInfo.java
index e3fa01b..88b3b4b 100644
--- a/src/main/java/com/android/tools/r8/experimental/graphinfo/GraphEdgeInfo.java
+++ b/src/main/java/com/android/tools/r8/experimental/graphinfo/GraphEdgeInfo.java
@@ -15,6 +15,7 @@
TargetedBySuper,
InvokedFrom,
InvokedFromLambdaCreatedIn,
+ AnnotatedOn,
ReferencedFrom,
ReflectiveUseFrom,
ReachableFromLiveType,
diff --git a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
index a51ca83..f011931 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -177,10 +177,10 @@
* for these.
*/
private final Set<DexType> liveTypes = Sets.newIdentityHashSet();
- /**
- * Set of annotation types that are instantiated.
- */
- private final Set<DexType> instantiatedAnnotations = Sets.newIdentityHashSet();
+
+ /** Set of annotation types that are instantiated. */
+ private final SetWithReason<DexAnnotation> instantiatedAnnotations =
+ new SetWithReason<>(this::registerAnnotation);
/** Set of types that are actually instantiated. These cannot be abstract. */
private final SetWithReason<DexType> instantiatedTypes = new SetWithReason<>(this::registerType);
/**
@@ -803,9 +803,6 @@
}
}
}
- if (!holder.annotations.isEmpty()) {
- processAnnotations(holder.annotations.annotations);
- }
// We also need to add the corresponding <clinit> to the set of live methods, as otherwise
// static field initialization (and other class-load-time sideeffects) will not happen.
KeepReason reason = KeepReason.reachableFromLiveType(type);
@@ -821,10 +818,15 @@
enqueueFirstNonSerializableClassInitializer(holder, reason);
}
- // If this type has deferred annotations, we have to process those now, too.
- Set<DexAnnotation> annotations = deferredAnnotations.remove(type);
- if (annotations != null) {
- annotations.forEach(this::handleAnnotationOfLiveType);
+ if (!holder.isLibraryClass()) {
+ if (!holder.annotations.isEmpty()) {
+ processAnnotations(holder, holder.annotations.annotations);
+ }
+ // If this type has deferred annotations, we have to process those now, too.
+ Set<DexAnnotation> annotations = deferredAnnotations.remove(type);
+ if (annotations != null) {
+ annotations.forEach(annotation -> handleAnnotationOfLiveType(holder, annotation));
+ }
}
Map<DexReference, Set<ProguardKeepRule>> dependentItems = rootSet.getDependentItems(holder);
@@ -834,36 +836,33 @@
}
}
- private void handleAnnotationOfLiveType(DexAnnotation annotation) {
- DexType type = annotation.annotation.type;
- // Record that it is instantiated if it should be kept when its type is live.
- if (shouldKeepAnnotation(annotation, true, appInfo.dexItemFactory, options)) {
- instantiatedAnnotations.add(type);
- }
- AnnotationReferenceMarker referenceMarker = new AnnotationReferenceMarker(
- annotation.annotation.type, appInfo.dexItemFactory);
- annotation.annotation.collectIndexedItems(referenceMarker);
+ private void handleAnnotationOfLiveType(DexDefinition holder, DexAnnotation annotation) {
+ handleAnnotation(holder, annotation, true);
}
- private void processAnnotations(DexAnnotation[] annotations) {
+ private void processAnnotations(DexDefinition holder, DexAnnotation[] annotations) {
for (DexAnnotation annotation : annotations) {
- processAnnotation(annotation);
+ processAnnotation(holder, annotation);
}
}
- private void processAnnotation(DexAnnotation annotation) {
+ private void processAnnotation(DexDefinition holder, DexAnnotation annotation) {
+ handleAnnotation(holder, annotation, false);
+ }
+
+ private void handleAnnotation(DexDefinition holder, DexAnnotation annotation, boolean forceLive) {
+ assert !holder.isDexClass() || !holder.asDexClass().isLibraryClass();
DexType type = annotation.annotation.type;
- if (liveTypes.contains(type)) {
- // The type of this annotation is already live, so pick up its dependencies.
- handleAnnotationOfLiveType(annotation);
- } else {
- // Record that it is instantiated if it should be kept although its type is not live.
- if (shouldKeepAnnotation(annotation, false, appInfo.dexItemFactory, options)) {
- instantiatedAnnotations.add(type);
- }
+ boolean isLive = forceLive || liveTypes.contains(type);
+ if (!shouldKeepAnnotation(annotation, isLive, appInfo.dexItemFactory, options)) {
// Remember this annotation for later.
deferredAnnotations.computeIfAbsent(type, ignore -> new HashSet<>()).add(annotation);
+ return;
}
+ instantiatedAnnotations.add(annotation, KeepReason.annotatedOn(holder));
+ AnnotationReferenceMarker referenceMarker =
+ new AnnotationReferenceMarker(annotation.annotation.type, appInfo.dexItemFactory);
+ annotation.annotation.collectIndexedItems(referenceMarker);
}
private void handleInvokeOfStaticTarget(DexMethod method, KeepReason reason) {
@@ -956,8 +955,11 @@
}
markTypeAsLive(method.method.holder);
markParameterAndReturnTypesAsLive(method);
- processAnnotations(method.annotations.annotations);
- method.parameterAnnotationsList.forEachAnnotation(this::processAnnotation);
+ if (!appInfo.definitionFor(method.method.holder).isLibraryClass()) {
+ processAnnotations(method, method.annotations.annotations);
+ method.parameterAnnotationsList.forEachAnnotation(
+ annotation -> processAnnotation(method, annotation));
+ }
if (Log.ENABLED) {
Log.verbose(getClass(), "Method `%s` is targeted.", method.method);
}
@@ -1614,8 +1616,11 @@
}
}
markParameterAndReturnTypesAsLive(method);
- processAnnotations(method.annotations.annotations);
- method.parameterAnnotationsList.forEachAnnotation(this::processAnnotation);
+ if (!appInfo.definitionFor(method.method.holder).isLibraryClass()) {
+ processAnnotations(method, method.annotations.annotations);
+ method.parameterAnnotationsList.forEachAnnotation(
+ annotation -> processAnnotation(method, annotation));
+ }
method.registerCodeReferences(new UseRegistry(options.itemFactory, method));
// Add all dependent members to the workqueue.
enqueueRootItems(rootSet.getDependentItems(method));
@@ -2013,8 +2018,11 @@
super(appInfo);
this.liveTypes = ImmutableSortedSet.copyOf(
PresortedComparable<DexType>::slowCompareTo, enqueuer.liveTypes);
- this.instantiatedAnnotations = ImmutableSortedSet.copyOf(
- PresortedComparable<DexType>::slowCompareTo, enqueuer.instantiatedAnnotations);
+ ImmutableSortedSet.Builder<DexType> builder =
+ ImmutableSortedSet.orderedBy(PresortedComparable<DexType>::slowCompareTo);
+ enqueuer.instantiatedAnnotations.items.forEach(
+ annotation -> builder.add(annotation.annotation.type));
+ this.instantiatedAnnotations = builder.build();
this.instantiatedTypes = ImmutableSortedSet.copyOf(
PresortedComparable<DexType>::slowCompareTo, enqueuer.instantiatedTypes.getItems());
this.instantiatedLambdas =
@@ -2795,6 +2803,14 @@
registerEdge(getClassGraphNode(type), reason);
}
+ private void registerAnnotation(DexAnnotation annotation, KeepReason reason) {
+ assert getSourceNode(reason) != null;
+ if (keptGraphConsumer == null) {
+ return;
+ }
+ registerEdge(getAnnotationGraphNode(annotation.annotation.type), reason);
+ }
+
private void registerMethod(DexEncodedMethod method, KeepReason reason) {
if (reason.edgeKind() == EdgeKind.IsLibraryMethod) {
// Don't report edges to actual library methods.
diff --git a/src/main/java/com/android/tools/r8/shaking/KeepReason.java b/src/main/java/com/android/tools/r8/shaking/KeepReason.java
index a797742..2dd52eb 100644
--- a/src/main/java/com/android/tools/r8/shaking/KeepReason.java
+++ b/src/main/java/com/android/tools/r8/shaking/KeepReason.java
@@ -6,6 +6,7 @@
import com.android.tools.r8.experimental.graphinfo.GraphEdgeInfo;
import com.android.tools.r8.experimental.graphinfo.GraphEdgeInfo.EdgeKind;
import com.android.tools.r8.experimental.graphinfo.GraphNode;
+import com.android.tools.r8.graph.DexDefinition;
import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexItem;
import com.android.tools.r8.graph.DexType;
@@ -17,6 +18,10 @@
public abstract GraphNode getSourceNode(Enqueuer enqueuer);
+ static KeepReason annotatedOn(DexDefinition definition) {
+ return new AnnotatedOn(definition);
+ }
+
static KeepReason dueToKeepRule(ProguardKeepRule rule) {
return new DueToKeepRule(rule);
}
@@ -302,6 +307,32 @@
}
}
+ private static class AnnotatedOn extends KeepReason {
+
+ private final DexDefinition holder;
+
+ private AnnotatedOn(DexDefinition holder) {
+ this.holder = holder;
+ }
+
+ @Override
+ public EdgeKind edgeKind() {
+ return EdgeKind.AnnotatedOn;
+ }
+
+ @Override
+ public GraphNode getSourceNode(Enqueuer enqueuer) {
+ if (holder.isDexClass()) {
+ return enqueuer.getClassGraphNode(holder.asDexClass().type);
+ } else if (holder.isDexEncodedField()) {
+ return enqueuer.getFieldGraphNode(holder.asDexEncodedField().field);
+ } else {
+ assert holder.isDexEncodedMethod();
+ return enqueuer.getMethodGraphNode(holder.asDexEncodedMethod().method);
+ }
+ }
+ }
+
private static class ReflectiveUseFrom extends BasedOnOtherMethod {
private ReflectiveUseFrom(DexEncodedMethod method) {
diff --git a/src/test/java/com/android/tools/r8/shaking/UnusedTypeInThrowingTest.java b/src/test/java/com/android/tools/r8/shaking/UnusedTypeInThrowingTest.java
index 6a03a43..d199f57 100644
--- a/src/test/java/com/android/tools/r8/shaking/UnusedTypeInThrowingTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/UnusedTypeInThrowingTest.java
@@ -4,16 +4,21 @@
package com.android.tools.r8.shaking;
+import static junit.framework.TestCase.assertTrue;
+
import com.android.tools.r8.CompilationFailedException;
import com.android.tools.r8.TestBase;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
import java.io.IOException;
-import org.junit.Ignore;
+import java.util.concurrent.ExecutionException;
import org.junit.Test;
class UnusedTypeInThrowing {
+ public static final String EXPECTED = "42";
+
public static void main(String[] args) throws UnusedTypeInThrowingThrowable {
- System.out.print("42");
+ System.out.print(EXPECTED);
}
}
@@ -25,14 +30,20 @@
static final Class MAIN_CLASS = UnusedTypeInThrowing.class;
@Test
- @Ignore("b/124019003")
- public void testTypeIsMarkedAsLive() throws IOException, CompilationFailedException {
- testForR8(Backend.CF)
- .addProgramClasses(MAIN_CLASS)
- .addProgramClasses(THROWABLE_CLASS)
- .addKeepMainRule(MAIN_CLASS)
- .addKeepRules("-keepattributes Exceptions")
- .run(MAIN_CLASS)
- .assertSuccessWithOutput("42");
+ public void testTypeIsMarkedAsLive()
+ throws IOException, CompilationFailedException, ExecutionException, NoSuchMethodException {
+ CodeInspector inspector =
+ testForR8(Backend.CF)
+ .enableGraphInspector()
+ .addProgramClasses(MAIN_CLASS)
+ .addProgramClasses(THROWABLE_CLASS)
+ .addKeepMainRule(MAIN_CLASS)
+ .addKeepRules("-keepattributes Exceptions")
+ .run(MAIN_CLASS)
+ .assertSuccessWithOutput(UnusedTypeInThrowing.EXPECTED)
+ .inspector();
+
+ assertTrue(inspector.clazz(THROWABLE_CLASS).isPresent());
+ // TODO(b/124217402) When done check that THROWABLE_CLASS is kept by the throwing annotation.
}
}
diff --git a/src/test/java/com/android/tools/r8/shaking/UnusedTypeInThrowingTestRunner.java b/src/test/java/com/android/tools/r8/shaking/UnusedTypeInThrowingTestRunner.java
deleted file mode 100644
index 2bf42a1..0000000
--- a/src/test/java/com/android/tools/r8/shaking/UnusedTypeInThrowingTestRunner.java
+++ /dev/null
@@ -1,37 +0,0 @@
-// Copyright (c) 2019, the R8 project authors. Please see the AUTHORS file
-// for details. All rights reserved. Use of this source code is governed by a
-// BSD-style license that can be found in the LICENSE file.
-
-package com.android.tools.r8.shaking;
-
-import com.android.tools.r8.CompilationFailedException;
-import com.android.tools.r8.CompilationMode;
-import com.android.tools.r8.TestBase;
-import com.google.common.collect.ImmutableList;
-import java.io.IOException;
-import java.nio.file.Path;
-import org.junit.Ignore;
-import org.junit.Test;
-
-public class UnusedTypeInThrowingTestRunner extends TestBase {
-
- static final Class THROWABLE_CLASS = UnusedTypeInThrowingThrowable.class;
- static final Class MAIN_CLASS = UnusedTypeInThrowingTest.class;
-
- @Test
- @Ignore("b/124019003")
- public void testTypeIsMarkedAsLive() throws IOException, CompilationFailedException {
- Path outDex = temp.newFile("out.zip").toPath();
- testForR8(Backend.CF)
- .addProgramClasses(MAIN_CLASS)
- .addProgramClasses(THROWABLE_CLASS)
- .addKeepMainRule(MAIN_CLASS)
- .addKeepRules(ImmutableList.of("-keepattributes Exceptions"))
- .setMode(CompilationMode.RELEASE)
- .enableInliningAnnotations()
- .minification(true)
- .compile()
- .run(MAIN_CLASS)
- .assertSuccessWithOutput("42");
- }
-}
