Skip to content

Commit

Permalink
Fix PeerBaseMethod to not rely on implicit ordering of Elements.getAl…
Browse files Browse the repository at this point in the history 
…lMethods.

This CL adds a new method, MoreElements#getAllMethods(), which returns all local and inherited methods (including static methods), but does not include any methods that have been overridden.

Note: we are still relying on an explicit ordering for static methods to ensure we don't include hidden static methods from super classes.

RELNOTES=Adds MoreElements#getAllMethods(), which returns all local and inherited methods (including static methods), but does not include any methods that have been overridden.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=257649768
  • Loading branch information
@bcorso @netdpb
bcorso authored and netdpb committed Aug 19, 2019
1 parent e1beeff commit 93fb9c5
Show file tree
Hide file tree
Showing 2 changed files with 147 additions and 37 deletions.
104 changes: 80 additions & 24 deletions common/src/main/java/com/google/auto/common/MoreElements.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.auto.common;

import static javax.lang.model.element.ElementKind.PACKAGE;
import static javax.lang.model.element.Modifier.STATIC;

import com.google.auto.common.Overrides.ExplicitOverrides;
import com.google.common.annotations.Beta;
Expand Down Expand Up @@ -277,9 +278,9 @@ public boolean apply(T input) {
}

/**
* Returns the set of all non-private methods from {@code type}, including methods that it
* inherits from its ancestors. Inherited methods that are overridden are not included in the
* result. So if {@code type} defines {@code public String toString()}, the returned set will
* Returns the set of all non-private, non-static methods from {@code type}, including methods
* that it inherits from its ancestors. Inherited methods that are overridden are not included in
* the result. So if {@code type} defines {@code public String toString()}, the returned set will
* contain that method, but not the {@code toString()} method defined by {@code Object}.
*
* <p>The returned set may contain more than one method with the same signature, if
Expand All @@ -288,6 +289,12 @@ public boolean apply(T input) {
* {@code void foo();}, and if it does not itself override the {@code foo()} method,
* then both {@code One.foo()} and {@code Two.foo()} will be in the returned set.
*
* <p>The order of the returned set is deterministic: within a class or interface, methods are in
* the order they appear in the source code; methods in ancestors come before methods in
* descendants; methods in interfaces come before methods in classes; and in a class or interface
* that has more than one superinterface, the interfaces are in the order of their appearance in
* {@code implements} or {@code extends}.
*
* @param type the type whose own and inherited methods are to be returned
* @param elementUtils an {@link Elements} object, typically returned by
* {@link javax.annotation.processing.AbstractProcessor#processingEnv processingEnv}<!--
Expand All @@ -305,9 +312,9 @@ public static ImmutableSet<ExecutableElement> getLocalAndInheritedMethods(
}

/**
* Returns the set of all non-private methods from {@code type}, including methods that it
* inherits from its ancestors. Inherited methods that are overridden are not included in the
* result. So if {@code type} defines {@code public String toString()}, the returned set will
* Returns the set of all non-private, non-static methods from {@code type}, including methods
* that it inherits from its ancestors. Inherited methods that are overridden are not included in
* the result. So if {@code type} defines {@code public String toString()}, the returned set will
* contain that method, but not the {@code toString()} method defined by {@code Object}.
*
* <p>The returned set may contain more than one method with the same signature, if
Expand All @@ -316,6 +323,12 @@ public static ImmutableSet<ExecutableElement> getLocalAndInheritedMethods(
* {@code void foo();}, and if it does not itself override the {@code foo()} method,
* then both {@code One.foo()} and {@code Two.foo()} will be in the returned set.
*
* <p>The order of the returned set is deterministic: within a class or interface, methods are in
* the order they appear in the source code; methods in ancestors come before methods in
* descendants; methods in interfaces come before methods in classes; and in a class or interface
* that has more than one superinterface, the interfaces are in the order of their appearance in
* {@code implements} or {@code extends}.
*
* @param type the type whose own and inherited methods are to be returned
* @param typeUtils a {@link Types} object, typically returned by
* {@link javax.annotation.processing.AbstractProcessor#processingEnv processingEnv}<!--
Expand All @@ -331,6 +344,20 @@ public static ImmutableSet<ExecutableElement> getLocalAndInheritedMethods(
return getLocalAndInheritedMethods(type, new ExplicitOverrides(typeUtils));
}

private static ImmutableSet<ExecutableElement> getLocalAndInheritedMethods(
TypeElement type, Overrides overrides) {
PackageElement pkg = getPackage(type);

ImmutableSet.Builder<ExecutableElement> methods = ImmutableSet.builder();
for (ExecutableElement method : getAllMethods(type, overrides)) {
// Filter out all static and non-visible methods.
if (!method.getModifiers().contains(STATIC) && methodVisibleFromPackage(method, pkg)) {
methods.add(method);
}
}
return methods.build();
}

/**
* Tests whether one method, as a member of a given type, overrides another method.
*
Expand All @@ -351,10 +378,43 @@ public static boolean overrides(
return new ExplicitOverrides(typeUtils).overrides(overrider, overridden, type);
}

private static ImmutableSet<ExecutableElement> getLocalAndInheritedMethods(
/**
* Returns the set of all methods from {@code type}, including methods that it inherits
* from its ancestors. Inherited methods that are overridden are not included in the
* result. So if {@code type} defines {@code public String toString()}, the returned set
* will contain that method, but not the {@code toString()} method defined by {@code Object}.
*
* <p>The returned set may contain more than one method with the same signature, if
* {@code type} inherits those methods from different ancestors. For example, if it
* inherits from unrelated interfaces {@code One} and {@code Two} which each define
* {@code void foo();}, and if it does not itself override the {@code foo()} method,
* then both {@code One.foo()} and {@code Two.foo()} will be in the returned set.
*
* <p>The order of the returned set is deterministic: within a class or interface, methods are in
* the order they appear in the source code; methods in ancestors come before methods in
* descendants; methods in interfaces come before methods in classes; and in a class or interface
* that has more than one superinterface, the interfaces are in the order of their appearance in
* {@code implements} or {@code extends}.
*
* @param type the type whose own and inherited methods are to be returned
* @param typeUtils a {@link Types} object, typically returned by
* {@link javax.annotation.processing.AbstractProcessor#processingEnv processingEnv}<!--
* -->.{@link javax.annotation.processing.ProcessingEnvironment#getTypeUtils
* getTypeUtils()}
* @param elementUtils an {@link Elements} object, typically returned by
* {@link javax.annotation.processing.AbstractProcessor#processingEnv processingEnv}<!--
* -->.{@link javax.annotation.processing.ProcessingEnvironment#getElementUtils
* getElementUtils()}
*/
public static ImmutableSet<ExecutableElement> getAllMethods(
TypeElement type, Types typeUtils, Elements elementUtils) {
return getAllMethods(type, new ExplicitOverrides(typeUtils));
}

