Skylark builtin function cleanups

Clean up related to Skylark builtin functions.

Replace "hidden" field of some annotations with a "documented" field
(with reversed semantics).

--
MOS_MIGRATED_REVID=90827020

7 files changed, 25 insertions, 21 deletions

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 c9f49ee390..dce075ad4a 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
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.syntax;
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.Type.ConversionException;
+import com.google.devtools.build.lib.syntax.SkylarkType.SkylarkFunctionType;
 
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
@@ -231,7 +232,9 @@ public class BuiltinFunction extends BaseFunction {
     int arguments = signature.getSignature().getShape().getArguments();
     innerArgumentCount = arguments + (extraArgs == null ? 0 : extraArgs.length);
     Class<?>[] parameterTypes = invokeMethod.getParameterTypes();
-    Preconditions.checkArgument(innerArgumentCount == parameterTypes.length, getName());
+    Preconditions.checkArgument(innerArgumentCount == parameterTypes.length,
+        "bad argument count for %s: method has %s arguments, type list has %s",
+        getName(), innerArgumentCount, parameterTypes.length);
 
     // TODO(bazel-team): also grab the returnType from the annotations,
     // and check it against method return type
@@ -240,10 +243,12 @@ public class BuiltinFunction extends BaseFunction {
         SkylarkType enforcedType = enforcedArgumentTypes.get(i);
         if (enforcedType != null) {
           Class<?> parameterType = parameterTypes[i];
-          String msg = String.format("fun %s, param %s, enforcedType: %s (%s); parameterType: %s",
-              getName(), signature.getSignature().getNames().get(i),
+          String msg = String.format(
+              "fun %s(%s), param %s, enforcedType: %s (%s); parameterType: %s",
+              getName(), signature, signature.getSignature().getNames().get(i),
               enforcedType, enforcedType.getClass(), parameterType);
-          if (enforcedType instanceof SkylarkType.Simple) {
+          if (enforcedType instanceof SkylarkType.Simple
+              || enforcedType instanceof SkylarkFunctionType) {
             Preconditions.checkArgument(
                 enforcedType == SkylarkType.of(parameterType), msg);
             // No need to enforce Simple types on the Skylark side, the JVM will do it for us.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java
index 91b7304cc7..8a8ff9e3ef 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java
@@ -20,7 +20,6 @@ import com.google.devtools.build.lib.util.LoggingUtil;
 
 import java.util.logging.Level;
 
-
 /**
  * Exceptions thrown during evaluation of BUILD ASTs or Skylark extensions.
  *
@@ -72,7 +71,7 @@ public class EvalException extends Exception {
       message = "";
     }
     if (cause != null) {
-      message = message + (message.isEmpty() ? "" : "\n") + cause.getMessage();
+      message = message + (message.isEmpty() ? "" : ": ") + cause.getMessage();
     }
     if (message.isEmpty()) {
       LoggingUtil.logToRemote(Level.SEVERE, "Invalid EvalException", cause);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkBuiltin.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkBuiltin.java
index a2f0d1b117..894a77a9ea 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkBuiltin.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkBuiltin.java
@@ -34,7 +34,7 @@ public @interface SkylarkBuiltin {
 
   Param[] optionalParams() default {};
 
-  boolean hidden() default false;
+  boolean documented() default true;
 
   Class<?> objectType() default Object.class;
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkCallable.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkCallable.java
index ae6987ff32..89d36f1a68 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkCallable.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkCallable.java
@@ -26,9 +26,9 @@ import java.lang.annotation.Target;
 public @interface SkylarkCallable {
   String name() default "";
 
-  String doc();
+  String doc() default ""; // only allowed to be empty if documented() is false.
 
-  boolean hidden() default false;
+  boolean documented() default true;
 
   boolean structField() default false;
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkModule.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkModule.java
index 96421b2d95..a9c0f9b1f4 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkModule.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkModule.java
@@ -30,7 +30,7 @@ public @interface SkylarkModule {
 
   String doc();
 
-  boolean hidden() default false;
+  boolean documented() default true;
 
   boolean namespace() default false;
 
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 0772c42cdd..8329a096af 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
@@ -46,7 +46,7 @@ public @interface SkylarkSignature {
 
   Param[] extraKeywords() default {};
 
-  boolean undocumented() default false;
+  boolean documented() default true;
 
   Class<?> objectType() default Object.class;
 
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 6d7b1dbb1f..bacb78a7dc 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
@@ -54,8 +54,8 @@ public class SkylarkSignatureProcessor {
         ? null : new HashMap<String, SkylarkType>();
 
     HashMap<String, String> doc = new HashMap<>();
-    boolean undocumented = annotation.undocumented();
-    if (annotation.doc().isEmpty() && !undocumented) {
+    boolean documented = annotation.documented();
+    if (annotation.doc().isEmpty() && documented) {
       throw new RuntimeException(String.format("function %s is undocumented", name));
     }
 
@@ -63,11 +63,11 @@ public class SkylarkSignatureProcessor {
         ? null : defaultValues.iterator();
     try {
       for (Param param : annotation.mandatoryPositionals()) {
-        paramList.add(getParameter(name, param, enforcedTypes, doc, undocumented,
+        paramList.add(getParameter(name, param, enforcedTypes, doc, documented,
                 /*mandatory=*/true, /*star=*/false, /*starStar=*/false, /*defaultValue=*/null));
       }
       for (Param param : annotation.optionalPositionals()) {
-        paramList.add(getParameter(name, param, enforcedTypes, doc, undocumented,
+        paramList.add(getParameter(name, param, enforcedTypes, doc, documented,
                 /*mandatory=*/false, /*star=*/false, /*starStar=*/false,
                 /*defaultValue=*/getDefaultValue(param, defaultValuesIterator)));
       }
@@ -79,22 +79,22 @@ public class SkylarkSignatureProcessor {
           Preconditions.checkArgument(annotation.extraPositionals().length == 1);
           starParam = annotation.extraPositionals()[0];
         }
-        paramList.add(getParameter(name, starParam, enforcedTypes, doc, undocumented,
+        paramList.add(getParameter(name, starParam, enforcedTypes, doc, documented,
                 /*mandatory=*/false, /*star=*/true, /*starStar=*/false, /*defaultValue=*/null));
       }
       for (Param param : annotation.mandatoryNamedOnly()) {
-        paramList.add(getParameter(name, param, enforcedTypes, doc, undocumented,
+        paramList.add(getParameter(name, param, enforcedTypes, doc, documented,
                 /*mandatory=*/true, /*star=*/false, /*starStar=*/false, /*defaultValue=*/null));
       }
       for (Param param : annotation.optionalNamedOnly()) {
-        paramList.add(getParameter(name, param, enforcedTypes, doc, undocumented,
+        paramList.add(getParameter(name, param, enforcedTypes, doc, documented,
                 /*mandatory=*/false, /*star=*/false, /*starStar=*/false,
                 /*defaultValue=*/getDefaultValue(param, defaultValuesIterator)));
       }
       if (annotation.extraKeywords().length > 0) {
         Preconditions.checkArgument(annotation.extraKeywords().length == 1);
         paramList.add(
-            getParameter(name, annotation.extraKeywords()[0], enforcedTypes, doc, undocumented,
+            getParameter(name, annotation.extraKeywords()[0], enforcedTypes, doc, documented,
                 /*mandatory=*/false, /*star=*/false, /*starStar=*/true, /*defaultValue=*/null));
       }
       FunctionSignature.WithValues<Object, SkylarkType> signature =
@@ -124,7 +124,7 @@ public class SkylarkSignatureProcessor {
   // Then the only per-parameter information needed is a documentation string.
   private static Parameter<Object, SkylarkType> getParameter(
       String name, Param param, Map<String, SkylarkType> enforcedTypes,
-      Map<String, String> paramDoc, boolean undocumented,
+      Map<String, String> paramDoc, boolean documented,
       boolean mandatory, boolean star, boolean starStar, @Nullable Object defaultValue)
       throws FunctionSignature.SignatureException {
 
@@ -166,7 +166,7 @@ public class SkylarkSignatureProcessor {
     if (enforcedTypes != null) {
       enforcedTypes.put(param.name(), enforcedType);
     }
-    if (param.doc().isEmpty() && !undocumented) {
+    if (param.doc().isEmpty() && documented) {
       throw new RuntimeException(String.format("parameter %s is undocumented", name));
     }
     if (paramDoc != null) {