Introduce extraPositonals and extraArguments to SkylarkCallable, to have parity with @SkylarkSignature.

This is necessary for several builtin functions that still use @SkylarkSignature, such as string format. These will be migrated in a future CL.

RELNOTES: None.
PiperOrigin-RevId: 192200282

9 files changed, 174 insertions, 57 deletions

diff --git a/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkBuiltinMethodDoc.java b/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkBuiltinMethodDoc.java
index bcb52923f9..4a2c5f8bad 100644
--- a/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkBuiltinMethodDoc.java
+++ b/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkBuiltinMethodDoc.java
@@ -13,10 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.docgen.skylark;
 
-import com.google.devtools.build.lib.skylarkinterface.Param;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
 import com.google.devtools.build.lib.syntax.BaseFunction;
-import java.util.ArrayList;
 import java.util.List;
 
 /**
@@ -33,8 +31,12 @@ public final class SkylarkBuiltinMethodDoc extends SkylarkMethodDoc {
     this.module = module;
     this.annotation = annotation;
     this.fieldClass = fieldClass;
-    this.params = new ArrayList<>();
-    processParams();
+    this.params =
+        SkylarkDocUtils.determineParams(
+            this,
+            annotation.parameters(),
+            annotation.extraPositionals(),
+            annotation.extraKeywords());
   }
 
   public SkylarkSignature getAnnotation() {
@@ -78,20 +80,4 @@ public final class SkylarkBuiltinMethodDoc extends SkylarkMethodDoc {
   public List<SkylarkParamDoc> getParams() {
     return params;
   }
-
-  private void processParams() {
-    processParams(adjustedParameters(annotation));
-    if (!annotation.extraPositionals().name().isEmpty()) {
-      this.params.add(new SkylarkParamDoc(this, annotation.extraPositionals()));
-    }
-    if (!annotation.extraKeywords().name().isEmpty()) {
-      this.params.add(new SkylarkParamDoc(this, annotation.extraKeywords()));
-    }
-  }
-
-  private void processParams(Param[] params) {
-    for (Param param : params) {
-      this.params.add(new SkylarkParamDoc(this, param));
-    }
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkDocUtils.java b/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkDocUtils.java
index ac73f65596..18300683af 100644
--- a/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkDocUtils.java
+++ b/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkDocUtils.java
@@ -14,7 +14,9 @@
 
 package com.google.devtools.build.docgen.skylark;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.docgen.DocgenConsts;
+import com.google.devtools.build.lib.skylarkinterface.Param;
 
 /** A utility class for the documentation generator. */
 public final class SkylarkDocUtils {
@@ -30,4 +32,26 @@ public final class SkylarkDocUtils {
         .replace("$BE_ROOT", DocgenConsts.BeDocsRoot)
         .replace("$DOC_EXT", DocgenConsts.documentationExtension);
   }
+
+  /**
+   * Returns a list of parameter documentation elements for a given method doc and the method's
+   * parameters.
+   */
+  static ImmutableList<SkylarkParamDoc> determineParams(
+      SkylarkMethodDoc methodDoc,
+      Param[] userSuppliedParams,
+      Param extraPositionals,
+      Param extraKeywords) {
+    ImmutableList.Builder<SkylarkParamDoc> paramsBuilder = ImmutableList.builder();
+    for (Param param : userSuppliedParams) {
+      paramsBuilder.add(new SkylarkParamDoc(methodDoc, param));
+    }
+    if (!extraPositionals.name().isEmpty()) {
+      paramsBuilder.add(new SkylarkParamDoc(methodDoc, extraPositionals));
+    }
+    if (!extraKeywords.name().isEmpty()) {
+      paramsBuilder.add(new SkylarkParamDoc(methodDoc, extraKeywords));
+    }
+    return paramsBuilder.build();
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkJavaMethodDoc.java b/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkJavaMethodDoc.java
index fe92bdfe90..60ca32e093 100644
--- a/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkJavaMethodDoc.java
+++ b/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkJavaMethodDoc.java
@@ -42,11 +42,9 @@ public final class SkylarkJavaMethodDoc extends SkylarkMethodDoc {
         : callable.name();
     this.method = method;
     this.callable = callable;
-    ImmutableList.Builder<SkylarkParamDoc> paramsBuilder = ImmutableList.builder();
-    for (Param param : callable.parameters()) {
-      paramsBuilder.add(new SkylarkParamDoc(this, param));
-    }
-    this.params = paramsBuilder.build();
+    this.params =
+        SkylarkDocUtils.determineParams(
+            this, callable.parameters(), callable.extraPositionals(), callable.extraKeywords());
   }
 
   public Method getMethod() {
diff --git a/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkMethodDoc.java b/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkMethodDoc.java
index 116fd4a528..b617803612 100644
--- a/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkMethodDoc.java
+++ b/src/main/java/com/google/devtools/build/docgen/skylark/SkylarkMethodDoc.java
@@ -72,6 +72,12 @@ public abstract class SkylarkMethodDoc extends SkylarkDoc {
       }
       argList.add(formatParameter(param));
     }
+    if (!annotation.extraPositionals().name().isEmpty()) {
+      argList.add("*" + annotation.extraPositionals().name());
+    }
+    if (!annotation.extraKeywords().name().isEmpty()) {
+      argList.add("**" + annotation.extraKeywords().name());
+    }
     return Joiner.on(", ").join(argList);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/Param.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/Param.java
index 8abb3448b9..8f10c72850 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/Param.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/Param.java
@@ -86,8 +86,14 @@ public @interface Param {
 
   /**
    * If true, the parameter may be specified as a named parameter. For example for an integer named
-   * parameter {@code foo} of a method {@code bar}, then the method call will look like
-   * {@code bar(foo=1)}.
+   * parameter {@code foo} of a method {@code bar}, then the method call will look like {@code
+   * bar(foo=1)}.
+   *
+   * <p>If false, then {@link #positional} must be true (otherwise there is no way to reference the
+   * parameter via an argument).
+   *
+   * <p>If this parameter represents the 'extra positionals' (args) or 'extra keywords' (kwargs)
+   * element of a method, this field has no effect.
    */
   boolean named() default false;
 
@@ -97,7 +103,13 @@ public @interface Param {
    * {@code bar(1)}. If {@link #named()} is {@code false}, then this will be the only way to call
    * {@code bar}.
    *
+   * <p>If false, then {@link #named} must be true (otherwise there is no way to reference the
+   * parameter via an argument)
+   *
    * <p>Positional arguments should come first.
+   *
+   * <p>If this parameter represents the 'extra positionals' (args) or 'extra keywords' (kwargs)
+   * element of a method, this field has no effect.
    */
   boolean positional() default true;
 
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java
index aed52245f9..92ef0a3a87 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java
@@ -27,12 +27,13 @@ import java.lang.annotation.Target;
  * <ul>
  *   <li>The method must be public.
  *   <li>If structField=true, there must be zero user-supplied parameters.
- *   <li>Method parameters must be supplied in the following order:
- *       <pre>method([positionals][other user-args]
+ *   <li>The underlying java method's parameters must be supplied in the following order:
+ *       <pre>method([positionals]*[named args]*(extra positionals list)(extra kwargs)
  *       (Location)(FuncallExpression)(Envrionment)(SkylarkSemantics))</pre>
- *       where Location, FuncallExpression, Environment, and SkylarkSemantics are supplied by the
- *       interpreter if and only if useLocation, useAst, useEnvironment, and useSkylarkSemantics are
- *       specified, respectively.
+ *       where (extra positionals list) is a SkylarkList if extraPositionals is defined, (extra
+ *       kwargs) is a SkylarkDict if extraKeywords is defined, and Location, FuncallExpression,
+ *       Environment, and SkylarkSemantics are supplied by the interpreter if and only if
+ *       useLocation, useAst, useEnvironment, and useSkylarkSemantics are specified, respectively.
  *   <li>The number of method parameters much match the number of annotation-declared parameters
  *       plus the number of interpreter-supplied parameters.
  * </ul>
@@ -82,6 +83,34 @@ public @interface SkylarkCallable {
   Param[] parameters() default {};
 
   /**
+   * Defines a catch-all list for additional unspecified positional parameters.
+   *
+   * <p>If this is left as default, it is an error for the caller to pass more positional arguments
+   * than are explicitly allowed by the method signature. If this is defined, all additional
+   * positional arguments are passed as elements of a {@link SkylarkList} to the method.
+   *
+   * <p>See python's <code>*args</code> (http://thepythonguru.com/python-args-and-kwargs/).
+   *
+   * <p>(If this is defined, the annotated method signature must contain a corresponding SkylarkList
+   * parameter. See the interface-level javadoc for details.)
+   */
+  Param extraPositionals() default @Param(name = "");
+
+  /**
+   * Defines a catch-all dictionary for additional unspecified named parameters.
+   *
+   * <p>If this is left as default, it is an error for the caller to pass any named arguments not
+   * explicitly declared by the method signature. If this is defined, all additional named arguments
+   * are passed as elements of a {@link SkylarkDict} to the method.
+   *
+   * <p>See python's <code>**kwargs</code> (http://thepythonguru.com/python-args-and-kwargs/).
+   *
+   * <p>(If this is defined, the annotated method signature must contain a corresponding SkylarkDict
+   * parameter. See the interface-level javadoc for details.)
+   */
+  Param extraKeywords() default @Param(name = "");
+
+  /**
    * Set it to true if the Java method may return <code>null</code> (which will then be converted to
    * <code>None</code>). If not set and the Java method returns null, an error will be raised.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java
index 8057b89f0d..a39acba89a 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java
@@ -68,6 +68,9 @@ import javax.tools.Diagnostic;
 public final class SkylarkCallableProcessor extends AbstractProcessor {
   private Messager messager;
 
+  private static final String SKYLARK_LIST = "com.google.devtools.build.lib.syntax.SkylarkList<?>";
+  private static final String SKYLARK_DICT =
+      "com.google.devtools.build.lib.syntax.SkylarkDict<?,?>";
   private static final String LOCATION = "com.google.devtools.build.lib.events.Location";
   private static final String AST = "com.google.devtools.build.lib.syntax.FuncallExpression";
   private static final String ENVIRONMENT = "com.google.devtools.build.lib.syntax.Environment";
@@ -100,7 +103,7 @@ public final class SkylarkCallableProcessor extends AbstractProcessor {
 
       try {
         verifyDocumented(methodElement, annotation);
-        verifyNotStructFieldWithInvalidExtraParams(methodElement, annotation);
+        verifyNotStructFieldWithParams(methodElement, annotation);
         verifyParamSemantics(methodElement, annotation);
         verifyNumberOfParameters(methodElement, annotation);
         verifyExtraInterpreterParams(methodElement, annotation);
@@ -121,15 +124,19 @@ public final class SkylarkCallableProcessor extends AbstractProcessor {
     }
   }
 
-  private void verifyNotStructFieldWithInvalidExtraParams(
+  private void verifyNotStructFieldWithParams(
       ExecutableElement methodElement, SkylarkCallable annotation)
       throws SkylarkCallableProcessorException {
     if (annotation.structField()) {
-      if (annotation.useAst() || annotation.useEnvironment() || annotation.useAst()) {
+      if (annotation.useAst()
+          || annotation.useEnvironment()
+          || annotation.useAst()
+          || !annotation.extraPositionals().name().isEmpty()
+          || !annotation.extraKeywords().name().isEmpty()) {
         throw new SkylarkCallableProcessorException(
             methodElement,
             "@SkylarkCallable-annotated methods with structField=true may not also specify "
-                + "useAst, useEnvironment, or useLocation");
+                + "useAst, useEnvironment, useLocation, extraPositionals, or extraKeywords");
       }
     }
   }
@@ -255,6 +262,32 @@ public final class SkylarkCallableProcessor extends AbstractProcessor {
     // TODO(cparsons): Matching by class name alone is somewhat brittle, but due to tangled
     // dependencies, it is difficult for this processor to depend directy on the expected
     // classes here.
+    if (!annotation.extraPositionals().name().isEmpty()) {
+      if (!SKYLARK_LIST.equals(methodSignatureParams.get(currentIndex).asType().toString())) {
+        throw new SkylarkCallableProcessorException(
+            methodElement,
+            String.format(
+                "Expected parameter index %d to be the %s type, matching extraPositionals, "
+                    + "but was %s",
+                currentIndex,
+                SKYLARK_LIST,
+                methodSignatureParams.get(currentIndex).asType().toString()));
+      }
+      currentIndex++;
+    }
+    if (!annotation.extraKeywords().name().isEmpty()) {
+      if (!SKYLARK_DICT.equals(methodSignatureParams.get(currentIndex).asType().toString())) {
+        throw new SkylarkCallableProcessorException(
+            methodElement,
+            String.format(
+                "Expected parameter index %d to be the %s type, matching extraKeywords, "
+                    + "but was %s",
+                currentIndex,
+                SKYLARK_DICT,
+                methodSignatureParams.get(currentIndex).asType().toString()));
+      }
+      currentIndex++;
+    }
     if (annotation.useLocation()) {
       if (!LOCATION.equals(methodSignatureParams.get(currentIndex).asType().toString())) {
         throw new SkylarkCallableProcessorException(
@@ -306,6 +339,8 @@ public final class SkylarkCallableProcessor extends AbstractProcessor {
 
   private int numExpectedExtraInterpreterParams(SkylarkCallable annotation) {
     int numExtraInterpreterParams = 0;
+    numExtraInterpreterParams += annotation.extraPositionals().name().isEmpty() ? 0 : 1;
+    numExtraInterpreterParams += annotation.extraKeywords().name().isEmpty() ? 0 : 1;
     numExtraInterpreterParams += annotation.useLocation() ? 1 : 0;
     numExtraInterpreterParams += annotation.useAst() ? 1 : 0;
     numExtraInterpreterParams += annotation.useEnvironment() ? 1 : 0;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index ef8db702ad..c9f3aee01d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.syntax.EvalException.EvalExceptionWithJavaCause;
 import com.google.devtools.build.lib.syntax.Runtime.NoneType;
+import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.StringUtilities;
 import java.io.IOException;
@@ -492,10 +493,16 @@ public final class FuncallExpression extends Expression {
       Map<String, Object> kwargs,
       MethodDescriptor method,
       Environment environment) {
+    SkylarkCallable callable = method.getAnnotation();
     ImmutableList.Builder<Object> builder = ImmutableList.builder();
+    ImmutableList.Builder<Object> extraArgsBuilder = ImmutableList.builder();
+    ImmutableMap.Builder<String, Object> extraKwargsBuilder = ImmutableMap.builder();
+    boolean acceptsExtraArgs = !callable.extraPositionals().name().isEmpty();
+    boolean acceptsExtraKwargs = !callable.extraKeywords().name().isEmpty();
     Class<?>[] javaMethodSignatureParams = method.getMethod().getParameterTypes();
-    SkylarkCallable callable = method.getAnnotation();
     int numExtraInterpreterParams = 0;
+    numExtraInterpreterParams += acceptsExtraArgs ? 1 : 0;
+    numExtraInterpreterParams += acceptsExtraKwargs ? 1 : 0;
     numExtraInterpreterParams += callable.useLocation() ? 1 : 0;
     numExtraInterpreterParams += callable.useAst() ? 1 : 0;
     numExtraInterpreterParams += callable.useEnvironment() ? 1 : 0;
@@ -535,7 +542,6 @@ public final class FuncallExpression extends Expression {
 
     // Then process parameters specified in callable.parameters()
     Set<String> keys = new LinkedHashSet<>(kwargs.keySet());
-    int paramIndex = mandatoryPositionals;
     // Positional parameters are always enumerated before non-positional parameters,
     // And default-valued positional parameters are always enumerated after other positional
     // parameters. These invariants are validated by the SkylarkCallable annotation processor.
@@ -543,13 +549,8 @@ public final class FuncallExpression extends Expression {
       SkylarkType type = getType(param);
       Object value = null;
 
-      if (argIndex < args.size()) { // Positional arguments remain.
+      if (argIndex < args.size() && param.positional()) { // Positional args and params remain.
         value = args.get(argIndex);
-        if (!param.positional()) {
-          return ArgumentListConversionResult.fromError(
-              String.format("expected no more than %s positional arguments, but got %s",
-                  paramIndex, args.size()));
-        }
         if (!type.contains(value)) {
           return ArgumentListConversionResult.fromError(
               String.format(
@@ -561,7 +562,7 @@ public final class FuncallExpression extends Expression {
               String.format("got multiple values for keyword argument '%s'", param.name()));
         }
         argIndex++;
-      } else { // No more positional arguments.
+      } else { // No more positional arguments, or no more positional parameters.
         if (param.named() && keys.remove(param.name())) { // Param specified by keyword argument.
           value = kwargs.get(param.name());
           if (!type.contains(value)) {
@@ -583,22 +584,41 @@ public final class FuncallExpression extends Expression {
             String.format("parameter '%s' cannot be None", param.name()));
       }
       builder.add(value);
-      paramIndex++;
     }
 
     if (argIndex < args.size()) {
-      return ArgumentListConversionResult.fromError(
-          String.format("expected no more than %s positional arguments, but got %s",
-              paramIndex, args.size()));
+      if (acceptsExtraArgs) {
+        for (; argIndex < args.size(); argIndex++) {
+          extraArgsBuilder.add(args.get(argIndex));
+        }
+      } else {
+        return ArgumentListConversionResult.fromError(
+            String.format(
+                "expected no more than %s positional arguments, but got %s",
+                argIndex, args.size()));
+      }
     }
     if (!keys.isEmpty()) {
-      return ArgumentListConversionResult.fromError(
-          String.format("unexpected keyword%s %s",
-              keys.size() > 1 ? "s" : "",
-              Joiner.on(",").join(Iterables.transform(keys, s -> "'" + s + "'"))));
+      if (acceptsExtraKwargs) {
+        for (String key : keys) {
+          extraKwargsBuilder.put(key, kwargs.get(key));
+        }
+      } else {
+        return ArgumentListConversionResult.fromError(
+            String.format(
+                "unexpected keyword%s %s",
+                keys.size() > 1 ? "s" : "",
+                Joiner.on(",").join(Iterables.transform(keys, s -> "'" + s + "'"))));
+      }
     }
 
-    // Then add any skylark-info arguments (for example the Environment).
+    // Then add any skylark-interpreter arguments (for example kwargs or the Environment).
+    if (acceptsExtraArgs) {
+      builder.add(Tuple.copyOf(extraArgsBuilder.build()));
+    }
+    if (acceptsExtraKwargs) {
+      builder.add(SkylarkDict.copyOf(environment, extraKwargsBuilder.build()));
+    }
     if (callable.useLocation()) {
       builder.add(getLocation());
     }
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 a2dc39a377..cbc653a087 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
@@ -68,9 +68,16 @@ public class SkylarkSignatureProcessor {
       parameters.add(parameter);
     }
 
-    return getSignatureForCallable(name, documented, parameters.build(), annotation.parameters(),
-        /*extraPositionals=*/null,
-        /*extraKeywords=*/null, /*defaultValues=*/null, paramDoc, enforcedTypesList);
+    return getSignatureForCallable(
+        name,
+        documented,
+        parameters.build(),
+        annotation.parameters(),
+        annotation.extraPositionals(),
+        annotation.extraKeywords(),
+        /*defaultValues=*/ null,
+        paramDoc,
+        enforcedTypesList);
   }