Only consider abstract methods that shadow abstract methods
Bug: 198542172
Change-Id: I08a610f7e31b37a23b51914977e23498ebe400cb
diff --git a/src/main/java/com/android/tools/r8/shaking/AbstractMethodRemover.java b/src/main/java/com/android/tools/r8/shaking/AbstractMethodRemover.java
index 4123c10..e0c7f83 100644
--- a/src/main/java/com/android/tools/r8/shaking/AbstractMethodRemover.java
+++ b/src/main/java/com/android/tools/r8/shaking/AbstractMethodRemover.java
@@ -11,7 +11,7 @@
import com.android.tools.r8.logging.Log;
import com.android.tools.r8.shaking.ScopedDexMethodSet.AddMethodIfMoreVisibleResult;
import com.android.tools.r8.utils.IterableUtils;
-import java.util.ArrayList;
+import com.android.tools.r8.utils.ListUtils;
import java.util.List;
/**
@@ -58,32 +58,31 @@
if (virtualMethods == null) {
return null;
}
- // Removal of abstract methods is rare, so avoid copying the array until we find one.
- List<DexEncodedMethod> methods = null;
- for (int i = 0; i < virtualMethods.size(); i++) {
- DexEncodedMethod method = virtualMethods.get(i);
- if (scope.addMethodIfMoreVisible(method) != AddMethodIfMoreVisibleResult.NOT_ADDED
- || !method.accessFlags.isAbstract()
- || appView.appInfo().isPinned(method.getReference())) {
- if (methods != null) {
- methods.add(method);
- }
- } else {
- if (methods == null) {
- methods = new ArrayList<>(virtualMethods.size() - 1);
- for (int j = 0; j < i; j++) {
- methods.add(virtualMethods.get(j));
- }
- }
- if (Log.ENABLED) {
- Log.debug(getClass(), "Removing abstract method %s.", method.getReference());
- }
- }
+ // Removal of abstract methods is rare, ListUtils.filterOrElse does no copying if nothing is
+ // filtered out.
+ List<DexEncodedMethod> filteredMethods =
+ ListUtils.filterOrElse(virtualMethods, this::isNonAbstractPinnedOrWideningVisibility);
+ return filteredMethods == virtualMethods
+ ? null
+ : filteredMethods.toArray(DexEncodedMethod.EMPTY_ARRAY);
+ }
+
+ private boolean isNonAbstractPinnedOrWideningVisibility(DexEncodedMethod method) {
+ if (!method.accessFlags.isAbstract()) {
+ return true;
}
- if (methods != null) {
- return methods.toArray(DexEncodedMethod.EMPTY_ARRAY);
+ // Check if the method widens visibility. Adding to the scope mutates it.
+ if (scope.addMethodIfMoreVisible(method) != AddMethodIfMoreVisibleResult.NOT_ADDED) {
+ return true;
}
- return null;
+ if (appView.appInfo().isPinned(method.getReference())) {
+ return true;
+ }
+ // We will filter the method out since it is not pinned.
+ if (Log.ENABLED) {
+ Log.debug(getClass(), "Removing abstract method %s.", method.getReference());
+ }
+ return false;
}
}
diff --git a/src/main/java/com/android/tools/r8/utils/ListUtils.java b/src/main/java/com/android/tools/r8/utils/ListUtils.java
index 891f1c2..8fd84fc 100644
--- a/src/main/java/com/android/tools/r8/utils/ListUtils.java
+++ b/src/main/java/com/android/tools/r8/utils/ListUtils.java
@@ -159,6 +159,14 @@
return mapOrElse(list, fn, list);
}
+ /**
+ * Takes elements from the input list depending on the predicate being true. Returns the filtered
+ * list otherwise returns the original list of none were removed.
+ */
+ public static <T> List<T> filterOrElse(List<T> list, Predicate<T> predicate) {
+ return mapOrElse(list, element -> predicate.test(element) ? element : null, list);
+ }
+
public static <T> ArrayList<T> newArrayList(T element) {
ArrayList<T> list = new ArrayList<>(1);
list.add(element);
diff --git a/src/test/java/com/android/tools/r8/bridgeremoval/KeepAbstractMethodShadowingTest.java b/src/test/java/com/android/tools/r8/bridgeremoval/KeepAbstractMethodShadowingTest.java
index 1836e19..04cacdf 100644
--- a/src/test/java/com/android/tools/r8/bridgeremoval/KeepAbstractMethodShadowingTest.java
+++ b/src/test/java/com/android/tools/r8/bridgeremoval/KeepAbstractMethodShadowingTest.java
@@ -5,7 +5,6 @@
package com.android.tools.r8.bridgeremoval;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
-import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import com.android.tools.r8.TestBase;
@@ -53,15 +52,13 @@
.addKeepMainRule(Main.class)
.addKeepClassAndMembersRules(A.class)
.run(parameters.getRuntime(), Main.class)
- // TODO(b/198542172): Should be AbstractMethodError.
- .assertSuccessWithOutputLines("Hello World")
- .inspect(
+ .assertFailureWithErrorThatThrows(AbstractMethodError.class)
+ .inspectFailure(
inspector -> {
ClassSubject clazz = inspector.clazz(B.class);
assertThat(clazz, isPresent());
MethodSubject foo = clazz.uniqueMethodWithName("foo");
- // TODO(b/198542172): Should be present.
- assertThat(foo, not(isPresent()));
+ assertThat(foo, isPresent());
});
}
