Merge "Add service loader test"
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 5333125..8f923b0 100644
--- a/src/main/java/com/android/tools/r8/naming/Minifier.java
+++ b/src/main/java/com/android/tools/r8/naming/Minifier.java
@@ -24,6 +24,7 @@
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.Timing;
import com.google.common.collect.ImmutableMap;
+import java.util.HashMap;
import java.util.IdentityHashMap;
import java.util.Map;
import java.util.Set;
@@ -91,6 +92,8 @@
private final AppInfo appInfo;
private final Map<String, String> packageRenaming;
private final Map<DexItem, DexString> renaming = new IdentityHashMap<>();
+ // This set is only used for asserting no duplicated names.
+ private final Map<DexString, DexType> renamedTypesForVerification;
private MinifiedRenaming(
ClassRenaming classRenaming,
@@ -103,6 +106,10 @@
renaming.putAll(methodRenaming.renaming);
renaming.putAll(methodRenaming.callSiteRenaming);
renaming.putAll(fieldRenaming.renaming);
+ renamedTypesForVerification = new HashMap<>();
+ for (Map.Entry<DexType, DexString> entry : classRenaming.classRenaming.entrySet()) {
+ renamedTypesForVerification.put(entry.getValue(), entry.getKey());
+ }
}
@Override
@@ -112,7 +119,18 @@
@Override
public DexString lookupDescriptor(DexType type) {
- return renaming.getOrDefault(type, type.descriptor);
+ DexString dexString = renaming.get(type);
+ if (dexString != null) {
+ return dexString;
+ }
+ assert type.isPrimitiveType()
+ || type.isVoidType()
+ || !renamedTypesForVerification.containsKey(type.descriptor)
+ : "Duplicate minified type '"
+ + type.descriptor
+ + "' already mapped for: "
+ + renamedTypesForVerification.get(type.descriptor);
+ return type.descriptor;
}
@Override
diff --git a/src/main/java/com/android/tools/r8/references/Reference.java b/src/main/java/com/android/tools/r8/references/Reference.java
index 0e3d988..decd262 100644
--- a/src/main/java/com/android/tools/r8/references/Reference.java
+++ b/src/main/java/com/android/tools/r8/references/Reference.java
@@ -7,6 +7,7 @@
import com.android.tools.r8.utils.DescriptorUtils;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.MapMaker;
+import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.util.List;
@@ -150,6 +151,17 @@
returnType == Void.TYPE ? null : typeFromClass(returnType));
}
+ /** Get a method reference from a Java reflection constructor. */
+ public static MethodReference methodFromMethod(Constructor<?> method) {
+ Class<?> holderClass = method.getDeclaringClass();
+ Class<?>[] parameterTypes = method.getParameterTypes();
+ ImmutableList.Builder<TypeReference> builder = ImmutableList.builder();
+ for (Class<?> parameterType : parameterTypes) {
+ builder.add(typeFromClass(parameterType));
+ }
+ return method(classFromClass(holderClass), "<init>", builder.build(), null);
+ }
+
/** Get a field reference from its full reference specification. */
public static FieldReference field(
ClassReference holderClass, String fieldName, TypeReference fieldType) {
diff --git a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
index f011931..6d24a8c 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -307,6 +307,10 @@
this.options = options;
}
+ private static <T> SetWithReason<T> newSetWithoutReasonReporter() {
+ return new SetWithReason<>((f, r) -> {});
+ }
+
private void enqueueRootItems(Map<DexReference, Set<ProguardKeepRule>> items) {
items.entrySet().forEach(this::enqueueRootItem);
}
@@ -1097,6 +1101,7 @@
SetWithReason<DexEncodedField> reachableFields = reachableInstanceFields.get(type);
if (reachableFields != null) {
for (DexEncodedField field : reachableFields.getItems()) {
+ // TODO(b/120959039): Should the reason this field is reachable come from the set?
markInstanceFieldAsLive(field, KeepReason.reachableFromLiveType(type));
}
}
@@ -1241,14 +1246,14 @@
if (encodedField.accessFlags.isStatic()) {
markStaticFieldAsLive(encodedField.field, reason);
} else {
- SetWithReason<DexEncodedField> reachable =
- reachableInstanceFields.computeIfAbsent(
- encodedField.field.clazz, ignore -> new SetWithReason<>((f, r) -> {}));
- // TODO(b/120959039): The reachable.add test might be hiding other paths to the field.
- if (reachable.add(encodedField, reason)
- && isInstantiatedOrHasInstantiatedSubtype(encodedField.field.clazz)) {
+ if (isInstantiatedOrHasInstantiatedSubtype(encodedField.field.clazz)) {
// We have at least one live subtype, so mark it as live.
markInstanceFieldAsLive(encodedField, reason);
+ } else {
+ // Add the field to the reachable set if the type later becomes instantiated.
+ reachableInstanceFields
+ .computeIfAbsent(encodedField.field.clazz, ignore -> newSetWithoutReasonReporter())
+ .add(encodedField, reason);
}
}
}
@@ -1292,7 +1297,7 @@
// TODO(b/120959039): The reachable.add test might be hiding other paths to the method.
SetWithReason<DexEncodedMethod> reachable =
reachableVirtualMethods.computeIfAbsent(
- encodedMethod.method.holder, (ignore) -> new SetWithReason<>((m, r) -> {}));
+ encodedMethod.method.holder, ignore -> newSetWithoutReasonReporter());
if (reachable.add(encodedMethod, reason)) {
// Abstract methods cannot be live.
if (!encodedMethod.accessFlags.isAbstract()) {
diff --git a/src/test/java/com/android/tools/r8/naming/MinifierTest.java b/src/test/java/com/android/tools/r8/naming/MinifierTest.java
index 473ca53..adf74ce 100644
--- a/src/test/java/com/android/tools/r8/naming/MinifierTest.java
+++ b/src/test/java/com/android/tools/r8/naming/MinifierTest.java
@@ -3,13 +3,16 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.naming;
+import static junit.framework.TestCase.assertTrue;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.fail;
import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexItemFactory;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexType;
+import com.android.tools.r8.naming.Minifier.MinifiedRenaming;
import com.android.tools.r8.utils.ListUtils;
import com.android.tools.r8.utils.Timing;
import java.nio.file.Paths;
@@ -40,6 +43,25 @@
inspection.accept(dexItemFactory, naming);
}
+ @Test
+ public void ensureClassesAddedToRenamingOrNoClashTest() throws Exception {
+ MinifiedRenaming naming =
+ (MinifiedRenaming) runMinifier(ListUtils.map(keepRulesFiles, Paths::get));
+ // Create a type that exists.
+ String existingType = "La/c;";
+ DexType d = dexItemFactory.createType(existingType);
+ try {
+ naming.lookupDescriptor(d);
+ } catch (AssertionError ae) {
+ assertTrue(
+ ae.getMessage()
+ .startsWith(
+ "Duplicate minified type '" + existingType + "' already mapped for: naming001."));
+ return;
+ }
+ fail("Should have thrown an error.");
+ }
+
@Parameters(name = "test: {0} keep: {1}")
public static Collection<Object[]> data() {
List<String> tests = Arrays.asList("naming001");
diff --git a/src/test/java/com/android/tools/r8/naming/b124357885/B124357885Test.java b/src/test/java/com/android/tools/r8/naming/b124357885/B124357885Test.java
new file mode 100644
index 0000000..3112ae7
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b124357885/B124357885Test.java
@@ -0,0 +1,97 @@
+// Copyright (c) 2019, 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.b124357885;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isRenamed;
+import static org.hamcrest.CoreMatchers.allOf;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.not;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.graph.DexAnnotationElement;
+import com.android.tools.r8.graph.DexValue;
+import com.android.tools.r8.graph.DexValue.DexValueArray;
+import com.android.tools.r8.graph.DexValue.DexValueString;
+import com.android.tools.r8.utils.DescriptorUtils;
+import com.android.tools.r8.utils.codeinspector.AnnotationSubject;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import java.lang.reflect.Method;
+import java.lang.reflect.ParameterizedType;
+import org.junit.Test;
+
+public class B124357885Test extends TestBase {
+
+ private void checkSignatureAnnotation(AnnotationSubject signature) {
+ DexAnnotationElement[] elements = signature.getAnnotation().elements;
+ assertEquals(1, elements.length);
+ assertEquals("value", elements[0].name.toString());
+ assertTrue(elements[0].value instanceof DexValueArray);
+ DexValueArray array = (DexValueArray) elements[0].value;
+ StringBuilder builder = new StringBuilder();
+ for (DexValue value : array.getValues()) {
+ assertTrue(value instanceof DexValueString);
+ builder.append(((DexValueString) value).value);
+ }
+ // TODO(124357885): This should be the minified name for FooImpl instead of Foo.
+ String fooDescriptor = DescriptorUtils.javaTypeToDescriptor(Foo.class.getTypeName());
+ StringBuilder expected =
+ new StringBuilder()
+ .append("()")
+ .append(fooDescriptor.substring(0, fooDescriptor.length() - 1)) // Remove the ;.
+ .append("<Ljava/lang/String;>")
+ .append(";"); // Add the ; here.
+ assertEquals(expected.toString(), builder.toString());
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(Backend.DEX)
+ .addProgramClasses(Main.class, Service.class, Foo.class, FooImpl.class)
+ .addKeepMainRule(Main.class)
+ .addKeepRules("-keepattributes Signature,InnerClasses,EnclosingMethod")
+ .compile()
+ .inspect(inspector -> {
+ assertThat(inspector.clazz(Main.class), allOf(isPresent(), not(isRenamed())));
+ assertThat(inspector.clazz(Service.class), allOf(isPresent(), isRenamed()));
+ assertThat(inspector.clazz(Foo.class), not(isPresent()));
+ assertThat(inspector.clazz(FooImpl.class), allOf(isPresent(), isRenamed()));
+ // TODO(124477502): Using uniqueMethodWithName("fooList") does not work.
+ assertEquals(1, inspector.clazz(Service.class).allMethods().size());
+ MethodSubject fooList = inspector.clazz(Service.class).allMethods().get(0);
+ AnnotationSubject signature = fooList.annotation("dalvik.annotation.Signature");
+ checkSignatureAnnotation(signature);
+ })
+ .run(Main.class)
+ .assertFailureWithErrorThatMatches(
+ containsString(
+ "java.lang.ClassNotFoundException: "
+ + "Didn't find class \"com.android.tools.r8.naming.b124357885.Foo\""));
+ }
+}
+
+class Main {
+ public static void main(String... args) throws Exception {
+ Method method = Service.class.getMethod("fooList");
+ ParameterizedType type = (ParameterizedType) method.getGenericReturnType();
+ Class<?> rawType = (Class<?>) type.getRawType();
+ System.out.println(rawType.getName());
+
+ // Convince R8 we only use subtypes to get class merging of Foo into FooImpl.
+ Foo<String> foo = new FooImpl<>();
+ System.out.println(foo);
+ }
+}
+
+interface Service {
+ Foo<String> fooList();
+}
+
+interface Foo<T> {}
+
+class FooImpl<T> implements Foo<T> {}
\ No newline at end of file
diff --git a/src/test/java/com/android/tools/r8/shaking/keptgraph/KeptByFieldReflectionTest.java b/src/test/java/com/android/tools/r8/shaking/keptgraph/KeptByFieldReflectionTest.java
index e518181..3ba9b99 100644
--- a/src/test/java/com/android/tools/r8/shaking/keptgraph/KeptByFieldReflectionTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/keptgraph/KeptByFieldReflectionTest.java
@@ -8,9 +8,7 @@
@Keep
public class KeptByFieldReflectionTest {
- // TODO(b/123262024): This field must be kept un-initialized. Otherwise the "-whyareyoukeeping"
- // output tested will hit the initialization in <init> and not the reflective access.
- public int foo;
+ public int foo = 42;
public static void main(String[] args) throws Exception {
// Due to b/123210548 the object cannot be created by a reflective newInstance call.
diff --git a/src/test/java/com/android/tools/r8/shaking/keptgraph/KeptByFieldReflectionTestRunner.java b/src/test/java/com/android/tools/r8/shaking/keptgraph/KeptByFieldReflectionTestRunner.java
index 62dcebe..6ba1f08 100644
--- a/src/test/java/com/android/tools/r8/shaking/keptgraph/KeptByFieldReflectionTestRunner.java
+++ b/src/test/java/com/android/tools/r8/shaking/keptgraph/KeptByFieldReflectionTestRunner.java
@@ -30,7 +30,7 @@
private static final Class<?> CLASS = KeptByFieldReflectionTest.class;
private static final Collection<Class<?>> CLASSES = Arrays.asList(CLASS);
- private final String EXPECTED_STDOUT = StringUtils.lines("got foo: 0");
+ private final String EXPECTED_STDOUT = StringUtils.lines("got foo: 42");
private final String EXPECTED_WHYAREYOUKEEPING =
StringUtils.lines(
@@ -55,6 +55,7 @@
public void test() throws Exception {
MethodReference mainMethod = methodFromMethod(CLASS.getDeclaredMethod("main", String[].class));
FieldReference fooField = fieldFromField(CLASS.getDeclaredField("foo"));
+ MethodReference fooInit = methodFromMethod(CLASS.getDeclaredConstructor());
if (backend == Backend.CF) {
testForJvm().addProgramClasses(CLASSES).run(CLASS).assertSuccessWithOutput(EXPECTED_STDOUT);
@@ -80,6 +81,11 @@
inspector.method(mainMethod).assertNotRenamed().assertKeptBy(keepMain);
+ // The field is primarily kept by the reflective lookup in main.
inspector.field(fooField).assertRenamed().assertReflectedFrom(mainMethod);
+
+ // The field is also kept by the write in Foo.<init>.
+ // We may want to change that behavior. See b/124428834.
+ inspector.field(fooField).assertRenamed().assertKeptBy(inspector.method(fooInit));
}
}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java
index 8e24088..87ae2ec 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java
@@ -116,4 +116,9 @@
public boolean hasLocalVariableTable() {
throw new Unreachable("Cannot determine if an absent method has a local variable table");
}
+
+ @Override
+ public AnnotationSubject annotation(String name) {
+ return new AbsentAnnotationSubject();
+ }
}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java
index 796532d..6581a47 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java
@@ -12,6 +12,7 @@
import com.android.tools.r8.graph.AppInfo;
import com.android.tools.r8.graph.CfCode;
import com.android.tools.r8.graph.Code;
+import com.android.tools.r8.graph.DexAnnotation;
import com.android.tools.r8.graph.DexCode;
import com.android.tools.r8.graph.DexDebugEvent;
import com.android.tools.r8.graph.DexDebugInfo;
@@ -283,4 +284,13 @@
public String toString() {
return dexMethod.toSourceString();
}
+
+ @Override
+ public AnnotationSubject annotation(String name) {
+ DexAnnotation annotation = codeInspector.findAnnotation(name, dexMethod.annotations);
+ return annotation == null
+ ? new AbsentAnnotationSubject()
+ : new FoundAnnotationSubject(annotation);
+ }
+
}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java
index 8fb4001..1575372 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java
@@ -72,4 +72,6 @@
public boolean isMethodSubject() {
return true;
}
+
+ public abstract AnnotationSubject annotation(String name);
}
diff --git a/tools/as_utils.py b/tools/as_utils.py
index 7a6528a..817f7ed 100644
--- a/tools/as_utils.py
+++ b/tools/as_utils.py
@@ -8,8 +8,6 @@
import os
import shutil
-import utils
-
def add_r8_dependency(checkout_dir, temp_dir, minified):
build_file = os.path.join(checkout_dir, 'build.gradle')
assert os.path.isfile(build_file), (
@@ -112,7 +110,7 @@
assert 'transformDexArchiveWithDexMergerFor' not in x
return 'transformClassesAndResourcesWithR8For' in x
- assert shrinker == 'proguard'
+ assert shrinker == 'pg'
return ('transformClassesAndResourcesWithProguard' in x
or 'transformClassesWithDexBuilderFor' in x
or 'transformDexArchiveWithDexMergerFor' in x)
diff --git a/tools/run_on_as_app.py b/tools/run_on_as_app.py
index 3b21ba9..d6dd647 100755
--- a/tools/run_on_as_app.py
+++ b/tools/run_on_as_app.py
@@ -728,13 +728,15 @@
if not options.no_build or options.golem:
gradle.RunGradle(['r8', 'r8lib'])
- assert os.path.isfile(utils.R8_JAR), 'Cannot build without r8.jar'
- assert os.path.isfile(utils.R8LIB_JAR), 'Cannot build without r8lib.jar'
-
# Make a copy of r8.jar and r8lib.jar such that they stay the same for
# the entire execution of this script.
- shutil.copyfile(utils.R8_JAR, os.path.join(temp_dir, 'r8.jar'))
- shutil.copyfile(utils.R8LIB_JAR, os.path.join(temp_dir, 'r8lib.jar'))
+ all_shrinkers_enabled = (options.shrinker == None)
+ if all_shrinkers_enabled or 'r8-nolib' in options.shrinker:
+ assert os.path.isfile(utils.R8_JAR), 'Cannot build without r8.jar'
+ shutil.copyfile(utils.R8_JAR, os.path.join(temp_dir, 'r8.jar'))
+ if all_shrinkers_enabled or 'r8' in options.shrinker:
+ assert os.path.isfile(utils.R8LIB_JAR), 'Cannot build without r8lib.jar'
+ shutil.copyfile(utils.R8LIB_JAR, os.path.join(temp_dir, 'r8lib.jar'))
result_per_shrinker_per_app = {}
