Improve the precision of lookupSingleInterfaceTarget()
Bug: 134649660, 145271928, 142116551
Change-Id: I1d732ed69347c51a75d77849eb9103ab575c209e
diff --git a/src/main/java/com/android/tools/r8/graph/DexClass.java b/src/main/java/com/android/tools/r8/graph/DexClass.java
index c340ae7..78ba461 100644
--- a/src/main/java/com/android/tools/r8/graph/DexClass.java
+++ b/src/main/java/com/android/tools/r8/graph/DexClass.java
@@ -626,6 +626,10 @@
return accessFlags.isAbstract();
}
+ public boolean isAnnotation() {
+ return accessFlags.isAnnotation();
+ }
+
public boolean isFinal() {
return accessFlags.isFinal();
}
diff --git a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
index f21bf79..0d32dcf 100644
--- a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
+++ b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
@@ -688,17 +688,19 @@
return true;
}
- public boolean isInstantiatedDirectly(DexProgramClass clazz) {
+ private boolean isInstantiatedDirectly(DexProgramClass clazz) {
assert checkIfObsolete();
DexType type = clazz.type;
return type.isD8R8SynthesizedClassType()
|| instantiatedTypes.contains(type)
- || instantiatedLambdas.contains(type)
|| instantiatedAnnotationTypes.contains(type);
}
public boolean isInstantiatedIndirectly(DexProgramClass clazz) {
assert checkIfObsolete();
+ if (hasAnyInstantiatedLambdas(clazz)) {
+ return true;
+ }
DexType type = clazz.type;
synchronized (indirectlyInstantiatedTypes) {
if (indirectlyInstantiatedTypes.containsKey(type)) {
@@ -832,28 +834,47 @@
return pinnedItems.contains(reference);
}
- public boolean isMethodPinnedDirectlyOrInAncestor(DexMethod method) {
- // Look in all ancestor types.
- DexClass currentClass = definitionFor(method.holder);
- if (currentClass == null || !currentClass.isProgramClass()) {
- return false;
+ private boolean canVirtualMethodBeImplementedInExtraSubclass(
+ DexProgramClass clazz, DexMethod method) {
+ // For functional interfaces that are instantiated by lambdas, we may not have synthesized all
+ // the lambda classes yet, and therefore the set of subtypes for the holder may still be
+ // incomplete.
+ if (hasAnyInstantiatedLambdas(clazz)) {
+ return true;
}
- Set<DexType> visited = SetUtils.newIdentityHashSet(currentClass.allImmediateSupertypes());
- Deque<DexType> worklist = new ArrayDeque<>(visited);
- while (!worklist.isEmpty()) {
- DexType type = worklist.removeFirst();
- assert visited.contains(type);
- DexClass clazz = definitionFor(type);
- if (clazz == null || !clazz.isProgramClass()) {
- continue;
+ // If `clazz` is kept and `method` is a library method or a library method override, then it is
+ // possible to create a class that inherits from `clazz` and overrides the library method.
+ // Similarly, if `clazz` is kept and `method` is kept directly on `clazz` or indirectly on one
+ // of its supertypes, then it is possible to create a class that inherits from `clazz` and
+ // overrides the kept method.
+ if (isPinned(clazz.type)) {
+ ResolutionResult resolutionResult = resolveMethod(clazz, method);
+ if (resolutionResult.hasSingleTarget()) {
+ DexEncodedMethod resolutionTarget = resolutionResult.getSingleTarget();
+ return !resolutionTarget.isProgramMethod(this)
+ || resolutionTarget.isLibraryMethodOverride().isPossiblyTrue()
+ || isVirtualMethodPinnedDirectlyOrInAncestor(clazz, method);
}
+ }
+ return false;
+ }
+
+ private boolean isVirtualMethodPinnedDirectlyOrInAncestor(
+ DexProgramClass currentClass, DexMethod method) {
+ // Look in all ancestor types, including `currentClass` itself.
+ Set<DexProgramClass> visited = SetUtils.newIdentityHashSet(currentClass);
+ Deque<DexProgramClass> worklist = new ArrayDeque<>(visited);
+ while (!worklist.isEmpty()) {
+ DexClass clazz = worklist.removeFirst();
+ assert visited.contains(clazz);
DexEncodedMethod methodInClass = clazz.lookupVirtualMethod(method);
if (methodInClass != null && isPinned(methodInClass.method)) {
return true;
}
for (DexType superType : clazz.allImmediateSupertypes()) {
- if (visited.add(superType)) {
- worklist.addLast(superType);
+ DexProgramClass superClass = asProgramClassOrNull(definitionFor(superType));
+ if (superClass != null && visited.add(superClass)) {
+ worklist.addLast(superClass);
}
}
}
@@ -1069,8 +1090,8 @@
DexEncodedMethod candidate,
boolean candidateIsReachable,
boolean checkForInterfaceConflicts) {
- // For kept types we do not know all subtypes, so abort if the method is also kept.
- if (isPinned(clazz.type) && isMethodPinnedDirectlyOrInAncestor(candidate.method)) {
+ // If the invoke could target a method in a class that is not visible to R8, then give up.
+ if (canVirtualMethodBeImplementedInExtraSubclass(clazz, method)) {
return DexEncodedMethod.SENTINEL;
}
// If the candidate is reachable, we already have a previous result.
@@ -1160,9 +1181,6 @@
if (directResult != null) {
return directResult;
}
- if (instantiatedLambdas.contains(method.holder)) {
- return null;
- }
// If the lower-bound on the receiver type is the same as the upper-bound, then we have exact
// runtime type information. In this case, the invoke will dispatch to the resolution result
@@ -1196,42 +1214,56 @@
if (topTarget == null || !SingleResolutionResult.isValidVirtualTarget(options(), topTarget)) {
return null;
}
- // For kept types we cannot ensure a single target.
- if (pinnedItems.contains(method.holder)) {
+
+ // If the invoke could target a method in a class that is not visible to R8, then give up.
+ if (canVirtualMethodBeImplementedInExtraSubclass(holder, method)) {
return null;
}
+
+ DexProgramClass refinedReceiverClass = definitionFor(refinedReceiverType).asProgramClass();
+ if (refinedReceiverClass == null) {
+ return null;
+ }
+
+ // For functional interfaces that are instantiated by lambdas, we may not have synthesized all
+ // the lambda classes yet, and therefore the set of subtypes for the holder may still be
+ // incomplete.
+ if (hasAnyInstantiatedLambdas(refinedReceiverClass)) {
+ return null;
+ }
+
+ Iterable<DexType> subtypesToExplore =
+ isInstantiatedDirectly(refinedReceiverClass)
+ ? Iterables.concat(ImmutableList.of(refinedReceiverType), subtypes(refinedReceiverType))
+ : subtypes(refinedReceiverType);
+
+ // The loop will ignore uninstantiated classes as they will not be a target at runtime.
DexEncodedMethod result = null;
- // The loop will ignore abstract classes that are not kept as they should not be a target
- // at runtime.
- Iterable<DexType> subTypesToExplore =
- refinedReceiverType == method.holder
- ? subtypes(method.holder)
- : Iterables.concat(
- ImmutableList.of(refinedReceiverType), subtypes(refinedReceiverType));
- for (DexType type : subTypesToExplore) {
- if (instantiatedLambdas.contains(type)) {
+ for (DexType type : subtypesToExplore) {
+ DexProgramClass clazz = definitionFor(type).asProgramClass();
+
+ // If the invoke could target a method in a class that is not visible to R8, then give up.
+ if (canVirtualMethodBeImplementedInExtraSubclass(clazz, method)) {
return null;
}
- if (pinnedItems.contains(type)) {
- // For kept classes we cannot ensure a single target.
+
+ if (!isInstantiatedDirectly(clazz)) {
+ // This is not a possible receiver at runtime.
+ continue;
+ }
+
+ // TODO(b/145344105): Abstract classes should never be considered instantiated.
+ // assert (!clazz.isAbstract() && !clazz.isInterface()) || clazz.isAnnotation();
+
+ DexEncodedMethod resolutionResult = resolveMethod(clazz, method).getSingleTarget();
+ if (resolutionResult == null || isInvalidSingleVirtualTarget(resolutionResult, topTarget)) {
+ // This will fail at runtime.
return null;
}
- DexClass clazz = definitionFor(type);
- if (clazz.isInterface()) {
- // Default methods are looked up when looking at a specific subtype that does not
- // override them, so we ignore interface methods here. Otherwise, we would look up
- // default methods that are factually never used.
- } else if (!clazz.accessFlags.isAbstract()) {
- DexEncodedMethod resolutionResult = resolveMethodOnClass(clazz, method).getSingleTarget();
- if (resolutionResult == null || isInvalidSingleVirtualTarget(resolutionResult, topTarget)) {
- // This will fail at runtime.
- return null;
- }
- if (result != null && result != resolutionResult) {
- return null;
- }
- result = resolutionResult;
+ if (result != null && result != resolutionResult) {
+ return null;
}
+ result = resolutionResult;
}
assert result == null || !isInvalidSingleVirtualTarget(result, topTarget);
return result == null || !result.isVirtualMethod() ? null : result;
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 9df6b42..245f3c6 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -2943,9 +2943,14 @@
DexProgramClass clazz = getProgramClassOrNull(type);
if (clazz != null && clazz.isInterface()) {
// Add this interface to the set of pinned items to ensure that we do not merge the
- // interface into its subtype and to ensure that the devirtualizer does not perform illegal
- // rewritings of invoke-interface instructions into invoke-virtual instructions.
+ // interface into its unique subtype, if any.
pinnedItems.add(clazz.type);
+
+ // Also pin all of its virtual methods to ensure that the devirtualizer does not perform
+ // illegal rewritings of invoke-interface instructions into invoke-virtual instructions.
+ for (DexEncodedMethod virtualMethod : clazz.virtualMethods()) {
+ pinnedItems.add(virtualMethod.method);
+ }
}
}
}
diff --git a/src/test/java/com/android/tools/r8/classmerging/InterfaceWithProxyTest.java b/src/test/java/com/android/tools/r8/classmerging/InterfaceWithProxyTest.java
index 912f44a..8284d3f 100644
--- a/src/test/java/com/android/tools/r8/classmerging/InterfaceWithProxyTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/InterfaceWithProxyTest.java
@@ -22,7 +22,7 @@
@Parameterized.Parameters(name = "{0}")
public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimes().build();
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
}
public InterfaceWithProxyTest(TestParameters parameters) {
@@ -37,7 +37,7 @@
.addKeepMainRule(TestClass.class)
.enableClassInliningAnnotations()
.enableInliningAnnotations()
- .setMinApi(parameters.getRuntime())
+ .setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), TestClass.class)
.assertSuccessWithOutput(expectedOutput);
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/extrasubclasses/AbstractClassAlsoImplementedByMissingClassTest.java b/src/test/java/com/android/tools/r8/ir/optimize/extrasubclasses/AbstractClassAlsoImplementedByMissingClassTest.java
index 722ac77..d1d5d5f 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/extrasubclasses/AbstractClassAlsoImplementedByMissingClassTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/extrasubclasses/AbstractClassAlsoImplementedByMissingClassTest.java
@@ -7,7 +7,6 @@
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.core.StringContains.containsString;
import com.android.tools.r8.NeverClassInline;
import com.android.tools.r8.TestBase;
@@ -63,7 +62,7 @@
.addProgramClasses(C.class, Helper.class)
.addRunClasspathFiles(r8Out)
.run(parameters.getRuntime(), TestClass.class)
- .assertFailureWithErrorThatMatches(containsString(ClassCastException.class.getTypeName()));
+ .assertSuccessWithOutputLines("Hello world!", "The end");
}
private void inspect(CodeInspector inspector) {
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/extrasubclasses/InterfaceAlsoImplementedByMissingClassTest.java b/src/test/java/com/android/tools/r8/ir/optimize/extrasubclasses/InterfaceAlsoImplementedByMissingClassTest.java
index fc722c8..818b1eb 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/extrasubclasses/InterfaceAlsoImplementedByMissingClassTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/extrasubclasses/InterfaceAlsoImplementedByMissingClassTest.java
@@ -5,6 +5,7 @@
package com.android.tools.r8.ir.optimize.extrasubclasses;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import com.android.tools.r8.NeverClassInline;
@@ -69,10 +70,10 @@
ClassSubject aClassSubject = inspector.clazz(A.class);
assertThat(aClassSubject.uniqueMethodWithName("kept"), isPresent());
- // TODO(b/134649660): I.notKept() and A.notKept() should not be present, because the only invoke
- // instruction targeting I.notKept() should have been inlined.
- assertThat(iClassSubject.uniqueMethodWithName("notKept"), isPresent());
- assertThat(aClassSubject.uniqueMethodWithName("notKept"), isPresent());
+ // I.notKept() and A.notKept() should not be present, because the only invoke instruction
+ // targeting I.notKept() should have been inlined.
+ assertThat(iClassSubject.uniqueMethodWithName("notKept"), not(isPresent()));
+ assertThat(aClassSubject.uniqueMethodWithName("notKept"), not(isPresent()));
}
static class TestClass {
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/InlineFunctionalInterfaceMethodImplementedByLambdasTest.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/InlineFunctionalInterfaceMethodImplementedByLambdasTest.java
index ed54346..4a9025a 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/InlineFunctionalInterfaceMethodImplementedByLambdasTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/InlineFunctionalInterfaceMethodImplementedByLambdasTest.java
@@ -5,6 +5,7 @@
package com.android.tools.r8.ir.optimize.inliner;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import com.android.tools.r8.NeverClassInline;
@@ -45,12 +46,13 @@
private void inspect(CodeInspector inspector) {
if (parameters.isDexRuntime()) {
- // TODO(b/145271928): Should be able to inline I.m().
+ assertThat(inspector.clazz(I.class), not(isPresent()));
+ } else {
+ // Used by the invoke-custom instruction.
assertThat(inspector.clazz(I.class), isPresent());
}
- // TODO(b/145271928): Should be able to inline A.m().
- assertThat(inspector.clazz(A.class), isPresent());
+ assertThat(inspector.clazz(A.class), not(isPresent()));
}
static class TestClass {
