bazel syntax: delete BuiltinFunction.Factory

The mechanism existed so that functions like glob could
be closures over the PackageContext, which is now accessible
as a thread-local value.

Also:
- replace FuncallExpression with Location in various calls
  All of these are for EvalException and could thus be deleted.
- document element types for StarlarkList<?> parameters
PiperOrigin-RevId: 276515374

src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeModule.java

@@ -19,17 +19,28 @@
 import com.google.devtools.build.lib.packages.Type.ConversionException;
 import com.google.devtools.build.lib.skylarkbuildapi.SkylarkNativeModuleApi;
 import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.syntax.FuncallExpression;
 import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkDict;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkUtils;
 import com.google.devtools.build.lib.syntax.StarlarkThread;
 
-/**
- * A class for the Skylark native module. TODO(laurentlb): Some definitions are duplicated from
- * PackageFactory.
- */
+/** The Skylark native module. */
+// TODO(laurentlb): Some definitions are duplicated from PackageFactory.
+// This class defines:
+// native.existing_rule
+// native.existing_rules
+// native.exports_files -- also global
+// native.glob          -- also global
+// native.package_group -- also global
+// native.package_name
+// native.repository_name
+//
+// PackageFactory also defines:
+// distribs            -- hidden?
+// licenses            -- hidden?
+// package             -- global
+// environment_group   -- hidden?
 public class SkylarkNativeModule implements SkylarkNativeModuleApi {
 
   @Override
@@ -38,23 +49,23 @@ public SkylarkList<?> glob(
       SkylarkList<?> exclude,
       Integer excludeDirectories,
       Object allowEmpty,
-      FuncallExpression ast,
+      Location loc,
       StarlarkThread thread)
       throws EvalException, ConversionException, InterruptedException {
-    SkylarkUtils.checkLoadingPhase(thread, "native.glob", ast.getLocation());
+    SkylarkUtils.checkLoadingPhase(thread, "native.glob", loc);
     try {
       return PackageFactory.callGlob(
-          null, include, exclude, excludeDirectories != 0, allowEmpty, ast, thread);
+          null, include, exclude, excludeDirectories != 0, allowEmpty, loc, thread);
     } catch (IllegalArgumentException e) {
-      throw new EvalException(ast.getLocation(), "illegal argument in call to glob", e);
+      throw new EvalException(loc, "illegal argument in call to glob", e);
     }
   }
 
   @Override
-  public Object existingRule(String name, FuncallExpression ast, StarlarkThread thread)
+  public Object existingRule(String name, Location loc, StarlarkThread thread)
       throws EvalException, InterruptedException {
-    SkylarkUtils.checkLoadingOrWorkspacePhase(thread, "native.existing_rule", ast.getLocation());
-    return PackageFactory.callExistingRule(name, ast, thread);
+    SkylarkUtils.checkLoadingOrWorkspacePhase(thread, "native.existing_rule", loc);
+    return PackageFactory.callExistingRule(name, loc, thread);
   }
 
   /*
@@ -63,40 +74,36 @@ If necessary, we could allow filtering by tag (anytag, alltags), name (regexp?),
   */
   @Override
   public SkylarkDict<String, SkylarkDict<String, Object>> existingRules(
-      FuncallExpression ast, StarlarkThread thread) throws EvalException, InterruptedException {
-    SkylarkUtils.checkLoadingOrWorkspacePhase(thread, "native.existing_rules", ast.getLocation());
-    return PackageFactory.callExistingRules(ast, thread);
+      Location loc, StarlarkThread thread) throws EvalException, InterruptedException {
+    SkylarkUtils.checkLoadingOrWorkspacePhase(thread, "native.existing_rules", loc);
+    return PackageFactory.callExistingRules(loc, thread);
   }
 
   @Override
   public Runtime.NoneType packageGroup(
       String name,
       SkylarkList<?> packages,
       SkylarkList<?> includes,
-      FuncallExpression ast,
+      Location loc,
       StarlarkThread thread)
       throws EvalException {
-    SkylarkUtils.checkLoadingPhase(thread, "native.package_group", ast.getLocation());
-    return PackageFactory.callPackageFunction(name, packages, includes, ast, thread);
+    SkylarkUtils.checkLoadingPhase(thread, "native.package_group", loc);
+    return PackageFactory.callPackageFunction(name, packages, includes, loc, thread);
   }
 
   @Override
   public Runtime.NoneType exportsFiles(
-      SkylarkList<?> srcs,
-      Object visibility,
-      Object licenses,
-      FuncallExpression ast,
-      StarlarkThread thread)
+      SkylarkList<?> srcs, Object visibility, Object licenses, Location loc, StarlarkThread thread)
       throws EvalException {
-    SkylarkUtils.checkLoadingPhase(thread, "native.exports_files", ast.getLocation());
-    return PackageFactory.callExportsFiles(srcs, visibility, licenses, ast, thread);
+    SkylarkUtils.checkLoadingPhase(thread, "native.exports_files", loc);
+    return PackageFactory.callExportsFiles(srcs, visibility, licenses, loc, thread);
   }
 
   @Override
