Fix Class.getMethods() for visibility bridge methods.
We had a bug where we couldn't find synthetic methods added by
subclasses to boost visibility of their superclass methods.
The old logic appeared to work fine with the covariant return
type synthetic methods, but broke down on these visibility
synthetic methods.
For reference, the general rules are:
- when there is a synthetic method for a covariant return type,
include it in getMethods() but prefer the natural method on
getMethod() calls.
- when there is a synthetic method for a visibility boost, prefer
it over the superclass method on getMethods() and getMethod()
calls.
Change-Id: Ied1f9fc9892d211647da87959746839f7e0e1140
http://b/2908173
diff --git a/luni/src/main/java/java/lang/ClassCache.java b/luni/src/main/java/java/lang/ClassCache.java
index b595de4..5588f62 100644
--- a/luni/src/main/java/java/lang/ClassCache.java
+++ b/luni/src/main/java/java/lang/ClassCache.java
@@ -175,25 +175,35 @@
getMethodsRecursive(clazz, allMethods);
/*
- * Keep only unique methods by signature. If two signatures differ only
- * by return type, covariant return types are in play. For consistency
- * with the RI we return both methods.
+ * Remove methods defined by multiple types, preferring to keep methods
+ * declared by derived types.
+ *
+ * Classes may define multiple methods with the same name and parameter
+ * types due to covariant return types. In this case both are returned,
+ * with the non-synthetic method first because it is preferred by
+ * getMethod(String,Class[]).
*/
Collections.sort(allMethods, Method.ORDER_BY_SIGNATURE);
- List<Method> uniqueMethods = new ArrayList<Method>(allMethods.size());
- if (!allMethods.isEmpty()) {
- Method last = allMethods.get(0);
- uniqueMethods.add(last);
- for (int i = 1; i < allMethods.size(); i++) {
- Method current = allMethods.get(i);
- if (Method.ORDER_BY_SIGNATURE.compare(last, current) != 0) {
- uniqueMethods.add(current);
- last = current;
- }
+ List<Method> natural = new ArrayList<Method>(allMethods.size());
+ List<Method> synthetic = new ArrayList<Method>(allMethods.size());
+ Method previous = null;
+ for (Method method : allMethods) {
+ if (previous != null
+ && Method.ORDER_BY_SIGNATURE.compare(method, previous) == 0
+ && method.getDeclaringClass() != previous.getDeclaringClass()) {
+ continue;
}
+ if (method.isSynthetic()) {
+ synthetic.add(method);
+ } else {
+ natural.add(method);
+ }
+ previous = method;
}
-
- return uniqueMethods.toArray(new Method[uniqueMethods.size()]);
+ List<Method> result = new ArrayList<Method>(allMethods.size());
+ result.addAll(natural);
+ result.addAll(synthetic);
+ return result.toArray(new Method[result.size()]);
}
/**
@@ -228,8 +238,7 @@
throw new NullPointerException("Method name must not be null.");
}
for (Method method : list) {
- if (!method.isSynthetic()
- && method.getName().equals(name)
+ if (method.getName().equals(name)
&& compareClassLists(method.getParameterTypes(), parameterTypes)) {
return method;
}
diff --git a/luni/src/main/java/java/lang/reflect/Method.java b/luni/src/main/java/java/lang/reflect/Method.java
index 9cf91f1..f46d76f 100644
--- a/luni/src/main/java/java/lang/reflect/Method.java
+++ b/luni/src/main/java/java/lang/reflect/Method.java
@@ -32,7 +32,6 @@
package java.lang.reflect;
-import dalvik.system.VMStack;
import java.lang.annotation.Annotation;
import java.util.Comparator;
import org.apache.harmony.kernel.vm.StringUtils;
@@ -47,9 +46,7 @@
public final class Method extends AccessibleObject implements GenericDeclaration, Member {
/**
- * Orders methods by their signature, including name, parameters and return
- * type. When one method overrides another, they will always have the same
- * type signature unless covariant return types are in play.
+ * Orders methods by their name and parameters.
*
* @hide
*/
@@ -70,12 +67,7 @@
}
}
- comparison = aParameters.length - bParameters.length;
- if (comparison != 0) {
- return comparison;
- }
-
- return a.returnType.getName().compareTo(b.returnType.getName());
+ return aParameters.length - bParameters.length;
}
};
diff --git a/luni/src/test/java/libcore/java/lang/reflect/MethodOverridesTest.java b/luni/src/test/java/libcore/java/lang/reflect/MethodOverridesTest.java
index 6cbe74c..8d46f45 100644
--- a/luni/src/test/java/libcore/java/lang/reflect/MethodOverridesTest.java
+++ b/luni/src/test/java/libcore/java/lang/reflect/MethodOverridesTest.java
@@ -18,6 +18,7 @@
import java.lang.reflect.Method;
import java.util.Arrays;
+import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;
@@ -53,7 +54,7 @@
public void testGetMethodsIncludesInheritedMethods() {
Set<String> signatures = signatures(Sub.class.getMethods());
- assertTrue(signatures.contains("void notOverridden[] throws []"));
+ assertContains(signatures, "void notOverridden[] throws []");
}
public void testGetDeclaredMethodsDoesNotIncludeInheritedMethods() {
@@ -68,20 +69,29 @@
public void testGetMethodsDoesNotIncludeExceptionChanges() throws NoSuchMethodException {
Set<String> signatures = signatures(Sub.class.getMethods());
- assertTrue(signatures.contains("void thrower[] throws []"));
+ assertContains(signatures, "void thrower[] throws []");
assertFalse(signatures.contains("void thrower[] throws [java.lang.Exception]"));
+ assertEquals(Sub.class, Sub.class.getMethod("thrower").getDeclaringClass());
}
public void testGetMethodsIncludesSyntheticMethods() throws NoSuchMethodException {
Set<String> signatures = signatures(Sub.class.getMethods());
- assertTrue(signatures.contains("java.lang.String returner[] throws []"));
- assertTrue(signatures.contains("java.lang.Object returner[] throws []"));
+ assertContains(signatures, "java.lang.String returner[] throws []");
+ assertContains(signatures, "java.lang.Object returner[] throws []");
+
+ Method method = Sub.class.getMethod("returner");
+ assertEquals(Sub.class, method.getDeclaringClass());
+ assertFalse(method.isSynthetic());
}
public void testGetDeclaredMethodsIncludesSyntheticMethods() throws NoSuchMethodException {
Set<String> signatures = signatures(Sub.class.getDeclaredMethods());
- assertTrue(signatures.contains("java.lang.String returner[] throws []"));
- assertTrue(signatures.contains("java.lang.Object returner[] throws []"));
+ assertContains(signatures, "java.lang.String returner[] throws []");
+ assertContains(signatures, "java.lang.Object returner[] throws []");
+
+ Method method = Sub.class.getMethod("returner");
+ assertEquals(Sub.class, method.getDeclaringClass());
+ assertFalse(method.isSynthetic());
}
public void testSubclassChangesVisibility() throws NoSuchMethodException {
@@ -89,10 +99,33 @@
int count = 0;
for (Method method : methods) {
if (signature(method).equals("void visibility[] throws []")) {
+ assertEquals(Sub.class, method.getDeclaringClass());
+ assertFalse(method.isSynthetic());
count++;
}
}
assertEquals(1, count);
+
+ Method method = Sub.class.getMethod("visibility");
+ assertEquals(Sub.class, method.getDeclaringClass());
+ assertFalse(method.isSynthetic());
+ }
+
+ public void testMoreVisibleSubclassChangesVisibility() throws NoSuchMethodException {
+ Method[] methods = PublicSub.class.getMethods();
+ int count = 0;
+ for (Method method : methods) {
+ if (signature(method).equals("void unchanged[] throws []")) {
+ assertEquals(PublicSub.class, method.getDeclaringClass());
+ assertTrue(method.isSynthetic());
+ count++;
+ }
+ }
+ assertEquals(1, count);
+
+ Method method = PublicSub.class.getMethod("unchanged");
+ assertEquals(PublicSub.class, method.getDeclaringClass());
+ assertTrue(method.isSynthetic());
}
public static class Super {
@@ -114,6 +147,12 @@
@Override public void visibility() {}
}
+ static class PackageSuper {
+ public void unchanged() {}
+ }
+
+ public static class PublicSub extends PackageSuper {}
+
/**
* Returns a method signature of this form:
* {@code java.lang.String concat[class java.lang.String] throws []}.
@@ -131,4 +170,8 @@
}
return signatures;
}
+
+ private <T> void assertContains(Collection<T> elements, T value) {
+ assertTrue("Expected " + value + " in " + elements, elements.contains(value));
+ }
}