private static ImmutableSet<ExecutableElement> getAllMethods(
TypeElement type, Overrides overrides) {
SetMultimap<String, ExecutableElement> methodMap = LinkedHashMultimap.create();
getLocalAndInheritedMethods(getPackage(type), type, methodMap);
getAllMethods(type, methodMap);
// Find methods that are overridden. We do this using `Elements.overrides`, which means
// that it is inherently a quadratic operation, since we have to compare every method against
// every other method. We reduce the performance impact by (a) grouping methods by name, since
Expand All @@ -370,6 +430,7 @@ private static ImmutableSet<ExecutableElement> getLocalAndInheritedMethods(
ExecutableElement methodJ = methodList.get(j);
if (overrides.overrides(methodJ, methodI, type)) {
overridden.add(methodI);
break;
}
}
}
Expand All @@ -379,29 +440,24 @@ private static ImmutableSet<ExecutableElement> getLocalAndInheritedMethods(
return ImmutableSet.copyOf(methods);
}

// Add to `methods` the instance methods from `type` that are visible to code in the
// package `pkg`. This means all the instance methods from `type` itself and all instance methods
// it inherits from its ancestors, except private methods and package-private methods in other
// packages. This method does not take overriding into account, so it will add both an ancestor
// method and a descendant method that overrides it.
// `methods` is a multimap from a method name to all of the methods with that name, including
// methods that override or overload one another. Within those methods, those in ancestor types
// always precede those in descendant types.
private static void getLocalAndInheritedMethods(
PackageElement pkg, TypeElement type, SetMultimap<String, ExecutableElement> methods) {
// Add to `methods` the static and instance methods from `type`. This means all methods from
// `type` itself and all methods it inherits from its ancestors. This method does not take
// overriding into account, so it will add both an ancestor method and a descendant method that
// overrides it. `methods` is a multimap from a method name to all of the methods with that name,
// including methods that override or overload one another. Within those methods, those in
// ancestor types always precede those in descendant types.
private static void getAllMethods(
TypeElement type, SetMultimap<String, ExecutableElement> methods) {
for (TypeMirror superInterface : type.getInterfaces()) {
getLocalAndInheritedMethods(pkg, MoreTypes.asTypeElement(superInterface), methods);
getAllMethods(MoreTypes.asTypeElement(superInterface), methods);
}
if (type.getSuperclass().getKind() != TypeKind.NONE) {
// Visit the superclass after superinterfaces so we will always see the implementation of a
// method after any interfaces that declared it.
getLocalAndInheritedMethods(pkg, MoreTypes.asTypeElement(type.getSuperclass()), methods);
getAllMethods(MoreTypes.asTypeElement(type.getSuperclass()), methods);
}
for (ExecutableElement method : ElementFilter.methodsIn(type.getEnclosedElements())) {
if (!method.getModifiers().contains(Modifier.STATIC)
&& methodVisibleFromPackage(method, pkg)) {
methods.put(method.getSimpleName().toString(), method);
}
methods.put(method.getSimpleName().toString(), method);
}
}

Expand Down
80 changes: 67 additions & 13 deletions common/src/test/java/com/google/auto/common/MoreElementsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,16 +227,24 @@ public void getAnnotationMirror() {
}

private abstract static class ParentClass {
static void staticMethod() {}

abstract String foo();
private void ignored() {}

private void privateMethod() {}
}

private interface ParentInterface {
static void staticMethod() {}

abstract int bar();

abstract int bar(long x);
}

private abstract static class Child extends ParentClass implements ParentInterface {
static void staticMethod() {}

@Override
public int bar() {
return 0;
Expand All @@ -263,12 +271,13 @@ public void getLocalAndInheritedMethods_Old() {
assertThat(childTypeMethods).containsAtLeastElementsIn(objectMethods);
Set<ExecutableElement> nonObjectMethods = Sets.difference(childTypeMethods, objectMethods);
assertThat(nonObjectMethods).containsExactly(
getMethod(ParentClass.class, "foo"),
getMethod(ParentInterface.class, "bar", longMirror),
getMethod(Child.class, "bar"),
getMethod(Child.class, "baz"),
getMethod(Child.class, "buh", intMirror),
getMethod(Child.class, "buh", intMirror, intMirror));
getMethod(ParentInterface.class, "bar", longMirror),
getMethod(ParentClass.class, "foo"),
getMethod(Child.class, "bar"),
getMethod(Child.class, "baz"),
getMethod(Child.class, "buh", intMirror),
getMethod(Child.class, "buh", intMirror, intMirror))
.inOrder();;
}

@Test
Expand All @@ -285,12 +294,40 @@ public void getLocalAndInheritedMethods() {
assertThat(childTypeMethods).containsAtLeastElementsIn(objectMethods);
Set<ExecutableElement> nonObjectMethods = Sets.difference(childTypeMethods, objectMethods);
assertThat(nonObjectMethods).containsExactly(
getMethod(ParentClass.class, "foo"),
getMethod(ParentInterface.class, "bar", longMirror),
getMethod(Child.class, "bar"),
getMethod(Child.class, "baz"),
getMethod(Child.class, "buh", intMirror),
getMethod(Child.class, "buh", intMirror, intMirror));
getMethod(ParentInterface.class, "bar", longMirror),
getMethod(ParentClass.class, "foo"),
getMethod(Child.class, "bar"),
getMethod(Child.class, "baz"),
getMethod(Child.class, "buh", intMirror),
getMethod(Child.class, "buh", intMirror, intMirror))
.inOrder();
}

@Test
public void getAllMethods() {
Elements elements = compilation.getElements();
Types types = compilation.getTypes();
TypeMirror intMirror = types.getPrimitiveType(TypeKind.INT);
TypeMirror longMirror = types.getPrimitiveType(TypeKind.LONG);
TypeElement childType = elements.getTypeElement(Child.class.getCanonicalName());
@SuppressWarnings("deprecation")
Set<ExecutableElement> childTypeMethods =
MoreElements.getAllMethods(childType, types, elements);
Set<ExecutableElement> objectMethods = allMethodsFromObject();
assertThat(childTypeMethods).containsAtLeastElementsIn(objectMethods);
Set<ExecutableElement> nonObjectMethods = Sets.difference(childTypeMethods, objectMethods);
assertThat(nonObjectMethods).containsExactly(
getMethod(ParentInterface.class, "staticMethod"),
getMethod(ParentInterface.class, "bar", longMirror),
getMethod(ParentClass.class, "staticMethod"),
getMethod(ParentClass.class, "foo"),
getMethod(ParentClass.class, "privateMethod"),
getMethod(Child.class, "staticMethod"),
getMethod(Child.class, "bar"),
getMethod(Child.class, "baz"),
getMethod(Child.class, "buh", intMirror),
getMethod(Child.class, "buh", intMirror, intMirror))
.inOrder();
}

static class Injectable {}
Expand Down Expand Up @@ -349,6 +386,23 @@ private Set<ExecutableElement> visibleMethodsFromObject() {
return methods;
}

private Set<ExecutableElement> allMethodsFromObject() {
Types types = compilation.getTypes();
TypeMirror intMirror = types.getPrimitiveType(TypeKind.INT);
TypeMirror longMirror = types.getPrimitiveType(TypeKind.LONG);
Set<ExecutableElement> methods = new HashSet<>();
methods.addAll(ElementFilter.methodsIn(objectElement.getEnclosedElements()));
assertThat(methods)
.containsAtLeast(
getMethod(Object.class, "clone"),
getMethod(Object.class, "registerNatives"),
getMethod(Object.class, "finalize"),
getMethod(Object.class, "wait"),
getMethod(Object.class, "wait", longMirror),
getMethod(Object.class, "wait", longMirror, intMirror));
return methods;
}

private ExecutableElement getMethod(Class<?> c, String methodName, TypeMirror... parameterTypes) {
TypeElement type = compilation.getElements().getTypeElement(c.getCanonicalName());
Types types = compilation.getTypes();
Expand Down

0 comments on commit 93fb9c5

Please sign in to comment.