Handle field collisions in class merger
Change-Id: Ica3207e70d7860826632ca4af6b29179f3fdda73
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 0ed3c11..e4838da 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -449,7 +449,7 @@
// different proto, it could be the case that a method with the given name is overloaded.
DexProto existing =
overloadingInfo.computeIfAbsent(signature.name, key -> signature.proto);
- if (!existing.equals(signature.proto)) {
+ if (existing != DexProto.SENTINEL && !existing.equals(signature.proto)) {
// Mark that this signature is overloaded by mapping it to SENTINEL.
overloadingInfo.put(signature.name, DexProto.SENTINEL);
}
@@ -815,25 +815,35 @@
mergeMethods(virtualMethods.values(), target.virtualMethods());
// Step 2: Merge fields
- Set<Wrapper<DexField>> existingFieldSignatures = new HashSet<>();
- addAll(existingFieldSignatures, target.fields(), FieldSignatureEquivalence.get());
+ Set<DexString> existingFieldNames = new HashSet<>();
+ for (DexEncodedField field : target.fields()) {
+ existingFieldNames.add(field.field.name);
+ }
+ // In principle, we could allow multiple fields with the same name, and then only rename the
+ // field in the end when we are done merging all the classes, if it it turns out that the two
+ // fields ended up having the same type. This would not be too expensive, since we visit the
+ // entire program using VerticalClassMerger.TreeFixer anyway.
+ //
+ // For now, we conservatively report that a signature is already taken if there is a field
+ // with the same name. If minification is used with -overloadaggressively, this is solved
+ // later anyway.
Predicate<DexField> availableFieldSignatures =
- field -> !existingFieldSignatures.contains(FieldSignatureEquivalence.get().wrap(field));
+ field -> !existingFieldNames.contains(field.name);
DexEncodedField[] mergedInstanceFields =
mergeFields(
source.instanceFields(),
target.instanceFields(),
availableFieldSignatures,
- existingFieldSignatures);
+ existingFieldNames);
DexEncodedField[] mergedStaticFields =
mergeFields(
source.staticFields(),
target.staticFields(),
availableFieldSignatures,
- existingFieldSignatures);
+ existingFieldNames);
// Step 3: Merge interfaces
Set<DexType> interfaces = mergeArrays(target.interfaces.values, source.interfaces.values);
@@ -999,13 +1009,13 @@
DexEncodedField[] sourceFields,
DexEncodedField[] targetFields,
Predicate<DexField> availableFieldSignatures,
- Set<Wrapper<DexField>> existingFieldSignatures) {
+ Set<DexString> existingFieldNames) {
DexEncodedField[] result = new DexEncodedField[sourceFields.length + targetFields.length];
// Add fields from source
int i = 0;
for (DexEncodedField field : sourceFields) {
DexEncodedField resultingField = renameFieldIfNeeded(field, availableFieldSignatures);
- existingFieldSignatures.add(FieldSignatureEquivalence.get().wrap(resultingField.field));
+ existingFieldNames.add(resultingField.field.name);
deferredRenamings.map(field.field, resultingField.field);
result[i] = resultingField;
i++;
diff --git a/src/test/examples/classmerging/FieldCollisionTest.java b/src/test/examples/classmerging/FieldCollisionTest.java
new file mode 100644
index 0000000..5bb2260
--- /dev/null
+++ b/src/test/examples/classmerging/FieldCollisionTest.java
@@ -0,0 +1,43 @@
+// Copyright (c) 2018, 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 classmerging;
+
+public class FieldCollisionTest {
+
+ private static final B SENTINEL_A = new B("A");
+ private static final B SENTINEL_B = new B("B");
+
+ public static void main(String[] args) {
+ B obj = new B();
+ System.out.println(obj.toString());
+ }
+
+ // Will be merged into B.
+ public static class A {
+
+ // After class merging, this field will have the same name and type as the field B.obj,
+ // unless we handle the collision.
+ protected final A obj = SENTINEL_A;
+ }
+
+ public static class B extends A {
+
+ protected final String message;
+ protected final B obj = SENTINEL_B;
+
+ public B() {
+ this(null);
+ }
+
+ public B(String message) {
+ this.message = message;
+ }
+
+ @Override
+ public String toString() {
+ return obj.message + System.lineSeparator() + ((B) super.obj).message;
+ }
+ }
+}
diff --git a/src/test/examples/classmerging/MethodCollisionTest.java b/src/test/examples/classmerging/MethodCollisionTest.java
new file mode 100644
index 0000000..a9010a4
--- /dev/null
+++ b/src/test/examples/classmerging/MethodCollisionTest.java
@@ -0,0 +1,57 @@
+// Copyright (c) 2018, 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 classmerging;
+
+public class MethodCollisionTest {
+
+ public static void main(String[] args) {
+ new B().m();
+ new D().m();
+ }
+
+ public static class A {
+
+ // After class merging, this method will have the same signature as the method B.m,
+ // unless we handle the collision.
+ private A m() {
+ System.out.println("A.m");
+ return null;
+ }
+
+ public void invokeM() {
+ m();
+ }
+ }
+
+ public static class B extends A {
+
+ private B m() {
+ System.out.println("B.m");
+ invokeM();
+ return null;
+ }
+ }
+
+ public static class C {
+
+ // After class merging, this method will have the same signature as the method D.m,
+ // unless we handle the collision.
+ public C m() {
+ System.out.println("C.m");
+ return null;
+ }
+ }
+
+ public static class D extends C {
+
+ public D m() {
+ System.out.println("D.m");
+ super.m();
+ return null;
+ }
+ }
+
+
+}
diff --git a/src/test/examples/classmerging/keep-rules.txt b/src/test/examples/classmerging/keep-rules.txt
index b3e0ad1..4ba014e 100644
--- a/src/test/examples/classmerging/keep-rules.txt
+++ b/src/test/examples/classmerging/keep-rules.txt
@@ -19,6 +19,12 @@
-keep public class classmerging.ExceptionTest {
public static void main(...);
}
+-keep public class classmerging.FieldCollisionTest {
+ public static void main(...);
+}
+-keep public class classmerging.MethodCollisionTest {
+ public static void main(...);
+}
-keep public class classmerging.RewritePinnedMethodTest {
public static void main(...);
}
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 e0d73e8..f28c86f 100644
--- a/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
@@ -187,6 +187,22 @@
}
@Test
+ public void testFieldCollision() throws Exception {
+ String main = "classmerging.FieldCollisionTest";
+ Path[] programFiles =
+ new Path[] {
+ CF_DIR.resolve("FieldCollisionTest.class"),
+ CF_DIR.resolve("FieldCollisionTest$A.class"),
+ CF_DIR.resolve("FieldCollisionTest$B.class")
+ };
+ Set<String> preservedClassNames =
+ ImmutableSet.of(
+ "classmerging.FieldCollisionTest",
+ "classmerging.FieldCollisionTest$B");
+ runTest(main, programFiles, preservedClassNames::contains);
+ }
+
+ @Test
public void testLambdaRewriting() throws Exception {
String main = "classmerging.LambdaRewritingTest";
Path[] programFiles =
@@ -210,6 +226,31 @@
}
@Test
+ public void testMethodCollision() throws Exception {
+ String main = "classmerging.MethodCollisionTest";
+ Path[] programFiles =
+ new Path[] {
+ CF_DIR.resolve("MethodCollisionTest.class"),
+ CF_DIR.resolve("MethodCollisionTest$A.class"),
+ CF_DIR.resolve("MethodCollisionTest$B.class"),
+ CF_DIR.resolve("MethodCollisionTest$C.class"),
+ CF_DIR.resolve("MethodCollisionTest$D.class")
+ };
+ // TODO(christofferqa): Currently we do not allow merging A into B because we find a collision.
+ // However, we are free to change the names of private methods, so we should handle them similar
+ // to fields (i.e., we should allow merging A into B). This would also improve the performance
+ // of the collision detector, because it would only have to consider non-private methods.
+ Set<String> preservedClassNames =
+ ImmutableSet.of(
+ "classmerging.MethodCollisionTest",
+ "classmerging.MethodCollisionTest$A",
+ "classmerging.MethodCollisionTest$B",
+ "classmerging.MethodCollisionTest$C",
+ "classmerging.MethodCollisionTest$D");
+ runTest(main, programFiles, preservedClassNames::contains);
+ }
+
+ @Test
public void testPinnedParameterTypes() throws Exception {
String main = "classmerging.PinnedParameterTypesTest";
Path[] programFiles =
