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

11 files changed, 49 insertions, 39 deletions

diff --git a/src/main/java/com/google/devtools/build/docgen/SkylarkDocumentationProcessor.java b/src/main/java/com/google/devtools/build/docgen/SkylarkDocumentationProcessor.java
index d93411d313..4c9743c7c7 100644
--- a/src/main/java/com/google/devtools/build/docgen/SkylarkDocumentationProcessor.java
+++ b/src/main/java/com/google/devtools/build/docgen/SkylarkDocumentationProcessor.java
@@ -114,7 +114,7 @@ public class SkylarkDocumentationProcessor {
     SkylarkModuleDoc topLevelModule = modules.remove(getTopLevelModule().name());
     generateModuleDoc(topLevelModule, sb);
     for (SkylarkModuleDoc module : modules.values()) {
-      if (!module.getAnnotation().hidden()) {
+      if (module.getAnnotation().documented()) {
         sb.append("<hr>");
         generateModuleDoc(module, sb);
       }
@@ -162,7 +162,7 @@ public class SkylarkDocumentationProcessor {
   private void generateBuiltinItemDoc(
       String moduleId, SkylarkBuiltinMethod method, StringBuilder sb) {
     SkylarkBuiltin annotation = method.annotation;
-    if (annotation.hidden()) {
+    if (!annotation.documented()) {
       return;
     }
     sb.append(String.format("<li><h3 id=\"modules.%s.%s\">%s</h3>\n",
@@ -194,9 +194,13 @@ public class SkylarkDocumentationProcessor {
 
   private void generateDirectJavaMethodDoc(String objectName, String methodName,
       Method method, SkylarkCallable annotation, StringBuilder sb) {
-    if (annotation.hidden()) {
+    if (!annotation.documented()) {
       return;
     }
+    if (annotation.doc().isEmpty()) {
+      throw new RuntimeException(String.format(
+          "empty SkylarkCallable.doc() for object %s, method %s", objectName, methodName));
+    }
 
     sb.append(String.format("<li><h3 id=\"modules.%s.%s\">%s</h3>\n%s\n",
             objectName,
@@ -325,7 +329,7 @@ public class SkylarkDocumentationProcessor {
     for (SkylarkModuleDoc doc : collectBuiltinModules().values()) {
       if (doc.getAnnotation() == getTopLevelModule()) {
         for (Map.Entry<String, SkylarkBuiltinMethod> entry : doc.getBuiltinMethods().entrySet()) {
-          if (!entry.getValue().annotation.hidden()) {
+          if (entry.getValue().annotation.documented()) {
             modules.put(entry.getKey(),
                 DocgenConsts.toCommandLineFormat(entry.getValue().annotation.doc()));
           }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index fe5c4534fe..4bafd3f2f9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -300,6 +300,7 @@ public final class BuildConfiguration implements Serializable {
     }
   }
 
+  /** TODO(bazel-team): document this */
   public static class PluginOptionConverter implements Converter<Map.Entry<String, String>> {
     @Override
     public Map.Entry<String, String> convert(String input) throws OptionsParsingException {
@@ -318,6 +319,7 @@ public final class BuildConfiguration implements Serializable {
     }
   }
 
+  /** TODO(bazel-team): document this */
   public static class RunsPerTestConverter extends PerLabelOptions.PerLabelOptionsConverter {
     @Override
     public PerLabelOptions convert(String input) throws OptionsParsingException {
@@ -1640,7 +1642,7 @@ public final class BuildConfiguration implements Serializable {
   /**
    * Returns a configuration fragment instances of the given class.
    */
-  @SkylarkCallable(name = "fragment", hidden = true,
+  @SkylarkCallable(name = "fragment", documented = false,
       doc = "Returns a configuration fragment using the key.")
   public <T extends Fragment> T getFragment(Class<T> clazz) {
     return clazz.cast(fragments.get(clazz));
diff --git a/src/main/java/com/google/devtools/build/lib/packages/MethodLibrary.java b/src/main/java/com/google/devtools/build/lib/packages/MethodLibrary.java
index 23e54a1507..57d986596b 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/MethodLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/MethodLibrary.java
@@ -416,7 +416,7 @@ public class MethodLibrary {
       };
 
   // slice operator
-  @SkylarkBuiltin(name = "$slice", hidden = true,
+  @SkylarkBuiltin(name = "$slice", documented = false,
       doc = "x[<code>start</code>:<code>end</code>] returns a slice or a list slice.")
   private static Function slice = new MixedModeFunction("$slice",
       ImmutableList.of("this", "start", "end"), 3, false) {
@@ -446,7 +446,7 @@ public class MethodLibrary {
   };
 
   // supported list methods
-  @SkylarkBuiltin(name = "append", hidden = true,
+  @SkylarkBuiltin(name = "append", documented = false,
       doc = "Adds an item to the end of the list.")
   private static Function append = new MixedModeFunction("append",
       ImmutableList.of("this", "x"), 2, false) {
@@ -459,7 +459,7 @@ public class MethodLibrary {
     }
   };
 
-  @SkylarkBuiltin(name = "extend", hidden = true,
+  @SkylarkBuiltin(name = "extend", documented = false,
       doc = "Adds all items to the end of the list.")
   private static Function extend = new MixedModeFunction("extend",
           ImmutableList.of("this", "x"), 2, false) {
@@ -474,7 +474,7 @@ public class MethodLibrary {
   };
 
   // dictionary access operator
-  @SkylarkBuiltin(name = "$index", hidden = true,
+  @SkylarkBuiltin(name = "$index", documented = false,
       doc = "Returns the nth element of a list or string, "
           + "or looks up a value in a dictionary.")
   private static Function indexOperator = new MixedModeFunction("$index",
@@ -608,7 +608,7 @@ public class MethodLibrary {
   }
 
   // unary minus
-  @SkylarkBuiltin(name = "-", hidden = true, doc = "Unary minus operator.")
+  @SkylarkBuiltin(name = "-", documented = false, doc = "Unary minus operator.")
   private static Function minus = new MixedModeFunction("-", ImmutableList.of("this"), 1, false) {
     @Override
     public Object call(Object[] args, FuncallExpression ast) throws ConversionException {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java
index ec00b301ef..28eaf79401 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java
@@ -407,7 +407,7 @@ public final class SkylarkRuleContext {
     return ruleContext.getLabel().toString();
   }
 
-  @SkylarkCallable(doc = "Splits a shell command to a list of tokens.", hidden = true)
+  @SkylarkCallable(doc = "Splits a shell command to a list of tokens.", documented = false)
   public List<String> tokenize(String optionString) throws FuncallException {
     List<String> options = new ArrayList<>();
     try {
@@ -421,7 +421,7 @@ public final class SkylarkRuleContext {
   @SkylarkCallable(doc =
       "Expands all references to labels embedded within a string for all files using a mapping "
     + "from definition labels (i.e. the label in the output type attribute) to files. Deprecated.",
-      hidden = true)
+      documented = false)
   public String expand(@Nullable String expression,
       List<Artifact> artifacts, Label labelResolver) throws FuncallException {
     try {
@@ -446,7 +446,7 @@ public final class SkylarkRuleContext {
   }
 
   // Kept for compatibility with old code.
-  @SkylarkCallable(hidden = true, doc = "")
+  @SkylarkCallable(documented = false)
   public Artifact newFile(Root root, String filename) {
     PathFragment fragment = ruleContext.getLabel().getPackageFragment();
     for (String pathFragmentString : filename.split("/")) {
@@ -465,19 +465,19 @@ public final class SkylarkRuleContext {
   }
 
   // Kept for compatibility with old code.
-  @SkylarkCallable(hidden = true, doc = "")
+  @SkylarkCallable(documented = false)
   public Artifact newFile(Root root, Artifact baseArtifact, String suffix) {
     PathFragment original = baseArtifact.getRootRelativePath();
     PathFragment fragment = original.replaceName(original.getBaseName() + suffix);
     return ruleContext.getAnalysisEnvironment().getDerivedArtifact(fragment, root);
   }
 
-  @SkylarkCallable(doc = "", hidden = true)
+  @SkylarkCallable(documented = false)
   public NestedSet<Artifact> middleMan(String attribute) {
     return AnalysisUtils.getMiddlemanFor(ruleContext, attribute);
   }
 
-  @SkylarkCallable(doc = "", hidden = true)
+  @SkylarkCallable(documented = false)
   public boolean checkPlaceholders(String template, List<String> allowedPlaceholders) {
     List<String> actualPlaceHolders = new LinkedList<>();
     Set<String> allowedPlaceholderSet = ImmutableSet.copyOf(allowedPlaceholders);
@@ -527,14 +527,14 @@ public final class SkylarkRuleContext {
     return executableRunfilesMap.get(executable);
   }
 
-  @SkylarkCallable(name = "info_file", structField = true, hidden = true,
+  @SkylarkCallable(name = "info_file", structField = true, documented = false,
       doc = "Returns the file that is used to hold the non-volatile workspace status for the " 
           + "current build request.")
   public Artifact getStableWorkspaceStatus() {
     return ruleContext.getAnalysisEnvironment().getStableWorkspaceStatusArtifact();
   }
 
-  @SkylarkCallable(name = "version_file", structField = true, hidden = true,
+  @SkylarkCallable(name = "version_file", structField = true, documented = false,
       doc = "Returns the file that is used to hold the volatile workspace status for the "
           + "current build request.")
   public Artifact getVolatileWorkspaceStatus() {
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) {