-  public String packageName(FuncallExpression ast, StarlarkThread thread) throws EvalException {
-    SkylarkUtils.checkLoadingPhase(thread, "native.package_name", ast.getLocation());
+  public String packageName(Location loc, StarlarkThread thread) throws EvalException {
+    SkylarkUtils.checkLoadingPhase(thread, "native.package_name", loc);
     PackageIdentifier packageId =
-        PackageFactory.getContext(thread, ast.getLocation()).getBuilder().getPackageIdentifier();
+        PackageFactory.getContext(thread, loc).getBuilder().getPackageIdentifier();
     return packageId.getPackageFragment().getPathString();
   }
 

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkNativeModuleApi.java

@@ -20,7 +20,6 @@
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
 import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.syntax.FuncallExpression;
 import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkDict;
 import com.google.devtools.build.lib.syntax.SkylarkList;
@@ -86,14 +85,14 @@ public interface SkylarkNativeModuleApi {
                     + " result must be non-empty (after the matches of the `exclude` patterns are"
                     + " excluded).")
       },
-      useAst = true,
+      useLocation = true,
       useStarlarkThread = true)
   public SkylarkList<?> glob(
       SkylarkList<?> include,
       SkylarkList<?> exclude,
       Integer excludeDirectories,
       Object allowEmpty,
-      FuncallExpression ast,
+      Location loc,
       StarlarkThread thread)
       throws EvalException, InterruptedException;
 
@@ -113,9 +112,9 @@ public SkylarkList<?> glob(
             legacyNamed = true,
             doc = "The name of the target.")
       },
-      useAst = true,
+      useLocation = true,
       useStarlarkThread = true)
-  public Object existingRule(String name, FuncallExpression ast, StarlarkThread thread)
+  public Object existingRule(String name, Location loc, StarlarkThread thread)
       throws EvalException, InterruptedException;
 
   @SkylarkCallable(
@@ -127,10 +126,10 @@ public Object existingRule(String name, FuncallExpression ast, StarlarkThread th
               + ""
               + "<p><i>Note: If possible, avoid using this function. It makes BUILD files brittle "
               + "and order-dependent.</i>",
-      useAst = true,
+      useLocation = true,
       useStarlarkThread = true)
   public SkylarkDict<String, SkylarkDict<String, Object>> existingRules(
-      FuncallExpression ast, StarlarkThread thread) throws EvalException, InterruptedException;
+      Location loc, StarlarkThread thread) throws EvalException, InterruptedException;
 
   @SkylarkCallable(
       name = "package_group",
@@ -161,13 +160,13 @@ public SkylarkDict<String, SkylarkDict<String, Object>> existingRules(
             positional = false,
             doc = "Other package groups that are included in this one.")
       },
-      useAst = true,
+      useLocation = true,
       useStarlarkThread = true)
   public Runtime.NoneType packageGroup(
       String name,
       SkylarkList<?> packages,
       SkylarkList<?> includes,
-      FuncallExpression ast,
+      Location loc,
       StarlarkThread thread)
       throws EvalException;
 
@@ -203,14 +202,10 @@ public Runtime.NoneType packageGroup(
             defaultValue = "None",
             doc = "Licenses to be specified.")
       },
