Change ProguardMapMinifier to handle applymapping use cases
This is the major change for applymapping. The change handles:
- Tracking all non-private members and propegate all names down the
type hierarchy.
- Allow for renaming both in classpath and program classes.
- Rewrite generic type signatures.
Bug: 126503704
Bug: 131532229
Bug: 128868424
Bug: 121305642
Bug: 128516926
Change-Id: Icb98459ba1b837a3e1cd5268d087cd446d8d0597
diff --git a/src/main/java/com/android/tools/r8/R8.java b/src/main/java/com/android/tools/r8/R8.java
index e01b4ed..0e69108 100644
--- a/src/main/java/com/android/tools/r8/R8.java
+++ b/src/main/java/com/android/tools/r8/R8.java
@@ -686,8 +686,8 @@
timing.end();
} else {
if (appView.appInfo().hasLiveness()) {
- // TODO(124726014): Rewrite signature annotations in lens rewriting instead of here?
- new GenericSignatureRewriter(appView.withLiveness()).run();
+ // TODO(b/124726014): Rewrite signature annotations in lens rewriting instead of here?
+ new GenericSignatureRewriter(appView.withLiveness()).run(appView.appInfo().classes());
}
namingLens = NamingLens.getIdentityLens();
}
diff --git a/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
index 10430cc..8018a2d 100644
--- a/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
@@ -128,7 +128,7 @@
timing.end();
timing.begin("rename-generic");
- new GenericSignatureRewriter(appView, renaming).run();
+ new GenericSignatureRewriter(appView, renaming).run(classes);
timing.end();
timing.begin("rename-arrays");
diff --git a/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java b/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
index 633a179..437d78b 100644
--- a/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
@@ -85,11 +85,15 @@
for (DexClass clazz : appView.appInfo().app().asDirect().allClasses()) {
ReservedFieldNamingState reservedNames = null;
for (DexEncodedField field : clazz.fields()) {
- if (strategy.isReserved(field, clazz)) {
+ DexString reservedName = strategy.getReservedNameOrDefault(field, clazz, null);
+ if (reservedName != null) {
if (reservedNames == null) {
reservedNames = getOrCreateReservedFieldNamingState(clazz.type);
}
- reservedNames.markReservedDirectly(field.field);
+ reservedNames.markReservedDirectly(reservedName, field.field.type);
+ if (reservedName != field.field.name) {
+ renaming.put(field.field, reservedName);
+ }
}
}
diff --git a/src/main/java/com/android/tools/r8/naming/FieldNamingState.java b/src/main/java/com/android/tools/r8/naming/FieldNamingState.java
index 67298e2..d8cb81d 100644
--- a/src/main/java/com/android/tools/r8/naming/FieldNamingState.java
+++ b/src/main/java/com/android/tools/r8/naming/FieldNamingState.java
@@ -50,10 +50,15 @@
DexEncodedField encodedField = appView.appInfo().resolveField(field);
if (encodedField != null) {
DexClass clazz = appView.definitionFor(encodedField.field.holder);
- if (clazz == null || strategy.isReserved(encodedField, clazz)) {
+ if (clazz == null) {
return field.name;
}
+ DexString reservedName = strategy.getReservedNameOrDefault(encodedField, clazz, null);
+ if (reservedName != null) {
+ return reservedName;
+ }
}
+ // TODO(b/133208730) If we cannot resolve the field, are we then allowed to rename it?
return getOrCreateInternalState(field).createNewName(field);
}
@@ -93,8 +98,7 @@
DexString name;
do {
name = strategy.next(field, this);
- } while (reservedNames.isReserved(name, field.type)
- && !strategy.breakOnNotAvailable(field, name));
+ } while (reservedNames.isReserved(name, field.type));
return name;
}
diff --git a/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java b/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java
index a43dcb8..4c751c6 100644
--- a/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java
@@ -289,7 +289,7 @@
for (MethodNamingState<?> namingState : globalStateMap.get(wrapper)) {
if (!namingState.isReserved(unifiedMethod.name, unifiedMethod.proto)) {
- namingState.reserveName(unifiedMethod.name, unifiedMethod.proto);
+ namingState.reserveName(unifiedMethod.name, unifiedMethod.proto, unifiedMethod.name);
changed = true;
}
}
diff --git a/src/main/java/com/android/tools/r8/naming/MemberNamingStrategy.java b/src/main/java/com/android/tools/r8/naming/MemberNamingStrategy.java
index 6510d6c..f33027b 100644
--- a/src/main/java/com/android/tools/r8/naming/MemberNamingStrategy.java
+++ b/src/main/java/com/android/tools/r8/naming/MemberNamingStrategy.java
@@ -18,11 +18,13 @@
DexString next(DexField field, InternalNamingState internalState);
- boolean breakOnNotAvailable(DexReference source, DexString name);
+ DexString getReservedNameOrDefault(
+ DexEncodedMethod method, DexClass holder, DexString defaultValue);
- boolean isReserved(DexEncodedMethod method, DexClass holder);
-
- boolean isReserved(DexEncodedField field, DexClass holder);
+ DexString getReservedNameOrDefault(
+ DexEncodedField field, DexClass holder, DexString defaultValue);
boolean allowMemberRenaming(DexClass holder);
+
+ void reportReservationError(DexReference source, DexString name);
}
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 b5b32b2..ba458bc 100644
--- a/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
@@ -72,9 +72,9 @@
* states of classes. Hence, skipping over names during interface naming does not impact their
* availability in the next phase.
*
- * <p>In the final stage, we assign names to methods by traversing the subtype tree, now allocating
- * separate naming states for each class starting from the frontier. In the first swoop, we allocate
- * all non-private methods, updating naming states accordingly.
+ * <p>In stage 4, we assign names to methods by traversing the subtype tree, now allocating separate
+ * naming states for each class starting from the frontier. In the first swoop, we allocate all
+ * non-private methods, updating naming states accordingly.
*
* <p>Finally, the computed renamings are returned as a map from {@link DexMethod} to {@link
* DexString}. The MethodNameMinifier object should not be retained to ensure all intermediate state
@@ -179,10 +179,12 @@
InterfaceMethodNameMinifier interfaceMethodNameMinifier =
new InterfaceMethodNameMinifier(
appView, desugaredCallSites, equivalence, frontierState, minifierState);
+ timing.end();
+ timing.begin("Phase 3");
interfaceMethodNameMinifier.assignNamesToInterfaceMethods(timing, interfaces);
timing.end();
- // Phase 3: Assign names top-down by traversing the subtype hierarchy.
- timing.begin("Phase 3");
+ // Phase 4: Assign names top-down by traversing the subtype hierarchy.
+ timing.begin("Phase 4");
assignNamesToClassesMethods(appView.dexItemFactory().objectType);
timing.end();
@@ -207,26 +209,37 @@
MethodNamingState<?> state =
computeStateIfAbsent(type, k -> minifierState.getState(holder.superType).createChild());
for (DexEncodedMethod method : holder.virtualMethodsSorted()) {
- assignNameToMethod(method, state);
+ assignNameToMethod(method, state, holder);
}
for (DexEncodedMethod method : holder.directMethodsSorted()) {
- assignNameToMethod(method, state);
+ assignNameToMethod(method, state, holder);
}
}
+
appView.appInfo().forAllExtendsSubtypes(type, this::assignNamesToClassesMethods);
}
- private void assignNameToMethod(DexEncodedMethod encodedMethod, MethodNamingState<?> state) {
+ private void assignNameToMethod(
+ DexEncodedMethod encodedMethod, MethodNamingState<?> state, DexClass holder) {
if (encodedMethod.accessFlags.isConstructor()) {
return;
}
DexMethod method = encodedMethod.method;
- if (!state.isReserved(method.name, method.proto)) {
- DexString renamedName = state.assignNewNameFor(method, method.name, method.proto);
- renaming.put(method, renamedName);
- if (!encodedMethod.accessFlags.isPrivate()) {
- state.addRenaming(method.name, method.proto, renamedName);
- }
+ DexString reservedName = strategy.getReservedNameOrDefault(encodedMethod, holder, method.name);
+ if (state.isReserved(reservedName, method.proto)) {
+ return;
+ }
+ DexString newName = state.assignNewNameFor(method, method.name, method.proto);
+ if (newName != method.name) {
+ addRenaming(encodedMethod, state, newName);
+ }
+ }
+
+ private void addRenaming(
+ DexEncodedMethod encodedMethod, MethodNamingState<?> state, DexString renamedName) {
+ renaming.put(encodedMethod.method, renamedName);
+ if (!encodedMethod.accessFlags.isPrivate()) {
+ state.addRenaming(encodedMethod.method.name, encodedMethod.method.proto, renamedName);
}
}
@@ -272,8 +285,37 @@
DexClass holder = appView.definitionFor(type);
if (holder != null) {
for (DexEncodedMethod method : shuffleMethods(holder.methods(), appView.options())) {
- if (strategy.isReserved(method, holder)) {
- reserveNamesForMethod(method.method, state);
+ DexString reservedName = strategy.getReservedNameOrDefault(method, holder, null);
+ if (reservedName != null) {
+ // If the mapping is incorrect, we might try to map a method to an existing name.
+ // class A {
+ // int m1(String foo) { ... }
+ // }
+ //
+ // class B extends A {
+ // int m2(String bar) { ... }
+ // }
+ //
+ // and the following reservations (mapping):
+ // A -> a:
+ // int m1(String foo) -> a
+ // B -> b:
+ // int m2(String foo) -> a <- ERROR
+ //
+ // In the example, the mapping file specifies that A.m1() should become a.a() and that
+ // B.m2() should become b.a(). This should not be allowed, since b.a() now overrides
+ // a.a(), unlike in the original program.
+ DexString previouslyReservedOriginalName =
+ state.getReservedOriginalName(reservedName, method.method.proto);
+ if (previouslyReservedOriginalName != null
+ && previouslyReservedOriginalName != method.method.name) {
+ strategy.reportReservationError(method.method, reservedName);
+ }
+ state.reserveName(reservedName, method.method.proto, method.method.name);
+ globalState.reserveName(reservedName, method.method.proto, method.method.name);
+ if (reservedName != method.method.name) {
+ addRenaming(method, state, reservedName);
+ }
}
}
}
@@ -281,11 +323,6 @@
return state;
}
- private void reserveNamesForMethod(DexMethod method, MethodNamingState<?> state) {
- state.reserveName(method.name, method.proto);
- globalState.reserveName(method.name, method.proto);
- }
-
public DexType get(DexType type) {
return frontiers.getOrDefault(type, type);
}
diff --git a/src/main/java/com/android/tools/r8/naming/MethodNamingState.java b/src/main/java/com/android/tools/r8/naming/MethodNamingState.java
index 8b9d042..6b48304 100644
--- a/src/main/java/com/android/tools/r8/naming/MethodNamingState.java
+++ b/src/main/java/com/android/tools/r8/naming/MethodNamingState.java
@@ -8,11 +8,9 @@
import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.utils.StringUtils;
-import com.google.common.collect.Sets;
import java.io.PrintStream;
import java.util.HashMap;
import java.util.Map;
-import java.util.Set;
import java.util.function.Function;
class MethodNamingState<KeyType> {
@@ -36,7 +34,7 @@
this.strategy = strategy;
}
- public MethodNamingState<KeyType> createChild() {
+ MethodNamingState<KeyType> createChild() {
return new MethodNamingState<>(this, keyTransform, strategy);
}
@@ -67,7 +65,7 @@
return state.getAssignedNameFor(name);
}
- public DexString assignNewNameFor(DexMethod source, DexString original, DexProto proto) {
+ DexString assignNewNameFor(DexMethod source, DexString original, DexProto proto) {
KeyType key = keyTransform.apply(proto);
DexString result = getAssignedNameFor(original, key);
if (result == null) {
@@ -77,13 +75,13 @@
return result;
}
- public void reserveName(DexString name, DexProto proto) {
+ void reserveName(DexString name, DexProto proto, DexString originalName) {
KeyType key = keyTransform.apply(proto);
InternalState state = getOrCreateInternalStateFor(key);
- state.reserveName(name);
+ state.reserveName(name, originalName);
}
- public boolean isReserved(DexString name, DexProto proto) {
+ boolean isReserved(DexString name, DexProto proto) {
KeyType key = keyTransform.apply(proto);
InternalState state = findInternalStateFor(key);
if (state == null) {
@@ -92,7 +90,16 @@
return state.isReserved(name);
}
- public boolean isAvailable(DexProto proto, DexString candidate) {
+ DexString getReservedOriginalName(DexString name, DexProto proto) {
+ KeyType key = keyTransform.apply(proto);
+ InternalState state = findInternalStateFor(key);
+ if (state == null) {
+ return null;
+ }
+ return state.getReservedOriginalName(name);
+ }
+
+ boolean isAvailable(DexProto proto, DexString candidate) {
KeyType key = keyTransform.apply(proto);
InternalState state = findInternalStateFor(key);
if (state == null) {
@@ -101,7 +108,7 @@
return state.isAvailable(candidate);
}
- public void addRenaming(DexString original, DexProto proto, DexString newName) {
+ void addRenaming(DexString original, DexProto proto, DexString newName) {
KeyType key = keyTransform.apply(proto);
InternalState state = getOrCreateInternalStateFor(key);
state.addRenaming(original, newName);
@@ -136,7 +143,7 @@
private static final int INITIAL_DICTIONARY_INDEX = 0;
private final InternalState parentInternalState;
- private Set<DexString> reservedNames = null;
+ private Map<DexString, DexString> reservedNames = null;
private Map<DexString, DexString> renamings = null;
private int virtualNameCount;
private int directNameCount = 0;
@@ -153,21 +160,32 @@
}
private boolean isReserved(DexString name) {
- return (reservedNames != null && reservedNames.contains(name))
+ return (reservedNames != null && reservedNames.containsKey(name))
|| (parentInternalState != null && parentInternalState.isReserved(name));
}
+ private DexString getReservedOriginalName(DexString name) {
+ DexString result = null;
+ if (reservedNames != null) {
+ result = reservedNames.get(name);
+ }
+ if (result == null && parentInternalState != null) {
+ result = parentInternalState.getReservedOriginalName(name);
+ }
+ return result;
+ }
+
private boolean isAvailable(DexString name) {
return !(renamings != null && renamings.containsValue(name))
- && !(reservedNames != null && reservedNames.contains(name))
+ && !(reservedNames != null && reservedNames.containsKey(name))
&& (parentInternalState == null || parentInternalState.isAvailable(name));
}
- void reserveName(DexString name) {
+ void reserveName(DexString name, DexString originalName) {
if (reservedNames == null) {
- reservedNames = Sets.newIdentityHashSet();
+ reservedNames = new HashMap<>();
}
- reservedNames.add(name);
+ reservedNames.put(name, originalName);
}
@Override
@@ -217,7 +235,7 @@
DexString name;
do {
name = strategy.next(source, this);
- } while (!isAvailable(name) && !strategy.breakOnNotAvailable(source, name));
+ } while (!isAvailable(name));
return name;
}
@@ -275,11 +293,11 @@
if (reservedNames == null || reservedNames.isEmpty()) {
out.print(" <NO RESERVED NAMES>");
} else {
- for (DexString reservedName : reservedNames) {
+ for (DexString reservedName : reservedNames.keySet()) {
out.print(System.lineSeparator());
out.print(indentation);
out.print(" ");
- out.print(reservedName.toSourceString());
+ out.print(reservedName.toSourceString() + "(by " + reservedNames.get(reservedName) + ")");
}
}
out.println();
diff --git a/src/main/java/com/android/tools/r8/naming/Minifier.java b/src/main/java/com/android/tools/r8/naming/Minifier.java
index c6ffef4..a3d15a2 100644
--- a/src/main/java/com/android/tools/r8/naming/Minifier.java
+++ b/src/main/java/com/android/tools/r8/naming/Minifier.java
@@ -185,23 +185,24 @@
}
@Override
- public boolean breakOnNotAvailable(DexReference source, DexString name) {
- return false;
- }
-
- @Override
- public boolean isReserved(DexEncodedMethod method, DexClass holder) {
+ public DexString getReservedNameOrDefault(
+ DexEncodedMethod method, DexClass holder, DexString defaultValue) {
if (!allowMemberRenaming(holder)
|| holder.accessFlags.isAnnotation()
- || method.accessFlags.isConstructor()) {
- return true;
+ || method.accessFlags.isConstructor()
+ || appView.rootSet().mayNotBeMinified(method.method, appView)) {
+ return method.method.name;
}
- return appView.rootSet().mayNotBeMinified(method.method, appView);
+ return defaultValue;
}
@Override
- public boolean isReserved(DexEncodedField field, DexClass holder) {
- return holder.isLibraryClass() || appView.rootSet().mayNotBeMinified(field.field, appView);
+ public DexString getReservedNameOrDefault(
+ DexEncodedField field, DexClass holder, DexString defaultValue) {
+ if (holder.isLibraryClass() || appView.rootSet().mayNotBeMinified(field.field, appView)) {
+ return field.field.name;
+ }
+ return defaultValue;
}
@Override
@@ -214,5 +215,11 @@
assert clazz != null && allowMemberRenaming(clazz);
return true;
}
+
+ @Override
+ public void reportReservationError(DexReference source, DexString name) {
+ assert false;
+ // This should only happen when applymapping is used and will be caught in that strategy.
+ }
}
}
diff --git a/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java b/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
index 784dc8b..6efddeb 100644
--- a/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
@@ -28,18 +28,46 @@
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.android.tools.r8.utils.Reporter;
import com.android.tools.r8.utils.Timing;
+import com.google.common.collect.BiMap;
+import com.google.common.collect.HashBiMap;
+import java.util.ArrayDeque;
import java.util.ArrayList;
+import java.util.Deque;
import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;
+/**
+ * The ProguardMapMinifier will assign names to classes and members following the initial naming
+ * seed given by the mapping files.
+ *
+ * <p>First the object hierarchy is traversed maintaining a collection of all program classes and
+ * classes that needs to be renamed in {@link #mappedClasses}. For each level we keep track of all
+ * renamed members and propagate all non-private items to descendants. This is necessary to ensure
+ * that virtual methods are renamed when there are "gaps" in the hierarchy. We keep track of all
+ * namings such that future renaming of non-private members will not collide or fail with an error.
+ *
+ * <p>Second, we compute desugared default interface methods and companion classes to ensure these
+ * can be referred to by clients.
+ *
+ * <p>Third, we traverse all reachable interfaces for class mappings and add them to our tracking
+ * maps. Otherwise, the minification follows the ordinary minification.
+ */
public class ProguardMapMinifier {
private final AppView<AppInfoWithLiveness> appView;
private final SeedMapper seedMapper;
private final Set<DexCallSite> desugaredCallSites;
+ private final BiMap<DexType, DexString> mappedNames = HashBiMap.create();
+ private final List<DexClass> mappedClasses = new ArrayList<>();
+ private final Map<DexReference, MemberNaming> memberNames = new IdentityHashMap<>();
+ private final Map<DexType, DexString> syntheticCompanionClasses = new IdentityHashMap<>();
+ private final Map<DexMethod, DexString> defaultInterfaceMethodImplementationNames =
+ new IdentityHashMap<>();
+ private final Map<DexMethod, DexString> additionalMethodNamings = new IdentityHashMap<>();
+ private final Map<DexField, DexString> additionalFieldNamings = new IdentityHashMap<>();
public ProguardMapMinifier(
AppView<AppInfoWithLiveness> appView,
@@ -51,70 +79,34 @@
}
public NamingLens run(Timing timing) {
- timing.begin("mapping classes");
+ timing.begin("MappingClasses");
+ computeMapping(appView.dexItemFactory().objectType, new ArrayDeque<>());
+ timing.end();
- Map<DexType, DexString> mappedNames = new IdentityHashMap<>();
- List<DexClass> mappedClasses = new ArrayList<>();
- Map<DexReference, MemberNaming> memberNames = new IdentityHashMap<>();
- Map<DexType, DexString> syntheticCompanionClasses = new IdentityHashMap<>();
- Map<DexMethod, DexString> defaultInterfaceMethodImplementationNames = new IdentityHashMap<>();
- for (String key : seedMapper.getKeyset()) {
- ClassNamingForMapApplier classNaming = seedMapper.getMapping(key);
- DexType type =
- appView.dexItemFactory().lookupType(appView.dexItemFactory().createString(key));
- if (type == null) {
- // The map contains additional mapping of classes compared to what we have seen. This should
- // have no effect.
- continue;
+ timing.begin("MappingDefaultInterfaceMethods");
+ computeDefaultInterfaceMethodMethods();
+ timing.end();
+
+ timing.begin("ComputeInterfaces");
+ // We have to compute interfaces
+ Set<DexClass> interfaces = new TreeSet<>((a, b) -> a.type.slowCompareTo(b.type));
+ for (DexClass dexClass : appView.appInfo().computeReachableInterfaces(desugaredCallSites)) {
+ ClassNamingForMapApplier classNaming = seedMapper.getClassNaming(dexClass.type);
+ if (classNaming != null) {
+ DexString mappedName = appView.dexItemFactory().createString(classNaming.renamedName);
+ checkAndAddMappedNames(dexClass.type, mappedName, classNaming.position);
}
- DexClass dexClass = appView.definitionFor(type);
- if (dexClass == null) {
- computeDefaultInterfaceMethodMappings(
- type,
- classNaming,
- syntheticCompanionClasses,
- defaultInterfaceMethodImplementationNames);
- continue;
- }
- DexString mappedName = appView.dexItemFactory().createString(classNaming.renamedName);
- DexType mappedType = appView.dexItemFactory().lookupType(mappedName);
- // The mappedType has to be available:
- // - If it is null we have not seen it.
- // - If the mapped type is itself the name is already reserved (by itself).
- // - If the there is no definition for the mapped type we will not get a naming clash.
- // Otherwise, there will be a naming conflict.
- if (mappedType != null && type != mappedType && appView.definitionFor(mappedType) != null) {
- appView
- .options()
- .reporter
- .error(
- ApplyMappingError.mapToExistingClass(
- type.toString(), mappedType.toString(), classNaming.position));
- }
- mappedNames.put(type, mappedName);
mappedClasses.add(dexClass);
- classNaming.forAllMethodNaming(
- memberNaming -> {
- Signature signature = memberNaming.getOriginalSignature();
- assert !signature.isQualified();
- DexMethod originalMethod =
- ((MethodSignature) signature).toDexMethod(appView.dexItemFactory(), type);
- assert !memberNames.containsKey(originalMethod);
- memberNames.put(originalMethod, memberNaming);
- });
- classNaming.forAllFieldNaming(
- memberNaming -> {
- Signature signature = memberNaming.getOriginalSignature();
- assert !signature.isQualified();
- DexField originalField =
- ((FieldSignature) signature).toDexField(appView.dexItemFactory(), type);
- assert !memberNames.containsKey(originalField);
- memberNames.put(originalField, memberNaming);
- });
+ interfaces.add(dexClass);
}
+ timing.end();
appView.options().reporter.failIfPendingErrors();
+ // To keep the order deterministic, we sort the classes by their type, which is a unique key.
+ mappedClasses.sort((a, b) -> a.type.slowCompareTo(b.type));
+
+ timing.begin("MinifyClasses");
ClassNameMinifier classNameMinifier =
new ClassNameMinifier(
appView,
@@ -129,23 +121,21 @@
classNameMinifier.computeRenaming(timing, syntheticCompanionClasses);
timing.end();
- Set<DexClass> interfaces = new TreeSet<>((a, b) -> a.type.slowCompareTo(b.type));
- interfaces.addAll(appView.appInfo().computeReachableInterfaces(desugaredCallSites));
-
ApplyMappingMemberNamingStrategy nameStrategy =
- new ApplyMappingMemberNamingStrategy(
- memberNames, appView.dexItemFactory(), appView.options().reporter);
+ new ApplyMappingMemberNamingStrategy(appView, memberNames);
timing.begin("MinifyMethods");
MethodRenaming methodRenaming =
new MethodNameMinifier(appView, nameStrategy)
.computeRenaming(interfaces, desugaredCallSites, timing);
// Amend the method renamings with the default interface methods.
methodRenaming.renaming.putAll(defaultInterfaceMethodImplementationNames);
+ methodRenaming.renaming.putAll(additionalMethodNamings);
timing.end();
timing.begin("MinifyFields");
FieldRenaming fieldRenaming =
new FieldNameMinifier(appView, nameStrategy).computeRenaming(interfaces, timing);
+ fieldRenaming.renaming.putAll(additionalFieldNamings);
timing.end();
appView.options().reporter.failIfPendingErrors();
@@ -159,7 +149,142 @@
return lens;
}
- private void computeDefaultInterfaceMethodMappings(
+ private void computeMapping(DexType type, Deque<Map<DexReference, MemberNaming>> buildUpNames) {
+ ClassNamingForMapApplier classNaming = seedMapper.getClassNaming(type);
+ DexClass dexClass = appView.definitionFor(type);
+
+ // Keep track of classes that needs to get renamed.
+ if (dexClass != null && (classNaming != null || dexClass.isProgramClass())) {
+ mappedClasses.add(dexClass);
+ }
+
+ Map<DexReference, MemberNaming> nonPrivateMembers = new IdentityHashMap<>();
+
+ if (classNaming != null) {
+ // TODO(b/133091438) assert that !dexClass.isLibaryClass();
+ DexString mappedName = appView.dexItemFactory().createString(classNaming.renamedName);
+ checkAndAddMappedNames(type, mappedName, classNaming.position);
+
+ classNaming.forAllMethodNaming(
+ memberNaming -> {
+ Signature signature = memberNaming.getOriginalSignature();
+ assert !signature.isQualified();
+ DexMethod originalMethod =
+ ((MethodSignature) signature).toDexMethod(appView.dexItemFactory(), type);
+ assert !memberNames.containsKey(originalMethod);
+ memberNames.put(originalMethod, memberNaming);
+ DexEncodedMethod encodedMethod = appView.definitionFor(originalMethod);
+ if (encodedMethod == null || !encodedMethod.accessFlags.isPrivate()) {
+ nonPrivateMembers.put(originalMethod, memberNaming);
+ }
+ });
+ classNaming.forAllFieldNaming(
+ memberNaming -> {
+ Signature signature = memberNaming.getOriginalSignature();
+ assert !signature.isQualified();
+ DexField originalField =
+ ((FieldSignature) signature).toDexField(appView.dexItemFactory(), type);
+ assert !memberNames.containsKey(originalField);
+ memberNames.put(originalField, memberNaming);
+ DexEncodedField encodedField = appView.definitionFor(originalField);
+ if (encodedField == null || !encodedField.accessFlags.isPrivate()) {
+ nonPrivateMembers.put(originalField, memberNaming);
+ }
+ });
+ } else {
+ // We have to ensure we do not rename to an existing member, that cannot be renamed.
+ if (dexClass == null || !appView.options().isMinifying()) {
+ checkAndAddMappedNames(type, type.descriptor, Position.UNKNOWN);
+ } else if (appView.options().isMinifying()
+ && appView.rootSet().mayNotBeMinified(type, appView)) {
+ checkAndAddMappedNames(type, type.descriptor, Position.UNKNOWN);
+ }
+ }
+
+ for (Map<DexReference, MemberNaming> parentMembers : buildUpNames) {
+ for (DexReference key : parentMembers.keySet()) {
+ if (key.isDexMethod()) {
+ DexMethod parentReference = key.asDexMethod();
+ DexMethod parentReferenceOnCurrentType =
+ appView
+ .dexItemFactory()
+ .createMethod(type, parentReference.proto, parentReference.name);
+ addMemberNaming(
+ key, parentReferenceOnCurrentType, parentMembers, additionalMethodNamings);
+ } else {
+ DexField parentReference = key.asDexField();
+ DexField parentReferenceOnCurrentType =
+ appView
+ .dexItemFactory()
+ .createField(type, parentReference.type, parentReference.name);
+ addMemberNaming(key, parentReferenceOnCurrentType, parentMembers, additionalFieldNamings);
+ }
+ }
+ }
+
+ if (nonPrivateMembers.size() > 0) {
+ buildUpNames.addLast(nonPrivateMembers);
+ appView
+ .appInfo()
+ .forAllExtendsSubtypes(type, subType -> computeMapping(subType, buildUpNames));
+ buildUpNames.removeLast();
+ } else {
+ appView
+ .appInfo()
+ .forAllExtendsSubtypes(type, subType -> computeMapping(subType, buildUpNames));
+ }
+ }
+
+ private <T extends DexReference> void addMemberNaming(
+ DexReference key,
+ T member,
+ Map<DexReference, MemberNaming> parentMembers,
+ Map<T, DexString> additionalMemberNamings) {
+ // We might have overridden a naming in the direct class namings above.
+ if (!memberNames.containsKey(member)) {
+ DexString renamedName =
+ appView.dexItemFactory().createString(parentMembers.get(key).getRenamedName());
+ memberNames.put(member, parentMembers.get(key));
+ additionalMemberNamings.put(member, renamedName);
+ }
+ }
+
+ private void checkAndAddMappedNames(DexType type, DexString mappedName, Position position) {
+ if (mappedNames.inverse().containsKey(mappedName)) {
+ appView
+ .options()
+ .reporter
+ .error(
+ ApplyMappingError.mapToExistingClass(
+ type.toString(), mappedName.toString(), position));
+ } else {
+ mappedNames.put(type, mappedName);
+ }
+ }
+
+ private void computeDefaultInterfaceMethodMethods() {
+ for (String key : seedMapper.getKeyset()) {
+ ClassNamingForMapApplier classNaming = seedMapper.getMapping(key);
+ DexType type =
+ appView.dexItemFactory().lookupType(appView.dexItemFactory().createString(key));
+ if (type == null) {
+ // The map contains additional mapping of classes compared to what we have seen. This should
+ // have no effect.
+ continue;
+ }
+ DexClass dexClass = appView.definitionFor(type);
+ if (dexClass == null) {
+ computeDefaultInterfaceMethodMappingsForType(
+ type,
+ classNaming,
+ syntheticCompanionClasses,
+ defaultInterfaceMethodImplementationNames);
+ continue;
+ }
+ }
+ }
+
+ private void computeDefaultInterfaceMethodMappingsForType(
DexType type,
ClassNamingForMapApplier classNaming,
Map<DexType, DexString> syntheticCompanionClasses,
@@ -224,65 +349,57 @@
private final Reporter reporter;
public ApplyMappingMemberNamingStrategy(
- Map<DexReference, MemberNaming> mappedNames, DexItemFactory factory, Reporter reporter) {
+ AppView<?> appView, Map<DexReference, MemberNaming> mappedNames) {
this.mappedNames = mappedNames;
- this.factory = factory;
- this.reporter = reporter;
+ this.factory = appView.dexItemFactory();
+ this.reporter = appView.options().reporter;
}
@Override
public DexString next(DexMethod method, InternalNamingState internalState) {
- return next(method);
+ assert !mappedNames.containsKey(method);
+ return method.name;
}
@Override
public DexString next(DexField field, InternalNamingState internalState) {
- return next(field);
+ assert !mappedNames.containsKey(field);
+ return field.name;
}
- private DexString next(DexReference reference) {
- if (mappedNames.containsKey(reference)) {
- return factory.createString(mappedNames.get(reference).getRenamedName());
- } else if (reference.isDexMethod()) {
- return reference.asDexMethod().name;
- } else {
- assert reference.isDexField();
- return reference.asDexField().name;
+ @Override
+ public DexString getReservedNameOrDefault(
+ DexEncodedMethod method, DexClass holder, DexString nullValue) {
+ if (mappedNames.containsKey(method.method)) {
+ return factory.createString(mappedNames.get(method.method).getRenamedName());
}
+ return nullValue;
}
@Override
- public boolean breakOnNotAvailable(DexReference source, DexString name) {
- // If we renamed a member to a name that exists in a subtype we should warn that potentially
- // a member lookup may no longer visit its parent.
- MemberNaming memberNaming = mappedNames.get(source);
- assert source.isDexMethod() || source.isDexField();
- ApplyMappingError applyMappingError = ApplyMappingError.mapToExistingMember(
- source.toSourceString(),
- name.toString(),
- memberNaming == null ? Position.UNKNOWN : memberNaming.position);
- if (source.isDexMethod()) {
- reporter.error(applyMappingError);
- } else {
- // TODO(b/128868424) Check if we can remove this warning for fields.
- reporter.warning(applyMappingError);
+ public DexString getReservedNameOrDefault(
+ DexEncodedField field, DexClass holder, DexString nullValue) {
+ if (mappedNames.containsKey(field.field)) {
+ return factory.createString(mappedNames.get(field.field).getRenamedName());
}
- return true;
- }
-
- @Override
- public boolean isReserved(DexEncodedMethod method, DexClass holder) {
- return false;
- }
-
- @Override
- public boolean isReserved(DexEncodedField field, DexClass holder) {
- return false;
+ return nullValue;
}
@Override
public boolean allowMemberRenaming(DexClass holder) {
return true;
}
+
+ @Override
+ public void reportReservationError(DexReference source, DexString name) {
+ MemberNaming memberNaming = mappedNames.get(source);
+ assert source.isDexMethod() || source.isDexField();
+ ApplyMappingError applyMappingError =
+ ApplyMappingError.mapToExistingMember(
+ source.toSourceString(),
+ name.toString(),
+ memberNaming == null ? Position.UNKNOWN : memberNaming.position);
+ reporter.error(applyMappingError);
+ }
}
}
diff --git a/src/main/java/com/android/tools/r8/naming/ReservedFieldNamingState.java b/src/main/java/com/android/tools/r8/naming/ReservedFieldNamingState.java
index 9885681..3e733fb 100644
--- a/src/main/java/com/android/tools/r8/naming/ReservedFieldNamingState.java
+++ b/src/main/java/com/android/tools/r8/naming/ReservedFieldNamingState.java
@@ -5,7 +5,6 @@
package com.android.tools.r8.naming;
import com.android.tools.r8.graph.AppView;
-import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.naming.ReservedFieldNamingState.InternalState;
@@ -25,10 +24,6 @@
return internalState != null && internalState.isReserved(name);
}
- public void markReservedDirectly(DexField field) {
- markReservedDirectly(field.name, field.type);
- }
-
public void markReservedDirectly(DexString name, DexType type) {
getOrCreateInternalState(type).markReservedDirectly(name);
}
diff --git a/src/main/java/com/android/tools/r8/naming/signature/GenericSignatureRewriter.java b/src/main/java/com/android/tools/r8/naming/signature/GenericSignatureRewriter.java
index f6eabad..7b3caa7 100644
--- a/src/main/java/com/android/tools/r8/naming/signature/GenericSignatureRewriter.java
+++ b/src/main/java/com/android/tools/r8/naming/signature/GenericSignatureRewriter.java
@@ -43,12 +43,15 @@
this.reporter = appView.options().reporter;
}
- public void run() {
+ public void run(Iterable<? extends DexClass> classes) {
final GenericSignatureCollector genericSignatureCollector = new GenericSignatureCollector();
final GenericSignatureParser<DexType> genericSignatureParser =
new GenericSignatureParser<>(genericSignatureCollector);
-
- for (DexClass clazz : appView.appInfo().classes()) {
+ // classes may not be the same as appInfo().classes() if applymapping is used on classpath
+ // arguments. If that is the case, the ProguardMapMinifier will pass in all classes that is
+ // either ProgramClass or has a mapping. This is then transitively called inside the
+ // ClassNameMinifier.
+ for (DexClass clazz : classes) {
clazz.annotations =
rewriteGenericSignatures(
clazz.annotations,
diff --git a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterDevirtualizationTest.java b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterDevirtualizationTest.java
index 500dbf5..2587730 100644
--- a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterDevirtualizationTest.java
+++ b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterDevirtualizationTest.java
@@ -13,7 +13,6 @@
import com.android.tools.r8.utils.StringUtils;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import org.junit.Assume;
-import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -99,7 +98,6 @@
.assertSuccessWithOutput(EXPECTED_OUTPUT);
}
- @Ignore("b/126503704")
@Test
public void devirtualizingNoRenamingOfOverriddenNotKeptInterfaceMethods() throws Exception {
R8TestCompileResult libraryResult =
@@ -131,7 +129,6 @@
.assertSuccessWithOutput(EXPECTED_OUTPUT);
}
- @Ignore("b/126503704")
@Test
public void devirtualizingNoRenamingOfOverriddenKeptInterfaceMethods() throws Exception {
R8TestCompileResult libraryResult =
diff --git a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingRotateNameClashTest.java b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingRotateNameClashTest.java
index dee54cb..3526829 100644
--- a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingRotateNameClashTest.java
+++ b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingRotateNameClashTest.java
@@ -11,7 +11,6 @@
import com.android.tools.r8.utils.StringUtils;
import java.io.IOException;
import java.util.concurrent.ExecutionException;
-import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -41,7 +40,6 @@
}
@Test
- @Ignore("b/131532229")
public void test_b131532229() throws ExecutionException, CompilationFailedException, IOException {
testForR8(parameters.getBackend())
.addLibraryClasses(A.class, B.class)
diff --git a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingVirtualInvokeTest.java b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingVirtualInvokeTest.java
index 7989a29..cc4e99c 100644
--- a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingVirtualInvokeTest.java
+++ b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingVirtualInvokeTest.java
@@ -12,6 +12,7 @@
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.naming.applymapping.ApplyMappingVirtualInvokeTest.TestClasses.LibraryBase;
+import com.android.tools.r8.naming.applymapping.ApplyMappingVirtualInvokeTest.TestClasses.LibraryInterface;
import com.android.tools.r8.naming.applymapping.ApplyMappingVirtualInvokeTest.TestClasses.LibrarySubclass;
import com.android.tools.r8.naming.applymapping.ApplyMappingVirtualInvokeTest.TestClasses.ProgramClass;
import com.android.tools.r8.naming.applymapping.ApplyMappingVirtualInvokeTest.TestClasses.ProgramSubclass;
@@ -20,7 +21,6 @@
import java.nio.file.Path;
import java.util.concurrent.ExecutionException;
import java.util.function.Function;
-import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -28,17 +28,28 @@
@RunWith(Parameterized.class)
public class ApplyMappingVirtualInvokeTest extends TestBase {
- public static final String EXPECTED_PROGRAM = StringUtils.lines("LibraryBase.foo");
+ public static final String EXPECTED_PROGRAM =
+ StringUtils.lines("LibraryBase.foo", "LibraryBase.bar");
public static final String EXPECTED_PROGRAM_SUBCLASS =
- StringUtils.lines("ProgramSubclass.foo", "LibraryBase.foo");
+ StringUtils.lines(
+ "ProgramSubclass.foo", "LibraryBase.foo", "ProgramSubclass.bar", "LibraryBase.bar");
public static class TestClasses {
- public static class LibraryBase {
+ public interface LibraryInterface {
+ void bar();
+ }
- void foo() {
+ public static class LibraryBase implements LibraryInterface {
+
+ public void foo() {
System.out.println("LibraryBase.foo");
}
+
+ @Override
+ public void bar() {
+ System.out.println("LibraryBase.bar");
+ }
}
public static class LibrarySubclass extends LibraryBase {}
@@ -47,6 +58,7 @@
public static void main(String[] args) {
new LibrarySubclass().foo();
+ new LibrarySubclass().bar();
}
}
@@ -54,17 +66,26 @@
public static void main(String[] args) {
new ProgramSubclass().foo();
+ new ProgramSubclass().bar();
}
@Override
- void foo() {
+ public void foo() {
System.out.println("ProgramSubclass.foo");
super.foo();
}
+
+ @Override
+ public void bar() {
+ System.out.println("ProgramSubclass.bar");
+ super.bar();
+ }
}
}
- private static final Class<?>[] LIBRARY_CLASSES = {LibraryBase.class, LibrarySubclass.class};
+ private static final Class<?>[] LIBRARY_CLASSES = {
+ LibraryInterface.class, LibraryBase.class, LibrarySubclass.class
+ };
private static final Class<?>[] PROGRAM_CLASSES = {ProgramClass.class, ProgramSubclass.class};
private final TestParameters parameters;
@@ -77,13 +98,15 @@
this.parameters = parameters;
}
- private static Function<Backend, R8TestCompileResult> compilationResults =
+ private static Function<TestParameters, R8TestCompileResult> compilationResults =
memoizeFunction(ApplyMappingVirtualInvokeTest::compile);
- private static R8TestCompileResult compile(Backend backend) throws CompilationFailedException {
- return testForR8(getStaticTemp(), backend)
+ private static R8TestCompileResult compile(TestParameters parameters)
+ throws CompilationFailedException {
+ return testForR8(getStaticTemp(), parameters.getBackend())
.addProgramClasses(LIBRARY_CLASSES)
.addKeepClassAndMembersRulesWithAllowObfuscation(LIBRARY_CLASSES)
+ .setMinApi(parameters.getRuntime())
.addOptionsModification(
options -> {
options.enableInlining = false;
@@ -116,14 +139,12 @@
.assertSuccessWithOutput(EXPECTED_PROGRAM_SUBCLASS);
}
- @Ignore("b/128868424")
@Test
public void testProgramClass()
throws ExecutionException, CompilationFailedException, IOException {
runTest(ProgramClass.class, EXPECTED_PROGRAM);
}
- @Ignore("b/128868424")
@Test
public void testProgramSubClass()
throws ExecutionException, IOException, CompilationFailedException {
@@ -132,7 +153,7 @@
private void runTest(Class<?> main, String expected)
throws ExecutionException, IOException, CompilationFailedException {
- R8TestCompileResult libraryCompileResult = compilationResults.apply(parameters.getBackend());
+ R8TestCompileResult libraryCompileResult = compilationResults.apply(parameters);
Path outPath = temp.newFile("out.zip").toPath();
libraryCompileResult.writeToZip(outPath);
testForR8(parameters.getBackend())
@@ -140,7 +161,7 @@
.noMinification()
.addProgramClasses(PROGRAM_CLASSES)
.addApplyMapping(libraryCompileResult.getProguardMap())
- .addLibraryClasses(LIBRARY_CLASSES)
+ .addClasspathClasses(LIBRARY_CLASSES)
.addLibraryFiles(runtimeJar(parameters.getBackend()))
.setMinApi(parameters.getRuntime())
.compile()
diff --git a/src/test/java/com/android/tools/r8/naming/applymapping/shared/NameClashTest.java b/src/test/java/com/android/tools/r8/naming/applymapping/shared/NameClashTest.java
index 7dd567a..d3df8d3 100644
--- a/src/test/java/com/android/tools/r8/naming/applymapping/shared/NameClashTest.java
+++ b/src/test/java/com/android/tools/r8/naming/applymapping/shared/NameClashTest.java
@@ -20,7 +20,6 @@
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.ClassRule;
-import org.junit.Ignore;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
@@ -139,6 +138,7 @@
.addKeepMainRule(MAIN)
.addKeepRules("-applymapping " + mappingFile)
.noTreeShaking()
+ .noMinification()
.compile()
.run(MAIN)
.assertSuccessWithOutput(EXPECTED_OUTPUT);
@@ -180,18 +180,6 @@
.assertSuccessWithOutput(EXPECTED_OUTPUT);
}
- private void testR8_minifiedLibraryJar(Path mappingFile) throws Exception {
- testForR8(Backend.DEX)
- .addLibraryFiles(ToolHelper.getDefaultAndroidJar(), libJar)
- .addProgramFiles(prgJarThatUsesMinifiedLib)
- .addKeepMainRule(MAIN)
- .addKeepRules("-applymapping " + mappingFile)
- .noTreeShaking()
- .compile()
- .run(MAIN)
- .assertSuccessWithOutput(EXPECTED_OUTPUT);
- }
-
@Test
public void testProguard_prgClassRenamedToExistingPrgClass() throws Exception {
FileUtils.writeTextFile(mappingFile, mappingToAlreadyMappedName());
@@ -431,11 +419,4 @@
assertThat(e.getMessage(), containsString("can't find superclass or interface A"));
}
}
-
- @Ignore("b/121305642")
- @Test
- public void testR8_minifiedLib() throws Exception {
- FileUtils.writeTextFile(mappingFile, invertedMapping());
- testR8_minifiedLibraryJar(mappingFile);
- }
}
diff --git a/src/test/java/com/android/tools/r8/naming/applymapping/sourcelibrary/ApplyMappingTest.java b/src/test/java/com/android/tools/r8/naming/applymapping/sourcelibrary/ApplyMappingTest.java
index f64424e..ec50858 100644
--- a/src/test/java/com/android/tools/r8/naming/applymapping/sourcelibrary/ApplyMappingTest.java
+++ b/src/test/java/com/android/tools/r8/naming/applymapping/sourcelibrary/ApplyMappingTest.java
@@ -29,7 +29,6 @@
import java.util.Iterator;
import java.util.concurrent.ExecutionException;
import org.junit.Before;
-import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -70,7 +69,6 @@
out = temp.newFolder("out").toPath();
}
- @Ignore("b/128516926")
@Test
public void test044_apply() throws Exception {
Path flag =
