Version 2.0.47
Cherry pick: Determine the use of not mapped types lazily for
applymapping
CL: https://r8-review.googlesource.com/49480
Bug: 149946708
Change-Id: Ia8f168e43e4889fd2f8173da91270b3d0267345e
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index c54e218..edc6674 100644
--- a/src/main/java/com/android/tools/r8/Version.java
+++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@
// This field is accessed from release scripts using simple pattern matching.
// Therefore, changing this field could break our release scripts.
- public static final String LABEL = "2.0.46";
+ public static final String LABEL = "2.0.47";
private Version() {
}
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 993363c..3d8d17a 100644
--- a/src/main/java/com/android/tools/r8/dex/ApplicationWriter.java
+++ b/src/main/java/com/android/tools/r8/dex/ApplicationWriter.java
@@ -246,7 +246,9 @@
if (options.encodeChecksums) {
encodeChecksums(virtualFiles);
}
+ namingLens.setIsSortingBeforeWriting(true);
application.dexItemFactory.sort(namingLens);
+ namingLens.setIsSortingBeforeWriting(false);
assert markers == null
|| markers.isEmpty()
|| application.dexItemFactory.extractMarkers() != null;
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 0c85641..3c599e6 100644
--- a/src/main/java/com/android/tools/r8/naming/MinifiedRenaming.java
+++ b/src/main/java/com/android/tools/r8/naming/MinifiedRenaming.java
@@ -33,7 +33,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 4fd6274..1a07b28 100644
--- a/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
@@ -37,6 +37,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;
@@ -92,6 +94,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));
@@ -100,7 +103,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);
}
@@ -114,7 +117,7 @@
subType -> {
DexClass dexClass = appView.definitionFor(subType);
if (dexClass != null && !dexClass.isInterface()) {
- computeMapping(subType, nonPrivateMembers);
+ computeMapping(subType, nonPrivateMembers, notMappedReferences);
}
});
assert nonPrivateMembers.isEmpty();
@@ -160,7 +163,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);
@@ -173,7 +178,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);
@@ -191,16 +199,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);
}
}
@@ -249,12 +256,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));
}
}
@@ -555,4 +564,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..8d6e7c5
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/ClassNameMinifierOriginalClassNameTest.java
@@ -0,0 +1,124 @@
+// 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())
+ .compileWithExpectedDiagnostics(
+ diagnosticMessages ->
+ diagnosticMessages.assertErrorMessageThatMatches(
+ 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");
+ }
+}
