Refactor getAnnotationFromParentClass() to a new SkylarkInterfaceUtils module

--
MOS_MIGRATED_REVID=135144084
diff --git a/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkMethodDoc.java b/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkMethodDoc.java
index dd70c81..116fd4a 100644
--- a/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkMethodDoc.java
+++ b/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkMethodDoc.java
@@ -17,8 +17,8 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.skylarkinterface.Param;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
-import com.google.devtools.build.lib.syntax.FuncallExpression;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.List;
@@ -53,7 +53,7 @@
   }
 
   private String getParameterString(Method method) {
-    SkylarkCallable annotation = FuncallExpression.getAnnotationFromParentClass(method);
+    SkylarkCallable annotation = SkylarkInterfaceUtils.getSkylarkCallable(method);
     int nbPositional = annotation.mandatoryPositionals();
     if (annotation.parameters().length > 0 && nbPositional < 0) {
       nbPositional = 0;
@@ -76,7 +76,7 @@
   }
 
   protected String getSignature(String objectName, String methodName, Method method) {
-    String args = FuncallExpression.getAnnotationFromParentClass(method).structField()
+    String args = SkylarkInterfaceUtils.getSkylarkCallable(method).structField()
         ? "" : "(" + getParameterString(method) + ")";
 
     return String.format("%s %s.%s%s",
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkInterfaceUtils.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkInterfaceUtils.java
new file mode 100644
index 0000000..95d4f4f
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkInterfaceUtils.java
@@ -0,0 +1,73 @@
+// Copyright 2016 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.skylarkinterface;
+
+import java.lang.reflect.Method;
+import javax.annotation.Nullable;
+
+/**
+ * Helpers for accessing Skylark interface annotations.
+ */
+public class SkylarkInterfaceUtils {
+
+  /**
+   * Returns the {@link SkylarkCallable} annotation for the given method, if it exists, and
+   * null otherwise. The method must be declared in {@code classObj} or one of its base classes
+   * or interfaces. The first annotation of an overridden version of the method that is found
+   * will be returned, starting with {@code classObj} and following its base classes and
+   * interfaces recursively.
+   */
+  @Nullable
+  public static SkylarkCallable getSkylarkCallable(Class<?> classObj, Method method) {
+    boolean keepLooking = false;
+    try {
+      Method superMethod = classObj.getMethod(method.getName(), method.getParameterTypes());
+      if (classObj.isAnnotationPresent(SkylarkModule.class)
+          && superMethod.isAnnotationPresent(SkylarkCallable.class)) {
+        return superMethod.getAnnotation(SkylarkCallable.class);
+      } else {
+        keepLooking = true;
+      }
+    } catch (NoSuchMethodException e) {
+      // The class might not have the specified method, so an exceptions is OK.
+      keepLooking = true;
+    }
+    if (keepLooking) {
+      if (classObj.getSuperclass() != null) {
+        SkylarkCallable annotation =
+            getSkylarkCallable(classObj.getSuperclass(), method);
+        if (annotation != null) {
+          return annotation;
+        }
+      }
+      for (Class<?> interfaceObj : classObj.getInterfaces()) {
+        SkylarkCallable annotation = getSkylarkCallable(interfaceObj, method);
+        if (annotation != null) {
+          return annotation;
+        }
+      }
+    }
+    return null;
+  }
+
+  /**
+   * Convenience version of {@code getAnnotationsFromParentClass(Class, Method)} that uses
+   * the declaring class of the method.
+   */
+  @Nullable
+  public static SkylarkCallable getSkylarkCallable(Method method) {
+    return getSkylarkCallable(method.getDeclaringClass(), method);
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index c41fcec..c476033 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -27,6 +27,7 @@
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.skylarkinterface.Param;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.syntax.EvalException.EvalExceptionWithJavaCause;
 import com.google.devtools.build.lib.syntax.Runtime.NoneType;
@@ -105,7 +106,7 @@
                     if (method.isSynthetic()) {
                       continue;
                     }
-                    SkylarkCallable callable = getAnnotationFromParentClass(method);
+                    SkylarkCallable callable = SkylarkInterfaceUtils.getSkylarkCallable(method);
                     if (callable == null) {
                       continue;
                     }
@@ -154,7 +155,7 @@
     for (Method method : classObj.getMethods()) {
       // Synthetic methods lead to false multiple matches
       if (!method.isSynthetic()) {
-        SkylarkCallable annotation = getAnnotationFromParentClass(classObj, method);
+        SkylarkCallable annotation = SkylarkInterfaceUtils.getSkylarkCallable(classObj, method);
         if (annotation != null) {
           methodMap.put(method, annotation);
         }
@@ -163,55 +164,6 @@
     return methodMap.build();
   }
 
-  /**
-   * Returns the {@link SkylarkCallable} annotation for the given method, if it exists, and
-   * null otherwise. The method must be declared in {@code classObj} or one of its base classes
-   * or interfaces. The first annotation of an overridden version of the method that is found
-   * will be returned, starting with {@code classObj} and following its base classes and
-   * interfaces recursively.
-   */
-  @Nullable
-  public static SkylarkCallable getAnnotationFromParentClass(Class<?> classObj, Method method) {
-    boolean keepLooking = false;
-    try {
-      Method superMethod = classObj.getMethod(method.getName(), method.getParameterTypes());
-      if (classObj.isAnnotationPresent(SkylarkModule.class)
-          && superMethod.isAnnotationPresent(SkylarkCallable.class)) {
-        return superMethod.getAnnotation(SkylarkCallable.class);
-      } else {
-        keepLooking = true;
-      }
-    } catch (NoSuchMethodException e) {
-      // The class might not have the specified method, so an exceptions is OK.
-      keepLooking = true;
-    }
-    if (keepLooking) {
-      if (classObj.getSuperclass() != null) {
-        SkylarkCallable annotation =
-            getAnnotationFromParentClass(classObj.getSuperclass(), method);
-        if (annotation != null) {
-          return annotation;
-        }
-      }
-      for (Class<?> interfaceObj : classObj.getInterfaces()) {
-        SkylarkCallable annotation = getAnnotationFromParentClass(interfaceObj, method);
-        if (annotation != null) {
-          return annotation;
-        }
-      }
-    }
-    return null;
-  }
-
-  /**
-   * Convenience version of {@code getAnnotationsFromParentClass(Class, Method)} that uses
-   * the declaring class of the method.
-   */
-  @Nullable
-  public static SkylarkCallable getAnnotationFromParentClass(Method method) {
-    return getAnnotationFromParentClass(method.getDeclaringClass(), method);
-  }
-
   private static class ArgumentListConversionResult {
     private final ImmutableList<Object> arguments;
     private final String error;
diff --git a/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationTest.java b/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationTest.java
index a536db7..4d0b556 100644
--- a/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationTest.java
+++ b/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationTest.java
@@ -109,8 +109,8 @@
   }
 
   /** MockClassA */
-  @SkylarkModule(name = "MockClassA", doc = "")
-  public static class MockClassA {
+  @SkylarkModule(name = "MockClassA", doc = "MockClassA")
+  private static class MockClassA {
     @SkylarkCallable(doc = "MockClassA#get")
     public Integer get() {
       return 0;
@@ -118,18 +118,18 @@
   }
 
   /** MockClassB */
-  @SkylarkModule(name = "MockClassB", doc = "")
-  public static class MockClassB {
-    @SkylarkCallable(doc = "")
+  @SkylarkModule(name = "MockClassB", doc = "MockClassB")
+  private static class MockClassB {
+    @SkylarkCallable(doc = "MockClassB#get")
     public MockClassA get() {
       return new MockClassA();
     }
   }
 
   /** MockClassC */
-  @SkylarkModule(name = "MockClassC", doc = "")
-  public static class MockClassC extends MockClassA {
-    @SkylarkCallable(doc = "")
+  @SkylarkModule(name = "MockClassC", doc = "MockClassC")
+  private static class MockClassC extends MockClassA {
+    @SkylarkCallable(doc = "MockClassC#get2")
     public Integer get2() {
       return 0;
     }
@@ -137,7 +137,7 @@
 
   /** MockClassD */
   @SkylarkModule(name = "MockClassD", doc = "MockClassD")
-  public static class MockClassD {
+  private static class MockClassD {
     @SkylarkCallable(
       doc = "MockClassD#test",
       mandatoryPositionals = 1,
@@ -154,7 +154,7 @@
 
   /** MockClassE */
   @SkylarkModule(name = "MockClassE", doc = "MockClassE")
-  public static class MockClassE extends MockClassA {
+  private static class MockClassE extends MockClassA {
     @Override
     public Integer get() {
       return 1;
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkInterfaceUtilsTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkInterfaceUtilsTest.java
new file mode 100644
index 0000000..4172024
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkInterfaceUtilsTest.java
@@ -0,0 +1,160 @@
+// Copyright 2016 The Bazel Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.syntax;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
+import java.lang.reflect.Method;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ *  Test Skylark interface annotations and utilities.
+ */
+@RunWith(JUnit4.class)
+public class SkylarkInterfaceUtilsTest {
+
+  /** MockClassA */
+  @SkylarkModule(name = "MockClassA", doc = "MockClassA")
+  public static class MockClassA {
+    @SkylarkCallable(doc = "MockClassA#foo")
+    public void foo() {}
+    @SkylarkCallable(doc = "MockClassA#bar")
+    public void bar() {}
+    public void baz() {}
+  }
+
+  /** MockInterfaceB1 */
+  @SkylarkModule(name = "MockInterfaceB1", doc = "MockInterfaceB1")
+  public static interface MockInterfaceB1 {
+    @SkylarkCallable(doc = "MockInterfaceB1#foo")
+    void foo();
+    @SkylarkCallable(doc = "MockInterfaceB1#bar")
+    void bar();
+    @SkylarkCallable(doc = "MockInterfaceB1#baz")
+    void baz();
+  }
+
+  /** MockInterfaceB2 */
+  @SkylarkModule(name = "MockInterfaceB2", doc = "MockInterfaceB2")
+  public static interface MockInterfaceB2 {
+    @SkylarkCallable(doc = "MockInterfaceB2#baz")
+    void baz();
+    @SkylarkCallable(doc = "MockInterfaceB2#qux")
+    void qux();
+  }
+
+  /** MockClassC */
+  @SkylarkModule(name = "MockClassC", doc = "MockClassC")
+  public static class MockClassC extends MockClassA implements MockInterfaceB1, MockInterfaceB2 {
+    @Override
+    @SkylarkCallable(doc = "MockClassC#foo")
+    public void foo() {}
+    @Override
+    public void bar() {}
+    @Override
+    public void baz() {}
+    @Override
+    public void qux() {}
+  }
+
+  /** MockClassD */
+  public static class MockClassD extends MockClassC {
+    @Override
+    @SkylarkCallable(doc = "MockClassD#foo")
+    public void foo() {}
+  }
+
+  @Test
+  public void testGetSkylarkCallableBasic() throws Exception {
+    // Normal case. Ensure two-arg form is consistent with one-arg form.
+    Method method = MockClassA.class.getMethod("foo");
+    SkylarkCallable ann = SkylarkInterfaceUtils.getSkylarkCallable(method);
+    assertThat(ann).isNotNull();
+    assertThat(ann.doc()).isEqualTo("MockClassA#foo");
+
+    SkylarkCallable ann2 = SkylarkInterfaceUtils.getSkylarkCallable(MockClassA.class, method);
+    assertThat(ann2).isEqualTo(ann);
+  }
+
+  @Test
+  public void testGetSkylarkCallableSubclass() throws Exception {
+    // Subclass's annotation is used.
+    Method method = MockClassC.class.getMethod("foo");
+    SkylarkCallable ann = SkylarkInterfaceUtils.getSkylarkCallable(method);
+    assertThat(ann).isNotNull();
+    assertThat(ann.doc()).isEqualTo("MockClassC#foo");
+  }
+
+  @Test
+  public void testGetSkylarkCallableSubclassNoSubannotation() throws Exception {
+    // Falls back on superclass's annotation. Superclass takes precedence over interface.
+    Method method = MockClassC.class.getMethod("bar");
+    SkylarkCallable ann = SkylarkInterfaceUtils.getSkylarkCallable(method);
+    assertThat(ann).isNotNull();
+    assertThat(ann.doc()).isEqualTo("MockClassA#bar");
+  }
+
+  @Test
+  public void testGetSkylarkCallableTwoargForm() throws Exception {
+    // Ensure that when passing superclass in directly, we bypass subclass's annotation.
+    Method method = MockClassC.class.getMethod("foo");
+    SkylarkCallable ann = SkylarkInterfaceUtils.getSkylarkCallable(MockClassA.class, method);
+    assertThat(ann).isNotNull();
+    assertThat(ann.doc()).isEqualTo("MockClassA#foo");
+  }
+
+  @Test
+  public void testGetSkylarkCallableNotFound() throws Exception {
+    // Null result when no annotation present...
+    Method method = MockClassA.class.getMethod("baz");
+    SkylarkCallable ann = SkylarkInterfaceUtils.getSkylarkCallable(method);
+    assertThat(ann).isNull();
+
+    // ... including when it's only present in a subclass that was bypassed.
+    method = MockClassC.class.getMethod("baz");
+    ann = SkylarkInterfaceUtils.getSkylarkCallable(MockClassA.class, method);
+    assertThat(ann).isNull();
+  }
+
+  @Test
+  public void testGetSkylarkCallableInterface() throws Exception {
+    // Search through parent interfaces. First interface takes priority.
+    Method method = MockClassC.class.getMethod("baz");
+    SkylarkCallable ann = SkylarkInterfaceUtils.getSkylarkCallable(method);
+    assertThat(ann).isNotNull();
+    assertThat(ann.doc()).isEqualTo("MockInterfaceB1#baz");
+
+    // Make sure both are still traversed.
+    method = MockClassC.class.getMethod("qux");
+    ann = SkylarkInterfaceUtils.getSkylarkCallable(method);
+    assertThat(ann).isNotNull();
+    assertThat(ann.doc()).isEqualTo("MockInterfaceB2#qux");
+  }
+
+  @Test
+  public void testGetSkylarkCallableIgnoreNonModules() throws Exception {
+    // Don't return SkylarkCallable annotations in classes and interfaces
+    // not marked @SkylarkModule.
+    Method method = MockClassD.class.getMethod("foo");
+    SkylarkCallable ann = SkylarkInterfaceUtils.getSkylarkCallable(method);
+    assertThat(ann).isNotNull();
+    assertThat(ann.doc()).isEqualTo("MockClassC#foo");
+  }
+}