Handle classes extending merged types in Proguard -if rules
Bug: 110141157
Change-Id: Icbcc79f618a29d6ff79c36fa380b3c95a95eeb1b
diff --git a/src/main/java/com/android/tools/r8/shaking/ProguardMemberRule.java b/src/main/java/com/android/tools/r8/shaking/ProguardMemberRule.java
index b0630ce..5424cc2 100644
--- a/src/main/java/com/android/tools/r8/shaking/ProguardMemberRule.java
+++ b/src/main/java/com/android/tools/r8/shaking/ProguardMemberRule.java
@@ -191,7 +191,7 @@
break;
}
// Type check.
- if (!matches(getType(), originalSignature.type, appView)) {
+ if (!getType().matches(originalSignature.type, appView)) {
break;
}
// Annotations check
@@ -228,7 +228,7 @@
return RootSetBuilder.containsAnnotation(annotation, method.annotations);
case METHOD:
// Check return type.
- if (!matches(type, originalSignature.proto.returnType, appView)) {
+ if (!type.matches(originalSignature.proto.returnType, appView)) {
break;
}
// Fall through for access flags, name and arguments.
@@ -259,7 +259,7 @@
break;
}
for (int i = 0; i < parameters.length; i++) {
- if (!matches(arguments.get(i), parameters[i], appView)) {
+ if (!arguments.get(i).matches(parameters[i], appView)) {
return false;
}
}
@@ -272,18 +272,6 @@
return false;
}
- private static boolean matches(
- ProguardTypeMatcher matcher, DexType type, AppView<? extends AppInfo> appView) {
- if (matcher.matches(type)) {
- return true;
- }
- if (appView.verticallyMergedClasses() != null) {
- return appView.verticallyMergedClasses().getSourcesFor(type).stream()
- .anyMatch(matcher::matches);
- }
- return false;
- }
-
Iterable<ProguardWildcard> getWildcards() {
return Iterables.concat(
ProguardTypeMatcher.getWildcardsOrEmpty(annotation),
diff --git a/src/main/java/com/android/tools/r8/shaking/ProguardTypeMatcher.java b/src/main/java/com/android/tools/r8/shaking/ProguardTypeMatcher.java
index 34ee554..747dea9 100644
--- a/src/main/java/com/android/tools/r8/shaking/ProguardTypeMatcher.java
+++ b/src/main/java/com/android/tools/r8/shaking/ProguardTypeMatcher.java
@@ -5,6 +5,8 @@
import static com.android.tools.r8.utils.DescriptorUtils.javaTypeToDescriptor;
+import com.android.tools.r8.graph.AppInfo;
+import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexItemFactory;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.shaking.ProguardConfigurationParser.IdentifierPatternWithWildcards;
@@ -31,8 +33,21 @@
TYPE
}
+ // Evaluates this matcher on the given type.
public abstract boolean matches(DexType type);
+ // Evaluates this matcher on the given type, and on all types that have been merged into the given
+ // type, if any.
+ public final boolean matches(DexType type, AppView<? extends AppInfo> appView) {
+ if (matches(type)) {
+ return true;
+ }
+ if (appView.verticallyMergedClasses() != null) {
+ return appView.verticallyMergedClasses().getSourcesFor(type).stream().anyMatch(this::matches);
+ }
+ return false;
+ }
+
protected Iterable<ProguardWildcard> getWildcards() {
return Collections::emptyIterator;
}
diff --git a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
index d5d5422..e117462 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
@@ -107,7 +107,10 @@
// TODO(herhut): Warn about broken supertype chain?
return false;
}
- if (name.matches(clazz.type) && containsAnnotation(annotation, clazz.annotations)) {
+ // TODO(b/110141157): Should the vertical class merger move annotations from the source to
+ // the target class? If so, it is sufficient only to apply the annotation-matcher to the
+ // annotations of `class`.
+ if (name.matches(clazz.type, appView) && containsAnnotation(annotation, clazz.annotations)) {
return true;
}
type = clazz.superType;
@@ -386,16 +389,17 @@
if (!satisfyAnnotation(rule, sourceClass)) {
return;
}
- // TODO(b/110141157): Handle the situation where the class in the extends/implements clause
- // has been merged.
- if (rule.hasInheritanceClassName()
- && !satisfyInheritanceRule(sourceClass, this::definitionForWithLiveTypes, rule)) {
- // Try another live type since the current one doesn't satisfy the inheritance rule.
- return;
- }
if (!rule.getClassNames().matches(sourceClass.type)) {
return;
}
+ if (rule.hasInheritanceClassName()) {
+ // Note that, in presence of vertical class merging, we check if the resulting class
+ // (i.e., the target class) satisfies the implements/extends-matcher.
+ if (!satisfyInheritanceRule(targetClass, this::definitionForWithLiveTypes, rule)) {
+ // Try another live type since the current one doesn't satisfy the inheritance rule.
+ return;
+ }
+ }
Collection<ProguardMemberRule> memberKeepRules = rule.getMemberRules();
if (memberKeepRules.isEmpty()) {
materializeIfRule(rule);
@@ -456,6 +460,8 @@
}
private DexClass definitionForWithLiveTypes(DexType type) {
+ assert appView.verticallyMergedClasses() == null
+ || !appView.verticallyMergedClasses().hasBeenMergedIntoSubtype(type);
return liveTypes.contains(type) ? appView.appInfo().definitionFor(type) : null;
}
}
@@ -592,8 +598,7 @@
ProguardTypeMatcher inheritanceClassName = rule.getInheritanceClassName();
ProguardTypeMatcher inheritanceAnnotation = rule.getInheritanceAnnotation();
boolean extendsExpected =
- anySuperTypeMatches(
- clazz.superType, definitionFor, inheritanceClassName, inheritanceAnnotation);
+ satisfyExtendsRule(clazz, definitionFor, inheritanceClassName, inheritanceAnnotation);
boolean implementsExpected = false;
if (!extendsExpected) {
implementsExpected =
@@ -618,6 +623,27 @@
return false;
}
+ private boolean satisfyExtendsRule(
+ DexClass clazz,
+ Function<DexType, DexClass> definitionFor,
+ ProguardTypeMatcher inheritanceClassName,
+ ProguardTypeMatcher inheritanceAnnotation) {
+ if (anySuperTypeMatches(
+ clazz.superType, definitionFor, inheritanceClassName, inheritanceAnnotation)) {
+ return true;
+ }
+
+ // It is possible that this class used to inherit from another class X, but no longer does it,
+ // because X has been merged into `clazz`.
+ if (appView.verticallyMergedClasses() != null) {
+ // TODO(b/110141157): Figure out what to do with annotations. Should the annotations of
+ // the DexClass corresponding to `sourceType` satisfy the `annotation`-matcher?
+ return appView.verticallyMergedClasses().getSourcesFor(clazz.type).stream()
+ .anyMatch(inheritanceClassName::matches);
+ }
+ return false;
+ }
+
private boolean allRulesSatisfied(Collection<ProguardMemberRule> memberKeepRules,
DexClass clazz) {
for (ProguardMemberRule rule : memberKeepRules) {
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/ExtendsMergedTypeDirectlyTest.java b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/ExtendsMergedTypeDirectlyTest.java
new file mode 100644
index 0000000..2523363
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/ExtendsMergedTypeDirectlyTest.java
@@ -0,0 +1,58 @@
+// Copyright (c) 2018, 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.shaking.ifrule.verticalclassmerging;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.core.IsNot.not;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+
+public class ExtendsMergedTypeDirectlyTest extends MergedTypeBaseTest {
+
+ static class TestClass extends C {
+
+ public static void main(String[] args) {
+ System.out.print("Hello world");
+ }
+ }
+
+ public ExtendsMergedTypeDirectlyTest(Backend backend, boolean enableVerticalClassMerging) {
+ super(backend, enableVerticalClassMerging);
+ }
+
+ @Override
+ public Class<?> getTestClass() {
+ return TestClass.class;
+ }
+
+ @Override
+ public String getConditionForProguardIfRule() {
+ // After class merging, TestClass will no longer extend C, but we should still keep the class
+ // Unused in the output.
+ return "-if class **$TestClass extends **$C";
+ }
+
+ @Override
+ public String getExpectedStdout() {
+ return "Hello world";
+ }
+
+ public void inspect(CodeInspector inspector) {
+ super.inspect(inspector);
+
+ if (enableVerticalClassMerging) {
+ // Check that TestClass no longer extends C.
+ ClassSubject testClassSubject = inspector.clazz(TestClass.class);
+ assertThat(testClassSubject, isPresent());
+ assertEquals("java.lang.Object", testClassSubject.getDexClass().superType.toSourceString());
+
+ // Check that C is no longer present.
+ assertThat(inspector.clazz(C.class), not(isPresent()));
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/ExtendsMergedTypeIndirectlyTest.java b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/ExtendsMergedTypeIndirectlyTest.java
new file mode 100644
index 0000000..0b3cb6f
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/ExtendsMergedTypeIndirectlyTest.java
@@ -0,0 +1,50 @@
+// Copyright (c) 2018, 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.shaking.ifrule.verticalclassmerging;
+
+import static org.junit.Assert.assertEquals;
+
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+
+public class ExtendsMergedTypeIndirectlyTest extends MergedTypeBaseTest {
+
+ static class TestClass extends B {
+
+ public static void main(String[] args) {
+ // The instantiation of B prevents it from being merged into TestClass.
+ System.out.print(new B().getClass().getTypeName());
+ }
+ }
+
+ public ExtendsMergedTypeIndirectlyTest(Backend backend, boolean enableVerticalClassMerging) {
+ super(backend, enableVerticalClassMerging);
+ }
+
+ @Override
+ public Class<?> getTestClass() {
+ return TestClass.class;
+ }
+
+ @Override
+ public String getConditionForProguardIfRule() {
+ // After class merging, B will no longer extend A (and therefore, TestClass will no longer
+ // extend A indirectly), but we should still keep the class Unused in the output.
+ return "-if class **$TestClass extends **$A";
+ }
+
+ @Override
+ public String getExpectedStdout() {
+ return B.class.getTypeName();
+ }
+
+ public void inspect(CodeInspector inspector) {
+ super.inspect(inspector);
+
+ // Verify that TestClass still inherits from B.
+ ClassSubject testClassSubject = inspector.clazz(TestClass.class);
+ assertEquals(B.class.getTypeName(), testClassSubject.getDexClass().superType.toSourceString());
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/IfRuleWithVerticalClassMerging.java b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/IfRuleWithVerticalClassMerging.java
index 6311791..18783750 100644
--- a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/IfRuleWithVerticalClassMerging.java
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/IfRuleWithVerticalClassMerging.java
@@ -60,7 +60,7 @@
// TODO(b/110141157):
// - Add tests where fields and methods get renamed due to naming conflicts.
-// - Add tests where the type in a implements/extends clause has changed.
+// - Add tests where the type in a implements clause has changed.
// - Add tests where the then-clause of an -if rule keeps a class that has been merged into another.
@RunWith(Parameterized.class)
public class IfRuleWithVerticalClassMerging extends TestBase {
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedTypeBaseTest.java b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedTypeBaseTest.java
index 853394f..d2ce210 100644
--- a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedTypeBaseTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedTypeBaseTest.java
@@ -5,6 +5,7 @@
package com.android.tools.r8.shaking.ifrule.verticalclassmerging;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
@@ -26,7 +27,8 @@
@RunWith(Parameterized.class)
public abstract class MergedTypeBaseTest extends TestBase {
- private final List<Class> CLASSES = ImmutableList.of(A.class, B.class, C.class, getTestClass());
+ private final List<Class> CLASSES =
+ ImmutableList.of(A.class, B.class, C.class, Unused.class, getTestClass());
static class A {}
@@ -34,8 +36,10 @@
static class C {}
- private final Backend backend;
- private final boolean enableVerticalClassMerging;
+ static class Unused {}
+
+ final Backend backend;
+ final boolean enableVerticalClassMerging;
public MergedTypeBaseTest(Backend backend, boolean enableVerticalClassMerging) {
this.backend = backend;
@@ -58,6 +62,15 @@
public abstract String getExpectedStdout();
+ public void inspect(CodeInspector inspector) {
+ assertThat(inspector.clazz(Unused.class), isPresent());
+
+ // Verify that A is no longer present when vertical class merging is enabled.
+ if (enableVerticalClassMerging) {
+ assertThat(inspector.clazz(A.class), not(isPresent()));
+ }
+ }
+
@Test
public void testIfRule() throws Exception {
String expected = getExpectedStdout();
@@ -69,12 +82,10 @@
" public static void main(java.lang.String[]);",
"}",
getConditionForProguardIfRule(),
- "-keep class " + C.class.getTypeName());
+ "-keep class " + Unused.class.getTypeName());
AndroidApp output = compileWithR8(readClasses(CLASSES), config, this::configure, backend);
assertEquals(expected, runOnVM(output, getTestClass(), backend));
-
- CodeInspector inspector = new CodeInspector(output);
- assertThat(inspector.clazz(MergedReturnTypeTest.C.class), isPresent());
+ inspect(new CodeInspector(output));
}
private void configure(InternalOptions options) {
