Partially resolve method name conflict in aggressively overloaded input.
This is a variant of go/r8g/16920 for methods, and only works for
resolving method name conflicts inside the same class. If already
aggressively overloaded method names are used in sub types, they aren't
resolvable with the current method minification.
Bug: 73149686
Change-Id: Id0c2db9e9b4853b729c2331bcbd781f837591d9a
diff --git a/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java b/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
index 936a06a..3138d32 100644
--- a/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
@@ -143,22 +143,34 @@
private void assignNamesToClassesMethods(DexType type, boolean doPrivates) {
DexClass holder = appInfo.definitionFor(type);
if (holder != null && !holder.isLibraryClass()) {
+ Map<DexMethod, DexString> renamingAtThisLevel = new IdentityHashMap<>();
NamingState<DexProto, ?> state =
computeStateIfAbsent(type, k -> getState(holder.superType).createChild());
- holder.forEachMethod(method -> assignNameToMethod(method, state, doPrivates));
+ holder.forEachMethod(method ->
+ assignNameToMethod(method, state, renamingAtThisLevel, doPrivates));
+ if (!doPrivates && !useUniqueMemberNames) {
+ renamingAtThisLevel.forEach((method, candidate) ->
+ state.addRenaming(method.name, method.proto, candidate));
+ }
}
type.forAllExtendsSubtypes(subtype -> assignNamesToClassesMethods(subtype, doPrivates));
}
private void assignNameToMethod(
- DexEncodedMethod encodedMethod, NamingState<DexProto, ?> state, boolean doPrivates) {
+ DexEncodedMethod encodedMethod,
+ NamingState<DexProto, ?> state,
+ Map<DexMethod, DexString> renamingAtThisLevel,
+ boolean doPrivates) {
if (encodedMethod.accessFlags.isPrivate() != doPrivates) {
return;
}
DexMethod method = encodedMethod.method;
if (!state.isReserved(method.name, method.proto)
&& !encodedMethod.accessFlags.isConstructor()) {
- renaming.put(method, state.assignNewNameFor(method.name, method.proto, !doPrivates));
+ DexString renamedName =
+ state.assignNewNameFor(method.name, method.proto, useUniqueMemberNames);
+ renaming.put(method, renamedName);
+ renamingAtThisLevel.put(method, renamedName);
}
}
diff --git a/src/test/java/com/android/tools/r8/naming/overloadaggressively/ValidNameConflictTest.java b/src/test/java/com/android/tools/r8/naming/overloadaggressively/ValidNameConflictTest.java
index 51523f2..f949aea 100644
--- a/src/test/java/com/android/tools/r8/naming/overloadaggressively/ValidNameConflictTest.java
+++ b/src/test/java/com/android/tools/r8/naming/overloadaggressively/ValidNameConflictTest.java
@@ -128,14 +128,13 @@
FieldSubject f2 = clazz.field("java.lang.Object", "same");
assertTrue(f2.isPresent());
assertFalse(f2.isRenamed());
- assertEquals(f1.getField().field.name, f2.getField().field.name);
+ assertEquals(f1.getFinalName(), f2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
assertEquals(javaOutput.stdout, artOutput.stdout);
}
-
@Test
public void remainFieldNameConflict_useuniqueclassmembernames() throws Exception {
Assume.assumeTrue(ToolHelper.artSupported());
@@ -158,7 +157,7 @@
FieldSubject f2 = clazz.field("java.lang.Object", "same");
assertTrue(f2.isPresent());
assertTrue(f2.isRenamed());
- assertEquals(f1.getField().field.name, f2.getField().field.name);
+ assertEquals(f1.getFinalName(), f2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
@@ -189,7 +188,7 @@
FieldSubject f2 = clazz.field("java.lang.Object", "same");
assertTrue(f2.isPresent());
assertTrue(f2.isRenamed());
- assertEquals(f1.getField().field.name, f2.getField().field.name);
+ assertEquals(f1.getFinalName(), f2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
@@ -217,7 +216,7 @@
FieldSubject f2 = clazz.field("java.lang.Object", "same");
assertTrue(f2.isPresent());
assertTrue(f2.isRenamed());
- assertNotEquals(f1.getField().field.name, f2.getField().field.name);
+ assertNotEquals(f1.getFinalName(), f2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
@@ -246,7 +245,7 @@
FieldSubject f2 = clazz.field("java.lang.Object", "same");
assertTrue(f2.isPresent());
assertTrue(f2.isRenamed());
- assertEquals(f1.getField().field.name, f2.getField().field.name);
+ assertEquals(f1.getFinalName(), f2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
@@ -304,7 +303,7 @@
MethodSubject m2 = clazz.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(m2.isPresent());
assertFalse(m2.isRenamed());
- assertEquals(m1.getMethod().method.name, m2.getMethod().method.name);
+ assertEquals(m1.getFinalName(), m2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
@@ -333,7 +332,7 @@
MethodSubject m2 = clazz.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(m2.isPresent());
assertTrue(m2.isRenamed());
- assertEquals(m1.getMethod().method.name, m2.getMethod().method.name);
+ assertEquals(m1.getFinalName(), m2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
@@ -364,14 +363,13 @@
MethodSubject m2 = clazz.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(m2.isPresent());
assertTrue(m2.isRenamed());
- assertEquals(m1.getMethod().method.name, m2.getMethod().method.name);
+ assertEquals(m1.getFinalName(), m2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
assertEquals(javaOutput.stdout, artOutput.stdout);
}
-
@Test
public void resolveMethodNameConflict_no_options() throws Exception {
Assume.assumeTrue(ToolHelper.artSupported());
@@ -393,8 +391,7 @@
MethodSubject m2 = clazz.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(m2.isPresent());
assertTrue(m2.isRenamed());
- // TODO(b/73149686): R8 should be able to fix this conflict w/o -overloadaggressively.
- assertEquals(m1.getMethod().method.name, m2.getMethod().method.name);
+ assertNotEquals(m1.getFinalName(), m2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
@@ -423,7 +420,7 @@
MethodSubject m2 = clazz.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(m2.isPresent());
assertTrue(m2.isRenamed());
- assertEquals(m1.getMethod().method.name, m2.getMethod().method.name);
+ assertEquals(m1.getFinalName(), m2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
@@ -494,7 +491,7 @@
MethodSubject m2 = sup.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(m2.isPresent());
assertFalse(m2.isRenamed());
- assertEquals(m1.getMethod().method.name, m2.getMethod().method.name);
+ assertEquals(m1.getFinalName(), m2.getFinalName());
ClassSubject sub = dexInspector.clazz(ANOTHER_CLASS);
assertTrue(sub.isPresent());
@@ -504,11 +501,11 @@
MethodSubject subM2 = sub.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(subM2.isPresent());
assertFalse(subM2.isRenamed());
- assertEquals(subM1.getMethod().method.name, subM2.getMethod().method.name);
+ assertEquals(subM1.getFinalName(), subM2.getFinalName());
// No matter what, overloading methods should be renamed to the same name.
- assertEquals(m1.getMethod().method.name, subM1.getMethod().method.name);
- assertEquals(m2.getMethod().method.name, subM2.getMethod().method.name);
+ assertEquals(m1.getFinalName(), subM1.getFinalName());
+ assertEquals(m2.getFinalName(), subM2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
@@ -537,7 +534,7 @@
MethodSubject m2 = sup.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(m2.isPresent());
assertTrue(m2.isRenamed());
- assertEquals(m1.getMethod().method.name, m2.getMethod().method.name);
+ assertEquals(m1.getFinalName(), m2.getFinalName());
ClassSubject sub = dexInspector.clazz(ANOTHER_CLASS);
assertTrue(sub.isPresent());
@@ -547,11 +544,11 @@
MethodSubject subM2 = sub.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(subM2.isPresent());
assertTrue(subM2.isRenamed());
- assertEquals(subM1.getMethod().method.name, subM2.getMethod().method.name);
+ assertEquals(subM1.getFinalName(), subM2.getFinalName());
// No matter what, overloading methods should be renamed to the same name.
- assertEquals(m1.getMethod().method.name, subM1.getMethod().method.name);
- assertEquals(m2.getMethod().method.name, subM2.getMethod().method.name);
+ assertEquals(m1.getFinalName(), subM1.getFinalName());
+ assertEquals(m2.getFinalName(), subM2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
@@ -582,7 +579,7 @@
MethodSubject m2 = sup.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(m2.isPresent());
assertTrue(m2.isRenamed());
- assertEquals(m1.getMethod().method.name, m2.getMethod().method.name);
+ assertEquals(m1.getFinalName(), m2.getFinalName());
ClassSubject sub = dexInspector.clazz(ANOTHER_CLASS);
assertTrue(sub.isPresent());
@@ -592,18 +589,17 @@
MethodSubject subM2 = sub.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(subM2.isPresent());
assertTrue(subM2.isRenamed());
- assertEquals(subM1.getMethod().method.name, subM2.getMethod().method.name);
+ assertEquals(subM1.getFinalName(), subM2.getFinalName());
// No matter what, overloading methods should be renamed to the same name.
- assertEquals(m1.getMethod().method.name, subM1.getMethod().method.name);
- assertEquals(m2.getMethod().method.name, subM2.getMethod().method.name);
+ assertEquals(m1.getFinalName(), subM1.getFinalName());
+ assertEquals(m2.getFinalName(), subM2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
assertEquals(javaOutput.stdout, artOutput.stdout);
}
-
@Test
public void resolveMethodNameConflictInHierarchy_no_options() throws Exception {
Assume.assumeTrue(ToolHelper.artSupported());
@@ -625,8 +621,7 @@
MethodSubject m2 = sup.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(m2.isPresent());
assertTrue(m2.isRenamed());
- // TODO(b/73149686): R8 should be able to fix this conflict w/o -overloadaggressively.
- assertEquals(m1.getMethod().method.name, m2.getMethod().method.name);
+ assertNotEquals(m1.getFinalName(), m2.getFinalName());
ClassSubject sub = dexInspector.clazz(ANOTHER_CLASS);
assertTrue(sub.isPresent());
@@ -637,11 +632,14 @@
assertTrue(subM2.isPresent());
assertTrue(subM2.isRenamed());
// TODO(b/73149686): R8 should be able to fix this conflict w/o -overloadaggressively.
- assertEquals(subM1.getMethod().method.name, subM2.getMethod().method.name);
+ assertEquals(subM1.getFinalName(), subM2.getFinalName());
- // No matter what, overloading methods should be renamed to the same name.
- assertEquals(m1.getMethod().method.name, subM1.getMethod().method.name);
- assertEquals(m2.getMethod().method.name, subM2.getMethod().method.name);
+ // TODO(b/73149686): The current impl of method minifier visits classes in a hierarchical order.
+ // Hence, already renamed same names in Super are not resolvable as the internal naming state
+ // uses only _name_ to resolve methods in the hierarchy.
+ // // No matter what, overloading methods should be renamed to the same name.
+ // assertEquals(m1.getFinalName(), subM1.getFinalName());
+ // assertEquals(m2.getFinalName(), subM2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
@@ -671,7 +669,7 @@
MethodSubject m2 = sup.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(m2.isPresent());
assertTrue(m2.isRenamed());
- assertEquals(m1.getMethod().method.name, m2.getMethod().method.name);
+ assertEquals(m1.getFinalName(), m2.getFinalName());
ClassSubject sub = dexInspector.clazz(ANOTHER_CLASS);
assertTrue(sub.isPresent());
@@ -681,11 +679,11 @@
MethodSubject subM2 = sub.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(subM2.isPresent());
assertTrue(subM2.isRenamed());
- assertEquals(subM1.getMethod().method.name, subM2.getMethod().method.name);
+ assertEquals(subM1.getFinalName(), subM2.getFinalName());
// No matter what, overloading methods should be renamed to the same name.
- assertEquals(m1.getMethod().method.name, subM1.getMethod().method.name);
- assertEquals(m2.getMethod().method.name, subM2.getMethod().method.name);
+ assertEquals(m1.getFinalName(), subM1.getFinalName());
+ assertEquals(m2.getFinalName(), subM2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
diff --git a/src/test/java/com/android/tools/r8/utils/DexInspector.java b/src/test/java/com/android/tools/r8/utils/DexInspector.java
index 95ecd4f..82d14de 100644
--- a/src/test/java/com/android/tools/r8/utils/DexInspector.java
+++ b/src/test/java/com/android/tools/r8/utils/DexInspector.java
@@ -614,6 +614,16 @@
public abstract Signature getOriginalSignature();
public abstract Signature getFinalSignature();
+
+ public String getOriginalName() {
+ Signature originalSignature = getOriginalSignature();
+ return originalSignature == null ? null : originalSignature.name;
+ }
+
+ public String getFinalName() {
+ Signature finalSignature = getFinalSignature();
+ return finalSignature == null ? null : finalSignature.name;
+ }
}
public abstract class MethodSubject extends MemberSubject {
