Reorder arguments to BuiltinFunction-s

By popular demand from other implementers, reorder BuiltinFunction arguments
so that mandatory named-only arguments come befor optional named-only arguments
rather than after. This will make Skylark internals slightly clearer and less
surprising, at the cost of eschewing a tiny optimization.

--
MOS_MIGRATED_REVID=89978554

2 files changed, 59 insertions, 51 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
index f1a6bd732e..f1b148f442 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
@@ -230,8 +230,6 @@ public abstract class BaseFunction implements Function {
     int numPositionalParams = numMandatoryPositionalParams + numOptionalPositionalParams;
     int numNamedOnlyParams = numMandatoryNamedOnlyParams + numOptionalNamedOnlyParams;
     int numNamedParams = numPositionalParams + numNamedOnlyParams;
-    int numOptionalParams = numOptionalPositionalParams + numOptionalNamedOnlyParams;
-    int endOptionalParams = numMandatoryPositionalParams + numOptionalParams;
     int kwParamIndex = names.size() - 1; // only valid if hasKwParam
 
     // (1) handle positional arguments
@@ -271,9 +269,11 @@ public abstract class BaseFunction implements Function {
         throw new EvalException(loc, String.format(
             "missing mandatory keyword arguments in call to %s", this));
       }
-      // Fill in defaults for missing optional parameters, that were conveniently grouped together.
+      // Fill in defaults for missing optional parameters, that were conveniently grouped together,
+      // thanks to the absence of mandatory named-only parameters as checked above.
       if (defaultValues != null) {
         int j = numPositionalArgs - numMandatoryPositionalParams;
+        int endOptionalParams = numPositionalParams + numOptionalNamedOnlyParams;
         for (int i = numPositionalArgs; i < endOptionalParams; i++) {
           arguments[i] = defaultValues.get(j++);
         }
@@ -337,26 +337,31 @@ public abstract class BaseFunction implements Function {
         if (arguments[i] == null) {
           throw new EvalException(loc, String.format(
               "missing mandatory positional argument '%s' while calling %s",
-              names.get(i), toString()));
+              names.get(i), this));
         }
       }
 
