Cleanup @SkylarkCallable parameters and their error handling.

This involves enforcing additional compiletime restrictions on Param ordering and semantics.

RELNOTES: None.
PiperOrigin-RevId: 191927206

4 files changed, 187 insertions, 119 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java
index 2c3e25acbc..4afe8c29d6 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java
@@ -821,25 +821,6 @@ public final class SkylarkAttr implements SkylarkValue {
             + "<a href=\"string.html\">strings</a>.",
     parameters = {
       @Param(
-        name = DEFAULT_ARG,
-        allowedTypes = {
-          @ParamType(type = SkylarkList.class, generic1 = String.class),
-          @ParamType(type = UserDefinedFunction.class)
-        },
-        defaultValue = "[]",
-        doc = DEFAULT_DOC,
-        named = true,
-        positional = false
-      ),
-      @Param(
-        name = DOC_ARG,
-        type = String.class,
-        defaultValue = "''",
-        doc = DOC_DOC,
-        named = true,
-        positional = false
-      ),
-      @Param(
         name = MANDATORY_ARG,
         type = Boolean.class,
         defaultValue = "False",
@@ -859,17 +840,36 @@ public final class SkylarkAttr implements SkylarkValue {
         defaultValue = "True",
         doc = ALLOW_EMPTY_DOC,
         named = true
+      ),
+      @Param(
+          name = DEFAULT_ARG,
+          allowedTypes = {
+              @ParamType(type = SkylarkList.class, generic1 = String.class),
+              @ParamType(type = UserDefinedFunction.class)
+          },
+          defaultValue = "[]",
+          doc = DEFAULT_DOC,
+          named = true,
+          positional = false
+      ),
+      @Param(
+          name = DOC_ARG,
+          type = String.class,
+          defaultValue = "''",
+          doc = DOC_DOC,
+          named = true,
+          positional = false
       )
     },
     useAst = true,
     useEnvironment = true
   )
   public Descriptor stringListAttribute(
-      SkylarkList<?> defaultList,
-      String doc,
       Boolean mandatory,
       Boolean nonEmpty,
       Boolean allowEmpty,
+      SkylarkList<?> defaultList,
+      String doc,
       FuncallExpression ast,
       Environment env)
       throws EvalException {
@@ -896,25 +896,6 @@ public final class SkylarkAttr implements SkylarkValue {
     doc = "Creates an attribute which is a <a href=\"list.html\">list</a> of ints.",
     parameters = {
       @Param(
-        name = DEFAULT_ARG,
-        allowedTypes = {
-          @ParamType(type = SkylarkList.class, generic1 = Integer.class),
-          @ParamType(type = UserDefinedFunction.class)
-        },
-        defaultValue = "[]",
-        doc = DEFAULT_DOC,
-        named = true,
-        positional = false
-      ),
-      @Param(
-        name = DOC_ARG,
-        type = String.class,
-        defaultValue = "''",
-        doc = DOC_DOC,
-        named = true,
-        positional = false
-      ),
-      @Param(
         name = MANDATORY_ARG,
         type = Boolean.class,
         defaultValue = "False",
@@ -934,17 +915,36 @@ public final class SkylarkAttr implements SkylarkValue {
         defaultValue = "True",
         doc = ALLOW_EMPTY_DOC,
         named = true
+      ),
+      @Param(
+          name = DEFAULT_ARG,
+          allowedTypes = {
+              @ParamType(type = SkylarkList.class, generic1 = Integer.class),
+              @ParamType(type = UserDefinedFunction.class)
+          },
+          defaultValue = "[]",
+          doc = DEFAULT_DOC,
+          named = true,
+          positional = false
+      ),
+      @Param(
+          name = DOC_ARG,
+          type = String.class,
+          defaultValue = "''",
+          doc = DOC_DOC,
+          named = true,
+          positional = false
       )
     },
     useAst = true,
     useEnvironment = true
   )
   public Descriptor intListAttribute(
-      SkylarkList<?> defaultList,
-      String doc,
       Boolean mandatory,
       Boolean nonEmpty,
       Boolean allowEmpty,
+      SkylarkList<?> defaultList,
+      String doc,
       FuncallExpression ast,
       Environment env)
       throws EvalException {
@@ -974,6 +974,13 @@ public final class SkylarkAttr implements SkylarkValue {
             + "See <a href=\"attr.html#label\">label</a> for more information.",
     parameters = {
       @Param(
+          name = ALLOW_EMPTY_ARG,
+          type = Boolean.class,
+          defaultValue = "True",
+          doc = ALLOW_EMPTY_DOC,
+          named = true
+      ),
+      @Param(
         name = DEFAULT_ARG,
         allowedTypes = {
           @ParamType(type = SkylarkList.class, generic1 = Label.class),
@@ -1049,13 +1056,6 @@ public final class SkylarkAttr implements SkylarkValue {
         doc = NON_EMPTY_DOC
       ),
       @Param(
-        name = ALLOW_EMPTY_ARG,
-        type = Boolean.class,
-        defaultValue = "True",
-        doc = ALLOW_EMPTY_DOC,
-        named = true
-      ),
-      @Param(
         name = CONFIGURATION_ARG,
         type = Object.class,
         noneable = true,
@@ -1078,6 +1078,7 @@ public final class SkylarkAttr implements SkylarkValue {
     useEnvironment = true
   )
   public Descriptor labelListAttribute(
+      Boolean allowEmpty,
       Object defaultList,
       String doc,
       Object allowFiles,
@@ -1086,7 +1087,6 @@ public final class SkylarkAttr implements SkylarkValue {
       SkylarkList<?> flags,
       Boolean mandatory,
       Boolean nonEmpty,
-      Boolean allowEmpty,
       Object cfg,
       SkylarkList<?> aspects,
       FuncallExpression ast,
@@ -1134,6 +1134,13 @@ public final class SkylarkAttr implements SkylarkValue {
             + "<a href=\"attr.html#label\">label</a> for more information.",
     parameters = {
       @Param(
+          name = ALLOW_EMPTY_ARG,
+          type = Boolean.class,
+          defaultValue = "True",
+          doc = ALLOW_EMPTY_DOC,
+          named = true
+      ),
+      @Param(
         name = DEFAULT_ARG,
         allowedTypes = {
           @ParamType(type = SkylarkDict.class),
@@ -1210,13 +1217,6 @@ public final class SkylarkAttr implements SkylarkValue {
         doc = NON_EMPTY_DOC
       ),
       @Param(
-        name = ALLOW_EMPTY_ARG,
-        type = Boolean.class,
-        defaultValue = "True",
-        doc = ALLOW_EMPTY_DOC,
-        named = true
-      ),
-      @Param(
         name = CONFIGURATION_ARG,
         type = Object.class,
         noneable = true,
@@ -1239,6 +1239,7 @@ public final class SkylarkAttr implements SkylarkValue {
     useEnvironment = true
   )
   public Descriptor labelKeyedStringDictAttribute(
+      Boolean allowEmpty,
       Object defaultList,
       String doc,
       Object allowFiles,
@@ -1247,7 +1248,6 @@ public final class SkylarkAttr implements SkylarkValue {
       SkylarkList<?> flags,
       Boolean mandatory,
       Boolean nonEmpty,
-      Boolean allowEmpty,
       Object cfg,
       SkylarkList<?> aspects,
       FuncallExpression ast,
@@ -1392,6 +1392,13 @@ public final class SkylarkAttr implements SkylarkValue {
             + "See <a href=\"attr.html#output\">output</a> for more information.",
     parameters = {
       @Param(
+          name = ALLOW_EMPTY_ARG,
+          type = Boolean.class,
+          defaultValue = "True",
+          doc = ALLOW_EMPTY_DOC,
+          named = true
+      ),
+      @Param(
         name = DEFAULT_ARG,
         allowedTypes = {
           @ParamType(type = SkylarkList.class, generic1 = Label.class),
@@ -1426,24 +1433,17 @@ public final class SkylarkAttr implements SkylarkValue {
         named = true,
         positional = false,
         doc = NON_EMPTY_DOC
-      ),
-      @Param(
-        name = ALLOW_EMPTY_ARG,
-        type = Boolean.class,
-        defaultValue = "True",
-        doc = ALLOW_EMPTY_DOC,
-        named = true
       )
     },
     useAst = true,
     useEnvironment = true
   )
   public Descriptor outputListAttribute(
+      Boolean allowEmpty,
       SkylarkList defaultList,
       String doc,
       Boolean mandatory,
       Boolean nonEmpty,
-      Boolean allowEmpty,
       FuncallExpression ast,
       Environment env)
       throws EvalException {
@@ -1472,6 +1472,13 @@ public final class SkylarkAttr implements SkylarkValue {
             + "<a href=\"string.html\">string</a> to <a href=\"string.html\">string</a>.",
     parameters = {
       @Param(
+          name = ALLOW_EMPTY_ARG,
+          type = Boolean.class,
+          defaultValue = "True",
+          doc = ALLOW_EMPTY_DOC,
+          named = true
+      ),
+      @Param(
         name = DEFAULT_ARG,
         allowedTypes = {
           @ParamType(type = SkylarkDict.class),
@@ -1505,24 +1512,17 @@ public final class SkylarkAttr implements SkylarkValue {
         named = true,
         positional = false,
         doc = NON_EMPTY_DOC
-      ),
-      @Param(
-        name = ALLOW_EMPTY_ARG,
-        type = Boolean.class,
-        defaultValue = "True",
-        doc = ALLOW_EMPTY_DOC,
-        named = true
       )
     },
     useAst = true,
     useEnvironment = true
   )
   public Descriptor stringDictAttribute(
+      Boolean allowEmpty,
       SkylarkDict<?, ?> defaultO,
       String doc,
       Boolean mandatory,
       Boolean nonEmpty,
-      Boolean allowEmpty,
       FuncallExpression ast,
       Environment env)
       throws EvalException {
@@ -1552,6 +1552,13 @@ public final class SkylarkAttr implements SkylarkValue {
             + "<a href=\"string.html\">string</a>.",
     parameters = {
       @Param(
+          name = ALLOW_EMPTY_ARG,
+          type = Boolean.class,
+          defaultValue = "True",
+          doc = ALLOW_EMPTY_DOC,
+          named = true
+      ),
+      @Param(
         name = DEFAULT_ARG,
         allowedTypes = {
           @ParamType(type = SkylarkDict.class),
@@ -1585,24 +1592,17 @@ public final class SkylarkAttr implements SkylarkValue {
         named = true,
         positional = false,
         doc = NON_EMPTY_DOC
-      ),
-      @Param(
-        name = ALLOW_EMPTY_ARG,
-        type = Boolean.class,
-        defaultValue = "True",
-        doc = ALLOW_EMPTY_DOC,
-        named = true
       )
     },
     useAst = true,
     useEnvironment = true
   )
   public Descriptor stringListDictAttribute(
+      Boolean allowEmpty,
       SkylarkDict<?, ?> defaultO,
       String doc,
       Boolean mandatory,
       Boolean nonEmpty,
-      Boolean allowEmpty,
       FuncallExpression ast,
       Environment env)
       throws EvalException {
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 5765205310..8abb3448b9 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
@@ -80,6 +80,7 @@ public @interface Param {
 
   /**
    * If true, this parameter can be passed the "None" value in addition to whatever types it allows.
+   * If false, this parameter cannot be passed "None", no matter the types it allows.
    */
   boolean noneable() default false;
 
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 10a42e4379..8057b89f0d 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
@@ -52,9 +52,14 @@ import javax.tools.Diagnostic;
  *   Each parameter, if explicitly typed, may only use either 'type' or 'allowedTypes',
  *   not both.
  * </li>
+ * <li>Each parameter must be positional or named (or both).</li>
+ * <li>Positional-only parameters must be specified before any named parameters.</li>
+ * <li>Positional parameters must be specified before any non-positional parameters.</li>
  * <li>
- *   Either the doc string is non-empty, or documented is false.
+ *   Positional parameters without default values must be specified before any
+ *   positional parameters with default values.
  * </li>
+ * <li>Either the doc string is non-empty, or documented is false.</li>
  * </ul>
  *
  * <p>These properties can be relied upon at runtime without additional checks.
@@ -131,7 +136,17 @@ public final class SkylarkCallableProcessor extends AbstractProcessor {
 
   private void verifyParamSemantics(ExecutableElement methodElement, SkylarkCallable annotation)
       throws SkylarkCallableProcessorException {
+    boolean allowPositionalNext = true;
+    boolean allowPositionalOnlyNext = true;
+    boolean allowNonDefaultPositionalNext = true;
+
     for (Param parameter : annotation.parameters()) {
+      if ((!parameter.positional()) && (!parameter.named())) {
+        throw new SkylarkCallableProcessorException(
+            methodElement,
+            String.format("Parameter '%s' must be either positional or named",
+                parameter.name()));
+      }
       if ("None".equals(parameter.defaultValue()) && !parameter.noneable()) {
         throw new SkylarkCallableProcessorException(
             methodElement,
@@ -148,6 +163,45 @@ public final class SkylarkCallableProcessor extends AbstractProcessor {
                 + " one may be specified.",
             parameter.name()));
       }
+
+      if (parameter.positional()) {
+        if (!allowPositionalNext) {
+          throw new SkylarkCallableProcessorException(
+              methodElement,
+              String.format(
+                  "Positional parameter '%s' is specified after one or more "
+                      + "non-positonal parameters",
+                  parameter.name()));
+        }
+        if (!parameter.named() && !allowPositionalOnlyNext) {
+          throw new SkylarkCallableProcessorException(
+              methodElement,
+              String.format(
+                  "Positional-only parameter '%s' is specified after one or more "
+                      + "named parameters",
+                  parameter.name()));
+        }
+        if (parameter.defaultValue().isEmpty()) { // There is no default value.
+          if (!allowNonDefaultPositionalNext) {
+            throw new SkylarkCallableProcessorException(
+                methodElement,
+                String.format(
+                    "Positional parameter '%s' has no default value but is specified after one "
+                        + "or more positional parameters with default values",
+                    parameter.name()));
+          }
+        } else { // There is a default value.
+          // No positional parameters without a default value can come after this parameter.
+          allowNonDefaultPositionalNext = false;
+        }
+      } else { // Not positional.
+        // No positional parameters can come after this parameter.
+        allowPositionalNext = false;
+      }
+      if (parameter.named()) {
+        // No positional-only parameters can come after this parameter.
+        allowPositionalOnlyNext = false;
+      }
     }
   }
 
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 597508f04d..ef8db702ad 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
@@ -460,9 +460,9 @@ public final class FuncallExpression extends Expression {
   }
 
   private static SkylarkType getType(Param param) {
+    SkylarkType result = SkylarkType.BOTTOM;
     if (param.allowedTypes().length > 0) {
       Preconditions.checkState(Object.class.equals(param.type()));
-      SkylarkType result = SkylarkType.BOTTOM;
       for (ParamType paramType : param.allowedTypes()) {
         SkylarkType t =
             paramType.generic1() != Object.class
@@ -470,14 +470,17 @@ public final class FuncallExpression extends Expression {
                 : SkylarkType.of(paramType.type());
         result = SkylarkType.Union.of(result, t);
       }
-      return result;
     } else {
-      SkylarkType type =
+      result =
           param.generic1() != Object.class
               ? SkylarkType.of(param.type(), param.generic1())
               : SkylarkType.of(param.type());
-      return type;
     }
+
+    if (param.noneable()) {
+      result = SkylarkType.Union.of(result, SkylarkType.NONE);
+    }
+    return result;
   }
 
   /**
@@ -509,74 +512,84 @@ public final class FuncallExpression extends Expression {
     if (mandatoryPositionals > args.size()) {
       return ArgumentListConversionResult.fromError("too few arguments");
     }
-    if (args.size() > mandatoryPositionals + callable.parameters().length) {
-      return ArgumentListConversionResult.fromError("too many arguments");
-    }
+
     // First process the legacy positional parameters.
-    int i = 0;
+    int argIndex = 0;
     if (mandatoryPositionals > 0) {
       for (Class<?> param : javaMethodSignatureParams) {
-        Object value = args.get(i);
+        Object value = args.get(argIndex);
         if (!param.isAssignableFrom(value.getClass())) {
           return ArgumentListConversionResult.fromError(
               String.format(
                   "Cannot convert parameter at position %d from type %s to type %s",
-                  i, EvalUtils.getDataTypeName(value), param.toString()));
+                  argIndex, EvalUtils.getDataTypeName(value), param.toString()));
         }
         builder.add(value);
-        i++;
-        if (i >= mandatoryPositionals) {
+        argIndex++;
+        if (argIndex >= mandatoryPositionals) {
           // Stops for specified parameters instead.
           break;
         }
       }
     }
 
-    // Then the parameters specified in callable.parameters()
+    // 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.
     for (Param param : callable.parameters()) {
       SkylarkType type = getType(param);
-      if (param.noneable()) {
-        type = SkylarkType.Union.of(type, SkylarkType.NONE);
-      }
       Object value = null;
-      if (i < args.size()) {
-        value = args.get(i);
+
+      if (argIndex < args.size()) { // Positional arguments remain.
+        value = args.get(argIndex);
         if (!param.positional()) {
           return ArgumentListConversionResult.fromError(
-              String.format("Parameter '%s' is not positional", param.name()));
-        } else if (!type.contains(value)) {
-          return ArgumentListConversionResult.fromError(
-              String.format(
-                  "expected value of type '%s' for parameter '%s'",
-                  type.toString(), param.name()));
+              String.format("expected no more than %s positional arguments, but got %s",
+                  paramIndex, args.size()));
         }
-        i++;
-      } else if (param.named() && keys.remove(param.name())) {
-        // Named parameters
-        value = kwargs.get(param.name());
         if (!type.contains(value)) {
           return ArgumentListConversionResult.fromError(
               String.format(
                   "expected value of type '%s' for parameter '%s'",
                   type.toString(), param.name()));
         }
-      } else {
-        // Use default value
-        if (param.defaultValue().isEmpty()) {
+        if (param.named() && keys.contains(param.name())) {
           return ArgumentListConversionResult.fromError(
-              String.format("parameter '%s' has no default value", param.name()));
+              String.format("got multiple values for keyword argument '%s'", param.name()));
+        }
+        argIndex++;
+      } else { // No more positional arguments.
+        if (param.named() && keys.remove(param.name())) { // Param specified by keyword argument.
+          value = kwargs.get(param.name());
+          if (!type.contains(value)) {
+            return ArgumentListConversionResult.fromError(
+                String.format(
+                    "expected value of type '%s' for parameter '%s'",
+                    type.toString(), param.name()));
+          }
+        } else { // Param not specified by user. Use default value.
+          if (param.defaultValue().isEmpty()) {
+            return ArgumentListConversionResult.fromError(
+                String.format("parameter '%s' has no default value", param.name()));
+          }
+          value = SkylarkSignatureProcessor.getDefaultValue(param, null);
         }
-        value = SkylarkSignatureProcessor.getDefaultValue(param, null);
       }
-      builder.add(value);
       if (!param.noneable() && value instanceof NoneType) {
         return ArgumentListConversionResult.fromError(
             String.format("parameter '%s' cannot be None", param.name()));
       }
+      builder.add(value);
+      paramIndex++;
     }
-    if (i < args.size()) {
-      return ArgumentListConversionResult.fromError("too many arguments");
+
+    if (argIndex < args.size()) {
+      return ArgumentListConversionResult.fromError(
+          String.format("expected no more than %s positional arguments, but got %s",
+              paramIndex, args.size()));
     }
     if (!keys.isEmpty()) {
       return ArgumentListConversionResult.fromError(