Account for missing types in repackaging
This CL will check for missing super types and missing type references
in code. When the minification by repackaging lands this should be
refined to no longer have the check.
Bug: 179889105
Change-Id: I09f6a6a13e8419aa94887e28c65203ca4edff2b5
diff --git a/src/main/java/com/android/tools/r8/repackaging/RepackagingConstraintGraph.java b/src/main/java/com/android/tools/r8/repackaging/RepackagingConstraintGraph.java
index ca181fd..ab5b01b 100644
--- a/src/main/java/com/android/tools/r8/repackaging/RepackagingConstraintGraph.java
+++ b/src/main/java/com/android/tools/r8/repackaging/RepackagingConstraintGraph.java
@@ -100,7 +100,8 @@
}
private void registerReferencesFromClass(DexProgramClass clazz) {
- RepackagingUseRegistry registry = new RepackagingUseRegistry(appView, this, clazz);
+ RepackagingUseRegistry registry =
+ new RepackagingUseRegistry(appView, this, clazz, libraryBoundaryNode);
// Trace the references to the immediate super types.
registry.registerTypeReference(clazz.getSuperType());
@@ -133,7 +134,8 @@
}
private void registerReferencesFromField(ProgramField field) {
- RepackagingUseRegistry registry = new RepackagingUseRegistry(appView, this, field);
+ RepackagingUseRegistry registry =
+ new RepackagingUseRegistry(appView, this, field, libraryBoundaryNode);
// Trace the type of the field.
registry.registerTypeReference(field.getReference().getType());
@@ -144,7 +146,8 @@
private void registerReferencesFromMethod(ProgramMethod method) {
DexEncodedMethod definition = method.getDefinition();
- RepackagingUseRegistry registry = new RepackagingUseRegistry(appView, this, method);
+ RepackagingUseRegistry registry =
+ new RepackagingUseRegistry(appView, this, method, libraryBoundaryNode);
// Trace the type references in the method signature.
definition.getProto().forEachType(registry::registerTypeReference);
diff --git a/src/main/java/com/android/tools/r8/repackaging/RepackagingUseRegistry.java b/src/main/java/com/android/tools/r8/repackaging/RepackagingUseRegistry.java
index a6ffc40..a013212 100644
--- a/src/main/java/com/android/tools/r8/repackaging/RepackagingUseRegistry.java
+++ b/src/main/java/com/android/tools/r8/repackaging/RepackagingUseRegistry.java
@@ -40,11 +40,13 @@
private final ProgramDefinition context;
private final InitClassLens initClassLens;
private final RepackagingConstraintGraph.Node node;
+ private final RepackagingConstraintGraph.Node missingTypeNode;
public RepackagingUseRegistry(
AppView<AppInfoWithLiveness> appView,
RepackagingConstraintGraph constraintGraph,
- ProgramDefinition context) {
+ ProgramDefinition context,
+ RepackagingConstraintGraph.Node missingTypeNode) {
super(appView.dexItemFactory());
this.appInfo = appView.appInfo();
this.options = appView.options();
@@ -52,6 +54,7 @@
this.context = context;
this.initClassLens = appView.initClassLens();
this.node = constraintGraph.getNode(context.getDefinition());
+ this.missingTypeNode = missingTypeNode;
}
private boolean isOnlyAccessibleFromSamePackage(DexClass referencedClass) {
@@ -111,16 +114,21 @@
private void registerMemberAccess(
MemberResolutionResult<?, ?> resolutionResult, boolean isInvoke) {
- SuccessfulMemberResolutionResult<?, ?> successfulResolutionResult =
- resolutionResult.asSuccessfulMemberResolutionResult();
- if (successfulResolutionResult == null) {
- // TODO(b/165783399): If we want to preserve errors in the original program, we need to look
- // at the failure dependencies. For example, if this method accesses in a package-private
- // method in another package, and we move the two methods to the same package, then the
- // invoke would no longer fail with an IllegalAccessError.
+ if (!resolutionResult.isSuccessfulMemberResolutionResult()) {
+ // To preserve errors in the original program, we need to look at the failure dependencies.
+ // For example, if this method accesses in a package-private method in another package, and we
+ // move the two methods to the same package, then the invoke would no longer fail with an
+ // IllegalAccessError.
+ if (isInvoke) {
+ // TODO(b/150589374): Only add this if we are in the minification mode of repackaging.
+ node.addNeighbor(missingTypeNode);
+ }
return;
}
+ SuccessfulMemberResolutionResult<?, ?> successfulResolutionResult =
+ resolutionResult.asSuccessfulMemberResolutionResult();
+
// Check access to the initial resolution holder.
DexClass initialResolutionHolder = successfulResolutionResult.getInitialResolutionHolder();
registerClassTypeAccess(initialResolutionHolder);
@@ -153,6 +161,9 @@
DexClass clazz = appInfo.definitionFor(type);
if (clazz != null) {
consumer.accept(clazz);
+ } else {
+ // The missing type reference can be package private and we cannot repackage.
+ node.addNeighbor(missingTypeNode);
}
}
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 06c03e7..b186a52 100644
--- a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
+++ b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
@@ -959,6 +959,11 @@
if (!keepInfo.getInfo(clazz).isRepackagingAllowed(options())) {
return false;
}
+ for (DexType superType : clazz.allImmediateSupertypes()) {
+ if (definitionFor(superType) == null) {
+ return false;
+ }
+ }
return clazz
.traverseProgramMembers(
member -> {
diff --git a/src/test/java/com/android/tools/r8/repackage/RepackageLambdaMissingInterfaceTest.java b/src/test/java/com/android/tools/r8/repackage/RepackageLambdaMissingInterfaceTest.java
index 622b8fe..63201fa 100644
--- a/src/test/java/com/android/tools/r8/repackage/RepackageLambdaMissingInterfaceTest.java
+++ b/src/test/java/com/android/tools/r8/repackage/RepackageLambdaMissingInterfaceTest.java
@@ -4,12 +4,12 @@
package com.android.tools.r8.repackage;
+import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.MatcherAssert.assertThat;
import com.android.tools.r8.NeverInline;
import com.android.tools.r8.R8TestRunResult;
import com.android.tools.r8.TestParameters;
-import com.android.tools.r8.ToolHelper.DexVm.Version;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -31,19 +31,16 @@
@Test
public void testR8() throws Exception {
- R8TestRunResult r8TestRunResult = runTest(true);
- if (parameters.isDexRuntime()
- && parameters.getDexRuntimeVersion().isOlderThanOrEqual(Version.V4_4_4)) {
- r8TestRunResult.assertFailureWithErrorThatThrows(NoClassDefFoundError.class);
- } else {
- r8TestRunResult.assertFailureWithErrorThatThrows(IllegalAccessError.class);
- }
+ runTest(true)
+ .assertFailureWithErrorThatThrowsIf(parameters.isDexRuntime(), AbstractMethodError.class)
+ .assertSuccessWithOutputLinesIf(parameters.isCfRuntime(), "0");
}
private R8TestRunResult runTest(boolean repackage) throws Exception {
return testForR8(parameters.getBackend())
.addProgramClasses(ClassWithLambda.class, Main.class)
.addKeepMainRule(Main.class)
+ .addKeepAttributeInnerClassesAndEnclosingMethod()
.applyIf(repackage, this::configureRepackaging)
.setMinApi(parameters.getApiLevel())
.addDontWarn(MissingInterface.class)
@@ -52,8 +49,20 @@
.compile()
.inspect(
inspector -> {
- // TODO(b/179889105): This should probably not be repackaged.
- assertThat(ClassWithLambda.class, isRepackagedIf(inspector, repackage));
+ // Find the generated lambda class
+ assertThat(
+ ClassWithLambda.class,
+ isRepackagedIf(inspector, repackage && parameters.isDexRuntime()));
+ if (!parameters.isDexRuntime()) {
+ return;
+ }
+ inspector.forAllClasses(
+ clazz -> {
+ if (clazz.isSynthesizedJavaLambdaClass()) {
+ assertThat(
+ clazz.getFinalName(), containsString(Main.class.getPackage().getName()));
+ }
+ });
})
.addRunClasspathClasses(MissingInterface.class)
.run(parameters.getRuntime(), Main.class);
diff --git a/src/test/java/com/android/tools/r8/repackage/RepackageMissingMemberReferenceTest.java b/src/test/java/com/android/tools/r8/repackage/RepackageMissingMemberReferenceTest.java
index e6f8e93..9dc8746 100644
--- a/src/test/java/com/android/tools/r8/repackage/RepackageMissingMemberReferenceTest.java
+++ b/src/test/java/com/android/tools/r8/repackage/RepackageMissingMemberReferenceTest.java
@@ -9,7 +9,6 @@
import com.android.tools.r8.NeverInline;
import com.android.tools.r8.R8TestRunResult;
import com.android.tools.r8.TestParameters;
-import com.android.tools.r8.ToolHelper.DexVm.Version;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -31,12 +30,7 @@
@Test
public void testR8() throws Exception {
- R8TestRunResult r8TestRunResult = runTest(true);
- if (parameters.isDexRuntime() && parameters.getDexRuntimeVersion() == Version.V8_1_0) {
- r8TestRunResult.assertSuccessWithOutputLines(EXPECTED);
- } else {
- r8TestRunResult.assertFailureWithErrorThatThrows(IllegalAccessError.class);
- }
+ runTest(true).assertSuccessWithOutputLines(EXPECTED);
}
private R8TestRunResult runTest(boolean repackage) throws Exception {
@@ -49,11 +43,8 @@
.enableInliningAnnotations()
.compile()
.inspect(
- inspector -> {
- // TODO(b/179889105): This should probably not be repackaged.
- assertThat(
- ClassWithMissingReferenceInCode.class, isRepackagedIf(inspector, repackage));
- })
+ inspector ->
+ assertThat(ClassWithMissingReferenceInCode.class, isNotRepackaged(inspector)))
.addRunClasspathClasses(MissingReference.class)
.run(parameters.getRuntime(), Main.class);
}
diff --git a/src/test/java/com/android/tools/r8/repackage/RepackageMissingSuperInterfaceTestTest.java b/src/test/java/com/android/tools/r8/repackage/RepackageMissingSuperInterfaceTestTest.java
index 8f08c58..c853a99 100644
--- a/src/test/java/com/android/tools/r8/repackage/RepackageMissingSuperInterfaceTestTest.java
+++ b/src/test/java/com/android/tools/r8/repackage/RepackageMissingSuperInterfaceTestTest.java
@@ -10,7 +10,6 @@
import com.android.tools.r8.NeverInline;
import com.android.tools.r8.R8TestRunResult;
import com.android.tools.r8.TestParameters;
-import com.android.tools.r8.ToolHelper.DexVm.Version;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -18,6 +17,8 @@
@RunWith(Parameterized.class)
public class RepackageMissingSuperInterfaceTestTest extends RepackageTestBase {
+ private final String[] EXPECTED = new String[] {"ClassImplementingMissingInterface::bar"};
+
public RepackageMissingSuperInterfaceTestTest(
String flattenPackageHierarchyOrRepackageClasses, TestParameters parameters) {
super(flattenPackageHierarchyOrRepackageClasses, parameters);
@@ -25,18 +26,12 @@
@Test
public void testR8WithoutRepackaging() throws Exception {
- runTest(false).assertSuccessWithOutputLines("ClassImplementingMissingInterface::bar");
+ runTest(false).assertSuccessWithOutputLines(EXPECTED);
}
@Test
public void testR8() throws Exception {
- R8TestRunResult r8TestRunResult = runTest(true);
- if (parameters.isDexRuntime()
- && parameters.getDexRuntimeVersion().isOlderThanOrEqual(Version.V4_4_4)) {
- r8TestRunResult.assertFailureWithErrorThatThrows(NoClassDefFoundError.class);
- } else {
- r8TestRunResult.assertFailureWithErrorThatThrows(IllegalAccessError.class);
- }
+ runTest(true).assertSuccessWithOutputLines(EXPECTED);
}
private R8TestRunResult runTest(boolean repackage) throws Exception {
@@ -51,9 +46,7 @@
.compile()
.inspect(
inspector -> {
- // TODO(b/179889105): This should probably not be repackaged.
- assertThat(
- ClassImplementingMissingInterface.class, isRepackagedIf(inspector, repackage));
+ assertThat(ClassImplementingMissingInterface.class, isNotRepackaged(inspector));
})
.addRunClasspathClasses(MissingInterface.class)
.run(parameters.getRuntime(), Main.class);
diff --git a/src/test/java/com/android/tools/r8/repackage/RepackageMissingSuperTypeTest.java b/src/test/java/com/android/tools/r8/repackage/RepackageMissingSuperTypeTest.java
index cb2ce41..5880f86 100644
--- a/src/test/java/com/android/tools/r8/repackage/RepackageMissingSuperTypeTest.java
+++ b/src/test/java/com/android/tools/r8/repackage/RepackageMissingSuperTypeTest.java
@@ -10,7 +10,6 @@
import com.android.tools.r8.NeverInline;
import com.android.tools.r8.R8TestRunResult;
import com.android.tools.r8.TestParameters;
-import com.android.tools.r8.ToolHelper.DexVm.Version;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -18,6 +17,14 @@
@RunWith(Parameterized.class)
public class RepackageMissingSuperTypeTest extends RepackageTestBase {
+ private final String[] EXPECTED =
+ new String[] {
+ "ClassWithSuperCall::foo",
+ "MissingSuperType::foo",
+ "ClassWithoutSuperCall::foo",
+ "MissingSuperType::foo"
+ };
+
public RepackageMissingSuperTypeTest(
String flattenPackageHierarchyOrRepackageClasses, TestParameters parameters) {
super(flattenPackageHierarchyOrRepackageClasses, parameters);
@@ -25,23 +32,12 @@
@Test
public void testR8WithoutRepackaging() throws Exception {
- runTest(false)
- .assertSuccessWithOutputLines(
- "ClassWithSuperCall::foo",
- "MissingSuperType::foo",
- "ClassWithoutSuperCall::foo",
- "MissingSuperType::foo");
+ runTest(false).assertSuccessWithOutputLines(EXPECTED);
}
@Test
public void testR8() throws Exception {
- R8TestRunResult r8TestRunResult = runTest(true);
- if (parameters.isDexRuntime()
- && parameters.getDexRuntimeVersion().isOlderThanOrEqual(Version.V4_4_4)) {
- r8TestRunResult.assertFailureWithErrorThatThrows(NoClassDefFoundError.class);
- } else {
- r8TestRunResult.assertFailureWithErrorThatThrows(IllegalAccessError.class);
- }
+ runTest(true).assertSuccessWithOutputLines(EXPECTED);
}
private R8TestRunResult runTest(boolean repackage) throws Exception {
@@ -60,9 +56,8 @@
.compile()
.inspect(
inspector -> {
- // TODO(b/179889105): These should probably not be repackaged.
- assertThat(ClassWithSuperCall.class, isRepackagedIf(inspector, repackage));
- assertThat(ClassWithoutSuperCall.class, isRepackagedIf(inspector, repackage));
+ assertThat(ClassWithSuperCall.class, isNotRepackaged(inspector));
+ assertThat(ClassWithoutSuperCall.class, isNotRepackaged(inspector));
})
.addRunClasspathClasses(MissingSuperType.class)
.run(parameters.getRuntime(), Main.class);
