Desugared lib keep rules generation bug
- Now records correctly keep class on
emulated interfaces.
- More tests are now testing desugared
library shrunk and non shrunk instead
of only non shrunk.
Bug: 134732760
Change-Id: I5aee500e962377c8ae0800bceb0478a44bd06fb3
diff --git a/src/main/java/com/android/tools/r8/dex/CodeToKeep.java b/src/main/java/com/android/tools/r8/dex/CodeToKeep.java
index c836e98..8aa69a3 100644
--- a/src/main/java/com/android/tools/r8/dex/CodeToKeep.java
+++ b/src/main/java/com/android/tools/r8/dex/CodeToKeep.java
@@ -21,16 +21,19 @@
public abstract class CodeToKeep {
static CodeToKeep createCodeToKeep(InternalOptions options, NamingLens namingLens) {
- if (!namingLens.hasPrefixRewritingLogic() || options.coreLibraryCompilation) {
+ if ((!namingLens.hasPrefixRewritingLogic() && options.emulateLibraryInterface.isEmpty())
+ || options.coreLibraryCompilation) {
return new NopCodeToKeep();
}
- return new DesugaredLibraryCodeToKeep(namingLens);
+ return new DesugaredLibraryCodeToKeep(namingLens, options);
}
abstract void recordMethod(DexMethod method);
abstract void recordField(DexField field);
+ abstract void recordClass(DexType type);
+
abstract boolean isNop();
abstract void generateKeepRules(InternalOptions options);
@@ -38,15 +41,25 @@
public static class DesugaredLibraryCodeToKeep extends CodeToKeep {
private final NamingLens namingLens;
+ private final Set<DexType> emulatedInterfaces = Sets.newIdentityHashSet();
private final Map<DexType, Pair<Set<DexField>, Set<DexMethod>>> toKeep =
new ConcurrentHashMap<>();
- public DesugaredLibraryCodeToKeep(NamingLens namingLens) {
+ public DesugaredLibraryCodeToKeep(NamingLens namingLens, InternalOptions options) {
+ // Any class implementing one interface should implement the other one.
+ // Interface method desugaring should have created the types if emulatedLibraryInterfaces
+ // are set.
+ for (String rewrittenName : options.emulateLibraryInterface.values()) {
+ emulatedInterfaces.add(
+ options.itemFactory.lookupType(
+ options.itemFactory.lookupString(
+ DescriptorUtils.javaTypeToDescriptor(rewrittenName))));
+ }
this.namingLens = namingLens;
}
private boolean shouldKeep(DexType type) {
- return namingLens.prefixRewrittenType(type) != null;
+ return namingLens.prefixRewrittenType(type) != null || emulatedInterfaces.contains(type);
}
@Override
@@ -76,6 +89,13 @@
}
}
+ @Override
+ void recordClass(DexType type) {
+ if (shouldKeep(type)) {
+ keepClass(type);
+ }
+ }
+
private void keepClass(DexType type) {
toKeep.putIfAbsent(
type, new Pair<>(Sets.newConcurrentHashSet(), Sets.newConcurrentHashSet()));
@@ -143,6 +163,9 @@
void recordField(DexField field) {}
@Override
+ void recordClass(DexType type) {}
+
+ @Override
boolean isNop() {
return true;
}
diff --git a/src/main/java/com/android/tools/r8/dex/FileWriter.java b/src/main/java/com/android/tools/r8/dex/FileWriter.java
index 59a2f55..6920e83 100644
--- a/src/main/java/com/android/tools/r8/dex/FileWriter.java
+++ b/src/main/java/com/android/tools/r8/dex/FileWriter.java
@@ -622,6 +622,10 @@
private void writeClassData(DexProgramClass clazz) {
assert clazz.hasMethodsOrFields();
+ desugaredLibraryCodeToKeep.recordClass(clazz.superType);
+ for (DexType itf : clazz.interfaces.values) {
+ desugaredLibraryCodeToKeep.recordClass(itf);
+ }
mixedSectionOffsets.setOffsetFor(clazz, dest.position());
dest.putUleb128(clazz.staticFields().size());
dest.putUleb128(clazz.instanceFields().size());
diff --git a/src/test/java/com/android/tools/r8/TestCompileResult.java b/src/test/java/com/android/tools/r8/TestCompileResult.java
index 8b303ad..3adf685 100644
--- a/src/test/java/com/android/tools/r8/TestCompileResult.java
+++ b/src/test/java/com/android/tools/r8/TestCompileResult.java
@@ -19,6 +19,7 @@
import com.android.tools.r8.utils.AndroidApiLevel;
import com.android.tools.r8.utils.AndroidApp;
import com.android.tools.r8.utils.DescriptorUtils;
+import com.android.tools.r8.utils.TriFunction;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import com.google.common.collect.ImmutableList;
@@ -172,6 +173,17 @@
return self();
}
+ public CR addDesugaredCoreLibraryRunClassPath(
+ TriFunction<AndroidApiLevel, String, Boolean, Path> classPathSupplier,
+ AndroidApiLevel minAPILevel,
+ String keepRules,
+ boolean shrink) {
+ if (minAPILevel.getLevel() < AndroidApiLevel.P.getLevel()) {
+ addRunClasspathFiles(classPathSupplier.apply(minAPILevel, keepRules, shrink));
+ }
+ return self();
+ }
+
public CR enableRuntimeAssertions() {
assert getBackend() == Backend.CF;
if (!this.vmArguments.contains("-ea")) {
diff --git a/src/test/java/com/android/tools/r8/desugar/corelib/CoreLibDesugarTestBase.java b/src/test/java/com/android/tools/r8/desugar/corelib/CoreLibDesugarTestBase.java
index ab28e74..9f708ce 100644
--- a/src/test/java/com/android/tools/r8/desugar/corelib/CoreLibDesugarTestBase.java
+++ b/src/test/java/com/android/tools/r8/desugar/corelib/CoreLibDesugarTestBase.java
@@ -26,6 +26,7 @@
private static Map<CacheEntry, TestCompileResult> computedLibraryCache =
new ConcurrentHashMap<>();
+ @Deprecated
protected boolean requiresCoreLibDesugaring(TestParameters parameters) {
// TODO(b/134732760): Use the two other APIS instead.
return requiresEmulatedInterfaceCoreLibDesugaring(parameters)
@@ -40,6 +41,10 @@
return parameters.getApiLevel().getLevel() < AndroidApiLevel.P.getLevel();
}
+ protected boolean requiresAnyCoreLibDesugaring(TestParameters parameters) {
+ return requiresRetargetCoreLibMemberDesugaring(parameters);
+ }
+
protected Path buildDesugaredLibrary(AndroidApiLevel apiLevel) throws RuntimeException {
return buildDesugaredLibrary(apiLevel, "", false);
}
diff --git a/src/test/java/com/android/tools/r8/desugar/corelib/CustomCollectionTest.java b/src/test/java/com/android/tools/r8/desugar/corelib/CustomCollectionTest.java
index 445b1dd..5682d45 100644
--- a/src/test/java/com/android/tools/r8/desugar/corelib/CustomCollectionTest.java
+++ b/src/test/java/com/android/tools/r8/desugar/corelib/CustomCollectionTest.java
@@ -9,7 +9,8 @@
import com.android.tools.r8.D8TestRunResult;
import com.android.tools.r8.TestParameters;
-import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.BooleanUtils;
+import com.android.tools.r8.utils.Box;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import com.android.tools.r8.utils.codeinspector.InstructionSubject;
import com.android.tools.r8.utils.codeinspector.InstructionSubject.JumboStringMode;
@@ -22,18 +23,22 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
@RunWith(Parameterized.class)
public class CustomCollectionTest extends CoreLibDesugarTestBase {
private final TestParameters parameters;
+ private final boolean shrinkCoreLibrary;
- @Parameterized.Parameters(name = "{0}")
- public static TestParametersCollection data() {
- return getTestParameters().withDexRuntimes().withAllApiLevels().build();
+ @Parameters(name = "{1}, shrinkCoreLibrary: {0}")
+ public static List<Object[]> data() {
+ return buildParameters(
+ BooleanUtils.values(), getTestParameters().withDexRuntimes().withAllApiLevels().build());
}
- public CustomCollectionTest(TestParameters parameters) {
+ public CustomCollectionTest(boolean shrinkDesugaredLibrary, TestParameters parameters) {
+ this.shrinkCoreLibrary = shrinkDesugaredLibrary;
this.parameters = parameters;
}
@@ -42,15 +47,23 @@
@Test
public void testCustomCollection() throws Exception {
+ Box<String> keepRulesHolder = new Box<>("");
D8TestRunResult d8TestRunResult =
testForD8()
.addInnerClasses(CustomCollectionTest.class)
.setMinApi(parameters.getApiLevel())
+ .addOptionsModification(
+ options ->
+ options.testing.desugaredLibraryKeepRuleConsumer =
+ (string, handler) -> keepRulesHolder.set(keepRulesHolder.get() + string))
.enableCoreLibraryDesugaring(parameters.getApiLevel())
.compile()
.inspect(this::assertCustomCollectionCallsCorrect)
.addDesugaredCoreLibraryRunClassPath(
- this::buildDesugaredLibrary, parameters.getApiLevel())
+ this::buildDesugaredLibrary,
+ parameters.getApiLevel(),
+ keepRulesHolder.get(),
+ shrinkCoreLibrary)
.run(parameters.getRuntime(), EXECUTOR)
.assertSuccess();
if (requiresEmulatedInterfaceCoreLibDesugaring(parameters)) {
diff --git a/src/test/java/com/android/tools/r8/desugar/corelib/DesugaredLibraryContentTest.java b/src/test/java/com/android/tools/r8/desugar/corelib/DesugaredLibraryContentTest.java
index 20d4868..b130ee9 100644
--- a/src/test/java/com/android/tools/r8/desugar/corelib/DesugaredLibraryContentTest.java
+++ b/src/test/java/com/android/tools/r8/desugar/corelib/DesugaredLibraryContentTest.java
@@ -7,18 +7,11 @@
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static junit.framework.TestCase.assertTrue;
import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
-import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.ir.desugar.BackportedMethodRewriter;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
-import com.android.tools.r8.utils.codeinspector.FoundClassSubject;
-import com.android.tools.r8.utils.codeinspector.FoundMethodSubject;
-import com.android.tools.r8.utils.codeinspector.InstructionSubject;
-import com.google.common.collect.ImmutableSet;
-import java.util.stream.Collectors;
import org.junit.Assume;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -39,8 +32,8 @@
}
@Test
- public void test() throws Exception {
- Assume.assumeTrue(requiresCoreLibDesugaring(parameters));
+ public void testDesugaredLibraryContent() throws Exception {
+ Assume.assumeTrue(requiresAnyCoreLibDesugaring(parameters));
CodeInspector inspector = new CodeInspector(buildDesugaredLibrary(parameters.getApiLevel()));
assertThat(inspector.clazz("j$.util.Optional"), isPresent());
assertThat(inspector.clazz("j$.util.OptionalInt"), isPresent());
@@ -48,82 +41,14 @@
assertThat(inspector.clazz("j$.util.OptionalDouble"), isPresent());
assertThat(inspector.clazz("j$.util.function.Function"), isPresent());
assertThat(inspector.clazz("j$.time.Clock"), isPresent());
-
- ImmutableSet interfaces =
- ImmutableSet.of(
- "java.util.List",
- "java.util.concurrent.TransferQueue",
- "java.util.concurrent.ConcurrentMap",
- "java.util.Queue",
- "java.util.concurrent.ConcurrentNavigableMap",
- "java.util.NavigableMap",
- "java.util.ListIterator",
- "java.util.SortedMap",
- "java.util.Set",
- "java.util.List",
- "java.util.NavigableSet",
- "java.util.SortedSet",
- "java.util.Deque",
- "java.util.concurrent.BlockingDeque",
- "java.util.concurrent.BlockingQueue");
-
- // TODO(134732760): Remove this debugging code.
inspector
.allClasses()
.forEach(
- clazz -> {
- if (clazz.getOriginalName().startsWith("java.")
- && !interfaces.contains(clazz.getOriginalName())) {
- System.out.println(clazz.getOriginalName());
- }
- });
-
- if (requiresCoreLibDesugaring(parameters)) {
- InstructionSubject long8Invoke =
- inspector
- .clazz("j$.util.stream.LongStream$-CC")
- .uniqueMethodWithName("range")
- .streamInstructions()
- .filter(InstructionSubject::isInvokeStatic)
- .collect(Collectors.toList())
- .get(1);
- assertFalse(long8Invoke.toString().contains("Long8;->"));
- assertTrue(long8Invoke.toString().contains("backportedMethods"));
- for (FoundClassSubject clazz : inspector.allClasses()) {
- if (!(clazz.getOriginalName().equals("j$.lang.Long8")
- || clazz.getOriginalName().equals("j$.lang.Integer8")
- || clazz.getOriginalName().equals("j$.lang.Double8"))) {
- for (FoundMethodSubject method : clazz.allMethods()) {
- if (!method.isAbstract()) {
- assertTrue(method.streamInstructions().noneMatch(instr -> instr.isInvoke() && (
- instr.toString().contains("Double8")
- || instr.toString().contains("Integer8")
- || instr.toString().contains("Long8"))
- ));
- }
- }
- }
- }
- }
-
- // TODO(134732760): This should be a 0 count.
- assertEquals(
- requiresCoreLibDesugaring(parameters) ? 0 : 5,
- inspector.allClasses().stream()
- .map(ClassSubject::getOriginalName)
- .filter(name -> name.startsWith("java."))
- .filter(name -> !interfaces.contains(name))
- .count());
-
- // TODO(134732760): Remove this when above is a 0 count.
- assertEquals(
- requiresCoreLibDesugaring(parameters) ? 0 : 5,
- inspector.allClasses().stream()
- .map(ClassSubject::getOriginalName)
- .filter(name -> name.startsWith("java."))
- .filter(name -> !interfaces.contains(name))
- .filter(name -> !name.contains("$-CC"))
- .filter(name -> !name.contains("-$$Lambda"))
- .count());
+ clazz ->
+ assertTrue(
+ clazz.getOriginalName().startsWith("j$.")
+ || clazz
+ .getOriginalName()
+ .contains(BackportedMethodRewriter.UTILITY_CLASS_NAME_PREFIX)));
}
}
diff --git a/src/test/java/com/android/tools/r8/desugar/corelib/EmulatedInterfacesTest.java b/src/test/java/com/android/tools/r8/desugar/corelib/EmulatedInterfacesTest.java
index cc3a1b0..5e241bb 100644
--- a/src/test/java/com/android/tools/r8/desugar/corelib/EmulatedInterfacesTest.java
+++ b/src/test/java/com/android/tools/r8/desugar/corelib/EmulatedInterfacesTest.java
@@ -44,7 +44,7 @@
@Test
public void testEmulatedInterface() throws Exception {
- Assume.assumeTrue((requiresCoreLibDesugaring(parameters)));
+ Assume.assumeTrue(requiresEmulatedInterfaceCoreLibDesugaring(parameters));
CodeInspector inspector =
new CodeInspector(
buildDesugaredLibrary(