-      useAst = true,
+      useLocation = true,
       useStarlarkThread = true)
   public Runtime.NoneType exportsFiles(
-      SkylarkList<?> srcs,
-      Object visibility,
-      Object licenses,
-      FuncallExpression ast,
-      StarlarkThread thread)
+      SkylarkList<?> srcs, Object visibility, Object licenses, Location loc, StarlarkThread thread)
       throws EvalException;
 
   @SkylarkCallable(
@@ -223,9 +218,9 @@ public Runtime.NoneType exportsFiles(
               + "<code>package_name()</code> will match the caller BUILD file package. "
               + "This function is equivalent to the deprecated variable <code>PACKAGE_NAME</code>.",
       parameters = {},
-      useAst = true,
+      useLocation = true,
       useStarlarkThread = true)
-  public String packageName(FuncallExpression ast, StarlarkThread thread) throws EvalException;
+  public String packageName(Location loc, StarlarkThread thread) throws EvalException;
 
   @SkylarkCallable(
       name = "repository_name",

src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkSignature.java

@@ -21,10 +21,10 @@
 /**
  * An annotation to mark built-in keyword argument methods accessible from Skylark.
  *
- * <p>Use this annotation around a {@link com.google.devtools.build.lib.syntax.BuiltinFunction} or a
- * {@link com.google.devtools.build.lib.syntax.BuiltinFunction.Factory}. The annotated function
- * should expect the arguments described by {@link #parameters()}, {@link #extraPositionals()}, and
- * {@link #extraKeywords()}. It should also expect the following extraneous arguments:
+ * <p>Use this annotation around a {@link com.google.devtools.build.lib.syntax.BuiltinFunction}. The
+ * annotated function should expect the arguments described by {@link #parameters()}, {@link
+ * #extraPositionals()}, and {@link #extraKeywords()}. It should also expect the following
+ * extraneous arguments:
  *
  * <ul>
  *   <li>{@link com.google.devtools.build.lib.events.Location} if {@link #useLocation()} is true.

src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java

@@ -55,7 +55,7 @@ public abstract class BaseFunction implements StarlarkCallable {
   // TODO(adonovan): this class has too many fields and relies too heavily on side effects and the
   // class hierarchy (the configure methods are the worse offenders). Turn fields into abstract
   // methods. Make processArguments a static function with multiple parameters, instead of a
-  // "mix-in" that accesses instance fields. And get rid of BuiltinFunction.Factory.
+  // "mix-in" that accesses instance fields.
 
   /**
    * The name of the function.
@@ -92,12 +92,9 @@ public abstract class BaseFunction implements StarlarkCallable {
   // Some functions are also Namespaces or other Skylark entities.
   @Nullable protected Class<?> objectType;
 
-  // Documentation for variables, if any
-  @Nullable protected List<String> paramDoc;
-
   // The types actually enforced by the Skylark runtime, as opposed to those enforced by the JVM,
   // or those displayed to the user in the documentation.
-  @Nullable protected List<SkylarkType> enforcedArgumentTypes;
+  @Nullable List<SkylarkType> enforcedArgumentTypes;
 
   /**
    * Returns the name of this function.
@@ -179,11 +176,6 @@ protected BaseFunction(@Nullable String name, FunctionSignature signature) {
     this(name, signature, /*defaultValues=*/ null, /*location=*/ null);
   }
 
-  /** Get parameter documentation as a list corresponding to each parameter */
-  List<String> getParamDoc() {
-    return paramDoc;
-  }
-
   /**
    * The size of the array required by the callee.
    */
