Predicates for distinguishing IllegalAccessOrNoSuchMethodResult
Change-Id: I29cd20af61d5bfd4c80e52206ab65b713a886406
diff --git a/src/main/java/com/android/tools/r8/graph/AccessControl.java b/src/main/java/com/android/tools/r8/graph/AccessControl.java
index 51ff1e2..cf2c789 100644
--- a/src/main/java/com/android/tools/r8/graph/AccessControl.java
+++ b/src/main/java/com/android/tools/r8/graph/AccessControl.java
@@ -25,15 +25,16 @@
public static OptionalBool isClassAccessible(
DexClass clazz,
- ProgramDefinition context,
+ Definition context,
ClassToFeatureSplitMap classToFeatureSplitMap,
SyntheticItems syntheticItems) {
if (!clazz.isPublic() && !clazz.getType().isSamePackage(context.getContextType())) {
return OptionalBool.FALSE;
}
if (clazz.isProgramClass()
+ && context.isProgramDefinition()
&& !classToFeatureSplitMap.isInBaseOrSameFeatureAs(
- clazz.asProgramClass(), context, syntheticItems)) {
+ clazz.asProgramClass(), context.asProgramDefinition(), syntheticItems)) {
return OptionalBool.UNKNOWN;
}
return OptionalBool.TRUE;
@@ -47,7 +48,7 @@
return isMemberAccessible(
resolutionResult.getResolutionPair(),
resolutionResult.getInitialResolutionHolder(),
- context,
+ context.getContextClass(),
appInfo);
}
@@ -56,13 +57,14 @@
DexClass initialResolutionHolder,
ProgramDefinition context,
AppView<? extends AppInfoWithClassHierarchy> appView) {
- return isMemberAccessible(member, initialResolutionHolder, context, appView.appInfo());
+ return isMemberAccessible(
+ member, initialResolutionHolder, context.getContextClass(), appView.appInfo());
}
- public static OptionalBool isMemberAccessible(
+ static OptionalBool isMemberAccessible(
DexClassAndMember<?, ?> member,
DexClass initialResolutionHolder,
- ProgramDefinition context,
+ DexClass context,
AppInfoWithClassHierarchy appInfo) {
AccessFlags<?> memberFlags = member.getDefinition().getAccessFlags();
OptionalBool classAccessibility =
@@ -78,25 +80,28 @@
return classAccessibility;
}
if (memberFlags.isPrivate()) {
- if (!isNestMate(member.getHolder(), context.getContextClass())) {
+ if (!isNestMate(member.getHolder(), context)) {
return OptionalBool.FALSE;
}
return classAccessibility;
}
- if (member.getHolderType().isSamePackage(context.getContextType())) {
+ if (member.getHolderType().isSamePackage(context.getType())) {
return classAccessibility;
}
- if (memberFlags.isProtected()
- && appInfo.isSubtype(context.getContextType(), member.getHolderType())) {
+ if (memberFlags.isProtected() && appInfo.isSubtype(context.getType(), member.getHolderType())) {
return classAccessibility;
}
return OptionalBool.FALSE;
}
- private static boolean isNestMate(DexClass clazz, DexProgramClass context) {
+ private static boolean isNestMate(DexClass clazz, DexClass context) {
if (clazz == context) {
return true;
}
+ if (context == null) {
+ assert false : "context should not be null";
+ return false;
+ }
if (!clazz.isInANest() || !context.isInANest()) {
return false;
}
diff --git a/src/main/java/com/android/tools/r8/graph/AppInfoWithClassHierarchy.java b/src/main/java/com/android/tools/r8/graph/AppInfoWithClassHierarchy.java
index 0056aff..2ee3013 100644
--- a/src/main/java/com/android/tools/r8/graph/AppInfoWithClassHierarchy.java
+++ b/src/main/java/com/android/tools/r8/graph/AppInfoWithClassHierarchy.java
@@ -711,7 +711,7 @@
// allowed because of nests, a NoSuchMethodError. Which error cannot be determined without
// knowing the calling context.
if (result.isPrivateMethod() && clazz != initialResolutionHolder) {
- return new IllegalAccessOrNoSuchMethodResult(result);
+ return new IllegalAccessOrNoSuchMethodResult(initialResolutionHolder, result);
}
return new SingleResolutionResult(initialResolutionHolder, clazz, result);
}
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 d846788..e3558a2 100644
--- a/src/main/java/com/android/tools/r8/graph/ResolutionResult.java
+++ b/src/main/java/com/android/tools/r8/graph/ResolutionResult.java
@@ -7,6 +7,7 @@
import com.android.tools.r8.ir.desugar.LambdaDescriptor;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.android.tools.r8.shaking.InstantiatedObject;
+import com.android.tools.r8.utils.BooleanBox;
import com.android.tools.r8.utils.Box;
import com.android.tools.r8.utils.OptionalBool;
import java.util.ArrayList;
@@ -60,6 +61,14 @@
return false;
}
+ public boolean isNoSuchMethodErrorResult(DexClass context, AppInfoWithClassHierarchy appInfo) {
+ return false;
+ }
+
+ public boolean isIllegalAccessErrorResult(DexClass context, AppInfoWithClassHierarchy appInfo) {
+ return false;
+ }
+
/** Returns non-null if isFailedResolution() is true, otherwise null. */
public FailedResolutionResult asFailedResolution() {
return null;
@@ -814,6 +823,10 @@
public boolean isVirtualTarget() {
return false;
}
+
+ public boolean hasMethodsCausingError() {
+ return false;
+ }
}
public static class ClassNotFoundResult extends FailedResolutionResult {
@@ -824,7 +837,7 @@
}
}
- abstract static class FailedResolutionWithCausingMethods extends FailedResolutionResult {
+ public abstract static class FailedResolutionWithCausingMethods extends FailedResolutionResult {
private final Collection<DexEncodedMethod> methodsCausingError;
@@ -836,6 +849,11 @@
public void forEachFailureDependency(Consumer<DexEncodedMethod> methodCausingFailureConsumer) {
this.methodsCausingError.forEach(methodCausingFailureConsumer);
}
+
+ @Override
+ public boolean hasMethodsCausingError() {
+ return methodsCausingError.size() > 0;
+ }
}
public static class IncompatibleClassResult extends FailedResolutionWithCausingMethods {
@@ -861,13 +879,70 @@
public static class NoSuchMethodResult extends FailedResolutionResult {
static final NoSuchMethodResult INSTANCE = new NoSuchMethodResult();
+
+ @Override
+ public boolean isNoSuchMethodErrorResult(DexClass context, AppInfoWithClassHierarchy appInfo) {
+ return true;
+ }
}
- public static class IllegalAccessOrNoSuchMethodResult extends FailedResolutionWithCausingMethods {
+ static class IllegalAccessOrNoSuchMethodResult extends FailedResolutionWithCausingMethods {
- public IllegalAccessOrNoSuchMethodResult(DexEncodedMethod methodCausingError) {
- super(Collections.singletonList(methodCausingError));
+ private final DexClass initialResolutionHolder;
+
+ public IllegalAccessOrNoSuchMethodResult(
+ DexClass initialResolutionHolder, Collection<DexEncodedMethod> methodsCausingError) {
+ super(methodsCausingError);
+ this.initialResolutionHolder = initialResolutionHolder;
+ }
+
+ public IllegalAccessOrNoSuchMethodResult(
+ DexClass initialResolutionHolder, DexEncodedMethod methodCausingError) {
+ this(initialResolutionHolder, Collections.singletonList(methodCausingError));
assert methodCausingError != null;
}
+
+ @Override
+ public boolean isIllegalAccessErrorResult(DexClass context, AppInfoWithClassHierarchy appInfo) {
+ if (!hasMethodsCausingError()) {
+ return false;
+ }
+ BooleanBox seenNoAccess = new BooleanBox(false);
+ forEachFailureDependency(
+ method -> {
+ DexClassAndMethod classAndMethod =
+ DexClassAndMethod.create(appInfo.definitionFor(method.getHolderType()), method);
+ seenNoAccess.or(
+ AccessControl.isMemberAccessible(
+ classAndMethod, initialResolutionHolder, context, appInfo)
+ .isFalse());
+ });
+ return seenNoAccess.get();
+ }
+
+ @Override
+ public boolean isNoSuchMethodErrorResult(DexClass context, AppInfoWithClassHierarchy appInfo) {
+ if (!hasMethodsCausingError()) {
+ return true;
+ }
+ if (isIllegalAccessErrorResult(context, appInfo)) {
+ return false;
+ }
+ // At this point we know we have methods causing errors but we have access to them. To be
+ // certain that this is the case where we have nest access but we are invoking a method with
+ // an incorrect symbolic reference, we directly test for it by having an assert.
+ assert verifyInvalidSymbolicReference();
+ return true;
+ }
+
+ private boolean verifyInvalidSymbolicReference() {
+ BooleanBox invalidSymbolicReference = new BooleanBox(true);
+ forEachFailureDependency(
+ method -> {
+ invalidSymbolicReference.and(
+ method.getHolderType() != initialResolutionHolder.getType());
+ });
+ return invalidSymbolicReference.get();
+ }
}
}
diff --git a/src/main/java/com/android/tools/r8/utils/BooleanBox.java b/src/main/java/com/android/tools/r8/utils/BooleanBox.java
index f6b790d..eff5807 100644
--- a/src/main/java/com/android/tools/r8/utils/BooleanBox.java
+++ b/src/main/java/com/android/tools/r8/utils/BooleanBox.java
@@ -45,4 +45,12 @@
public void unset() {
set(false);
}
+
+ public void and(boolean value) {
+ this.value = value && this.value;
+ }
+
+ public void or(boolean value) {
+ this.value = value || this.value;
+ }
}
diff --git a/src/test/java/com/android/tools/r8/TestParameters.java b/src/test/java/com/android/tools/r8/TestParameters.java
index 3bc2439..5a7423a 100644
--- a/src/test/java/com/android/tools/r8/TestParameters.java
+++ b/src/test/java/com/android/tools/r8/TestParameters.java
@@ -36,6 +36,13 @@
.isGreaterThanOrEqualTo(TestBase.apiLevelWithDefaultInterfaceMethodsSupport());
}
+ public boolean canUseDefaultAndStaticInterfaceMethodsWhenDesugaring() {
+ assert isCfRuntime() || isDexRuntime();
+ assert apiLevel != null;
+ return getApiLevel()
+ .isGreaterThanOrEqualTo(TestBase.apiLevelWithDefaultInterfaceMethodsSupport());
+ }
+
// Convenience predicates.
public boolean isDexRuntime() {
return runtime.isDex();
diff --git a/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithPublicStaticResolutionInvokeVirtualTest.java b/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithPublicStaticResolutionInvokeVirtualTest.java
index 6cfb572..b968127 100644
--- a/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithPublicStaticResolutionInvokeVirtualTest.java
+++ b/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithPublicStaticResolutionInvokeVirtualTest.java
@@ -33,7 +33,8 @@
@Parameterized.Parameters(name = "{0}, invalid:{1}")
public static List<Object[]> data() {
return buildParameters(
- getTestParameters().withAllRuntimesAndApiLevels().build(), BooleanUtils.values());
+ getTestParameters().withAllRuntimes().withAllApiLevelsAlsoForCf().build(),
+ BooleanUtils.values());
}
public DefaultInterfaceMethodDesugaringWithPublicStaticResolutionInvokeVirtualTest(
diff --git a/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithPublicStaticResolutionOnClassTest.java b/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithPublicStaticResolutionOnClassTest.java
index 6321064..1684c5a 100644
--- a/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithPublicStaticResolutionOnClassTest.java
+++ b/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithPublicStaticResolutionOnClassTest.java
@@ -27,7 +27,7 @@
@Parameterized.Parameters(name = "{0}")
public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimesAndApiLevels().build();
+ return getTestParameters().withAllRuntimes().withAllApiLevelsAlsoForCf().build();
}
public DefaultInterfaceMethodDesugaringWithPublicStaticResolutionOnClassTest(
@@ -64,8 +64,7 @@
@Test
public void testD8() throws Exception {
- assumeTrue(parameters.isDexRuntime());
- testForD8()
+ testForD8(parameters.getBackend())
.addProgramClasses(getProgramClasses())
.addProgramClassFileData(getProgramClassData())
.setMinApi(parameters.getApiLevel())
@@ -73,7 +72,7 @@
.run(parameters.getRuntime(), TestClass.class)
// TODO(b/182335909): Ideally, this should also throw ICCE when desugaring.
.applyIf(
- !parameters.canUseDefaultAndStaticInterfaceMethods()
+ !parameters.canUseDefaultAndStaticInterfaceMethodsWhenDesugaring()
|| parameters.isDexRuntimeVersion(Version.V7_0_0),
r -> r.assertSuccessWithOutput(EXPECTED_INVALID),
r -> r.assertFailureWithErrorThatThrows(IncompatibleClassChangeError.class));
diff --git a/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithPublicStaticResolutionTest.java b/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithPublicStaticResolutionTest.java
index fcf3080..c442b75 100644
--- a/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithPublicStaticResolutionTest.java
+++ b/src/test/java/com/android/tools/r8/desugaring/interfacemethods/DefaultInterfaceMethodDesugaringWithPublicStaticResolutionTest.java
@@ -25,7 +25,7 @@
@Parameterized.Parameters(name = "{0}")
public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimesAndApiLevels().build();
+ return getTestParameters().withAllRuntimes().withAllApiLevelsAlsoForCf().build();
}
public DefaultInterfaceMethodDesugaringWithPublicStaticResolutionTest(TestParameters parameters) {
@@ -61,8 +61,7 @@
@Test
public void testD8() throws Exception {
- assumeTrue(parameters.isDexRuntime());
- testForD8()
+ testForD8(parameters.getBackend())
.addProgramClasses(getProgramClasses())
.addProgramClassFileData(getProgramClassData())
.setMinApi(parameters.getApiLevel())
diff --git a/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodTest.java b/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodTest.java
index 75d4bbc..4d4419f 100644
--- a/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/VirtualOverrideOfPrivateStaticMethodTest.java
@@ -4,17 +4,17 @@
package com.android.tools.r8.resolution;
import static com.android.tools.r8.ToolHelper.getMostRecentAndroidJar;
-import static org.hamcrest.CoreMatchers.containsString;
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.TestParameters;
import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.ToolHelper.DexVm;
+import com.android.tools.r8.graph.DexClass;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.ResolutionResult;
-import com.android.tools.r8.graph.ResolutionResult.IllegalAccessOrNoSuchMethodResult;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.google.common.collect.ImmutableList;
import java.io.IOException;
@@ -30,17 +30,24 @@
public class VirtualOverrideOfPrivateStaticMethodTest extends TestBase {
public interface I {
- default void f() {}
+ default void f() {
+ System.out.println("I::f");
+ }
}
public static class A {
- private static void f() {}
+ private static void f() {
+ System.out.println("A::f");
+ }
}
public static class B extends A implements I {}
public static class C extends B {
- public void f() {}
+ @Override
+ public void f() {
+ System.out.println("C::f");
+ }
}
public static class Main {
@@ -64,13 +71,13 @@
.appInfo();
}
- private static DexMethod buildMethod(Class clazz, String name) {
+ private static DexMethod buildMethod(Class<?> clazz, String name) {
return buildNullaryVoidMethod(clazz, name, appInfo.dexItemFactory());
}
@Parameters(name = "{0}")
public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimes().withAllApiLevels().build();
+ return getTestParameters().withAllRuntimes().withAllApiLevelsAlsoForCf().build();
}
private final TestParameters parameters;
@@ -85,30 +92,48 @@
@Test
public void resolveTarget() {
ResolutionResult resolutionResult = appInfo.resolveMethodOnClass(methodOnB, methodOnB.holder);
- assertTrue(resolutionResult instanceof IllegalAccessOrNoSuchMethodResult);
+ DexClass context = appInfo.definitionFor(methodOnB.holder);
+ assertTrue(resolutionResult.isIllegalAccessErrorResult(context, appInfo));
}
@Test
- public void runTest() throws ExecutionException, CompilationFailedException, IOException {
- if (parameters.isCfRuntime()) {
- testForJvm()
- .addProgramClasses(CLASSES)
- .run(parameters.getRuntime(), Main.class)
- .assertFailureWithErrorThatMatches(containsString(expectedRuntimeError()));
- }
+ public void testJvm() throws Exception {
+ assumeTrue(parameters.isCfRuntime());
+ testForJvm()
+ .addProgramClasses(CLASSES)
+ .run(parameters.getRuntime(), Main.class)
+ .assertFailureWithErrorThatThrows(IllegalAccessError.class);
+ }
+
+ @Test
+ public void testD8() throws ExecutionException, CompilationFailedException, IOException {
+ testForD8(parameters.getBackend())
+ .addProgramClasses(CLASSES)
+ .setMinApi(parameters.getApiLevel())
+ .run(parameters.getRuntime(), Main.class)
+ // TODO(b/182335909): Ideally, this should IllegalAccessError.
+ .applyIf(
+ parameters.canUseDefaultAndStaticInterfaceMethodsWhenDesugaring()
+ && parameters.isCfRuntime(),
+ r -> r.assertFailureWithErrorThatThrows(IllegalAccessError.class),
+ r -> r.assertSuccessWithOutputLines("C::f"));
+ }
+
+ @Test
+ public void testR8() throws ExecutionException, CompilationFailedException, IOException {
testForR8(parameters.getBackend())
.addProgramClasses(CLASSES)
.addKeepMainRule(Main.class)
.setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), Main.class)
- .assertFailureWithErrorThatMatches(containsString(expectedRuntimeError()));
+ .assertFailureWithErrorThatThrows(expectedRuntimeError());
}
- private String expectedRuntimeError() {
+ private Class<? extends Throwable> expectedRuntimeError() {
if (parameters.isDexRuntime()
&& parameters.getRuntime().asDex().getVm().isOlderThanOrEqual(DexVm.ART_4_4_4_HOST)) {
- return "IncompatibleClassChangeError";
+ return IncompatibleClassChangeError.class;
}
- return "IllegalAccessError";
+ return IllegalAccessError.class;
}
}
diff --git a/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialMethodAccessWithIntermediateTest.java b/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialMethodAccessWithIntermediateTest.java
index ce6f4d7..5498d06 100644
--- a/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialMethodAccessWithIntermediateTest.java
+++ b/src/test/java/com/android/tools/r8/resolution/access/NestInvokeSpecialMethodAccessWithIntermediateTest.java
@@ -18,7 +18,6 @@
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.ResolutionResult;
-import com.android.tools.r8.graph.ResolutionResult.IllegalAccessOrNoSuchMethodResult;
import com.android.tools.r8.references.Reference;
import com.android.tools.r8.transformers.ClassFileTransformer;
import com.android.tools.r8.utils.BooleanUtils;
@@ -124,7 +123,11 @@
// Resolution fails when there is a mismatch between the symbolic reference and the definition.
if (!symbolicReferenceIsDefiningType) {
- assertTrue(resolutionResult instanceof IllegalAccessOrNoSuchMethodResult);
+ if (inSameNest) {
+ assertTrue(resolutionResult.isNoSuchMethodErrorResult(callerClassDefinition, appInfo));
+ } else {
+ assertTrue(resolutionResult.isIllegalAccessErrorResult(callerClassDefinition, appInfo));
+ }
return;
}
