Avoid vertical class merging across package boundaries
Change-Id: I5e2574af7350d43f98f37c99ca8a440338a3ead3
diff --git a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
index 2d80d83..c347dba 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -113,11 +113,20 @@
}
private boolean isMergeCandidate(DexProgramClass clazz, Set<DexType> pinnedTypes) {
- if (clazz.isLibraryClass()
- || appInfo.instantiatedTypes.contains(clazz.type)
+ if (appInfo.instantiatedTypes.contains(clazz.type)
|| appInfo.isPinned(clazz.type)
- || pinnedTypes.contains(clazz.type)
- || clazz.type.getSingleSubtype() == null) {
+ || pinnedTypes.contains(clazz.type)) {
+ return false;
+ }
+ DexType singleSubtype = clazz.type.getSingleSubtype();
+ if (singleSubtype == null) {
+ // TODO(christofferqa): Even if [clazz] has multiple subtypes, we could still merge it into
+ // its subclass if [clazz] is not live. This should only be done, though, if it does not
+ // lead to members being duplicated.
+ return false;
+ }
+ DexClass targetClass = appInfo.definitionFor(singleSubtype);
+ if (mergeMayLeadToIllegalAccesses(clazz, targetClass)) {
return false;
}
for (DexEncodedField field : clazz.fields()) {
@@ -137,6 +146,20 @@
return true;
}
+ private boolean mergeMayLeadToIllegalAccesses(DexClass clazz, DexClass singleSubclass) {
+ if (clazz.type.isSamePackage(singleSubclass.type)) {
+ return false;
+ }
+ // TODO(christofferqa): To merge [clazz] into a class from another package we need to ensure:
+ // (A) All accesses to [clazz] and its members from inside the current package of [clazz] will
+ // continue to work. This is guaranteed if [clazz] is public and all members of [clazz] are
+ // either private or public.
+ // (B) All accesses from [clazz] to classes or members from the current package of [clazz] will
+ // continue to work. This is guaranteed if the methods of [clazz] do not access any private
+ // or protected classes or members from the current package of [clazz].
+ return true;
+ }
+
private void addProgramMethods(Set<Wrapper<DexMethod>> set, DexMethod method,
Equivalence<DexMethod> equivalence) {
DexClass definition = appInfo.definitionFor(method.holder);
diff --git a/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java b/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
index 52e2313..7016163 100644
--- a/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
@@ -238,7 +238,6 @@
assertEquals(2, numberOfMoveExceptionInstructions);
}
- @Ignore("b/73958515")
@Test
public void testNoIllegalClassAccess() throws Exception {
String main = "classmerging.SimpleInterfaceAccessTest";
@@ -258,18 +257,36 @@
"classmerging.pkg.SimpleInterfaceImplRetriever",
"classmerging.pkg.SimpleInterfaceImplRetriever$SimpleInterfaceImpl");
runTest(main, programFiles, preservedClassNames);
+ }
+
+ @Ignore("b/73958515")
+ @Test
+ public void testNoIllegalClassAccessWithAccessModifications() throws Exception {
// If access modifications are allowed then SimpleInterface should be merged into
// SimpleInterfaceImpl.
- preservedClassNames =
+ String main = "classmerging.SimpleInterfaceAccessTest";
+ Path[] programFiles =
+ new Path[] {
+ CF_DIR.resolve("SimpleInterface.class"),
+ CF_DIR.resolve("SimpleInterfaceAccessTest.class"),
+ CF_DIR.resolve("pkg/SimpleInterfaceImplRetriever.class"),
+ CF_DIR.resolve("pkg/SimpleInterfaceImplRetriever$SimpleInterfaceImpl.class")
+ };
+ ImmutableSet<String> preservedClassNames =
ImmutableSet.of(
"classmerging.SimpleInterfaceAccessTest",
"classmerging.pkg.SimpleInterfaceImplRetriever",
"classmerging.pkg.SimpleInterfaceImplRetriever$SimpleInterfaceImpl");
+ // Allow access modifications (and prevent SimpleInterfaceImplRetriever from being removed as
+ // a result of inlining).
runTest(
main,
programFiles,
preservedClassNames,
- getProguardConfig(EXAMPLE_KEEP, "-allowaccessmodification"));
+ getProguardConfig(
+ EXAMPLE_KEEP,
+ "-allowaccessmodification",
+ "-keep public class classmerging.pkg.SimpleInterfaceImplRetriever"));
}
@Test
@@ -328,6 +345,7 @@
}
for (String rule : additionalRules) {
builder.append(rule);
+ builder.append(System.lineSeparator());
}
return builder.toString();
}
