Fix NPE in skylark documentation processor

Crash was triggered by overriding a @SkylarkCallable without repeating the
annotation in the subclass's method.

--
MOS_MIGRATED_REVID=134797463

3 files changed, 48 insertions, 6 deletions

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 c0ab2667f3..dd70c81b82 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
@@ -18,6 +18,7 @@ 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.SkylarkSignature;
+import com.google.devtools.build.lib.syntax.FuncallExpression;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.List;
@@ -52,7 +53,7 @@ public abstract class SkylarkMethodDoc extends SkylarkDoc {
   }
 
   private String getParameterString(Method method) {
-    SkylarkCallable annotation = method.getAnnotation(SkylarkCallable.class);
+    SkylarkCallable annotation = FuncallExpression.getAnnotationFromParentClass(method);
     int nbPositional = annotation.mandatoryPositionals();
     if (annotation.parameters().length > 0 && nbPositional < 0) {
       nbPositional = 0;
@@ -75,7 +76,7 @@ public abstract class SkylarkMethodDoc extends SkylarkDoc {
   }
 
   protected String getSignature(String objectName, String methodName, Method method) {
-    String args = method.getAnnotation(SkylarkCallable.class).structField()
+    String args = FuncallExpression.getAnnotationFromParentClass(method).structField()
         ? "" : "(" + getParameterString(method) + ")";
 
     return String.format("%s %s.%s%s",
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 2782bc8958..c41fcec477 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
@@ -105,8 +105,7 @@ public final class FuncallExpression extends Expression {
                     if (method.isSynthetic()) {
                       continue;
                     }
-                    SkylarkCallable callable =
-                        getAnnotationFromParentClass(method.getDeclaringClass(), method);
+                    SkylarkCallable callable = getAnnotationFromParentClass(method);
                     if (callable == null) {
                       continue;
                     }
@@ -164,8 +163,15 @@ public final class FuncallExpression extends Expression {
     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
-  private static SkylarkCallable getAnnotationFromParentClass(Class<?> classObj, Method method) {
+  public static SkylarkCallable getAnnotationFromParentClass(Class<?> classObj, Method method) {
     boolean keepLooking = false;
     try {
       Method superMethod = classObj.getMethod(method.getName(), method.getParameterTypes());
@@ -197,6 +203,15 @@ public final class FuncallExpression extends Expression {
     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 cb8a039a88..a536db7945 100644
--- a/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationTest.java
+++ b/src/test/java/com/google/devtools/build/docgen/SkylarkDocumentationTest.java
@@ -108,14 +108,16 @@ public class SkylarkDocumentationTest extends SkylarkTestCase {
     assertThat(collect(SkylarkRuleContext.class)).isNotEmpty();
   }
 
+  /** MockClassA */
   @SkylarkModule(name = "MockClassA", doc = "")
   public static class MockClassA {
-    @SkylarkCallable(doc = "")
+    @SkylarkCallable(doc = "MockClassA#get")
     public Integer get() {
       return 0;
     }
   }
 
+  /** MockClassB */
   @SkylarkModule(name = "MockClassB", doc = "")
   public static class MockClassB {
     @SkylarkCallable(doc = "")
@@ -124,6 +126,7 @@ public class SkylarkDocumentationTest extends SkylarkTestCase {
     }
   }
 
+  /** MockClassC */
   @SkylarkModule(name = "MockClassC", doc = "")
   public static class MockClassC extends MockClassA {
     @SkylarkCallable(doc = "")
@@ -132,6 +135,7 @@ public class SkylarkDocumentationTest extends SkylarkTestCase {
     }
   }
 
+  /** MockClassD */
   @SkylarkModule(name = "MockClassD", doc = "MockClassD")
   public static class MockClassD {
     @SkylarkCallable(
@@ -148,6 +152,15 @@ public class SkylarkDocumentationTest extends SkylarkTestCase {
     }
   }
 
+  /** MockClassE */
+  @SkylarkModule(name = "MockClassE", doc = "MockClassE")
+  public static class MockClassE extends MockClassA {
+    @Override
+    public Integer get() {
+      return 1;
+    }
+  }
+
   @Test
   public void testSkylarkJavaInterfaceExplorerOnSimpleClass() throws Exception {
     Map<String, SkylarkModuleDoc> objects = collect(MockClassA.class);
@@ -185,6 +198,19 @@ public class SkylarkDocumentationTest extends SkylarkTestCase {
     assertThat(methodDoc.getParams()).hasSize(3);
   }
 
+  @Test
+  public void testSkylarkCallableOverriding() throws Exception {
+    Map<String, SkylarkModuleDoc> objects = collect(MockClassE.class);
+    assertThat(objects).hasSize(1);
+    assertThat(objects).containsKey("MockClassE");
+    SkylarkModuleDoc moduleDoc = objects.get("MockClassE");
+    assertThat(moduleDoc.getDocumentation()).isEqualTo("MockClassE.");
+    assertThat(moduleDoc.getMethods()).hasSize(1);
+    SkylarkMethodDoc methodDoc = moduleDoc.getMethods().iterator().next();
+    assertThat(methodDoc.getDocumentation()).isEqualTo("MockClassA#get.");
+    assertThat(methodDoc.getSignature()).isEqualTo("int MockClassE.get()");
+  }
+
   private Iterable<Method> extractMethods(Collection<SkylarkJavaMethodDoc> methods) {
     return Iterables.transform(methods, new Function<SkylarkJavaMethodDoc, Method>() {
       @Override