-      for (int i = numNamedParams - numMandatoryNamedOnlyParams; i < numNamedParams; i++) {
+      int endMandatoryNamedOnlyParams = numPositionalParams + numMandatoryNamedOnlyParams;
+      for (int i = numPositionalParams; i < endMandatoryNamedOnlyParams; i++) {
         if (arguments[i] == null) {
           throw new EvalException(loc, String.format(
               "missing mandatory named-only argument '%s' while calling %s",
-              names.get(i), toString()));
+              names.get(i), this));
         }
       }
 
       // Get defaults for those parameters that weren't passed.
       if (defaultValues != null) {
-        for (int j = numPositionalArgs > numMandatoryPositionalParams
-                 ? numPositionalArgs - numMandatoryPositionalParams : 0;
-             j < numOptionalParams; j++) {
-          int i = j + numMandatoryPositionalParams;
+        for (int i = Math.max(numPositionalArgs, numMandatoryPositionalParams);
+             i < numPositionalParams; i++) {
           if (arguments[i] == null) {
-            arguments[i] = defaultValues.get(j);
+            arguments[i] = defaultValues.get(i - numMandatoryPositionalParams);
+          }
+        }
+        int numMandatoryParams = numMandatoryPositionalParams + numMandatoryNamedOnlyParams;
+        for (int i = numMandatoryParams + numOptionalPositionalParams; i < numNamedParams; i++) {
+          if (arguments[i] == null) {
+            arguments[i] = defaultValues.get(i - numMandatoryParams);
           }
         }
       }
@@ -442,11 +447,11 @@ public abstract class BaseFunction implements Function {
    * Render this object in the form of an equivalent Python function signature.
    */
   public String toString() {
-    StringBuffer sb = new StringBuffer();
+    StringBuilder sb = new StringBuilder();
     sb.append(getName());
     if (signature != null) {
       sb.append('(');
-      signature.toStringBuffer(sb);
+      signature.toStringBuilder(sb);
       sb.append(')');
     } // if unconfigured, don't even output parentheses
     return sb.toString();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java b/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
index 0208cd7265..23d8e8c5a5 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
@@ -38,20 +38,24 @@ import javax.annotation.Nullable;
  * <p>To enable various optimizations in the argument processing routine,
  * we sort arguments according the following constraints, enabling corresponding optimizations:
  * <ol>
- * <li>the positional mandatories come just before the positional optionals,
+ * <li>The positional mandatories come just before the positional optionals,
  *   so they can be filled in one go.
- * <li>the optionals are grouped together, so we can iterate over them in one go.
- * <li>positionals come first, so it's easy to prepend extra positional arguments such as "self"
+ * <li>Positionals come first, so it's easy to prepend extra positional arguments such as "self"
  *   to an argument list, and we optimize for the common case of no key-only mandatory parameters.
  *   key-only parameters are thus grouped together.
  *   positional mandatory and key-only mandatory parameters are separate,
  *   but there no loop over a contiguous chunk of them, anyway.
- * <li>the named are all grouped together, with star and star_star rest arguments coming last.
+ * <li>The named are all grouped together, with star and star_star rest arguments coming last.
+ * <li>Mandatory arguments in each category (positional and named-only) come before the optional
+ *   arguments, for the sake of slightly better clarity to human implementers. This eschews an
+ *   optimization whereby grouping optionals together allows to iterate over them in one go instead
+ *   of two; however, this relatively minor optimization only matters when keyword arguments are
+ *   passed, at which point it is dwarfed by the slowness of keyword processing.
  * </ol>
  *
  * <p>Parameters are thus sorted in the following obvious order:
  * positional mandatory arguments (if any), positional optional arguments (if any),
- * key-only optional arguments (if any), key-only mandatory arguments (if any),
+ * key-only mandatory arguments (if any), key-only optional arguments (if any),
  * then star argument (if any), then star_star argument (if any).
  */
 public abstract class FunctionSignature implements Serializable {
@@ -138,14 +142,14 @@ public abstract class FunctionSignature implements Serializable {
   public abstract ImmutableList<String> getNames();
 
   /** append a representation of this signature to a string buffer. */
-  public StringBuffer toStringBuffer(StringBuffer sb) {
-    return WithValues.<Object, SkylarkType>create(this).toStringBuffer(sb);
+  public StringBuilder toStringBuilder(StringBuilder sb) {
+    return WithValues.<Object, SkylarkType>create(this).toStringBuilder(sb);
   }
 
   @Override
   public String toString() {
-    StringBuffer sb = new StringBuffer();
-    toStringBuffer(sb);
+    StringBuilder sb = new StringBuilder();
+    toStringBuilder(sb);
     return sb.toString();
   }
 
@@ -228,9 +232,10 @@ public abstract class FunctionSignature implements Serializable {
       ArrayList<String> params = new ArrayList<>();
       ArrayList<V> defaults = new ArrayList<>();
       ArrayList<T> types = new ArrayList<>();
-      // mandatory named-only parameters are kept aside to be spliced after the optional ones.
-      ArrayList<String> mandatoryNamedOnlyParams = new ArrayList<>();
-      ArrayList<T> mandatoryNamedOnlyTypes = new ArrayList<>();
+      // optional named-only parameters are kept aside to be spliced after the mandatory ones.
+      ArrayList<String> optionalNamedOnlyParams = new ArrayList<>();
+      ArrayList<T> optionalNamedOnlyTypes = new ArrayList<>();
+      ArrayList<V> optionalNamedOnlyDefaultValues = new ArrayList<>();
       boolean defaultRequired = false; // true after mandatory positionals and before star.
       Set<String> paramNameSet = new HashSet<>(); // set of names, to avoid duplicates
 
@@ -261,35 +266,33 @@ public abstract class FunctionSignature implements Serializable {
             star = name;
             starType = type;
           }
-        } else if (hasStar && param.isMandatory()) {
-          // mandatory named-only, added contiguously at the end, to simplify calls
-          mandatoryNamedOnlyParams.add(name);
-          mandatoryNamedOnlyTypes.add(type);
-          mandatoryNamedOnly++;
+        } else if (hasStar && param.isOptional()) {
+          optionalNamedOnly++;
+          optionalNamedOnlyParams.add(name);
+          optionalNamedOnlyTypes.add(type);
+          optionalNamedOnlyDefaultValues.add(param.getDefaultValue());
         } else {
           params.add(name);
           types.add(type);
-          if (param.isMandatory()) {
-            if (defaultRequired) {
+          if (param.isOptional()) {
+            optionalPositionals++;
+            defaults.add(param.getDefaultValue());
+            defaultRequired = true;
+          } else if (hasStar) {
+            mandatoryNamedOnly++;
+          } else if (defaultRequired) {
               throw new SignatureException(
                   "a mandatory positional parameter must not follow an optional parameter",
                   param);
-            }
+          } else {
             mandatoryPositionals++;
-          } else { // At this point, it's an optional parameter
-            defaults.add(param.getDefaultValue());
-            if (hasStar) {
-              // named-only optional
-              optionalNamedOnly++;
-            } else {
-              optionalPositionals++;
-              defaultRequired = true;
-            }
           }
         }
       }
-      params.addAll(mandatoryNamedOnlyParams);
-      types.addAll(mandatoryNamedOnlyTypes);
+      params.addAll(optionalNamedOnlyParams);
+      types.addAll(optionalNamedOnlyTypes);
+      defaults.addAll(optionalNamedOnlyDefaultValues);
+
       if (star != null) {
         params.add(star);
         types.add(starType);
@@ -312,7 +315,7 @@ public abstract class FunctionSignature implements Serializable {
     /**
      * Append a representation of this signature to a string buffer.
      */
-    public StringBuffer toStringBuffer(final StringBuffer sb) {
+    public StringBuilder toStringBuilder(final StringBuilder sb) {
       FunctionSignature signature = getSignature();
       Shape shape = signature.getShape();
       final ImmutableList<String> names = signature.getNames();
@@ -329,7 +332,7 @@ public abstract class FunctionSignature implements Serializable {
       int namedOnly = mandatoryNamedOnly + optionalNamedOnly;
       int named = positionals + namedOnly;
       int args = named + (starArg ? 1 : 0) + (kwArg ? 1 : 0);
-      int endOptionals = positionals + optionalNamedOnly;
+      int endMandatoryNamedOnly = positionals + mandatoryNamedOnly;
       boolean hasStar = starArg || (namedOnly > 0);
       int iStarArg = named;
       int iKwArg = args - 1;
@@ -374,11 +377,11 @@ public abstract class FunctionSignature implements Serializable {
           sb.append(names.get(iStarArg));
         }
       }
-      for (; i < endOptionals; i++) {
-        show.optional(i);
+      for (; i < endMandatoryNamedOnly; i++) {
+        show.mandatory(i);
       }
       for (; i < named; i++) {
-        show.mandatory(i);
+        show.optional(i);
       }
       if (kwArg) {
         show.comma();
@@ -391,8 +394,8 @@ public abstract class FunctionSignature implements Serializable {
 
     @Override
     public String toString() {
-      StringBuffer sb = new StringBuffer();
-      toStringBuffer(sb);
+      StringBuilder sb = new StringBuilder();
+      toStringBuilder(sb);
       return sb.toString();
     }
   }