Move package name strategy from class minifier to repackaging
Bug: 178567065
Change-Id: Ic33d4c7278ca20b0e4670f9e73ed3257170fd3ba
diff --git a/src/main/java/com/android/tools/r8/jar/CfApplicationWriter.java b/src/main/java/com/android/tools/r8/jar/CfApplicationWriter.java
index 5ade4de..9da3a40 100644
--- a/src/main/java/com/android/tools/r8/jar/CfApplicationWriter.java
+++ b/src/main/java/com/android/tools/r8/jar/CfApplicationWriter.java
@@ -3,7 +3,6 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.jar;
-import static com.android.tools.r8.repackaging.Repackaging.DefaultRepackagingConfiguration.TEMPORARY_PACKAGE_NAME;
import static com.android.tools.r8.utils.InternalOptions.ASM_VERSION;
import com.android.tools.r8.ByteDataView;
@@ -186,7 +185,6 @@
}
String desc = namingLens.lookupDescriptor(clazz.type).toString();
String name = namingLens.lookupInternalName(clazz.type);
- assert !name.contains(TEMPORARY_PACKAGE_NAME);
String signature = clazz.getClassSignature().toRenamedString(namingLens, isTypeMissing);
String superName =
clazz.type == options.itemFactory.objectType
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 04bdf33..e53f285 100644
--- a/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
@@ -18,10 +18,8 @@
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.graph.InnerClassAttribute;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
-import com.android.tools.r8.shaking.ProguardPackageNameList;
import com.android.tools.r8.utils.DescriptorUtils;
import com.android.tools.r8.utils.InternalOptions;
-import com.android.tools.r8.utils.StringUtils;
import com.android.tools.r8.utils.Timing;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
@@ -37,9 +35,7 @@
private final AppView<AppInfoWithLiveness> appView;
private final ClassNamingStrategy classNamingStrategy;
- private final PackageNamingStrategy packageNamingStrategy;
private final Iterable<? extends DexClass> classes;
- private final Set<String> usedPackagePrefixes = Sets.newHashSet();
private final Set<String> usedTypeNames = Sets.newHashSet();
private final Map<DexType, DexString> renaming = Maps.newIdentityHashMap();
private final Map<String, Namespace> states = new HashMap<>();
@@ -48,27 +44,19 @@
private final Namespace topLevelState;
private final boolean allowMixedCaseNaming;
private final Predicate<String> isUsed;
- private final ProguardPackageNameList keepPackageNames;
ClassNameMinifier(
AppView<AppInfoWithLiveness> appView,
ClassNamingStrategy classNamingStrategy,
- PackageNamingStrategy packageNamingStrategy,
Iterable<? extends DexClass> classes) {
this.appView = appView;
this.classNamingStrategy = classNamingStrategy;
- this.packageNamingStrategy = packageNamingStrategy;
this.classes = classes;
InternalOptions options = appView.options();
this.keepInnerClassStructure = options.keepInnerClassStructure();
// Initialize top-level naming state.
topLevelState = new Namespace("");
- String newPackageDescriptor =
- StringUtils.replaceAll(options.getProguardConfiguration().getPackagePrefix(), ".", "/");
- if (!newPackageDescriptor.isEmpty()) {
- registerPackagePrefixesAsUsed(newPackageDescriptor);
- }
states.put("", topLevelState);
if (options.getProguardConfiguration().hasDontUseMixedCaseClassnames()) {
@@ -78,7 +66,6 @@
allowMixedCaseNaming = true;
isUsed = usedTypeNames::contains;
}
- keepPackageNames = options.getProguardConfiguration().getKeepPackageNamesPatterns();
}
private void setUsedTypeName(String typeName) {
@@ -176,8 +163,6 @@
private void registerClassAsUsed(DexType type, DexString descriptor) {
renaming.put(type, descriptor);
- registerPackagePrefixesAsUsed(
- getParentPackagePrefix(getClassBinaryNameFromDescriptor(descriptor.toSourceString())));
setUsedTypeName(descriptor.toString());
if (keepInnerClassStructure) {
DexType outerClass = getOutClassForType(type);
@@ -192,16 +177,6 @@
}
}
- /** Registers the given package prefix and all of parent packages as used. */
- private void registerPackagePrefixesAsUsed(String packagePrefix) {
- String usedPrefix = packagePrefix;
- while (usedPrefix.length() > 0) {
- usedPackagePrefixes.add(usedPrefix);
- states.computeIfAbsent(usedPrefix, Namespace::new);
- usedPrefix = getParentPackagePrefix(usedPrefix);
- }
- }
-
private DexType getOutClassForType(DexType type) {
DexClass clazz = appView.definitionFor(type);
if (clazz == null) {
@@ -249,29 +224,8 @@
private Namespace getStateForClass(DexType type) {
String packageName = getPackageBinaryNameFromJavaType(type.getPackageDescriptor());
- // Check whether the given class should be kept.
- // or check whether the given class belongs to a package that is kept for another class.
- if (keepPackageNames.matches(type)) {
- return states.computeIfAbsent(packageName, Namespace::new);
- }
- return getStateForPackagePrefix(packageName);
- }
-
- private Namespace getStateForPackagePrefix(String prefix) {
- Namespace state = states.get(prefix);
- if (state == null) {
- // Calculate the parent package prefix, e.g., La/b/c -> La/b
- String parentPackage = getParentPackagePrefix(prefix);
- // Create a state for parent package prefix, if necessary, in a recursive manner.
- // That recursion should end when the parent package hits the top-level, "".
- Namespace superState = getStateForPackagePrefix(parentPackage);
- // From the super state, get a renamed package prefix for the current level.
- String renamedPackagePrefix = superState.nextPackagePrefix();
- // Create a new state, which corresponds to a new name space, for the current level.
- state = new Namespace(renamedPackagePrefix);
- states.put(prefix, state);
- }
- return state;
+ // Packages are repackaged and obfuscated when doing repackaging.
+ return states.computeIfAbsent(packageName, Namespace::new);
}
private Namespace getStateForOuterClass(DexType outer, String innerClassSeparator) {
@@ -326,13 +280,6 @@
return candidate;
}
- String nextPackagePrefix() {
- String next = packageNamingStrategy.next(packagePrefix, this, usedPackagePrefixes::contains);
- assert !usedPackagePrefixes.contains(next);
- usedPackagePrefixes.add(next);
- return next;
- }
-
@Override
public int getDictionaryIndex() {
return dictionaryIndex;
@@ -367,10 +314,6 @@
boolean isRenamedByApplyMapping(DexType type);
}
- protected interface PackageNamingStrategy {
- String next(char[] packagePrefix, InternalNamingState state, Predicate<String> isUsed);
- }
-
/**
* Compute parent package prefix from the given package prefix.
*
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 049b286..c5b8ced 100644
--- a/src/main/java/com/android/tools/r8/naming/Minifier.java
+++ b/src/main/java/com/android/tools/r8/naming/Minifier.java
@@ -20,7 +20,6 @@
import com.android.tools.r8.graph.SubtypingInfo;
import com.android.tools.r8.naming.ClassNameMinifier.ClassNamingStrategy;
import com.android.tools.r8.naming.ClassNameMinifier.ClassRenaming;
-import com.android.tools.r8.naming.ClassNameMinifier.PackageNamingStrategy;
import com.android.tools.r8.naming.FieldNameMinifier.FieldRenaming;
import com.android.tools.r8.naming.MethodNameMinifier.MethodRenaming;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
@@ -55,7 +54,6 @@
new ClassNameMinifier(
appView,
new MinificationClassNamingStrategy(appView),
- new MinificationPackageNamingStrategy(appView),
// Use deterministic class order to make sure renaming is deterministic.
appView.appInfo().classesWithDeterministicOrder());
ClassRenaming classRenaming = classNameMinifier.computeRenaming(timing);
@@ -106,13 +104,14 @@
private final MixedCasing mixedCasing;
BaseMinificationNamingStrategy(List<String> obfuscationDictionary, boolean dontUseMixedCasing) {
+ assert obfuscationDictionary != null;
this.obfuscationDictionary = obfuscationDictionary;
- this.obfuscationDictionaryForLookup = new HashSet<>(this.obfuscationDictionary);
+ this.obfuscationDictionaryForLookup = new HashSet<>(obfuscationDictionary);
this.mixedCasing =
dontUseMixedCasing ? MixedCasing.DONT_USE_MIXED_CASE : MixedCasing.USE_MIXED_CASE;
- assert obfuscationDictionary != null;
}
+ /** TODO(b/182992598): using char[] could give problems with unicode */
String nextName(char[] packagePrefix, InternalNamingState state) {
StringBuilder nextName = new StringBuilder();
nextName.append(packagePrefix);
@@ -186,24 +185,44 @@
}
}
- static class MinificationPackageNamingStrategy extends BaseMinificationNamingStrategy
- implements PackageNamingStrategy {
+ public static class MinificationPackageNamingStrategy extends BaseMinificationNamingStrategy {
- MinificationPackageNamingStrategy(AppView<?> appView) {
+ private final InternalNamingState namingState =
+ new InternalNamingState() {
+
+ private int dictionaryIndex = 0;
+ private int nameIndex = 1;
+
+ @Override
+ public int getDictionaryIndex() {
+ return dictionaryIndex;
+ }
+
+ @Override
+ public int incrementDictionaryIndex() {
+ return dictionaryIndex++;
+ }
+
+ @Override
+ public int incrementNameIndex() {
+ return nameIndex++;
+ }
+ };
+
+ public MinificationPackageNamingStrategy(AppView<?> appView) {
super(
appView.options().getProguardConfiguration().getPackageObfuscationDictionary(),
appView.options().getProguardConfiguration().hasDontUseMixedCaseClassnames());
}
- @Override
- public String next(char[] packagePrefix, InternalNamingState state, Predicate<String> isUsed) {
+ public String next(String packagePrefix, Predicate<String> isUsed) {
// Note that the differences between this method and the other variant for class renaming are
// 1) this one uses the different dictionary and counter,
// 2) this one does not append ';' at the end, and
- // 3) this one removes 'L' at the beginning to make the return value a binary form.
+ // 3) this one assumes no 'L' at the beginning to make the return value a binary form.
String nextPackageName;
do {
- nextPackageName = nextName(packagePrefix, state).substring(1);
+ nextPackageName = nextName(packagePrefix.toCharArray(), namingState);
} while (isUsed.test(nextPackageName));
return nextPackageName;
}
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 a8e299a..3b2c0d5 100644
--- a/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
@@ -29,7 +29,6 @@
import com.android.tools.r8.naming.MemberNaming.Signature;
import com.android.tools.r8.naming.MethodNameMinifier.MethodRenaming;
import com.android.tools.r8.naming.Minifier.MinificationClassNamingStrategy;
-import com.android.tools.r8.naming.Minifier.MinificationPackageNamingStrategy;
import com.android.tools.r8.naming.Minifier.MinifierMemberNamingStrategy;
import com.android.tools.r8.position.Position;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
@@ -137,11 +136,6 @@
appView,
new ApplyMappingClassNamingStrategy(
appView, mappedNames, seedMapper.getMappedToDescriptorNames()),
- // The package naming strategy will actually not be used since all classes and methods
- // will be output with identity name if not found in mapping. However, there is a check
- // in the ClassNameMinifier that the strategy should produce a "fresh" name so we just
- // use the existing strategy.
- new MinificationPackageNamingStrategy(appView),
classesWithDeterministicOrder(mappedClasses));
ClassRenaming classRenaming = classNameMinifier.computeRenaming(timing);
timing.end();
diff --git a/src/main/java/com/android/tools/r8/repackaging/Repackaging.java b/src/main/java/com/android/tools/r8/repackaging/Repackaging.java
index 056abce..48227b4 100644
--- a/src/main/java/com/android/tools/r8/repackaging/Repackaging.java
+++ b/src/main/java/com/android/tools/r8/repackaging/Repackaging.java
@@ -21,6 +21,7 @@
import com.android.tools.r8.graph.ProgramPackageCollection;
import com.android.tools.r8.graph.SortedProgramPackageCollection;
import com.android.tools.r8.graph.TreeFixerBase;
+import com.android.tools.r8.naming.Minifier.MinificationPackageNamingStrategy;
import com.android.tools.r8.repackaging.RepackagingLens.Builder;
import com.android.tools.r8.shaking.AnnotationFixer;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
@@ -35,6 +36,7 @@
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.IdentityHashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
@@ -202,19 +204,17 @@
Iterator<ProgramPackage> iterator = packages.iterator();
while (iterator.hasNext()) {
ProgramPackage pkg = iterator.next();
- String newPackageDescriptor =
- repackagingConfiguration.getNewPackageDescriptor(pkg, seenPackageDescriptors);
- if (pkg.getPackageDescriptor().equals(newPackageDescriptor)) {
+ if (repackagingConfiguration.isPackageInTargetLocation(pkg)) {
for (DexProgramClass alreadyRepackagedClass : pkg) {
if (!appView.appInfo().isRepackagingAllowed(alreadyRepackagedClass)) {
mappings.put(alreadyRepackagedClass.getType(), alreadyRepackagedClass.getType());
}
}
for (DexProgramClass alreadyRepackagedClass : pkg) {
- processClass(alreadyRepackagedClass, pkg, newPackageDescriptor, mappings);
+ processClass(alreadyRepackagedClass, pkg, pkg.getPackageDescriptor(), mappings);
}
- packageMappings.put(pkg.getPackageDescriptor(), newPackageDescriptor);
- seenPackageDescriptors.add(newPackageDescriptor);
+ packageMappings.put(pkg.getPackageDescriptor(), pkg.getPackageDescriptor());
+ seenPackageDescriptors.add(pkg.getPackageDescriptor());
iterator.remove();
}
}
@@ -228,15 +228,28 @@
ExecutorService executorService)
throws ExecutionException {
// For each package, find the set of classes that can be repackaged, and move them to the
- // desired package.
+ // desired package. We iterate all packages first to see if any classes are pinned and cannot
+ // be moved, to properly reserve their package.
+ Map<ProgramPackage, Collection<DexProgramClass>> packagesWithClassesToRepackage =
+ new IdentityHashMap<>();
for (ProgramPackage pkg : packages) {
+ Collection<DexProgramClass> classesToRepackage =
+ computeClassesToRepackage(pkg, executorService);
+ packagesWithClassesToRepackage.put(pkg, classesToRepackage);
+ // Reserve the package name to ensure that we are not renaming to a package we cannot move.
+ if (classesToRepackage.size() != pkg.classesInPackage().size()) {
+ seenPackageDescriptors.add(pkg.getPackageDescriptor());
+ }
+ }
+ for (ProgramPackage pkg : packages) {
+ Collection<DexProgramClass> classesToRepackage = packagesWithClassesToRepackage.get(pkg);
+ if (classesToRepackage.isEmpty()) {
+ continue;
+ }
// Already processed packages should have been removed.
String newPackageDescriptor =
repackagingConfiguration.getNewPackageDescriptor(pkg, seenPackageDescriptors);
- assert !pkg.getPackageDescriptor().equals(newPackageDescriptor);
-
- Collection<DexProgramClass> classesToRepackage =
- computeClassesToRepackage(pkg, executorService);
+ assert !repackagingConfiguration.isPackageInTargetLocation(pkg);
for (DexProgramClass classToRepackage : classesToRepackage) {
processClass(classToRepackage, pkg, newPackageDescriptor, mappings);
}
@@ -249,8 +262,6 @@
classesToRepackage.size() == pkg.classesInPackage().size()
? newPackageDescriptor
: pkg.getPackageDescriptor());
- // TODO(b/165783399): Investigate if repackaging can lead to different dynamic dispatch. See,
- // for example, CrossPackageInvokeSuperToPackagePrivateMethodTest.
}
}
@@ -298,6 +309,8 @@
String getNewPackageDescriptor(ProgramPackage pkg, Set<String> seenPackageDescriptors);
+ boolean isPackageInTargetLocation(ProgramPackage pkg);
+
DexType getRepackagedType(
DexProgramClass clazz,
DexProgramClass outerClass,
@@ -310,12 +323,13 @@
private final AppView<?> appView;
private final DexItemFactory dexItemFactory;
private final ProguardConfiguration proguardConfiguration;
- public static final String TEMPORARY_PACKAGE_NAME = "TEMPORARY_PACKAGE_NAME_FOR_";
+ private final MinificationPackageNamingStrategy packageMinificationStrategy;
public DefaultRepackagingConfiguration(AppView<?> appView) {
this.appView = appView;
this.dexItemFactory = appView.dexItemFactory();
this.proguardConfiguration = appView.options().getProguardConfiguration();
+ this.packageMinificationStrategy = new MinificationPackageNamingStrategy(appView);
}
@Override
@@ -326,13 +340,7 @@
proguardConfiguration.getPackageObfuscationMode();
if (packageObfuscationMode.isRepackageClasses()) {
return newPackageDescriptor;
- }
- assert packageObfuscationMode.isFlattenPackageHierarchy()
- || packageObfuscationMode.isMinification();
- if (!newPackageDescriptor.isEmpty()) {
- newPackageDescriptor += DESCRIPTOR_PACKAGE_SEPARATOR;
- }
- if (packageObfuscationMode.isMinification()) {
+ } else if (packageObfuscationMode.isMinification()) {
assert !proguardConfiguration.hasApplyMappingFile();
// Always keep top-level classes since there packages can never be minified.
if (pkg.getPackageDescriptor().equals("")
@@ -340,18 +348,37 @@
|| mayHavePinnedPackagePrivateOrProtectedItem(pkg)) {
return pkg.getPackageDescriptor();
}
- // For us to rename shaking/A to a/a if we have a class shaking/Kept, we have to propose
- // a different name than the last package name - the class will be minified in
- // ClassNameMinifier.
- newPackageDescriptor += TEMPORARY_PACKAGE_NAME + pkg.getLastPackageName();
+ // Plain minification do not support using a specified package prefix.
+ newPackageDescriptor = "";
} else {
- newPackageDescriptor += pkg.getLastPackageName();
+ assert packageObfuscationMode.isFlattenPackageHierarchy();
+ if (!newPackageDescriptor.isEmpty()) {
+ newPackageDescriptor += DESCRIPTOR_PACKAGE_SEPARATOR;
+ }
}
- String finalPackageDescriptor = newPackageDescriptor;
- for (int i = 1; seenPackageDescriptors.contains(finalPackageDescriptor); i++) {
- finalPackageDescriptor = newPackageDescriptor + INNER_CLASS_SEPARATOR + i;
+ return packageMinificationStrategy.next(
+ newPackageDescriptor, seenPackageDescriptors::contains);
+ }
+
+ @Override
+ public boolean isPackageInTargetLocation(ProgramPackage pkg) {
+ String newPackageDescriptor =
+ DescriptorUtils.getBinaryNameFromJavaType(proguardConfiguration.getPackagePrefix());
+ PackageObfuscationMode packageObfuscationMode =
+ proguardConfiguration.getPackageObfuscationMode();
+ if (packageObfuscationMode.isRepackageClasses()) {
+ return pkg.getPackageDescriptor().equals(newPackageDescriptor);
+ } else if (packageObfuscationMode.isMinification()) {
+ // Always keep top-level classes since there packages can never be minified.
+ return pkg.getPackageDescriptor().equals("")
+ || proguardConfiguration.getKeepPackageNamesPatterns().matches(pkg)
+ || mayHavePinnedPackagePrivateOrProtectedItem(pkg);
+ } else {
+ assert packageObfuscationMode.isFlattenPackageHierarchy();
+ // For flatten we will move the package into the package specified by the prefix so we can
+ // always minify the last part.
+ return false;
}
- return finalPackageDescriptor;
}
private boolean mayHavePinnedPackagePrivateOrProtectedItem(ProgramPackage pkg) {
@@ -428,6 +455,11 @@
}
@Override
+ public boolean isPackageInTargetLocation(ProgramPackage pkg) {
+ return true;
+ }
+
+ @Override
public DexType getRepackagedType(
DexProgramClass clazz,
DexProgramClass outerClass,
diff --git a/src/test/java/com/android/tools/r8/repackage/RepackageFeatureWithSyntheticsTest.java b/src/test/java/com/android/tools/r8/repackage/RepackageFeatureWithSyntheticsTest.java
index bd90c14..5e62057 100644
--- a/src/test/java/com/android/tools/r8/repackage/RepackageFeatureWithSyntheticsTest.java
+++ b/src/test/java/com/android/tools/r8/repackage/RepackageFeatureWithSyntheticsTest.java
@@ -106,14 +106,14 @@
// If it is, the access will fail resulting in a runtime exception.
compileResult.inspect(
baseInspector -> {
- assertThat(FIRST_FOO, isRepackagedAsExpected(baseInspector, "b"));
+ assertThat(FIRST_FOO, isRepackagedAsExpected(baseInspector, "a"));
assertThat(FIRST_PKG_PRIVATE, isNotRepackaged(baseInspector));
assertEquals(
getTestClasses().size() + expectedSyntheticsInBase,
baseInspector.allClasses().size());
},
featureInspector -> {
- assertThat(FIRST_FIRST_FOO, isRepackagedAsExpected(featureInspector, "a"));
+ assertThat(FIRST_FIRST_FOO, isRepackagedAsExpected(featureInspector, "b"));
assertThat(FIRST_FIRST_PKG_PRIVATE, isNotRepackaged(featureInspector));
assertEquals(
getFeatureClasses().size() + expectedSyntheticsInFeature,
diff --git a/src/test/java/com/android/tools/r8/repackage/RepackageWithSyntheticItemTest.java b/src/test/java/com/android/tools/r8/repackage/RepackageWithSyntheticItemTest.java
index 4df6622..1a3bfca 100644
--- a/src/test/java/com/android/tools/r8/repackage/RepackageWithSyntheticItemTest.java
+++ b/src/test/java/com/android/tools/r8/repackage/RepackageWithSyntheticItemTest.java
@@ -67,7 +67,7 @@
// TODO(b/172014416): We should not be able to look this up through the repackage name
String expectedOriginalNamePrefix =
isFlattenPackageHierarchy()
- ? "foo.repackage.RepackageWithSyntheticItemTest$A"
+ ? "foo.a.RepackageWithSyntheticItemTest$A"
: "foo.RepackageWithSyntheticItemTest$A";
assertThat(
classesStartingWithfoo.get(0).getOriginalName(),
