Determine forwarding target using lookup virtual dispatch target.
Bug: 152163087
Change-Id: I4fd32fa47507dc0964061ff6b93b581b2c746267
diff --git a/src/main/java/com/android/tools/r8/graph/ResolutionResult.java b/src/main/java/com/android/tools/r8/graph/ResolutionResult.java
index e123de4..3de6525 100644
--- a/src/main/java/com/android/tools/r8/graph/ResolutionResult.java
+++ b/src/main/java/com/android/tools/r8/graph/ResolutionResult.java
@@ -45,6 +45,10 @@
return false;
}
+ public boolean isIncompatibleClassChangeErrorResult() {
+ return false;
+ }
+
/** Returns non-null if isFailedResolution() is true, otherwise null. */
public FailedResolutionResult asFailedResolution() {
return null;
@@ -101,7 +105,7 @@
InstantiatedObject instance, AppInfoWithClassHierarchy appInfo);
public abstract DexClassAndMethod lookupVirtualDispatchTarget(
- DexProgramClass dynamicInstance, AppInfoWithClassHierarchy appInfo);
+ DexClass dynamicInstance, AppInfoWithClassHierarchy appInfo);
public abstract LookupTarget lookupVirtualDispatchTarget(
LambdaDescriptor lambdaInstance, AppInfoWithClassHierarchy appInfo);
@@ -516,7 +520,7 @@
@Override
public DexClassAndMethod lookupVirtualDispatchTarget(
- DexProgramClass dynamicInstance, AppInfoWithClassHierarchy appInfo) {
+ DexClass dynamicInstance, AppInfoWithClassHierarchy appInfo) {
return lookupVirtualDispatchTarget(dynamicInstance, appInfo, initialResolutionHolder.type);
}
@@ -542,9 +546,7 @@
}
private DexClassAndMethod lookupVirtualDispatchTarget(
- DexProgramClass dynamicInstance,
- AppInfoWithClassHierarchy appInfo,
- DexType resolutionHolder) {
+ DexClass dynamicInstance, AppInfoWithClassHierarchy appInfo, DexType resolutionHolder) {
assert appInfo.isSubtype(dynamicInstance.type, resolutionHolder)
: dynamicInstance.type + " is not a subtype of " + resolutionHolder;
// TODO(b/148591377): Enable this assertion.
@@ -584,7 +586,7 @@
}
private DexClassAndMethod lookupMaximallySpecificDispatchTarget(
- DexProgramClass dynamicInstance, AppInfoWithClassHierarchy appInfo) {
+ DexClass dynamicInstance, AppInfoWithClassHierarchy appInfo) {
return appInfo.lookupMaximallySpecificMethod(dynamicInstance, resolvedMethod.method);
}
@@ -702,7 +704,7 @@
@Override
public DexClassAndMethod lookupVirtualDispatchTarget(
- DexProgramClass dynamicInstance, AppInfoWithClassHierarchy appInfo) {
+ DexClass dynamicInstance, AppInfoWithClassHierarchy appInfo) {
return null;
}
@@ -808,6 +810,11 @@
? INSTANCE
: new IncompatibleClassResult(methodsCausingError);
}
+
+ @Override
+ public boolean isIncompatibleClassChangeErrorResult() {
+ return true;
+ }
}
public static class NoSuchMethodResult extends FailedResolutionResult {
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/ClassProcessor.java b/src/main/java/com/android/tools/r8/ir/desugar/ClassProcessor.java
index ff5dd2f..82c7c6a 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/ClassProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/ClassProcessor.java
@@ -4,6 +4,7 @@
package com.android.tools.r8.ir.desugar;
+import com.android.tools.r8.errors.CompilationError;
import com.android.tools.r8.graph.AppInfoWithClassHierarchy;
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexAnnotationSet;
@@ -18,9 +19,9 @@
import com.android.tools.r8.graph.MethodAccessFlags;
import com.android.tools.r8.graph.ParameterAnnotationsList;
import com.android.tools.r8.graph.ResolutionResult;
-import com.android.tools.r8.graph.ResolutionResult.IncompatibleClassResult;
import com.android.tools.r8.ir.synthetic.ExceptionThrowingSourceCode;
import com.android.tools.r8.ir.synthetic.SynthesizedCode;
+import com.android.tools.r8.position.MethodPosition;
import com.android.tools.r8.utils.MethodSignatureEquivalence;
import com.google.common.base.Equivalence.Wrapper;
import com.google.common.collect.ImmutableList;
@@ -74,14 +75,6 @@
return signatures.size() == merged.size() ? this : new MethodSignatures(merged);
}
- MethodSignatures merge(List<MethodSignatures> others) {
- MethodSignatures merged = this;
- for (MethodSignatures other : others) {
- merged = merged.merge(others);
- }
- return merged;
- }
-
boolean isEmpty() {
return signatures.isEmpty();
}
@@ -151,7 +144,7 @@
}
}
- // Specialized context to disable reporting when traversing the library strucure.
+ // Specialized context to disable reporting when traversing the library structure.
private static class LibraryReportingContext extends ReportingContext {
static final LibraryReportingContext LIBRARY_CONTEXT = new LibraryReportingContext();
@@ -281,22 +274,36 @@
return ClassInfo.create(superInfo, additionalForwards.build());
}
- // Resolves a method signature from the point of 'clazz', if it must target a default method
+ // Looks up a method signature from the point of 'clazz', if it can dispatch to a default method
// the 'addForward' call-back is called with the target of the forward.
private void resolveForwardForSignature(
DexClass clazz, DexMethod method, BiConsumer<DexClass, DexEncodedMethod> addForward) {
- ResolutionResult resolution = appView.appInfo().resolveMethod(clazz, method);
- // If resolution fails, install a method throwing IncompatibleClassChangeError.
- if (resolution.isFailedResolution()) {
- if (resolution instanceof IncompatibleClassResult) {
+ // Resolve the default method at base type as the symbolic holder at call sites is not known.
+ // The dispatch target is then looked up from the possible "instance" class.
+ // Doing so can cause an invalid invoke to become valid (at runtime resolution at a subtype
+ // might have failed which is hidden by the insertion of the forward method). However, not doing
+ // so could cause valid dispatches to become invalid by resolving to private overrides.
+ DexClassAndMethod virtualDispatchTarget =
+ appView
+ .appInfo()
+ .resolveMethodOnInterface(method.holder, method)
+ .lookupVirtualDispatchTarget(clazz, appView.appInfo());
+ if (virtualDispatchTarget == null) {
+ // If no target is found due to multiple default method targets, preserve ICCE behavior.
+ ResolutionResult resolutionFromSubclass = appView.appInfo().resolveMethod(clazz, method);
+ if (resolutionFromSubclass.isIncompatibleClassChangeErrorResult()) {
addICCEThrowingMethod(method, clazz);
+ return;
}
+ assert resolutionFromSubclass.isFailedResolution()
+ || resolutionFromSubclass.getSingleTarget().isPrivateMethod();
return;
}
- DexEncodedMethod target = resolution.getSingleTarget();
- DexClass targetHolder = appView.definitionFor(target.holder());
+
+ DexEncodedMethod target = virtualDispatchTarget.getMethod();
+ DexClass targetHolder = virtualDispatchTarget.getHolder();
// Don-t forward if the target is explicitly marked as 'dont-rewrite'
- if (targetHolder == null || dontRewrite(targetHolder, target)) {
+ if (dontRewrite(targetHolder, target)) {
return;
}
@@ -375,6 +382,16 @@
if (!clazz.isProgramClass()) {
return;
}
+
+ DexEncodedMethod methodOnSelf = clazz.lookupMethod(target.method);
+ if (methodOnSelf != null) {
+ throw new CompilationError(
+ "Attempt to add forwarding method that conflicts with existing method.",
+ null,
+ clazz.getOrigin(),
+ new MethodPosition(methodOnSelf.method));
+ }
+
DexMethod method = target.method;
// NOTE: Never add a forwarding method to methods of classes unknown or coming from android.jar
// even if this results in invalid code, these classes are never desugared.
diff --git a/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringImpossibleTest.java b/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringImpossibleTest.java
new file mode 100644
index 0000000..88ede59
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringImpossibleTest.java
@@ -0,0 +1,128 @@
+// Copyright (c) 2020, 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.desugaring.interfacemethods;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assume.assumeTrue;
+
+import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestDiagnosticMessages;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.StringUtils;
+import com.google.common.collect.ImmutableList;
+import java.util.Collection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class DefaultInterfaceMethodDesugaringImpossibleTest extends TestBase {
+
+ private static final String EXPECTED = StringUtils.lines("I.m()");
+
+ private final TestParameters parameters;
+
+ @Parameterized.Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public DefaultInterfaceMethodDesugaringImpossibleTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ private Collection<Class<?>> getProgramClasses() {
+ return ImmutableList.of(TestClass.class, I.class);
+ }
+
+ private Collection<byte[]> getProgramClassData() throws Exception {
+ return ImmutableList.of(
+ transformer(A.class)
+ .setAccessFlags(
+ A.class.getDeclaredMethod("m"),
+ flags -> {
+ assert flags.isPublic();
+ flags.unsetPublic();
+ flags.setPrivate();
+ flags.setStatic();
+ })
+ .transform());
+ }
+
+ @Test
+ public void testJVM() throws Exception {
+ assumeTrue(parameters.isCfRuntime());
+ testForJvm()
+ .addProgramClasses(getProgramClasses())
+ .addProgramClassFileData(getProgramClassData())
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutput(EXPECTED);
+ }
+
+ @Test
+ public void testD8() throws Exception {
+ assumeTrue(parameters.isDexRuntime());
+ try {
+ testForD8()
+ .addProgramClasses(getProgramClasses())
+ .addProgramClassFileData(getProgramClassData())
+ .setMinApi(parameters.getApiLevel())
+ .compileWithExpectedDiagnostics(this::checkDesugarError)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutput(EXPECTED);
+ assertTrue(parameters.canUseDefaultAndStaticInterfaceMethods());
+ } catch (CompilationFailedException e) {
+ assertFalse(parameters.canUseDefaultAndStaticInterfaceMethods());
+ }
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ try {
+ testForR8(parameters.getBackend())
+ .addProgramClasses(getProgramClasses())
+ .addProgramClassFileData(getProgramClassData())
+ .addKeepAllClassesRule()
+ .setMinApi(parameters.getApiLevel())
+ .compileWithExpectedDiagnostics(this::checkDesugarError)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutput(EXPECTED);
+ assertTrue(parameters.canUseDefaultAndStaticInterfaceMethods());
+ } catch (CompilationFailedException e) {
+ assertFalse(parameters.canUseDefaultAndStaticInterfaceMethods());
+ }
+ }
+
+ private void checkDesugarError(TestDiagnosticMessages diagnostics) {
+ if (!parameters.canUseDefaultAndStaticInterfaceMethods()) {
+ diagnostics.assertErrorMessageThatMatches(containsString("forwarding method that conflicts"));
+ }
+ }
+
+ static class TestClass {
+
+ public static void main(String[] args) {
+ I a = new A();
+ a.m();
+ }
+ }
+
+ interface I {
+
+ default void m() {
+ System.out.println("I.m()");
+ }
+ }
+
+ static class A implements I {
+
+ public /* will be: private static */ void m() {
+ System.out.println("A.m()");
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithStaticResolutionInvokeVirtualTest.java b/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithStaticResolutionInvokeVirtualTest.java
index bcea446..a8c5c6f 100644
--- a/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithStaticResolutionInvokeVirtualTest.java
+++ b/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithStaticResolutionInvokeVirtualTest.java
@@ -106,11 +106,18 @@
result.assertFailureWithErrorThatThrows(IncompatibleClassChangeError.class);
return;
}
- // Then up to 6.0 the the runtime just ignores privates leading to incorrectly hitting I.m
+ // Then up to 6.0 the runtime just ignores privates leading to incorrectly hitting I.m
if (isDexOlderThanOrEqual(Version.V6_0_1)) {
result.assertSuccessWithOutput(EXPECTED);
return;
}
+ if (!unexpectedArtFailure() && !parameters.canUseDefaultAndStaticInterfaceMethods()) {
+ assert false : "Dead code until future ART behavior change. See b/152199517";
+ // Desugaring will insert a forwarding bridge which will hide the "invalid invoke" case.
+ // Thus, a future ART runtime that does not have the invalid IAE for the private override
+ // will end up calling the forward method to I.m.
+ result.assertSuccessWithOutput(EXPECTED);
+ }
// The expected behavior is IAE since the resolved method is private.
result.assertFailureWithErrorThatThrows(IllegalAccessError.class);
return;
diff --git a/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithStaticResolutionTest.java b/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithStaticResolutionTest.java
index bdd7960..c649c8d 100644
--- a/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithStaticResolutionTest.java
+++ b/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithStaticResolutionTest.java
@@ -1,13 +1,11 @@
package com.android.tools.r8.desugaring.interfacemethods;
-import static org.hamcrest.CoreMatchers.containsString;
import static org.junit.Assume.assumeTrue;
-import com.android.tools.r8.D8TestRunResult;
-import com.android.tools.r8.R8TestRunResult;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.StringUtils;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -15,6 +13,8 @@
@RunWith(Parameterized.class)
public class DefaultInterfaceMethodDesugaringWithStaticResolutionTest extends TestBase {
+ private static final String EXPECTED = StringUtils.lines("I.m()");
+
private final TestParameters parameters;
@Parameterized.Parameters(name = "{0}")
@@ -32,43 +32,29 @@
testForJvm()
.addTestClasspath()
.run(parameters.getRuntime(), TestClass.class)
- .assertSuccessWithOutputLines("I.m()");
+ .assertSuccessWithOutput(EXPECTED);
}
@Test
public void testD8() throws Exception {
assumeTrue(parameters.isDexRuntime());
- D8TestRunResult result =
- testForD8()
- .addInnerClasses(DefaultInterfaceMethodDesugaringWithStaticResolutionTest.class)
- .setMinApi(parameters.getApiLevel())
- .compile()
- .run(parameters.getRuntime(), TestClass.class);
- // TODO(b/152163087): Should always succeed with "I.m()".
- if (parameters.canUseDefaultAndStaticInterfaceMethods()) {
- result.assertSuccessWithOutputLines("I.m()");
- } else {
- result.assertFailureWithErrorThatMatches(
- containsString(AbstractMethodError.class.getTypeName()));
- }
+ testForD8()
+ .addInnerClasses(DefaultInterfaceMethodDesugaringWithStaticResolutionTest.class)
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutput(EXPECTED);
}
@Test
public void testR8() throws Exception {
- R8TestRunResult result =
- testForR8(parameters.getBackend())
- .addInnerClasses(DefaultInterfaceMethodDesugaringWithStaticResolutionTest.class)
- .addKeepAllClassesRule()
- .setMinApi(parameters.getApiLevel())
- .compile()
- .run(parameters.getRuntime(), TestClass.class);
- // TODO(b/152163087): Should always succeed with "I.m()".
- if (parameters.canUseDefaultAndStaticInterfaceMethods()) {
- result.assertSuccessWithOutputLines("I.m()");
- } else {
- result.assertFailureWithErrorThatMatches(
- containsString(AbstractMethodError.class.getTypeName()));
- }
+ testForR8(parameters.getBackend())
+ .addInnerClasses(DefaultInterfaceMethodDesugaringWithStaticResolutionTest.class)
+ .addKeepAllClassesRule()
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutput(EXPECTED);
}
static class TestClass {
