More Skylark function cleanups

--
MOS_MIGRATED_REVID=91713784

7 files changed, 55 insertions, 51 deletions

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 9466c694b1..07988a9374 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
@@ -173,7 +173,7 @@ public class MethodLibrary {
         @Param(name = "old", type = String.class, doc = "The string to be replaced."),
         @Param(name = "new", type = String.class, doc = "The string to replace with.")},
       optionalPositionals = {
-        @Param(name = "maxsplit", type = Integer.class,
+        @Param(name = "maxsplit", type = Integer.class, noneable = true, defaultValue = "None",
             doc = "The maximum number of replacements.")},
       useLocation = true)
   private static Function replace =
@@ -208,9 +208,10 @@ public class MethodLibrary {
       mandatoryPositionals = {
         @Param(name = "self", type = String.class, doc = "This string.")},
       optionalPositionals = {
-        @Param(name = "sep", type = String.class,
+        @Param(name = "sep", type = String.class, defaultValue = "' '",
             doc = "The string to split on, default is space (\" \")."),
-        @Param(name = "maxsplit", type = Integer.class, doc = "The maximum number of splits.")},
+        @Param(name = "maxsplit", type = Integer.class, noneable = true, defaultValue = "None",
+            doc = "The maximum number of splits.")},
       useEnvironment = true)
   private static Function split = new MixedModeFunction("split",
       ImmutableList.of("this", "sep", "maxsplit"), 1, false) {
@@ -890,10 +891,10 @@ public class MethodLibrary {
             + "otherwise value of stop and the actual start is 0"),
       },
       optionalPositionals = {
-        @Param(name = "stop", type = Integer.class, noneable = true,
+        @Param(name = "stop", type = Integer.class, noneable = true, defaultValue = "None",
             doc = "optional index of the first item <i>not</i> to be included in the "
             + "resulting list; generation of the list stops before <code>stop</code> is reached."),
-        @Param(name = "step", type = Integer.class,
+        @Param(name = "step", type = Integer.class, defaultValue = "1",
             doc = "The increment (default is 1). It may be negative.")},
       useLocation = true)
   private static final Function range =
@@ -999,7 +1000,8 @@ public class MethodLibrary {
         @Param(name = "object", doc = "The struct which's field is accessed."),
         @Param(name = "name", doc = "The name of the struct field.")},
       optionalPositionals = {
-        @Param(name = "default", doc = "The default value to return in case the struct "
+        @Param(name = "default", defaultValue = "None",
+            doc = "The default value to return in case the struct "
             + "doesn't have a field of the given name.")},
       useLocation = true)
   private static final Function getattr = new MixedModeFunction(
@@ -1014,8 +1016,9 @@ public class MethodLibrary {
         if (args[2] != null) {
           return args[2];
         } else {
-          throw new EvalException(ast.getLocation(), "Object of type '"
-              + EvalUtils.getDataTypeName(obj) + "' has no field '" + name + "'");
+          throw new EvalException(ast.getLocation(),
+              String.format("Object of type '%s' has no field %s",
+                  EvalUtils.getDataTypeName(obj), EvalUtils.prettyPrintValue(name)));
         }
       }
       return result;
@@ -1088,7 +1091,7 @@ public class MethodLibrary {
   @SkylarkSignature(name = "print", returnType = Environment.NoneType.class,
       doc = "Prints <code>msg</code> to the console.",
       optionalNamedOnly = {
-        @Param(name = "sep", type = String.class,
+        @Param(name = "sep", type = String.class, defaultValue = "' '",
             doc = "The separator string between the objects, default is space (\" \").")},
       // NB: as compared to Python3, we're missing optional named-only arguments 'end' and 'file'
       extraPositionals = {@Param(name = "args", doc = "The objects to print.")},
@@ -1222,18 +1225,7 @@ public class MethodLibrary {
   private static final List<Function> skylarkGlobalFunctions = ImmutableList
       .<Function>builder()
       .addAll(pureGlobalFunctions)
-      .add(list)
-      .add(struct)
-      .add(hasattr)
-      .add(getattr)
-      .add(set)
-      .add(dir)
-      .add(enumerate)
-      .add(range)
-      .add(type)
-      .add(fail)
-      .add(print)
-      .add(zip)
+      .add(list, struct, hasattr, getattr, set, dir, enumerate, range, type, fail, print, zip)
       .build();
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index aa9b8112d7..ac669e64de 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -447,8 +447,7 @@ public final class PackageFactory {
    */
 
   /**
-   * Returns a function-value implementing "glob" in the specified package
-   * context.
+   * Returns a function-value implementing "glob" in the specified package context.
    *
    * @param async if true, start globs in the background but don't block on their completion.
    *        Only use this for heuristic preloading.
@@ -465,7 +464,7 @@ public final class PackageFactory {
       };
   }
 
-  static Object callGlob(@Nullable PackageContext originalContext, boolean async,
+  static GlobList<String> callGlob(@Nullable PackageContext originalContext, boolean async,
       FuncallExpression ast, Environment env, Object[] namedArguments)
           throws EvalException, ConversionException, InterruptedException {
     // Skylark build extensions need to get the PackageContext from the Environment;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeModule.java
index cbc308e631..a5b438c3f2 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeModule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeModule.java
@@ -18,6 +18,7 @@ import com.google.devtools.build.lib.packages.Type.ConversionException;
 import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
+import com.google.devtools.build.lib.syntax.GlobList;
 import com.google.devtools.build.lib.syntax.SkylarkFunction;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkModule;
@@ -37,7 +38,7 @@ import java.util.Map;
 public class SkylarkNativeModule {
 
   @SkylarkSignature(name = "glob", objectType = SkylarkNativeModule.class,
-      returnType = SkylarkList.class,
+      returnType = GlobList.class,
       doc = "Glob returns a list of every file in the current package that:<ul>\n"
           + "<li>Matches at least one pattern in <code>include</code>.</li>\n"
           + "<li>Does not match any of the patterns in <code>exclude</code> "
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
index 408c4841b7..7d39e0dcfa 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.rules;
 
 import static com.google.devtools.build.lib.syntax.SkylarkType.castList;
 
-import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
 import com.google.devtools.build.lib.packages.Attribute.SkylarkLateBound;
@@ -24,6 +23,7 @@ import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.packages.Type.ConversionException;
 import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.EvalUtils;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.syntax.SkylarkCallbackFunction;
@@ -75,15 +75,18 @@ public final class SkylarkAttr {
       + "executable file, or to a rule that outputs an executable file. Access the labels with "
       + "<code>ctx.executable.&lt;attribute_name&gt;</code>.";
 
+  private static boolean containsNonNoneKey(Map<String, Object> arguments, String key) {
+    return arguments.containsKey(key) && arguments.get(key) != Environment.NONE;
+  }
+
   private static Attribute.Builder<?> createAttribute(Type<?> type, Map<String, Object> arguments,
       FuncallExpression ast, SkylarkEnvironment env) throws EvalException, ConversionException {
-    final Location loc = ast.getLocation();
     // We use an empty name now so that we can set it later.
     // This trick makes sense only in the context of Skylark (builtin rules should not use it).
     Attribute.Builder<?> builder = Attribute.attr("", type);
 
     Object defaultValue = arguments.get("default");
-    if (defaultValue != null) {
+    if (!EvalUtils.isNullOrNone(defaultValue)) {
       if (defaultValue instanceof UserDefinedFunction) {
         // Late bound attribute. Non label type attributes already caused a type check error.
         builder.value(new SkylarkLateBound(
@@ -97,23 +100,23 @@ public final class SkylarkAttr {
       builder.setPropertyFlag(flag);
     }
 
-    if (arguments.containsKey("mandatory") && (Boolean) arguments.get("mandatory")) {
+    if (containsNonNoneKey(arguments, "mandatory") && (Boolean) arguments.get("mandatory")) {
       builder.setPropertyFlag("MANDATORY");
     }
 
-    if (arguments.containsKey("non_empty") && (Boolean) arguments.get("non_empty")) {
+    if (containsNonNoneKey(arguments, "non_empty") && (Boolean) arguments.get("non_empty")) {
       builder.setPropertyFlag("NON_EMPTY");
     }
 
-    if (arguments.containsKey("executable") && (Boolean) arguments.get("executable")) {
+    if (containsNonNoneKey(arguments, "executable") && (Boolean) arguments.get("executable")) {
       builder.setPropertyFlag("EXECUTABLE");
     }
 
-    if (arguments.containsKey("single_file") && (Boolean) arguments.get("single_file")) {
+    if (containsNonNoneKey(arguments, "single_file") && (Boolean) arguments.get("single_file")) {
       builder.setPropertyFlag("SINGLE_ARTIFACT");
     }
 
-    if (arguments.containsKey("allow_files")) {
+    if (containsNonNoneKey(arguments, "allow_files")) {
       Object fileTypesObj = arguments.get("allow_files");
       if (fileTypesObj == Boolean.TRUE) {
         builder.allowedFileTypes(FileTypeSet.ANY_FILE);
@@ -122,29 +125,30 @@ public final class SkylarkAttr {
       } else if (fileTypesObj instanceof SkylarkFileType) {
         builder.allowedFileTypes(((SkylarkFileType) fileTypesObj).getFileTypeSet());
       } else {
-        throw new EvalException(loc, "allow_files should be a boolean or a filetype object.");
+        throw new EvalException(ast.getLocation(),
+            "allow_files should be a boolean or a filetype object.");
       }
     } else if (type.equals(Type.LABEL) || type.equals(Type.LABEL_LIST)) {
       builder.allowedFileTypes(FileTypeSet.NO_FILE);
     }
 
     Object ruleClassesObj = arguments.get("allow_rules");
-    if (ruleClassesObj != null) {
+    if (ruleClassesObj != null && ruleClassesObj != Environment.NONE) {
       builder.allowedRuleClasses(castList(ruleClassesObj, String.class,
               "allowed rule classes for attribute definition"));
     }
 
-    if (arguments.containsKey("providers")) {
+    if (containsNonNoneKey(arguments, "providers")) {
       builder.mandatoryProviders(castList(arguments.get("providers"), String.class));
     }
 
-    if (arguments.containsKey("cfg")) {
+    if (containsNonNoneKey(arguments, "cfg")) {
       builder.cfg((ConfigurationTransition) arguments.get("cfg"));
     }
     return builder;
   }
 
-  private static Object createAttribute(Map<String, Object> kwargs, Type<?> type,
+  private static Attribute.Builder<?> createAttribute(Map<String, Object> kwargs, Type<?> type,
       FuncallExpression ast, Environment env) throws EvalException {
     try {
       return createAttribute(type, kwargs, ast, (SkylarkEnvironment) env);
@@ -181,7 +185,7 @@ public final class SkylarkAttr {
       returnType = Attribute.Builder.class,
       optionalNamedOnly = {
         @Param(name = "default", type = String.class,
-            defaultValue = "\"\"", doc = DEFAULT_DOC + " If not specified, default is \"\"."),
+            defaultValue = "''", doc = DEFAULT_DOC + " If not specified, default is \"\"."),
         @Param(name = "flags", type = SkylarkList.class, generic1 = String.class,
             defaultValue = "[]", doc = FLAGS_DOC),
         @Param(name = "mandatory", type = Boolean.class,
@@ -239,13 +243,18 @@ public final class SkylarkAttr {
       objectType = SkylarkAttr.class,
       returnType = Attribute.class,
       optionalPositionals = {
-      @Param(name = "default", type = SkylarkList.class, generic1 = String.class,
-          doc = DEFAULT_DOC + " If not specified, default is []."),
-      @Param(name = "flags", type = SkylarkList.class, generic1 = String.class, doc = FLAGS_DOC),
-      @Param(name = "mandatory", type = Boolean.class, doc = MANDATORY_DOC),
-      @Param(name = "non_empty", type = Boolean.class, doc = NON_EMPTY_DOC),
-      @Param(name = "cfg", type = ConfigurationTransition.class,
-          doc = CONFIGURATION_DOC)})
+        @Param(name = "default", type = SkylarkList.class, generic1 = String.class,
+            defaultValue = "[]",
+            doc = DEFAULT_DOC + " If not specified, default is []."),
+        @Param(name = "flags", type = SkylarkList.class, generic1 = String.class,
+            defaultValue = "[]", doc = FLAGS_DOC),
+        @Param(name = "mandatory", type = Boolean.class, defaultValue = "False",
+            doc = MANDATORY_DOC),
+        @Param(name = "non_empty", type = Boolean.class, defaultValue = "False",
+            doc = NON_EMPTY_DOC),
+        @Param(name = "cfg", type = ConfigurationTransition.class, noneable = true,
+            defaultValue = "None", doc = CONFIGURATION_DOC)},
+      useAst = true, useEnvironment = true)
   private static SkylarkFunction stringList = new SkylarkFunction("string_list") {
       @Override
       public Object call(Map<String, Object> kwargs, FuncallExpression ast, Environment env)
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java
index 005b528833..a5ddf3d15e 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java
@@ -117,7 +117,7 @@ public abstract class BlazeModule {
    * May yield a supplier that provides factories for the Preprocessor to apply. Only one of the
    * configured modules may return non-null.
    *
-   * The factory yielded by the supplier will be checked with
+   * <p>The factory yielded by the supplier will be checked with
    * {@link Preprocessor.Factory#isStillValid} at the beginning of each incremental build. This
    * allows modules to have preprocessors customizable by flags.
    *
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
index b09371ac10..9cd37e7369 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
@@ -61,8 +61,8 @@ public final class DotExpression extends Expression {
           throw new EvalException(getLocation(), customErrorMessage);
         }
       }
-      throw new EvalException(getLocation(), "Object of type '"
-          + EvalUtils.getDataTypeName(objValue) + "' has no field '" + name + "'");
+      throw new EvalException(getLocation(), String.format("Object of type '%s' has no field %s",
+              EvalUtils.getDataTypeName(objValue), EvalUtils.prettyPrintValue(name)));
     }
     return result;
   }
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 1d9cd818bd..42946eefb1 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
@@ -190,9 +190,12 @@ public class SkylarkSignatureProcessor {
       return new Parameter.Star<>(param.name(), officialType);
     } else if (mandatory) {
       return new Parameter.Mandatory<>(param.name(), officialType);
-    } else {
-      return new Parameter.Optional<>(param.name(), officialType, defaultValue);
+    } else if (defaultValue != null && enforcedType != null) {
+      Preconditions.checkArgument(enforcedType.contains(defaultValue),
+          "In function '%s', parameter '%s' has default value %s that isn't of enforced type %s",
+          name, param.name(), EvalUtils.prettyPrintValue(defaultValue), enforcedType);
     }
+    return new Parameter.Optional<>(param.name(), officialType, defaultValue);
   }
 
   private static Object getDefaultValue(Param param, Iterator<Object> iterator) {