Debug BuiltinFunction

--
MOS_MIGRATED_REVID=90004683

4 files changed, 42 insertions, 21 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/Type.java b/src/main/java/com/google/devtools/build/lib/packages/Type.java
index 633f0e6faf..e869096e95 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Type.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Type.java
@@ -84,26 +84,38 @@ public abstract class Type<T> {
   // this over selectableConvert.
 
   /**
-   * Equivalent to {@code convert(x, what, null)}. Useful for converting values to types that do not
-   * involve the type {@code LABEL} and hence do not require the label of the current package.
+   * Equivalent to {@link #convert(Object, String, Label)} where the label is {@code null}.
+   * Useful for converting values to types that do not involve the type {@code LABEL}
+   * and hence do not require the label of the current package.
    */
   public final T convert(Object x, String what) throws ConversionException {
     return convert(x, what, null);
   }
 
   /**
-   * Like {@code convert(x, what, label)}, but converts skylark {@code None} to java {@code null}.
+   * Like {@link #convert(Object, String, Label)}, but converts skylark {@code None}
+   * to given {@code defaultValue}.
    */
-  @Nullable public final T convertOptional (Object x, String what, @Nullable Label currentRule)
+  @Nullable public final T convertOptional(Object x,
+      String what, @Nullable Label currentRule, T defaultValue)
       throws ConversionException {
     if (EvalUtils.isNullOrNone(x)) {
-      return null;
+      return defaultValue;
     }
     return convert(x, what, currentRule);
   }
 
   /**
-   * Like {@code convert(x, what)}, but converts skylark {@code NONE} to java {@code null}.
+   * Like {@link #convert(Object, String, Label)}, but converts skylark {@code None}
+   * to java {@code null}.
+   */
+  @Nullable public final T convertOptional(Object x, String what, @Nullable Label currentRule)
+      throws ConversionException {
+    return convertOptional(x, what, currentRule, null);
+  }
+
+  /**
+   * Like {@link #convert(Object, String)}, but converts skylark {@code NONE} to java {@code null}.
    */
   @Nullable public final T convertOptional(Object x, String what) throws ConversionException {
     return convertOptional(x, what, null);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
index 23b5c7dd03..c9f49ee390 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
@@ -154,10 +154,10 @@ public class BuiltinFunction extends BaseFunction {
           || e instanceof ClassCastException
           || e instanceof ExecutionException
           || e instanceof IllegalStateException) {
-        throw new EvalException(loc, e.getMessage(), e);
+        throw new EvalException(loc, e);
       } else if (e instanceof IllegalArgumentException) {
         // Assume it was thrown by SkylarkType.cast and has a good message.
-        throw new EvalException(loc, String.format("%s\nin call to %s", e.getMessage(), this), e);
+        throw new EvalException(loc, "Illegal argument in call to " + getName(), e);
       } else {
         throw badCallException(loc, e, args);
       }
@@ -168,12 +168,12 @@ public class BuiltinFunction extends BaseFunction {
         final Class<?>[] types = invokeMethod.getParameterTypes();
         for (int i = 0; i < args.length; i++) {
           if (args[i] != null && !types[i].isAssignableFrom(args[i].getClass())) {
-            final String paramName = i < len
+            String paramName = i < len
                 ? signature.getSignature().getNames().get(i) : extraArgs[i - len].name();
             throw new EvalException(loc, String.format(
                 "expected %s for '%s' while calling %s but got %s instead",
                 EvalUtils.getDataTypeNameFromClass(types[i]), paramName, getName(),
-                EvalUtils.getDataTypeName(args[i])), e);
+                EvalUtils.getDataTypeName(args[i])));
           }
         }
         throw badCallException(loc, e, args);
@@ -209,7 +209,7 @@ public class BuiltinFunction extends BaseFunction {
     for (Method method : this.getClass().getDeclaredMethods()) {
       method.setAccessible(true);
       if (name.equals(method.getName())) {
-        if (method != null) {
+        if (found != null) {
           throw new IllegalArgumentException(String.format(
               "function %s has more than one method named %s", getName(), name));
         }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignature.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignature.java
index 1bd34a9ded..0772c42cdd 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignature.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignature.java
@@ -32,7 +32,7 @@ public @interface SkylarkSignature {
 
   String name();
 
-  String doc();
+  String doc() default "";
 
   Param[] mandatoryPositionals() default {};
 
@@ -71,7 +71,7 @@ public @interface SkylarkSignature {
 
     String name();
 
-    String doc();
+    String doc() default "";
 
     String defaultValue() default "";
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
index 614018875a..c772038276 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
@@ -52,17 +52,22 @@ public class SkylarkSignatureProcessor {
     ArrayList<Parameter<Object, SkylarkType>> paramList = new ArrayList<>();
     HashMap<String, SkylarkType> enforcedTypes = enforcedTypesList == null
         ? null : new HashMap<String, SkylarkType>();
+
     HashMap<String, String> doc = new HashMap<>();
+    boolean undocumented = annotation.undocumented();
+    if (annotation.doc().isEmpty() && !undocumented) {
+      throw new RuntimeException(String.format("function %s is undocumented", name));
+    }
 
     Iterator<Object> defaultValuesIterator = defaultValues == null
         ? null : defaultValues.iterator();
     try {
       for (Param param : annotation.mandatoryPositionals()) {
-        paramList.add(getParameter(name, param, doc, enforcedTypes,
+        paramList.add(getParameter(name, param, enforcedTypes, doc, undocumented,
                 /*mandatory=*/true, /*star=*/false, /*starStar=*/false, /*defaultValue=*/null));
       }
       for (Param param : annotation.optionalPositionals()) {
-        paramList.add(getParameter(name, param, doc, enforcedTypes,
+        paramList.add(getParameter(name, param, enforcedTypes, doc, undocumented,
                 /*mandatory=*/false, /*star=*/false, /*starStar=*/false,
                 /*defaultValue=*/getDefaultValue(param, defaultValuesIterator)));
       }
@@ -74,22 +79,22 @@ public class SkylarkSignatureProcessor {
           Preconditions.checkArgument(annotation.extraPositionals().length == 1);
           starParam = annotation.extraPositionals()[0];
         }
-        paramList.add(getParameter(name, starParam, doc, enforcedTypes,
+        paramList.add(getParameter(name, starParam, enforcedTypes, doc, undocumented,
                 /*mandatory=*/false, /*star=*/true, /*starStar=*/false, /*defaultValue=*/null));
       }
       for (Param param : annotation.optionalNamedOnly()) {
-        paramList.add(getParameter(name, param, doc, enforcedTypes,
+        paramList.add(getParameter(name, param, enforcedTypes, doc, undocumented,
                 /*mandatory=*/false, /*star=*/false, /*starStar=*/false,
                 /*defaultValue=*/getDefaultValue(param, defaultValuesIterator)));
       }
       for (Param param : annotation.mandatoryNamedOnly()) {
-        paramList.add(getParameter(name, param, doc, enforcedTypes,
+        paramList.add(getParameter(name, param, enforcedTypes, doc, undocumented,
                 /*mandatory=*/true, /*star=*/false, /*starStar=*/false, /*defaultValue=*/null));
       }
       if (annotation.extraKeywords().length > 0) {
         Preconditions.checkArgument(annotation.extraKeywords().length == 1);
         paramList.add(
-            getParameter(name, annotation.extraKeywords()[0], doc, enforcedTypes,
+            getParameter(name, annotation.extraKeywords()[0], enforcedTypes, doc, undocumented,
                 /*mandatory=*/false, /*star=*/false, /*starStar=*/true, /*defaultValue=*/null));
       }
       FunctionSignature.WithValues<Object, SkylarkType> signature =
@@ -117,8 +122,9 @@ public class SkylarkSignatureProcessor {
   // process it? (builtin function call not allowed when evaluating values, but more complex
   // values are possible by referencing variables in some definition environment).
   // Then the only per-parameter information needed is a documentation string.
-  private static Parameter<Object, SkylarkType> getParameter(String name,
-      Param param, Map<String, String> paramDoc, Map<String, SkylarkType> enforcedTypes,
+  private static Parameter<Object, SkylarkType> getParameter(
+      String name, Param param, Map<String, SkylarkType> enforcedTypes,
+      Map<String, String> paramDoc, boolean undocumented,
       boolean mandatory, boolean star, boolean starStar, @Nullable Object defaultValue)
       throws FunctionSignature.SignatureException {
 
@@ -160,6 +166,9 @@ public class SkylarkSignatureProcessor {
     if (enforcedTypes != null) {
       enforcedTypes.put(param.name(), enforcedType);
     }
+    if (param.doc().isEmpty() && !undocumented) {
+      throw new RuntimeException(String.format("parameter %s is undocumented", name));
+    }
     if (paramDoc != null) {
       paramDoc.put(param.name(), param.doc());
     }