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
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 d93411d..4c9743c 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 @@
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 @@
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 @@
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 @@
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 fe5c453..4bafd3f 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 @@
}
}
+ /** 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 @@
}
}
+ /** TODO(bazel-team): document this */
public static class RunsPerTestConverter extends PerLabelOptions.PerLabelOptionsConverter {
@Override
public PerLabelOptions convert(String input) throws OptionsParsingException {
@@ -1640,7 +1642,7 @@
/**
* 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 23e54a1..57d9865 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 @@
};
// 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 @@
};
// 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 @@
}
};
- @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 @@
};
// 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 @@
}
// 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 ec00b30..28eaf79 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 @@
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 @@
@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 @@
}
// 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 @@
}
// 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 @@
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 c9f49ee..dce075a 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 @@
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 @@
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 @@
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 91b7304..8a8ff9e 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 java.util.logging.Level;
-
/**
* Exceptions thrown during evaluation of BUILD ASTs or Skylark extensions.
*
@@ -72,7 +71,7 @@
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 a2f0d1b..894a77a 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 @@
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 ae6987f..89d36f1 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 @@
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 96421b2..a9c0f9b 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 @@
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 0772c42..8329a09 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 @@
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 6d7b1db..bacb78a 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 @@
? 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 @@
? 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 @@
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 @@
// 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 @@
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) {
