More skylark function cleanups

--
MOS_MIGRATED_REVID=91407816

5 files changed, 91 insertions, 88 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 7104ead7c2..84775f4caa 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
@@ -68,7 +68,7 @@ public class MethodLibrary {
   // Convert string index in the same way Python does.
   // If index is negative, starts from the end.
   // If index is outside bounds, it is restricted to the valid range.
-  private static int getClampedIndex(int index, int length) {
+  private static int clampIndex(int index, int length) {
     if (index < 0) {
       index += length;
     }
@@ -77,14 +77,22 @@ public class MethodLibrary {
 
   // Emulate Python substring function
   // It converts out of range indices, and never fails
-  private static String getPythonSubstring(String str, int start, int end) {
-    start = getClampedIndex(start, str.length());
-    end = getClampedIndex(end, str.length());
-    if (start > end) {
-      return "";
+  private static String pythonSubstring(String str, int start, Object end, String msg)
+      throws ConversionException {
+    if (start == 0 && EvalUtils.isNullOrNone(end)) {
+      return str;
+    }
+    start = clampIndex(start, str.length());
+    int stop;
+    if (EvalUtils.isNullOrNone(end)) {
+      stop = str.length();
     } else {
-      return str.substring(start, end);
+      stop = clampIndex(Type.INTEGER.convert(end, msg), str.length());
     }
+    if (start >= stop) {
+      return "";
+    }
+    return str.substring(start, stop);
   }
 
   public static int getListIndex(Object key, int listSize, FuncallExpression ast)
@@ -193,13 +201,25 @@ public class MethodLibrary {
       int maxsplit = args[2] != null
           ? Type.INTEGER.convert(args[2], "'split' argument") + 1 // last is remainder
           : -1;
-      String[] ss = Pattern.compile(sep, Pattern.LITERAL).split(thiz,
-                                                                maxsplit);
+      String[] ss = Pattern.compile(sep, Pattern.LITERAL).split(thiz, maxsplit);
       List<String> result = java.util.Arrays.asList(ss);
       return env.isSkylarkEnabled() ? SkylarkList.list(result, String.class) : result;
     }
   };
 
+  /**
+   * Common implementation for find, rfind, index, rindex.
+   * @param forward true if we want to return the last matching index.
+   */
+  private static int stringFind(boolean forward,
+      String self, String sub, int start, Object end, String msg)
+      throws ConversionException {
+    String substr = pythonSubstring(self, start, end, msg);
+    int subpos = forward ? substr.indexOf(sub) : substr.lastIndexOf(sub);
+    start = clampIndex(start, self.length());
+    return subpos < 0 ? subpos : subpos + start;
+  }
+
   // Common implementation for find, rfind, index, rindex.
   // forward is true iff we want to return the last matching index.
   private static int stringFind(String functionName, boolean forward, Object[] args)
@@ -207,17 +227,10 @@ public class MethodLibrary {
     String thiz = Type.STRING.convert(args[0], functionName + " operand");
     String sub = Type.STRING.convert(args[1], functionName + " argument");
     int start = 0;
-    if (args[2] != null) {
+    if (!EvalUtils.isNullOrNone(args[2])) {
       start = Type.INTEGER.convert(args[2], functionName + " argument");
     }
-    int end = thiz.length();
-    if (args[3] != null) {
-      end = Type.INTEGER.convert(args[3], functionName + " argument");
-    }
-    String substr = getPythonSubstring(thiz, start, end);
-    int subpos = forward ? substr.indexOf(sub) : substr.lastIndexOf(sub);
-    start = getClampedIndex(start, thiz.length());
-    return subpos < 0 ? subpos : subpos + start;
+    return stringFind(forward, thiz, sub, start, args[3], functionName + " argument");
   }
 
   @SkylarkBuiltin(name = "rfind", objectType = StringModule.class, returnType = Integer.class,
@@ -275,7 +288,8 @@ public class MethodLibrary {
           int res = stringFind("rindex", false, args);
           if (res < 0) {
             throw new EvalException(ast.getLocation(),
-                "substring '" + args[1] + "' not found in '" + args[0] + "'");
+                "substring " + EvalUtils.prettyPrintValue(args[1])
+                + " not found in " + EvalUtils.prettyPrintValue(args[0]));
           }
           return res;
         }
@@ -299,7 +313,8 @@ public class MethodLibrary {
           int res = stringFind("index", true, args);
           if (res < 0) {
             throw new EvalException(ast.getLocation(),
-                "substring '" + args[1] + "' not found in '" + args[0] + "'");
+                "substring " + EvalUtils.prettyPrintValue(args[1])
+                + " not found in " + EvalUtils.prettyPrintValue(args[0]));
           }
           return res;
         }
@@ -325,11 +340,7 @@ public class MethodLibrary {
           if (args[2] != null) {
             start = Type.INTEGER.convert(args[2], "'count' argument");
           }
-          int end = thiz.length();
-          if (args[3] != null) {
-            end = Type.INTEGER.convert(args[3], "'count' argument");
-          }
-          String str = getPythonSubstring(thiz, start, end);
+          String str = pythonSubstring(thiz, start, args[3], "'end' argument to 'count'");
           if (sub.isEmpty()) {
             return str.length() + 1;
           }
@@ -363,12 +374,8 @@ public class MethodLibrary {
           if (args[2] != null) {
             start = Type.INTEGER.convert(args[2], "'endswith' argument");
           }
-          int end = thiz.length();
-          if (args[3] != null) {
-            end = Type.INTEGER.convert(args[3], "");
-          }
-
-          return getPythonSubstring(thiz, start, end).endsWith(sub);
+          return pythonSubstring(thiz, start, args[3], "'end' argument to 'endswith'")
+              .endsWith(sub);
         }
       };
 
@@ -391,11 +398,8 @@ public class MethodLibrary {
       if (args[2] != null) {
         start = Type.INTEGER.convert(args[2], "'startswith' argument");
       }
-      int end = thiz.length();
-      if (args[3] != null) {
-        end = Type.INTEGER.convert(args[3], "'startswith' argument");
-      }
-      return getPythonSubstring(thiz, start, end).startsWith(sub);
+      return pythonSubstring(thiz, start, args[3], "'end' argument to 'startswith'")
+          .startsWith(sub);
     }
   };
 
@@ -427,13 +431,13 @@ public class MethodLibrary {
       // Substring
       if (args[0] instanceof String) {
         String thiz = Type.STRING.convert(args[0], "substring operand");
-        return getPythonSubstring(thiz, left, right);
+        return pythonSubstring(thiz, left, right, "");
       }
 
       // List slice
       List<Object> list = Type.OBJECT_LIST.convert(args[0], "list operand");
-      left = getClampedIndex(left, list.size());
-      right = getClampedIndex(right, list.size());
+      left = clampIndex(left, list.size());
+      right = clampIndex(right, list.size());
 
       List<Object> result = Lists.newArrayList();
       for (int i = left; i < right; i++) {
@@ -502,7 +506,8 @@ public class MethodLibrary {
       if (collectionCandidate instanceof Map<?, ?>) {
         Map<?, ?> dictionary = (Map<?, ?>) collectionCandidate;
         if (!dictionary.containsKey(key)) {
-          throw new EvalException(ast.getLocation(), "Key '" + key + "' not found in dictionary");
+          throw new EvalException(ast.getLocation(),
+              "Key " + EvalUtils.prettyPrintValue(key) + " not found in dictionary");
         }
         return dictionary.get(key);
       } else if (collectionCandidate instanceof List<?>) {
@@ -578,6 +583,8 @@ public class MethodLibrary {
           + "</pre>\n")
   private static Function keys = new NoArgFunction("keys") {
     @Override
+    @SuppressWarnings("unchecked") // Skylark will only call this on a dict; and
+    // allowed keys are all Comparable... if not mutually, it's OK to get a runtime exception.
     public Object call(Object self, FuncallExpression ast, Environment env)
         throws EvalException, InterruptedException {
       Map<Comparable<?>, Object> dict = (Map<Comparable<?>, Object>) self;
@@ -700,7 +707,8 @@ public class MethodLibrary {
           try {
             return Integer.parseInt(str);
           } catch (NumberFormatException e) {
-            throw new EvalException(ast.getLocation(), "invalid literal for int(): " + str);
+            throw new EvalException(ast.getLocation(),
+                "invalid literal for int(): " + EvalUtils.prettyPrintValue(str));
           }
         }
       };
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkModules.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkModules.java
index e75ebb25e2..7ab0cee813 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkModules.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkModules.java
@@ -47,10 +47,13 @@ public class SkylarkModules {
    * modules. They are also registered with the {@link ValidationEnvironment} and the
    * {@link SkylarkEnvironment}. Note that only {@link SkylarkFunction}s are handled properly.
    */
+  // TODO(bazel-team): find a more general, more automated way of registering classes and building
+  // initial environments. And don't give syntax.Environment and packages.MethodLibrary a special
+  // treatment, have them use the same registration mechanism as other classes currently below.
   public static final ImmutableList<Class<?>> MODULES = ImmutableList.of(
-      SkylarkNativeModule.class,
       SkylarkAttr.class,
       SkylarkCommandLine.class,
+      SkylarkNativeModule.class,
       SkylarkRuleClassFunctions.class,
       SkylarkRuleImplementationFunctions.class);
 
@@ -140,7 +143,7 @@ public class SkylarkModules {
   }
 
   /**
-   * Collects the SkylarkFunctions from the fields of the class of the object parameter
+   * Collects the Functions from the fields of the class of the object parameter
    * and adds them into the builder.
    */
   private static void collectSkylarkFunctionsAndObjectsFromFields(Class<?> type,
@@ -148,18 +151,19 @@ public class SkylarkModules {
     try {
       for (Field field : type.getDeclaredFields()) {
         if (field.isAnnotationPresent(SkylarkBuiltin.class)) {
-          // Fields in Skylark modules are sometimes private. Nevertheless they have to
-          // be annotated with SkylarkBuiltin.
+          // Fields in Skylark modules are sometimes private.
+          // Nevertheless they have to be annotated with SkylarkBuiltin.
           field.setAccessible(true);
           SkylarkBuiltin annotation = field.getAnnotation(SkylarkBuiltin.class);
+          Object value = field.get(null);
           if (SkylarkFunction.class.isAssignableFrom(field.getType())) {
-            SkylarkFunction function = (SkylarkFunction) field.get(null);
+            SkylarkFunction function = (SkylarkFunction) value;
             if (!function.isConfigured()) {
               function.configure(annotation);
             }
             functions.add(function);
           } else {
-            objects.put(annotation.name(), field.get(null));
+            objects.put(annotation.name(), value);
           }
         }
       }
@@ -170,7 +174,7 @@ public class SkylarkModules {
   }
 
   /**
-   * Collects the SkylarkFunctions from the fields of the class of the object parameter
+   * Collects the Functions from the fields of the class of the object parameter
    * and adds their class and their corresponding return value to the builder.
    */
   private static void collectSkylarkTypesFromFields(Class<?> classObject, Set<String> builtIn) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
index 071185ccba..9f5b6be371 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
@@ -212,7 +212,8 @@ public class SkylarkRuleClassFunctions {
   private static final SkylarkFunction rule = new SkylarkFunction("rule") {
 
         @Override
-        @SuppressWarnings("rawtypes")
+        @SuppressWarnings({"rawtypes", "unchecked"}) // castMap produces
+        // an Attribute.Builder instead of a Attribute.Builder<?> but it's OK.
         public Object call(Map<String, Object> arguments, FuncallExpression ast,
             Environment funcallEnv) throws EvalException, ConversionException {
           final Location loc = ast.getLocation();
@@ -282,6 +283,8 @@ public class SkylarkRuleClassFunctions {
       this.type = type;
     }
 
+    @SuppressWarnings("unchecked") // the magic hidden $pkg_context variable is guaranteed
+    // to be a PackageContext
     @Override
     public Object call(List<Object> args, Map<String, Object> kwargs, FuncallExpression ast,
         Environment env) throws EvalException, InterruptedException {
@@ -363,9 +366,9 @@ public class SkylarkRuleClassFunctions {
     @Override
     public Object call(Map<String, Object> arguments, Location loc) throws EvalException,
         ConversionException {
-      ClassObject object = (ClassObject) arguments.get("self");
+      ClassObject self = (ClassObject) arguments.get("self");
       StringBuilder sb = new StringBuilder();
-      printTextMessage(object, sb, 0, loc);
+      printTextMessage(self, sb, 0, loc);
       return sb.toString();
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
index 034bf3b50a..c8448930b2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
@@ -16,10 +16,10 @@ package com.google.devtools.build.lib.skyframe;
 
 import static com.google.devtools.build.lib.syntax.Environment.NONE;
 
-import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.cmdline.LabelValidator;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.ExternalPackage.Binding;
 import com.google.devtools.build.lib.packages.ExternalPackage.Builder;
@@ -28,16 +28,15 @@ import com.google.devtools.build.lib.packages.Package.NameConflictException;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleFactory;
-import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.packages.Type.ConversionException;
-import com.google.devtools.build.lib.syntax.AbstractFunction;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
+import com.google.devtools.build.lib.syntax.BuiltinFunction;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
 import com.google.devtools.build.lib.syntax.Function;
+import com.google.devtools.build.lib.syntax.FunctionSignature;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.syntax.Label.SyntaxException;
-import com.google.devtools.build.lib.syntax.MixedModeFunction;
 import com.google.devtools.build.lib.syntax.ParserInputSource;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -134,16 +133,16 @@ public class WorkspaceFileFunction implements SkyFunction {
     return null;
   }
 
-  private static Function newWorkspaceNameFunction(final Builder builder) {
-    List<String> params = ImmutableList.of("name");
-    return new MixedModeFunction("workspace", params, 1, true) {
-      @Override
-      public Object call(Object[] namedArgs, FuncallExpression ast) throws EvalException,
-          ConversionException, InterruptedException {
-        String name = Type.STRING.convert(namedArgs[0], "'name' argument");
+  // TODO(bazel-team): use @SkylarkSignature annotations on a BuiltinFunction.Factory
+  // for signature + documentation of this and other functions in this file.
+  private static BuiltinFunction newWorkspaceNameFunction(final Builder builder) {
+    return new BuiltinFunction("workspace",
+        FunctionSignature.namedOnly("name"), BuiltinFunction.USE_LOC) {
+      public Object invoke(String name,
+          Location loc) throws EvalException {
         String errorMessage = LabelValidator.validateTargetName(name);
         if (errorMessage != null) {
-          throw new EvalException(ast.getLocation(), errorMessage);
+          throw new EvalException(loc, errorMessage);
         }
         builder.setWorkspaceName(name);
         return NONE;
@@ -151,24 +150,19 @@ public class WorkspaceFileFunction implements SkyFunction {
     };
   }
 
-  private static Function newBindFunction(final Builder builder) {
-    List<String> params = ImmutableList.of("name", "actual");
-    return new MixedModeFunction(BIND, params, 2, true) {
-      @Override
-      public Object call(Object[] namedArgs, FuncallExpression ast)
-              throws EvalException, ConversionException {
-        String name = Type.STRING.convert(namedArgs[0], "'name' argument");
-        String actual = Type.STRING.convert(namedArgs[1], "'actual' argument");
-
+  private static BuiltinFunction newBindFunction(final Builder builder) {
+    return new BuiltinFunction(BIND,
+        FunctionSignature.namedOnly("name", "actual"), BuiltinFunction.USE_LOC) {
+      public Object invoke(String name, String actual,
+          Location loc) throws EvalException, ConversionException, InterruptedException {
         Label nameLabel = null;
         try {
           nameLabel = Label.parseAbsolute("//external:" + name);
           builder.addBinding(
-              nameLabel, new Binding(Label.parseRepositoryLabel(actual), ast.getLocation()));
+              nameLabel, new Binding(Label.parseRepositoryLabel(actual), loc));
         } catch (SyntaxException e) {
-          throw new EvalException(ast.getLocation(), e.getMessage());
+          throw new EvalException(loc, e.getMessage());
         }
-
         return NONE;
       }
     };
@@ -178,18 +172,12 @@ public class WorkspaceFileFunction implements SkyFunction {
    * Returns a function-value implementing the build rule "ruleClass" (e.g. cc_library) in the
    * specified package context.
    */
-  private static Function newRuleFunction(final RuleFactory ruleFactory,
+  private static BuiltinFunction newRuleFunction(final RuleFactory ruleFactory,
       final Builder builder, final String ruleClassName) {
-    return new AbstractFunction(ruleClassName) {
-      @Override
-      public Object call(List<Object> args, Map<String, Object> kwargs, FuncallExpression ast,
-          com.google.devtools.build.lib.syntax.Environment env)
-          throws EvalException {
-        if (!args.isEmpty()) {
-          throw new EvalException(ast.getLocation(),
-              "build rules do not accept positional parameters");
-        }
-
+    return new BuiltinFunction(ruleClassName,
+        FunctionSignature.KWARGS, BuiltinFunction.USE_AST) {
+      public Object invoke(Map<String, Object> kwargs,
+          FuncallExpression ast) throws EvalException {
         try {
           RuleClass ruleClass = ruleFactory.getRuleClass(ruleClassName);
           builder.createAndAddRepositoryRule(ruleClass, kwargs, ast);
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 dce075ad4a..fb9e2fdabb 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
@@ -246,11 +246,11 @@ public class BuiltinFunction extends BaseFunction {
           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);
+              enforcedType, enforcedType.getType(), parameterType);
           if (enforcedType instanceof SkylarkType.Simple
               || enforcedType instanceof SkylarkFunctionType) {
             Preconditions.checkArgument(
-                enforcedType == SkylarkType.of(parameterType), msg);
+                enforcedType.getType() == parameterType, msg);
             // No need to enforce Simple types on the Skylark side, the JVM will do it for us.
             enforcedArgumentTypes.set(i, null);
           } else if (enforcedType instanceof SkylarkType.Combination) {