@@ -496,12 +488,10 @@ private String printType(int i) {
   public void configure(SkylarkSignature annotation) {
     Preconditions.checkState(!isConfigured()); // must not be configured yet
 
-    this.paramDoc = new ArrayList<>();
-
     // side effect: appends to getEnforcedArgumentTypes()
     SkylarkSignatureProcessor.SignatureInfo info =
         SkylarkSignatureProcessor.getSignatureForCallable(
-            getName(), annotation, paramDoc, getEnforcedArgumentTypes());
+            getName(), annotation, /*paramDoc=*/ new ArrayList<>(), getEnforcedArgumentTypes());
     this.signature = info.signature;
     this.paramTypes = info.types;
     this.defaultValues = info.defaultValues;

src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java

@@ -86,21 +86,11 @@ protected BuiltinFunction(String name, FunctionSignature signature, ExtraArgKind
     configure();
   }
 
-  /** Creates a BuiltinFunction from the given name and a Factory */
-  protected BuiltinFunction(String name, Factory factory) {
-    super(name);
-    configure(factory);
-  }
-
   @Override
   protected int getArgArraySize() {
     return innerArgumentCount;
   }
 
-  ExtraArgKind[] getExtraArgs() {
-    return extraArgs;
-  }
-
   @Override
   @Nullable
   public Object call(Object[] args, @Nullable FuncallExpression ast, StarlarkThread thread)
@@ -198,7 +188,7 @@ private IllegalStateException badCallException(Location loc, Throwable e, Object
             stacktraceToString(e.getStackTrace()),
             this,
             Arrays.asList(invokeMethod.getParameterTypes()),
-            enforcedArgumentTypes),
+            getEnforcedArgumentTypes()),
         e);
   }
 
@@ -244,7 +234,9 @@ protected void configure() {
                   parameterType);
           if (enforcedType instanceof SkylarkType.Simple
               || enforcedType instanceof SkylarkFunctionType) {
-            Preconditions.checkArgument(enforcedType.getType() == parameterType, msg);
+            Preconditions.checkArgument(
+                parameterType.isAssignableFrom(enforcedType.getType()), 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) {
@@ -271,24 +263,6 @@ protected void configure() {
     }
   }
 
-  /** Configure by copying another function's configuration */
-  // Alternatively, we could have an extension BuiltinFunctionSignature of FunctionSignature,
-  // and use *that* instead of a Factory.
-  private void configure(BuiltinFunction.Factory factory) {
-    // this function must not be configured yet, but the factory must be
-    Preconditions.checkState(!isConfigured());
-    Preconditions.checkState(
-        factory.isConfigured(), "function factory is not configured for %s", getName());
-
-    this.paramDoc = factory.getParamDoc();
-    this.signature = factory.getSignature();
-    this.defaultValues = factory.getDefaultValues();
-    this.paramTypes = factory.getEnforcedArgumentTypes();
-    this.extraArgs = factory.getExtraArgs();
-    this.objectType = factory.getObjectType();
-    configure();
-  }
-
   /** Returns list, or null if all its elements are null. */
   @Nullable
   private static <E> List<E> valueListOrNull(List<E> list) {
@@ -322,50 +296,6 @@ Method findMethod(final String name) {
     return found;
   }
 
-  /**
-   * A Factory allows for a @SkylarkSignature annotation to be provided and processed in advance
-   * for a function that will be defined later as a closure (see e.g. in PackageFactory).
-   *
-   * <p>Each instance of this class must define a method create that closes over some (final)
-   * variables and returns a BuiltinFunction.
-   */
-  public abstract static class Factory extends BuiltinFunction {
-    @Nullable private Method createMethod;
-
-    /** Create unconfigured function Factory from its name */
-    public Factory(String name) {
-      super(name);
-    }
-
-    @Override
-    public void configure() {
-      if (createMethod != null) {
-        return;
-      }
-      createMethod = findMethod("create");
-    }
-
-    @Override
-    public Object call(Object[] args, @Nullable FuncallExpression ast, StarlarkThread thread)
-        throws EvalException {
-      throw new EvalException(null, "tried to invoke a Factory for function " + this);
-    }
-
-    /** Instantiate the Factory
-     * @param args arguments to pass to the create method
-     * @return a new BuiltinFunction that closes over the arguments
-     */
-    public BuiltinFunction apply(Object... args) {
-      try {
-        return (BuiltinFunction) createMethod.invoke(this, args);
-      } catch (InvocationTargetException | IllegalArgumentException | IllegalAccessException e) {
-        throw new RuntimeException(String.format(
-            "Exception while applying BuiltinFunction.Factory %s: %s",
-            this, e.getMessage()), e);
-      }
-    }
-  }
-
   @Override
   public void repr(SkylarkPrinter printer) {
     printer.append("<built-in function " + getName() + ">");