Handle return type changes in Proguard -if rules
Bug: 110141157
Change-Id: If96af826ea94a83b3359af26835af7ecd9ff4ec3
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 d1bfc9a..0a2e4f9 100644
--- a/src/main/java/com/android/tools/r8/shaking/ProguardMemberRule.java
+++ b/src/main/java/com/android/tools/r8/shaking/ProguardMemberRule.java
@@ -228,9 +228,7 @@
return RootSetBuilder.containsAnnotation(annotation, method.annotations);
case METHOD:
// Check return type.
- // TODO(b/110141157): The name of the return type may have changed as a result of vertical
- // class merging. We should use the original type name.
- if (!type.matches(originalSignature.proto.returnType)) {
+ if (!matches(type, originalSignature.proto.returnType, appView)) {
break;
}
// Fall through for access flags, name and arguments.
@@ -276,6 +274,18 @@
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/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 9d2e352..32fd8da 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
@@ -59,11 +59,11 @@
}
// TODO(b/110141157):
-// - Add tests where the return type of a kept method changes.
// - Add tests where the parameter type of a kept method changes.
// - Add tests where the type of a kept field changes.
// - 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 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/MergedReturnTypeTest.java b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedReturnTypeTest.java
new file mode 100644
index 0000000..f5c8411
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedReturnTypeTest.java
@@ -0,0 +1,99 @@
+// 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.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ir.optimize.Inliner.Reason;
+import com.android.tools.r8.utils.AndroidApp;
+import com.android.tools.r8.utils.InternalOptions;
+import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import java.util.Collection;
+import java.util.List;
+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 MergedReturnTypeTest extends TestBase {
+
+ private static final List<Class> CLASSES =
+ ImmutableList.of(A.class, B.class, C.class, TestClass.class);
+
+ static class A {}
+
+ static class B extends A {
+
+ @Override
+ public String toString() {
+ return "B";
+ }
+ }
+
+ static class C {}
+
+ static class TestClass {
+
+ public static void main(String[] args) {
+ System.out.print(method());
+ }
+
+ public static A method() {
+ return new B();
+ }
+ }
+
+ private final Backend backend;
+ private final boolean enableVerticalClassMerging;
+
+ public MergedReturnTypeTest(Backend backend, boolean enableVerticalClassMerging) {
+ this.backend = backend;
+ this.enableVerticalClassMerging = enableVerticalClassMerging;
+ }
+
+ @Parameters(name = "Backend: {0}, vertical class merging: {1}")
+ public static Collection<Object[]> data() {
+ // We don't run this on Proguard, as Proguard does not merge A into B.
+ return ImmutableList.of(
+ new Object[] {Backend.DEX, true},
+ new Object[] {Backend.DEX, false},
+ new Object[] {Backend.CF, true},
+ new Object[] {Backend.CF, false});
+ }
+
+ @Test
+ public void testIfRule() throws Exception {
+ String expected = "B";
+ assertEquals(expected, runOnJava(TestClass.class));
+
+ String config =
+ StringUtils.joinLines(
+ "-keep class **$TestClass {",
+ " public static void main(java.lang.String[]);",
+ "}",
+ "-if class **$TestClass { public static **$A method(); }",
+ "-keep class **$C");
+ AndroidApp output = compileWithR8(readClasses(CLASSES), config, this::configure, backend);
+ assertEquals(expected, runOnVM(output, TestClass.class, backend));
+
+ CodeInspector inspector = new CodeInspector(output);
+ assertThat(inspector.clazz(C.class), isPresent());
+ }
+
+ private void configure(InternalOptions options) {
+ options.enableMinification = false;
+ options.enableVerticalClassMerging = enableVerticalClassMerging;
+
+ // TODO(b/110148109): Allow ordinary method inlining when -if rules work with inlining.
+ options.testing.validInliningReasons = ImmutableSet.of(Reason.FORCE);
+ }
+}
