From 501b6c76e7d92ccfacdee1662eaabd131a5f2b6f Mon Sep 17 00:00:00 2001
From: alandonovan <adonovan@google.com>
Date: Fri, 13 Nov 2020 16:43:45 -0500
Subject: [PATCH] starlark: disallow keyword argument after *args (#317)

f(*args, k=v) is a now an error. Use f(k=v, *args).

Arguments are evaluated left-to-right, as in Python2 (but not Python3).
See https://github.com/bazelbuild/starlark/issues/13 for spec change.
---
 resolve/resolve.go              |  8 +++++---
 resolve/testdata/resolve.star   |  6 +++---
 starlark/eval_test.go           |  6 +-----
 starlark/testdata/function.star | 16 ++++++++--------
 4 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/resolve/resolve.go b/resolve/resolve.go
index 26d74b4f..8c1b7304 100644
--- a/resolve/resolve.go
+++ b/resolve/resolve.go
@@ -741,7 +741,9 @@ func (r *resolver) expr(e syntax.Expr) {
 				// k=v
 				n++
 				if seenKwargs {
-					r.errorf(pos, "argument may not follow **kwargs")
+					r.errorf(pos, "keyword argument may not follow **kwargs")
+				} else if seenVarargs {
+					r.errorf(pos, "keyword argument may not follow *args")
 				}
 				x := binop.X.(*syntax.Ident)
 				if seenName[x.Name] {
@@ -757,9 +759,9 @@ func (r *resolver) expr(e syntax.Expr) {
 				// positional argument
 				p++
 				if seenVarargs {
-					r.errorf(pos, "argument may not follow *args")
+					r.errorf(pos, "positional argument may not follow *args")
 				} else if seenKwargs {
-					r.errorf(pos, "argument may not follow **kwargs")
+					r.errorf(pos, "positional argument may not follow **kwargs")
 				} else if len(seenName) > 0 {
 					r.errorf(pos, "positional argument may not follow named")
 				}
diff --git a/resolve/testdata/resolve.star b/resolve/testdata/resolve.star
index 8399bf39..fb4c78e0 100644
--- a/resolve/testdata/resolve.star
+++ b/resolve/testdata/resolve.star
@@ -289,12 +289,12 @@ f(**{}, *[]) ### `\*args may not follow \*\*kwargs`
 f(**{}, **{}) ### `multiple \*\*kwargs not allowed`
 
 ---
-# Only keyword arguments may follow *args in a call.
+# Only **kwargs may follow *args in a call.
 def f(*args, **kwargs):
   pass
 
-f(*[], 1) ### `argument may not follow \*args`
-f(*[], a=1) # ok
+f(*[], 1) ### `positional argument may not follow \*args`
+f(*[], a=1) ### `keyword argument may not follow \*args`
 f(*[], *[]) ### `multiple \*args not allowed`
 f(*[], **{}) # ok
 
diff --git a/starlark/eval_test.go b/starlark/eval_test.go
index 00b6e6d1..a2d61c5c 100644
--- a/starlark/eval_test.go
+++ b/starlark/eval_test.go
@@ -297,6 +297,7 @@ def j(a, b=42, *args, c, d=123, e, **kwargs):
 		t.Fatal(err)
 	}
 
+	// All errors are dynamic; see resolver for static errors.
 	for _, test := range []struct{ src, want string }{
 		// a()
 		{`a()`, `None`},
@@ -352,8 +353,6 @@ def j(a, b=42, *args, c, d=123, e, **kwargs):
 		{`f(0, b=1)`, `(0, 1, (), {})`},
 		{`f(0, a=1)`, `function f got multiple values for parameter "a"`},
 		{`f(0, b=1, c=2)`, `(0, 1, (), {"c": 2})`},
-		{`f(0, 1, x=2, *[3, 4], y=5, **dict(z=6))`, // github.com/google/skylark/issues/135
-			`(0, 1, (3, 4), {"x": 2, "y": 5, "z": 6})`},
 
 		// g(a, b=42, *args, c=123, **kwargs)
 		{`g()`, `function g missing 1 argument (a)`},
@@ -365,8 +364,6 @@ def j(a, b=42, *args, c, d=123, e, **kwargs):
 		{`g(0, b=1)`, `(0, 1, (), 123, {})`},
 		{`g(0, a=1)`, `function g got multiple values for parameter "a"`},
 		{`g(0, b=1, c=2, d=3)`, `(0, 1, (), 2, {"d": 3})`},
-		{`g(0, 1, x=2, *[3, 4], y=5, **dict(z=6))`,
-			`(0, 1, (3, 4), 123, {"x": 2, "y": 5, "z": 6})`},
 
 		// h(a, b=42, *, c=123, **kwargs)
 		{`h()`, `function h missing 1 argument (a)`},
@@ -379,7 +376,6 @@ def j(a, b=42, *args, c, d=123, e, **kwargs):
 		{`h(0, b=1, c=2)`, `(0, 1, 2, {})`},
 		{`h(0, b=1, d=2)`, `(0, 1, 123, {"d": 2})`},
 		{`h(0, b=1, c=2, d=3)`, `(0, 1, 2, {"d": 3})`},
-		{`h(0, b=1, c=2, d=3)`, `(0, 1, 2, {"d": 3})`},
 
 		// i(a, b=42, *, c, d=123, e, **kwargs)
 		{`i()`, `function i missing 3 arguments (a, c, e)`},
diff --git a/starlark/testdata/function.star b/starlark/testdata/function.star
index a1096844..9b74740a 100644
--- a/starlark/testdata/function.star
+++ b/starlark/testdata/function.star
@@ -223,20 +223,20 @@ load("assert.star", "assert")
 r = []
 
 def id(x):
-       r.append(x)
-       return x
+  r.append(x)
+  return x
 
 def f(*args, **kwargs):
   return (args, kwargs)
 
-y = f(id(1), id(2), x=id(3), *[id(4)], y=id(5), **dict(z=id(6)))
-assert.eq(y, ((1, 2, 4), dict(x=3, y=5, z=6)))
+y = f(id(1), id(2), x=id(3), *[id(4)], **dict(z=id(5)))
+assert.eq(y, ((1, 2, 4), dict(x=3, z=5)))
 
-# This matches Python2, but not Starlark-in-Java:
+# This matches Python2 and Starlark-in-Java, but not Python3 [1 2 4 3 6].
 # *args and *kwargs are evaluated last.
-# See github.com/bazelbuild/starlark#13 for pending spec change.
-assert.eq(r, [1, 2, 3, 5, 4, 6])
-
+# (Python[23] also allows keyword arguments after *args.)
+# See github.com/bazelbuild/starlark#13 for spec change.
+assert.eq(r, [1, 2, 3, 4, 5])
 
 ---
 # option:nesteddef option:recursion