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

3 files changed, 15 insertions, 12 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java
index 6833622e28..9deb6244ac 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java
@@ -80,6 +80,7 @@ public class BaseFunctionTest extends AbstractEvaluationTestCase {
     "mixed(1, 2, 3)",
     "mixed(1, 2, wiz=3, quux=4)",
     "mixed(foo=1)",
+    "mixed(bar=2)",
     "mixed(foo=1, bar=2)",
     "mixed(bar=2, foo=1)",
     "mixed(2, foo=1)",
@@ -111,6 +112,7 @@ public class BaseFunctionTest extends AbstractEvaluationTestCase {
         "too many (3) positional arguments in call to mixed(foo, bar = ?)",
         "unexpected keywords 'quux', 'wiz' in call to mixed(foo, bar = ?)",
         "[1, null]",
+        "missing mandatory positional argument 'foo' while calling mixed(foo, bar = ?)",
         "[1, 2]",
         "[1, 2]",
         "argument 'foo' passed both by position and by name in call to mixed(foo, bar = ?)",
@@ -119,17 +121,18 @@ public class BaseFunctionTest extends AbstractEvaluationTestCase {
 
   @Test
   public void testOnlyNamedArguments() throws Exception {
-    checkBaseFunctions(true, "mixed(*, foo = ?, bar)",
-        "missing mandatory keyword arguments in call to mixed(*, foo = ?, bar)",
-        "mixed(*, foo = ?, bar) does not accept positional arguments, but got 1",
-        "mixed(*, foo = ?, bar) does not accept positional arguments, but got 2",
-        "mixed(*, foo = ?, bar) does not accept positional arguments, but got 3",
-        "mixed(*, foo = ?, bar) does not accept positional arguments, but got 2",
-        "missing mandatory named-only argument 'bar' while calling mixed(*, foo = ?, bar)",
+    checkBaseFunctions(true, "mixed(*, foo, bar = ?)",
+        "missing mandatory keyword arguments in call to mixed(*, foo, bar = ?)",
+        "mixed(*, foo, bar = ?) does not accept positional arguments, but got 1",
+        "mixed(*, foo, bar = ?) does not accept positional arguments, but got 2",
+        "mixed(*, foo, bar = ?) does not accept positional arguments, but got 3",
+        "mixed(*, foo, bar = ?) does not accept positional arguments, but got 2",
+        "[1, null]",
+        "missing mandatory named-only argument 'foo' while calling mixed(*, foo, bar = ?)",
         "[1, 2]",
         "[1, 2]",
-        "mixed(*, foo = ?, bar) does not accept positional arguments, but got 1",
-        "unexpected keyword 'wiz' in call to mixed(*, foo = ?, bar)");
+        "mixed(*, foo, bar = ?) does not accept positional arguments, but got 1",
+        "unexpected keyword 'wiz' in call to mixed(*, foo, bar = ?)");
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
index 2eea35a1d9..a55fc01401 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
@@ -383,7 +383,7 @@ public class FunctionTest extends AbstractEvaluationTestCase {
     assertEquals("xyce", env.lookup("v2"));
     assertEquals("xyzf", env.lookup("v3"));
     UserDefinedFunction foo = (UserDefinedFunction) env.lookup("foo");
-    assertEquals("foo(a, b = \"b\", *, d = \"d\", c)", foo.toString());
+    assertEquals("foo(a, b = \"b\", *, c, d = \"d\")", foo.toString());
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
index 683f2c7f48..9157a09e9c 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
@@ -1018,9 +1018,9 @@ public class ParserTest extends AbstractParserTestCase {
     assertThat(statements.get(0)).isInstanceOf(FunctionDefStatement.class);
     FunctionDefStatement stmt = (FunctionDefStatement) statements.get(0);
     FunctionSignature sig = stmt.getArgs().getSignature();
-    // Note the reordering of mandatory named-only at the end.
+    // Note the reordering of optional named-only at the end.
     assertThat(sig.getNames()).isEqualTo(ImmutableList.<String>of(
-        "a", "b1", "b2", "d", "c1", "c2"));
+        "a", "b1", "b2", "c1", "c2", "d"));
     FunctionSignature.Shape shape = sig.getShape();
     assertThat(shape.getMandatoryPositionals()).isEqualTo(1);
     assertThat(shape.getOptionalPositionals()).isEqualTo(2);