Determine the use of not mapped types lazily for applymapping
When compiling an app with already minified input, like the play core
module, R8 will remove some classes and rename others with a high
probability of us rotating names. When running tests afterwards, we
would report an error if we found a mapping to a class that has no
mapping since such types could collide if both were used in the tests.
This CL pushes this tests to the NamingLens such that unused
references in the test will not throw an error. Also, the error is
lowered to a warning.
Bug: 149946708
Change-Id: I70e690532bd512fba870f82e1561955c85519e11
diff --git a/src/main/java/com/android/tools/r8/dex/ApplicationWriter.java b/src/main/java/com/android/tools/r8/dex/ApplicationWriter.java
index 2992824..32a4bf7 100644
--- a/src/main/java/com/android/tools/r8/dex/ApplicationWriter.java
+++ b/src/main/java/com/android/tools/r8/dex/ApplicationWriter.java
@@ -250,7 +250,9 @@
if (appView != null) {
appView.appInfo().disableDefinitionForAssert();
}
+ namingLens.setIsSortingBeforeWriting(true);
application.dexItemFactory.sort(namingLens);
+ namingLens.setIsSortingBeforeWriting(false);
if (appView != null) {
appView.appInfo().enableDefinitionForAssert();
}
diff --git a/src/main/java/com/android/tools/r8/naming/ApplyMappingError.java b/src/main/java/com/android/tools/r8/naming/ApplyMappingError.java
index 7b76acb..2ef940e 100644
--- a/src/main/java/com/android/tools/r8/naming/ApplyMappingError.java
+++ b/src/main/java/com/android/tools/r8/naming/ApplyMappingError.java
@@ -14,8 +14,9 @@
"'%s' cannot be mapped to '%s' because it is in conflict with an existing ";
private static final String EXISTING_MESSAGE_END =
". This usually happens when compiling a test application against a source application and "
- + "having short generic names in the test application. Try giving '%s' a more specific "
- + "name or add a keep rule to keep '%s'.";
+ + "there are used classes in the test that was not given a -keep rule when compiling the "
+ + "app. Try either renaming '%s' such that it will not collide or add a keep rule to "
+ + "keep '%s'.";
protected static final String EXISTING_CLASS_MESSAGE =
EXISTING_MESSAGE_START + "class with the same name" + EXISTING_MESSAGE_END;
diff --git a/src/main/java/com/android/tools/r8/naming/MinifiedRenaming.java b/src/main/java/com/android/tools/r8/naming/MinifiedRenaming.java
index 734c0b0..609feaa 100644
--- a/src/main/java/com/android/tools/r8/naming/MinifiedRenaming.java
+++ b/src/main/java/com/android/tools/r8/naming/MinifiedRenaming.java
@@ -34,7 +34,7 @@
class MinifiedRenaming extends NamingLens {
- private final AppView<?> appView;
+ final AppView<?> appView;
private final Map<String, String> packageRenaming;
private final Map<DexItem, DexString> renaming = new IdentityHashMap<>();
diff --git a/src/main/java/com/android/tools/r8/naming/NamingLens.java b/src/main/java/com/android/tools/r8/naming/NamingLens.java
index f1a7d84..d1c195f 100644
--- a/src/main/java/com/android/tools/r8/naming/NamingLens.java
+++ b/src/main/java/com/android/tools/r8/naming/NamingLens.java
@@ -41,6 +41,8 @@
*/
public abstract class NamingLens {
+ protected boolean isSortingBeforeWriting;
+
public abstract String lookupPackageName(String packageName);
public abstract DexString lookupDescriptor(DexType type);
@@ -163,6 +165,10 @@
return true;
}
+ public void setIsSortingBeforeWriting(boolean isSorting) {
+ isSortingBeforeWriting = isSorting;
+ }
+
private static class IdentityLens extends NamingLens {
private IdentityLens() {
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 471d66f..9f32739 100644
--- a/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
@@ -36,6 +36,8 @@
import com.google.common.collect.Maps;
import java.util.ArrayDeque;
import java.util.Deque;
+import java.util.HashMap;
+import java.util.HashSet;
import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
@@ -87,6 +89,7 @@
public NamingLens run(ExecutorService executorService, Timing timing) throws ExecutionException {
ArrayDeque<Map<DexReference, MemberNaming>> nonPrivateMembers = new ArrayDeque<>();
+ Set<DexReference> notMappedReferences = new HashSet<>();
timing.begin("MappingInterfaces");
Set<DexClass> interfaces = new TreeSet<>((a, b) -> a.type.slowCompareTo(b.type));
@@ -96,7 +99,7 @@
if (dexClass.isInterface()) {
// Only visit top level interfaces because computeMapping will visit the hierarchy.
if (dexClass.interfaces.isEmpty()) {
- computeMapping(dexClass.type, nonPrivateMembers);
+ computeMapping(dexClass.type, nonPrivateMembers, notMappedReferences);
}
interfaces.add(dexClass);
}
@@ -113,7 +116,7 @@
subType -> {
DexClass dexClass = appView.definitionFor(subType);
if (dexClass != null && !dexClass.isInterface()) {
- computeMapping(subType, nonPrivateMembers);
+ computeMapping(subType, nonPrivateMembers, notMappedReferences);
}
});
assert nonPrivateMembers.isEmpty();
@@ -158,7 +161,9 @@
appView.options().reporter.failIfPendingErrors();
- NamingLens lens = new MinifiedRenaming(appView, classRenaming, methodRenaming, fieldRenaming);
+ NamingLens lens =
+ new ProguardMapMinifiedRenaming(
+ appView, classRenaming, methodRenaming, fieldRenaming, notMappedReferences);
timing.begin("MinifyIdentifiers");
new IdentifierMinifier(appView, lens).run(executorService);
@@ -171,7 +176,10 @@
return lens;
}
- private void computeMapping(DexType type, Deque<Map<DexReference, MemberNaming>> buildUpNames) {
+ private void computeMapping(
+ DexType type,
+ Deque<Map<DexReference, MemberNaming>> buildUpNames,
+ Set<DexReference> notMappedReferences) {
ClassNamingForMapApplier classNaming = seedMapper.getClassNaming(type);
DexClass dexClass = appView.definitionFor(type);
@@ -189,16 +197,15 @@
if (dexClass != null) {
KotlinMetadataRewriter.removeKotlinMetadataFromRenamedClass(appView, dexClass);
}
-
classNaming.forAllMemberNaming(
memberNaming -> addMemberNamings(type, memberNaming, nonPrivateMembers, false));
} 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);
+ notMappedReferences.add(type);
} else if (appView.options().isMinifying()
&& appView.rootSet().mayNotBeMinified(type, appView)) {
- checkAndAddMappedNames(type, type.descriptor, Position.UNKNOWN);
+ notMappedReferences.add(type);
}
}
@@ -247,12 +254,14 @@
buildUpNames.addLast(nonPrivateMembers);
appView
.appInfo()
- .forAllImmediateExtendsSubtypes(type, subType -> computeMapping(subType, buildUpNames));
+ .forAllImmediateExtendsSubtypes(
+ type, subType -> computeMapping(subType, buildUpNames, notMappedReferences));
buildUpNames.removeLast();
} else {
appView
.appInfo()
- .forAllImmediateExtendsSubtypes(type, subType -> computeMapping(subType, buildUpNames));
+ .forAllImmediateExtendsSubtypes(
+ type, subType -> computeMapping(subType, buildUpNames, notMappedReferences));
}
}
@@ -553,4 +562,52 @@
// reporter.error(applyMappingError);
}
}
+
+ public static class ProguardMapMinifiedRenaming extends MinifiedRenaming {
+
+ private final Set<DexReference> unmappedReferences;
+ private final Map<DexString, DexType> classRenamingsMappingToDifferentName;
+
+ ProguardMapMinifiedRenaming(
+ AppView<?> appView,
+ ClassRenaming classRenaming,
+ MethodRenaming methodRenaming,
+ FieldRenaming fieldRenaming,
+ Set<DexReference> unmappedReferences) {
+ super(appView, classRenaming, methodRenaming, fieldRenaming);
+ this.unmappedReferences = unmappedReferences;
+ classRenamingsMappingToDifferentName = new HashMap<>();
+ classRenaming.classRenaming.forEach(
+ (type, dexString) -> {
+ if (type.descriptor != dexString) {
+ classRenamingsMappingToDifferentName.put(dexString, type);
+ }
+ });
+ }
+
+ @Override
+ public DexString lookupDescriptor(DexType type) {
+ if (!isSortingBeforeWriting) {
+ checkForUseOfNotMappedReference(type);
+ }
+ return super.lookupDescriptor(type);
+ }
+
+ private void checkForUseOfNotMappedReference(DexType type) {
+ if (unmappedReferences.contains(type)
+ && classRenamingsMappingToDifferentName.containsKey(type.descriptor)) {
+ // Type is an unmapped reference and there is a mapping from some other type to this one.
+ // We are emitting a warning here, since this will generally be undesired behavior.
+ DexType mappedType = classRenamingsMappingToDifferentName.get(type.descriptor);
+ appView
+ .options()
+ .reporter
+ .error(
+ ApplyMappingError.mapToExistingClass(
+ mappedType.toString(), type.toSourceString(), Position.UNKNOWN));
+ // Remove the type to ensure us only reporting the error once.
+ unmappedReferences.remove(type);
+ }
+ }
+ }
}
diff --git a/src/test/java/com/android/tools/r8/naming/ClassNameMinifierOriginalClassNameTest.java b/src/test/java/com/android/tools/r8/naming/ClassNameMinifierOriginalClassNameTest.java
new file mode 100644
index 0000000..9d4c4fb
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/ClassNameMinifierOriginalClassNameTest.java
@@ -0,0 +1,125 @@
+// Copyright (c) 2020, 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 com.android.tools.r8.naming;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isRenamed;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assume.assumeTrue;
+
+import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.R8TestCompileResult;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.naming.testclasses.A;
+import com.android.tools.r8.naming.testclasses.B;
+import com.android.tools.r8.utils.FileUtils;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.concurrent.ExecutionException;
+import java.util.function.Function;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+/** This test is designed to test the workaround for b/149946708. */
+@RunWith(Parameterized.class)
+public class ClassNameMinifierOriginalClassNameTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public ClassNameMinifierOriginalClassNameTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ private static Function<TestParameters, R8TestCompileResult> compilationResults =
+ memoizeFunction(ClassNameMinifierOriginalClassNameTest::compile);
+
+ private static R8TestCompileResult compile(TestParameters parameters)
+ throws CompilationFailedException, IOException, ExecutionException {
+ // Adding the obfuscation dictionary just ensures that we assign a name to B that will collide
+ // independent of minification scheme.
+ Path dictionary = getStaticTemp().newFolder().toPath().resolve("dictionary.txt");
+ FileUtils.writeTextFile(dictionary, "A");
+ return testForR8(getStaticTemp(), parameters.getBackend())
+ .addProgramClasses(A.class, B.class)
+ .addKeepClassAndMembersRulesWithAllowObfuscation(B.class)
+ .addKeepRules("-classobfuscationdictionary " + dictionary.toString(), "-keeppackagenames")
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .inspect(
+ inspector -> {
+ assertEquals(1, inspector.allClasses().size());
+ assertThat(inspector.clazz(B.class), isRenamed());
+ assertEquals(A.class.getTypeName(), inspector.clazz(B.class).getFinalName());
+ });
+ }
+
+ @Test
+ public void testR8() throws ExecutionException, CompilationFailedException, IOException {
+ R8TestCompileResult libraryCompileResult = compilationResults.apply(parameters);
+ testForR8(parameters.getBackend())
+ .addProgramClasses(Main.class)
+ .setMinApi(parameters.getApiLevel())
+ .addKeepMainRule(Main.class)
+ .noMinification()
+ .addClasspathClasses(A.class, B.class)
+ .addApplyMapping(libraryCompileResult.getProguardMap())
+ .compile()
+ .addRunClasspathFiles(libraryCompileResult.writeToZip())
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines("B.foo");
+ }
+
+ @Test
+ public void testR8WithReferenceToNotMapped() {
+ assumeTrue(parameters.isDexRuntime());
+ R8TestCompileResult libraryCompileResult = compilationResults.apply(parameters);
+ assertThrows(
+ CompilationFailedException.class,
+ () ->
+ testForR8(parameters.getBackend())
+ .addProgramClasses(MainWithReferenceToNotMapped.class)
+ .setMinApi(parameters.getApiLevel())
+ .addKeepMainRule(MainWithReferenceToNotMapped.class)
+ .noMinification()
+ .addClasspathClasses(A.class, B.class)
+ .addApplyMapping(libraryCompileResult.getProguardMap())
+ .allowDiagnosticWarningMessages()
+ .compileWithExpectedDiagnostics(
+ diagnosticMessages ->
+ diagnosticMessages.assertAllWarningMessagesMatch(
+ containsString(
+ "'"
+ + B.class.getTypeName()
+ + "' cannot be mapped to '"
+ + A.class.getTypeName()
+ + "' because it is in conflict"))));
+ }
+
+ public static class Main {
+
+ public static void main(String[] args) {
+ new B().foo();
+ }
+ }
+
+ public static class MainWithReferenceToNotMapped {
+
+ public static void main(String[] args) {
+ System.out.println(new A());
+ new B().foo();
+ }
+ }
+}
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 0b37f73..3466e21 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
@@ -401,21 +401,6 @@
}
@Test
- public void testR8_originalLibClassRenamedToExistingName() throws Exception {
- FileUtils.writeTextFile(mappingFile, mappingToExistingClassName());
- try {
- testR8_originalLibraryJar(mappingFile);
- fail("Expect compilation failure.");
- } catch (CompilationFailedException e) {
- assertThat(e.getCause().getMessage(), containsString("cannot be mapped to"));
- assertThat(
- e.getCause().getMessage(),
- containsString("because it is in conflict with an existing class with the same name."));
- assertThat(e.getCause().getMessage(), containsString(ProgramClass.class.getTypeName()));
- }
- }
-
- @Test
public void testProguard_minifiedLib() throws Exception {
FileUtils.writeTextFile(mappingFile, invertedMapping());
try {
diff --git a/src/test/java/com/android/tools/r8/naming/testclasses/A.java b/src/test/java/com/android/tools/r8/naming/testclasses/A.java
new file mode 100644
index 0000000..28a93a6
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/testclasses/A.java
@@ -0,0 +1,7 @@
+// Copyright (c) 2020, 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 com.android.tools.r8.naming.testclasses;
+
+public class A {}
diff --git a/src/test/java/com/android/tools/r8/naming/testclasses/B.java b/src/test/java/com/android/tools/r8/naming/testclasses/B.java
new file mode 100644
index 0000000..a364510
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/testclasses/B.java
@@ -0,0 +1,12 @@
+// Copyright (c) 2020, 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 com.android.tools.r8.naming.testclasses;
+
+public class B extends A {
+
+ public void foo() {
+ System.out.println("B.foo");
+ }
+}
