Merge "Keep fewer default constructors in Proguard compat mode"
diff --git a/src/main/java/com/android/tools/r8/code/CheckCast.java b/src/main/java/com/android/tools/r8/code/CheckCast.java
index d3576c1..9416a7b 100644
--- a/src/main/java/com/android/tools/r8/code/CheckCast.java
+++ b/src/main/java/com/android/tools/r8/code/CheckCast.java
@@ -39,7 +39,7 @@
@Override
public void registerUse(UseRegistry registry) {
- registry.registerTypeReference(getType());
+ registry.registerCheckCast(getType());
}
public DexType getType() {
diff --git a/src/main/java/com/android/tools/r8/code/ConstClass.java b/src/main/java/com/android/tools/r8/code/ConstClass.java
index f140022..ef8b257 100644
--- a/src/main/java/com/android/tools/r8/code/ConstClass.java
+++ b/src/main/java/com/android/tools/r8/code/ConstClass.java
@@ -39,7 +39,7 @@
@Override
public void registerUse(UseRegistry registry) {
- registry.registerTypeReference(getType());
+ registry.registerConstClass(getType());
}
public DexType getType() {
diff --git a/src/main/java/com/android/tools/r8/graph/UseRegistry.java b/src/main/java/com/android/tools/r8/graph/UseRegistry.java
index 78b2f56..48ef91d 100644
--- a/src/main/java/com/android/tools/r8/graph/UseRegistry.java
+++ b/src/main/java/com/android/tools/r8/graph/UseRegistry.java
@@ -27,6 +27,14 @@
public abstract boolean registerTypeReference(DexType type);
+ public boolean registerConstClass(DexType type) {
+ return registerTypeReference(type);
+ }
+
+ public boolean registerCheckCast(DexType type) {
+ return registerTypeReference(type);
+ }
+
public void registerMethodHandle(DexMethodHandle methodHandle) {
switch (methodHandle.type) {
case INSTANCE_GET:
diff --git a/src/main/java/com/android/tools/r8/jar/JarRegisterEffectsVisitor.java b/src/main/java/com/android/tools/r8/jar/JarRegisterEffectsVisitor.java
index f9a7f49..37c9aea 100644
--- a/src/main/java/com/android/tools/r8/jar/JarRegisterEffectsVisitor.java
+++ b/src/main/java/com/android/tools/r8/jar/JarRegisterEffectsVisitor.java
@@ -35,6 +35,8 @@
DexType type = application.getTypeFromName(name);
if (opcode == org.objectweb.asm.Opcodes.NEW) {
registry.registerNewInstance(type);
+ } else if (opcode == Opcodes.CHECKCAST) {
+ registry.registerCheckCast(type);
} else {
registry.registerTypeReference(type);
}
@@ -51,7 +53,7 @@
// Nothing to register for method type, it represents only a prototype not associated with a
// method name.
if (((Type) cst).getSort() != Type.METHOD) {
- registry.registerTypeReference(application.getType((Type) cst));
+ registry.registerConstClass(application.getType((Type) cst));
}
} else if (cst instanceof Handle) {
registerMethodHandleType((Handle) cst);
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 f36f8a6..9aa73de 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -203,11 +203,30 @@
}
private void enqueueRootItems(Map<DexItem, ProguardKeepRule> items) {
- workList.addAll(
- items.entrySet().stream().map(Action::forRootItem).collect(Collectors.toList()));
+ items.entrySet().forEach(this::enqueueRootItem);
pinnedItems.addAll(items.keySet());
}
+ private void enqueueRootItem(Map.Entry<DexItem, ProguardKeepRule> root) {
+ DexItem item = root.getKey();
+ KeepReason reason = KeepReason.dueToKeepRule(root.getValue());
+ if (item instanceof DexClass) {
+ DexClass clazz = (DexClass) item;
+ workList.add(Action.markInstantiated(clazz, reason));
+ if (options.forceProguardCompatibility && clazz.hasDefaultInitializer()) {
+ ProguardKeepRule rule = ProguardConfigurationUtils.buildDefaultInitializerKeepRule(clazz);
+ proguardCompatibilityWorkList.add(Action.markMethodLive(
+ clazz.getDefaultInitializer(), KeepReason.dueToProguardCompatibilityKeepRule(rule)));
+ }
+ } else if (item instanceof DexEncodedField) {
+ workList.add(Action.markFieldKept((DexEncodedField) item, reason));
+ } else if (item instanceof DexEncodedMethod) {
+ workList.add(Action.markMethodKept((DexEncodedMethod) item, reason));
+ } else {
+ throw new IllegalArgumentException(item.toString());
+ }
+ }
+
//
// Things to do with registering events. This is essentially the interface for byte-code
// traversals.
@@ -372,6 +391,16 @@
}
@Override
+ public boolean registerConstClass(DexType type) {
+ return registerConstClassOrCheckCast(type);
+ }
+
+ @Override
+ public boolean registerCheckCast(DexType type) {
+ return registerConstClassOrCheckCast(type);
+ }
+
+ @Override
public boolean registerTypeReference(DexType type) {
DexType baseType = type.toBaseType(appInfo.dexItemFactory);
if (baseType.isClassType()) {
@@ -380,6 +409,25 @@
}
return false;
}
+
+ private boolean registerConstClassOrCheckCast(DexType type) {
+ if (options.forceProguardCompatibility) {
+ DexType baseType = type.toBaseType(appInfo.dexItemFactory);
+ if (baseType.isClassType()) {
+ DexClass baseClass = appInfo.definitionFor(baseType);
+ if (baseClass != null) {
+ markClassAsInstantiatedWithCompatRule(baseClass);
+ } else {
+ // This handles reporting of missing classes.
+ markTypeAsLive(baseType);
+ }
+ return true;
+ }
+ return false;
+ } else {
+ return registerTypeReference(type);
+ }
+ }
}
private DexMethod getInvokeSuperTarget(DexMethod method, DexEncodedMethod currentMethod) {
@@ -439,14 +487,12 @@
annotations.forEach(this::handleAnnotationOfLiveType);
}
- // Add all dependent static members to the workqueue.
- enqueueRootItems(rootSet.getDependentStaticMembers(type));
-
- // For Proguard compatibility keep the default initializer for live types.
if (options.forceProguardCompatibility) {
- if (holder.isProgramClass() && holder.hasDefaultInitializer()) {
- markClassAsInstantiatedWithCompatRule(holder);
- }
+ // Add all dependent members to the workqueue.
+ enqueueRootItems(rootSet.getDependentItems(type));
+ } else {
+ // Add all dependent static members to the workqueue.
+ enqueueRootItems(rootSet.getDependentStaticMembers(type));
}
}
}
@@ -566,6 +612,7 @@
if (!instantiatedTypes.add(clazz.type, reason)) {
return;
}
+
collectProguardCompatibilityRule(reason);
if (Log.ENABLED) {
Log.verbose(getClass(), "Class `%s` is instantiated, processing...", clazz);
@@ -1286,20 +1333,6 @@
return new Action(Kind.MARK_FIELD_KEPT, method, null, reason);
}
- public static Action forRootItem(Map.Entry<DexItem, ProguardKeepRule> root) {
- DexItem item = root.getKey();
- KeepReason reason = KeepReason.dueToKeepRule(root.getValue());
- if (item instanceof DexClass) {
- return markInstantiated((DexClass) item, reason);
- } else if (item instanceof DexEncodedField) {
- return markFieldKept((DexEncodedField) item, reason);
- } else if (item instanceof DexEncodedMethod) {
- return markMethodKept((DexEncodedMethod) item, reason);
- } else {
- throw new IllegalArgumentException(item.toString());
- }
- }
-
private enum Kind {
MARK_REACHABLE_VIRTUAL,
MARK_REACHABLE_INTERFACE,
diff --git a/src/test/java/com/android/tools/r8/shaking/forceproguardcompatibility/defaultctor/ImplicitlyKeptDefaultConstructorTest.java b/src/test/java/com/android/tools/r8/shaking/forceproguardcompatibility/defaultctor/ImplicitlyKeptDefaultConstructorTest.java
index 39e804a..4cb7750 100644
--- a/src/test/java/com/android/tools/r8/shaking/forceproguardcompatibility/defaultctor/ImplicitlyKeptDefaultConstructorTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/forceproguardcompatibility/defaultctor/ImplicitlyKeptDefaultConstructorTest.java
@@ -40,6 +40,13 @@
}
}
+class MainCheckCastSubClass {
+
+ public static void main(String[] args) {
+ System.out.println((SubClass) null);
+ }
+}
+
class MainClassForNameSubClass {
public static void main(String[] args) throws Exception{
@@ -164,6 +171,31 @@
}
@Test
+ public void testCheckCast() throws Exception {
+ // Reference to the class constant keeps the default constructor.
+ Class mainClass = MainCheckCastSubClass.class;
+ runTest(
+ mainClass,
+ ImmutableList.of(mainClass, SuperClass.class, SubClass.class),
+ keepMainProguardConfiguration(mainClass),
+ // TODO(74423424): Proguard eliminates the check-cast on null.
+ this::checkAllClassesPresentWithDefaultConstructor,
+ this::checkOnlyMainPresent);
+ }
+
+
+ @Test
+ public void testCheckCastWithoutInlining() throws Exception {
+ // Reference to the class constant keeps the default constructor.
+ Class mainClass = MainCheckCastSubClass.class;
+ runTest(
+ mainClass,
+ ImmutableList.of(mainClass, SuperClass.class, SubClass.class),
+ keepMainProguardConfiguration(mainClass, ImmutableList.of("-dontoptimize")),
+ this::checkAllClassesPresentWithDefaultConstructor);
+ }
+
+ @Test
public void testClassForName() throws Exception {
// Class.forName with a constant string keeps the default constructor.
Class mainClass = MainClassForNameSubClass.class;
@@ -230,7 +262,7 @@
ImmutableList.of(mainClass, StaticFieldNotInitialized.class),
keepMainProguardConfiguration(mainClass),
// TODO(74379749): Proguard does not keep the class with the un-initialized static field.
- this::checkAllClassesPresentWithDefaultConstructor,
+ this::checkAllClassesPresentOnlyMainWithDefaultConstructor,
this::checkOnlyMainPresent);
}
@@ -241,9 +273,6 @@
mainClass,
ImmutableList.of(mainClass, StaticFieldInitialized.class),
keepMainProguardConfiguration(mainClass),
- // TODO(74233021): Proguard does not keep the default constructor for the class with the
- // un-initialized static field.
- this::checkAllClassesPresentWithDefaultConstructor,
this::checkAllClassesPresentOnlyMainWithDefaultConstructor);
}
@@ -265,9 +294,6 @@
mainClass,
ImmutableList.of(mainClass, StaticFieldNotInitialized.class),
keepMainProguardConfiguration(mainClass, ImmutableList.of("-dontoptimize")),
- // TODO(74233021): Proguard does not keep the default constructor for the class with the
- // initialized static field.
- this::checkAllClassesPresentWithDefaultConstructor,
this::checkAllClassesPresentOnlyMainWithDefaultConstructor);
}
@@ -278,9 +304,6 @@
mainClass,
ImmutableList.of(mainClass, StaticFieldInitialized.class),
keepMainProguardConfiguration(mainClass, ImmutableList.of("-dontoptimize")),
- // TODO(74233021): Proguard does not keep the default constructor for the class with the
- // un-initialized static field.
- this::checkAllClassesPresentWithDefaultConstructor,
this::checkAllClassesPresentOnlyMainWithDefaultConstructor);
}
@@ -291,9 +314,6 @@
mainClass,
ImmutableList.of(mainClass, StaticMethod.class),
keepMainProguardConfiguration(mainClass, ImmutableList.of("-dontoptimize")),
- // TODO(74233021): Proguard does not keep the default constructor for the class with the
- // initialized static field.
- this::checkAllClassesPresentWithDefaultConstructor,
this::checkAllClassesPresentOnlyMainWithDefaultConstructor);
}
}
diff --git a/src/test/java/com/android/tools/r8/shaking/keepclassmembers/KeepClassMembersTest.java b/src/test/java/com/android/tools/r8/shaking/keepclassmembers/KeepClassMembersTest.java
index 7904400..7f8e5fb 100644
--- a/src/test/java/com/android/tools/r8/shaking/keepclassmembers/KeepClassMembersTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/keepclassmembers/KeepClassMembersTest.java
@@ -4,26 +4,61 @@
package com.android.tools.r8.shaking.keepclassmembers;
-import static org.junit.Assert.assertEquals;
+import static com.android.tools.r8.utils.DexInspectorMatchers.isAbstract;
+import static com.android.tools.r8.utils.DexInspectorMatchers.isPresent;
+import static org.hamcrest.core.IsNot.not;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
-import com.android.tools.r8.TestBase;
+import com.android.tools.r8.shaking.forceproguardcompatibility.ProguardCompatabilityTestBase;
import com.android.tools.r8.utils.DexInspector;
import com.android.tools.r8.utils.DexInspector.ClassSubject;
+import com.android.tools.r8.utils.DexInspector.FieldSubject;
+import com.android.tools.r8.utils.DexInspector.MethodSubject;
import com.google.common.collect.ImmutableList;
import org.junit.Test;
-public class KeepClassMembersTest extends TestBase {
+public class KeepClassMembersTest extends ProguardCompatabilityTestBase {
- public void runTest(Class mainClass, Class<?> staticClass,
- boolean forceProguardCompatibility) throws Exception {
- boolean staticClassHasDefaultConstructor = true;
- try {
- staticClass.getDeclaredConstructor();
- } catch (NoSuchMethodException e) {
- staticClassHasDefaultConstructor = false;
+ private void check(DexInspector inspector, Class mainClass, Class<?> staticClass,
+ boolean forceProguardCompatibility, boolean fromProguard) {
+ assertTrue(inspector.clazz(mainClass).isPresent());
+ ClassSubject staticClassSubject = inspector.clazz(staticClass);
+ assertThat(staticClassSubject, isPresent());
+ assertThat(staticClassSubject.method("int", "getA", ImmutableList.of()), isPresent());
+ assertThat(staticClassSubject.method("int", "getB", ImmutableList.of()), not(isPresent()));
+ assertThat(staticClassSubject.field("int", "a"), isPresent());
+ assertThat(staticClassSubject.field("int", "b"), isPresent());
+ assertThat(staticClassSubject.field("int", "c"), not(isPresent()));
+ // Neither Proguard not R8 keeps any constructors.
+ staticClassSubject.forAllMethods(method -> assertFalse(method.isInstanceInitializer()));
+ assertThat(staticClassSubject.init(ImmutableList.of()), not(isPresent()));
+ MethodSubject getIMethod = staticClassSubject.method("int", "getI", ImmutableList.of());
+ FieldSubject iField = staticClassSubject.field("int", "i");
+ if (forceProguardCompatibility) {
+ if (fromProguard) {
+ // Proguard keeps the instance method and it code even though the class does not have a
+ // constructor, and therefore cannot be instantiated.
+ assertThat(getIMethod, isPresent());
+ assertThat(iField, isPresent());
+ } else {
+ // Force Proguard compatibility keeps the instance method, but makes it abstract as
+ // the class does not have a constructor, and therefore cannot be instantiated.
+ assertThat(getIMethod, isAbstract());
+ // As the method is abstract the referenced field is not present.
+ assertThat(iField, not(isPresent()));
+ }
+ } else {
+ assertThat(getIMethod, not(isPresent()));
+ assertThat(iField, not(isPresent()));
}
+ assertThat(staticClassSubject.method("int", "getJ", ImmutableList.of()), not(isPresent()));
+ assertThat(staticClassSubject.field("int", "j"), not(isPresent()));
+ }
+
+ private void runTest(Class mainClass, Class<?> staticClass,
+ boolean forceProguardCompatibility) throws Exception {
String proguardConfig = String.join("\n", ImmutableList.of(
"-keepclassmembers class **.PureStatic* {",
" public static int b;",
@@ -35,27 +70,16 @@
"}",
"-dontoptimize", "-dontobfuscate"
));
- DexInspector inspector = new DexInspector(
- compileWithR8(ImmutableList.of(mainClass, staticClass), proguardConfig,
- options -> options.forceProguardCompatibility = forceProguardCompatibility));
- assertTrue(inspector.clazz(mainClass).isPresent());
- ClassSubject staticClassSubject = inspector.clazz(staticClass);
- assertTrue(staticClassSubject.isPresent());
- assertTrue(staticClassSubject.method("int", "getA", ImmutableList.of()).isPresent());
- assertFalse(staticClassSubject.method("int", "getB", ImmutableList.of()).isPresent());
- assertTrue(staticClassSubject.field("int", "a").isPresent());
- assertTrue(staticClassSubject.field("int", "b").isPresent());
- assertFalse(staticClassSubject.field("int", "c").isPresent());
- // Force Proguard compatibility keeps the default constructor if present and then assumes
- // instantiated, hence keeps the instance method as well.
- assertEquals(forceProguardCompatibility && staticClassHasDefaultConstructor,
- staticClassSubject.init(ImmutableList.of()).isPresent());
- assertEquals(forceProguardCompatibility && staticClassHasDefaultConstructor,
- staticClassSubject.method("int", "getI", ImmutableList.of()).isPresent());
- assertEquals(forceProguardCompatibility && staticClassHasDefaultConstructor,
- staticClassSubject.field("int", "i").isPresent());
- assertFalse(staticClassSubject.method("int", "getJ", ImmutableList.of()).isPresent());
- assertFalse(staticClassSubject.field("int", "j").isPresent());
+ DexInspector inspector;
+ inspector = new DexInspector(
+ compileWithR8(ImmutableList.of(mainClass, staticClass), proguardConfig,
+ options -> options.forceProguardCompatibility = forceProguardCompatibility));
+ check(inspector, mainClass, staticClass, forceProguardCompatibility, false);
+
+ if (isRunProguard()) {
+ inspector = runProguard(ImmutableList.of(mainClass, staticClass), proguardConfig);
+ check(inspector, mainClass, staticClass, true, true);
+ }
}
@Test
diff --git a/src/test/java/com/android/tools/r8/utils/DexInspectorMatchers.java b/src/test/java/com/android/tools/r8/utils/DexInspectorMatchers.java
index 8e1db2c..fc9bc29 100644
--- a/src/test/java/com/android/tools/r8/utils/DexInspectorMatchers.java
+++ b/src/test/java/com/android/tools/r8/utils/DexInspectorMatchers.java
@@ -5,6 +5,9 @@
package com.android.tools.r8.utils;
import com.android.tools.r8.utils.DexInspector.ClassSubject;
+import com.android.tools.r8.utils.DexInspector.FieldSubject;
+import com.android.tools.r8.utils.DexInspector.MethodSubject;
+import com.android.tools.r8.utils.DexInspector.Subject;
import com.google.common.collect.ImmutableList;
import org.hamcrest.Description;
import org.hamcrest.Matcher;
@@ -12,22 +15,46 @@
public class DexInspectorMatchers {
- public static Matcher<ClassSubject> isPresent() {
- return new TypeSafeMatcher<ClassSubject>() {
+ public static Matcher<Subject> isPresent() {
+ return new TypeSafeMatcher<Subject>() {
+ private String type(Subject subject) {
+ String type = "<unknown subject type>";
+ if (subject instanceof ClassSubject) {
+ type = "class";
+ } else if (subject instanceof MethodSubject) {
+ type = "method";
+ } else if (subject instanceof FieldSubject) {
+ type = "field";
+ }
+ return type;
+ }
+
+ private String name(Subject subject) {
+ String name = "<unknown>";
+ if (subject instanceof ClassSubject) {
+ name = ((ClassSubject) subject).getOriginalName();
+ } else if (subject instanceof MethodSubject) {
+ name = ((MethodSubject) subject).getOriginalName();
+ } else if (subject instanceof FieldSubject) {
+ name = ((FieldSubject) subject).getOriginalName();
+ }
+ return name;
+ }
+
@Override
- public boolean matchesSafely(final ClassSubject clazz) {
+ public boolean matchesSafely(final Subject clazz) {
return clazz.isPresent();
}
@Override
public void describeTo(final Description description) {
- description.appendText("class present");
+ description.appendText(" present");
}
@Override
- public void describeMismatchSafely(final ClassSubject clazz, Description description) {
+ public void describeMismatchSafely(final Subject subject, Description description) {
description
- .appendText("class ").appendValue(clazz.getOriginalName()).appendText(" was not");
+ .appendText(type(subject) + " ").appendValue(name(subject)).appendText(" was not");
}
};
}
@@ -51,4 +78,24 @@
}
};
}
+
+ public static Matcher<MethodSubject> isAbstract() {
+ return new TypeSafeMatcher<MethodSubject>() {
+ @Override
+ public boolean matchesSafely(final MethodSubject method) {
+ return method.isPresent() && method.isAbstract();
+ }
+
+ @Override
+ public void describeTo(final Description description) {
+ description.appendText("method abstract");
+ }
+
+ @Override
+ public void describeMismatchSafely(final MethodSubject method, Description description) {
+ description
+ .appendText("method ").appendValue(method.getOriginalName()).appendText(" was not");
+ }
+ };
+ }
}
