Support required keyword-only parameters (google#151)

In def f(a, b=1, *, d, e=2, f), the parameters d and f
are keyword-only (because they come after the * or *args parameter),
but have no default value, so they are mandatory.

Also:
- improve the error messages
- spec update
- avoid need for "defined" bitset in setArgs

Updates bazelbuild/starlark#23

doc/spec.md

@@ -941,7 +941,7 @@ f(1, 2, 3, 4)           # (1, 2, (3, 4))
 
 <b>Keyword-variadic functions:</b> Some functions allow callers to
 provide an arbitrary sequence of `name=value` keyword arguments.
-A function definition may include a final _keyworded arguments_ or
+A function definition may include a final _keyword arguments_ or
 _kwargs_ parameter, indicated by a double-star preceding the parameter
 name: `**kwargs`.
 Any surplus named arguments that do not correspond to named parameters
@@ -959,8 +959,8 @@ f(x=2, y=1, z=3)        # (2, 1, {"z": 3})
 It is a static error if any two parameters of a function have the same name.
 
 Just as a function definition may accept an arbitrary number of
-positional or keyworded arguments, a function call may provide an
-arbitrary number of positional or keyworded arguments supplied by a
+positional or named arguments, a function call may provide an
+arbitrary number of positional or named arguments supplied by a
 list or dictionary:
 
 ```python
@@ -2529,11 +2529,27 @@ name preceded by a `*`.  This is the called the _varargs_ parameter,
 and it accumulates surplus positional arguments specified by a call.
 It is conventionally named `*args`.
 
-The varargs parameter may be followed by zero or more optional
-parameters, again of the form `name=expression`, but these optional parameters
-differ from earlier ones in that they are "keyword-only":
-a call must provide their values as keyword arguments,
-not positional ones.
+The varargs parameter may be followed by zero or more
+parameters, again of the forms `name` or `name=expression`,
+but these parameters differ from earlier ones in that they are
+_keyword-only_: if a call provides their values, it must do so as
+keyword arguments, not positional ones.
+
+```python
+def f(a, *, b=2, c):
+  print(a, b, c)
+
+f(1)                    # error: function f missing 1 argument (c)
+f(1, 3)                 # error: function f accepts 1 positional argument (2 given)
+f(1, c=3)               # "1 2 3"
+
+def g(a, *args, b=2, c):
+  print(a, b, c, args)
+
+g(1, 3)                 # error: function g missing 1 argument (c)
+g(1, 4, c=3)            # "1 2 3 (4,)"
+
+```
 
 A non-variadic function may also declare keyword-only parameters,
 by using a bare `*` in place of the `*args` parameter.

internal/compile/compile.go

@@ -37,7 +37,7 @@ import (
 const debug = false // TODO(adonovan): use a bitmap of options; and regexp to match files
 
 // Increment this to force recompilation of saved bytecode files.
-const Version = 6
+const Version = 7
 
 type Opcode uint8
 
@@ -85,9 +85,10 @@ const (
 	UMINUS // x UMINUS -x
 	TILDE  // x TILDE ~x
 
-	NONE  // - NONE None
-	TRUE  // - TRUE True
-	FALSE // - FALSE False
+	NONE      // - NONE None
+	TRUE      // - TRUE True
+	FALSE     // - FALSE False
+	MANDATORY // - MANDATORY Mandatory	     [sentinel value for required kwonly args]
 
 	ITERPUSH    //       iterable ITERPUSH -     [pushes the iterator stack]
 	ITERPOP     //              - ITERPOP -      [pops the iterator stack]
@@ -110,21 +111,21 @@ const (
 	ITERJMP //            - ITERJMP<addr> elem   (and fall through) [acts on topmost iterator]
 	//       or:          - ITERJMP<addr> -      (and jump)
 
-	CONSTANT    //                - CONSTANT<constant>  value
-	MAKETUPLE   //        x1 ... xn MAKETUPLE<n>        tuple
-	MAKELIST    //        x1 ... xn MAKELIST<n>         list
-	MAKEFUNC    //      args kwargs MAKEFUNC<func>      fn
-	LOAD        //  from1 ... fromN module LOAD<n>      v1 ... vN
-	SETLOCAL    //            value SETLOCAL<local>     -
-	SETGLOBAL   //            value SETGLOBAL<global>   -
-	LOCAL       //                - LOCAL<local>        value
-	FREE        //                - FREE<freevar>       value
-	GLOBAL      //                - GLOBAL<global>      value
-	PREDECLARED //                - PREDECLARED<name>   value
-	UNIVERSAL   //                - UNIVERSAL<name>     value
-	ATTR        //                x ATTR<name>          y           y = x.name
-	SETFIELD    //              x y SETFIELD<name>      -           x.name = y
-	UNPACK      //         iterable UNPACK<n>           vn ... v1
+	CONSTANT    //                 - CONSTANT<constant>  value
+	MAKETUPLE   //         x1 ... xn MAKETUPLE<n>        tuple
+	MAKELIST    //         x1 ... xn MAKELIST<n>         list
+	MAKEFUNC    // defaults freevars MAKEFUNC<func>      fn
+	LOAD        //   from1 ... fromN module LOAD<n>      v1 ... vN
+	SETLOCAL    //             value SETLOCAL<local>     -
+	SETGLOBAL   //             value SETGLOBAL<global>   -
+	LOCAL       //                 - LOCAL<local>        value
+	FREE        //                 - FREE<freevar>       value
+	GLOBAL      //                 - GLOBAL<global>      value
+	PREDECLARED //                 - PREDECLARED<name>   value
+	UNIVERSAL   //                 - UNIVERSAL<name>     value
+	ATTR        //                 x ATTR<name>          y           y = x.name
+	SETFIELD    //               x y SETFIELD<name>      -           x.name = y
+	UNPACK      //          iterable UNPACK<n>           vn ... v1
 
 	// n>>8 is #positional args and n&0xff is #named args (pairs).
 	CALL        // fn positional named                CALL<n>        result
@@ -175,6 +176,7 @@ var opcodeNames = [...]string{
 	MAKEFUNC:    "makefunc",
 	MAKELIST:    "makelist",
 	MAKETUPLE:   "maketuple",
+	MANDATORY:   "mandatory",
 	MINUS:       "minus",
 	NEQ:         "neq",
 	NONE:        "none",
@@ -244,6 +246,7 @@ var stackEffect = [...]int8{
 	MAKEFUNC:    -1,
 	MAKELIST:    variableStackEffect,
 	MAKETUPLE:   variableStackEffect,
+	MANDATORY:   +1,
 	MINUS:       -1,
 	NEQ:         -1,
 	NONE:        +1,
@@ -1680,12 +1683,24 @@ func (fcomp *fcomp) function(pos syntax.Position, name string, f *syntax.Functio
 	// so record the position.
 	fcomp.setPos(pos)
 
-	// Generate tuple of parameter defaults.
+	// Generate tuple of parameter defaults. For:
+	//  def f(p1, p2=dp2, p3=dp3, *, k1, k2=dk2, k3, **kwargs)
+	// the tuple is:
+	//  (dp2, dp3, MANDATORY, dk2, MANDATORY).
 	n := 0
+	seenStar := false
 	for _, param := range f.Params {
-		if binary, ok := param.(*syntax.BinaryExpr); ok {
-			fcomp.expr(binary.Y)
+		switch param := param.(type) {
+		case *syntax.BinaryExpr:
+			fcomp.expr(param.Y)
 			n++
+		case *syntax.UnaryExpr:
+			seenStar = true // * or *args (also **kwargs)
+		case *syntax.Ident:
+			if seenStar {
+				fcomp.emit(MANDATORY)
+				n++
+			}
 		}
 	}
 	fcomp.emit1(MAKETUPLE, uint32(n))

resolve/resolve.go

@@ -775,7 +775,7 @@ func (r *resolver) function(pos syntax.Position, name string, function *syntax.F
 			if starStar != nil {
 				r.errorf(param.NamePos, "required parameter may not follow **%s", starStar.Name)
 			} else if star != nil {
-				r.errorf(param.NamePos, "required parameter may not follow * parameter")
+				numKwonlyParams++
 			} else if seenOptional {
 				r.errorf(param.NamePos, "required parameter may not follow optional")
 			}
@@ -827,7 +827,7 @@ func (r *resolver) function(pos syntax.Position, name string, function *syntax.F
 			}
 			function.HasVarargs = true
 		} else if numKwonlyParams == 0 {
-			r.errorf(star.OpPos, "bare * must be followed by optional parameters")
+			r.errorf(star.OpPos, "bare * must be followed by keyword-only parameters")
 		}
 	}
 	if starStar != nil {

resolve/testdata/resolve.star

@@ -233,27 +233,27 @@ def h(**kwargs1, **kwargs2): ### `multiple \*\* parameters not allowed`
 ---
 # Only keyword-only params and **kwargs may follow *args in a declaration.
 
-def f(*args, x): ### `required parameter may not follow \* parameter`
+def f(*args, x): # ok
   pass
 
 def g(*args1, *args2): ### `multiple \* parameters not allowed`
   pass
 
-def h(*, ### `bare \* must be followed by optional parameters`
+def h(*, ### `bare \* must be followed by keyword-only parameters`
       *): ### `multiple \* parameters not allowed`
   pass
 
 def i(*args, *): ### `multiple \* parameters not allowed`
   pass
 
-def j(*,      ### `bare \* must be followed by optional parameters`
+def j(*,      ### `bare \* must be followed by keyword-only parameters`
       *args): ### `multiple \* parameters not allowed`
   pass
 
-def k(*, **kwargs): ### `bare \* must be followed by optional parameters`
+def k(*, **kwargs): ### `bare \* must be followed by keyword-only parameters`
   pass
 
-def l(*): ### `bare \* must be followed by optional parameters`
+def l(*): ### `bare \* must be followed by keyword-only parameters`
   pass
 
 def m(*args, a=1, **kwargs): # ok

starlark/eval.go

@@ -1116,12 +1116,29 @@ func asIndex(v Value, len int, result *int) error {
 // setArgs sets the values of the formal parameters of function fn in
 // based on the actual parameter values in args and kwargs.
 func setArgs(locals []Value, fn *Function, args Tuple, kwargs []Tuple) error {
-	// This is adapted from the algorithm from PyEval_EvalCodeEx.
+
+	// This is the general schema of a function:
+	//
+	//   def f(p1, p2=dp2, p3=dp3, *args, k1, k2=dk2, k3, **kwargs)
+	//
+	// The p parameters are non-kwonly, and may be specified positionally.
+	// The k parameters are kwonly, and must be specified by name.
+	// The defaults tuple is (dp2, dp3, mandatory, dk2, mandatory).
+	//
+	// Arguments are processed as follows:
+	// - positional arguments are bound to a prefix of [p1, p2, p3].
+	// - surplus positional arguments are bound to *args.
+	// - keyword arguments are bound to any of {p1, p2, p3, k1, k2, k3};
+	//   duplicate bindings are rejected.
+	// - surplus keyword arguments are bound to **kwargs.
+	// - defaults are bound to each parameter from p2 to k3 if no value was set.
+	//   default values come from the tuple above.
+	//   It is an error if the tuple entry for an unset parameter is 'mandatory'.
 
 	// Nullary function?
 	if fn.NumParams() == 0 {
 		if nactual := len(args) + len(kwargs); nactual > 0 {
-			return fmt.Errorf("function %s takes no arguments (%d given)", fn.Name(), nactual)
+			return fmt.Errorf("function %s accepts no arguments (%d given)", fn.Name(), nactual)
 		}
 		return nil
 	}
@@ -1133,7 +1150,7 @@ func setArgs(locals []Value, fn *Function, args Tuple, kwargs []Tuple) error {
 		return z
 	}
 
-	// nparams is the number of ordinary parameters (sans * or **).
+	// nparams is the number of ordinary parameters (sans *args and **kwargs).
 	nparams := fn.NumParams()
 	var kwdict *Dict
 	if fn.HasKwargs() {
@@ -1145,32 +1162,29 @@ func setArgs(locals []Value, fn *Function, args Tuple, kwargs []Tuple) error {
 		nparams--
 	}
 
+	// nonkwonly is the number of non-kwonly parameters.
+	nonkwonly := nparams - fn.NumKwonlyParams()
+
 	// Too many positional args?
 	n := len(args)
-	maxpos := nparams - fn.NumKwonlyParams()
-	if len(args) > maxpos {
+	if len(args) > nonkwonly {
 		if !fn.HasVarargs() {
-			return fmt.Errorf("function %s takes %s %d positional argument%s (%d given)",
+			return fmt.Errorf("function %s accepts %s%d positional argument%s (%d given)",
 				fn.Name(),
-				cond(len(fn.defaults) > 0, "at most", "exactly"),
-				maxpos,
-				cond(nparams == 1, "", "s"),
-				len(args)+len(kwargs))
+				cond(len(fn.defaults) > fn.NumKwonlyParams(), "at most ", ""),
+				nonkwonly,
+				cond(nonkwonly == 1, "", "s"),
+				len(args))
 		}
-		n = maxpos
+		n = nonkwonly
 	}
 
-	// set of defined (regular) parameters
-	var defined intset
-	defined.init(nparams)
-
-	// ordinary parameters
+	// Bind positional arguments to non-kwonly parameters.
 	for i := 0; i < n; i++ {
 		locals[i] = args[i]
-		defined.set(i)
 	}
 
-	// variadic arguments
+	// Bind surplus positional arguments to *args parameter.
 	if fn.HasVarargs() {
 		tuple := make(Tuple, len(args)-n)
 		for i := n; i < len(args); i++ {
@@ -1179,50 +1193,56 @@ func setArgs(locals []Value, fn *Function, args Tuple, kwargs []Tuple) error {
 		locals[nparams] = tuple
 	}
 
-	// keyword arguments
+	// Bind keyword arguments to parameters.
 	paramIdents := fn.funcode.Locals[:nparams]
 	for _, pair := range kwargs {
 		k, v := pair[0].(String), pair[1]
 		if i := findParam(paramIdents, string(k)); i >= 0 {
-			if defined.set(i) {
-				return fmt.Errorf("function %s got multiple values for keyword argument %s", fn.Name(), k)
+			if locals[i] != nil {
+				return fmt.Errorf("function %s got multiple values for parameter %s", fn.Name(), k)
 			}
 			locals[i] = v
 			continue
 		}
 		if kwdict == nil {
 			return fmt.Errorf("function %s got an unexpected keyword argument %s", fn.Name(), k)
 		}
-		n := kwdict.Len()
+		oldlen := kwdict.Len()
 		kwdict.SetKey(k, v)
-		if kwdict.Len() == n {
-			return fmt.Errorf("function %s got multiple values for keyword argument %s", fn.Name(), k)
+		if kwdict.Len() == oldlen {
+			return fmt.Errorf("function %s got multiple values for parameter %s", fn.Name(), k)
 		}
 	}
 
-	// default values
+	// Are defaults required?
 	if n < nparams || fn.NumKwonlyParams() > 0 {
 		m := nparams - len(fn.defaults) // first default
 
-		// report errors for missing non-optional arguments
-		i := n
-		for ; i < m; i++ {
-			if !defined.get(i) {
-				return fmt.Errorf("function %s takes %s %d positional argument%s (%d given)",
-					fn.Name(),
-					cond(fn.HasVarargs() || len(fn.defaults) > 0, "at least", "exactly"),
-					m,
-					cond(m == 1, "", "s"),
-					defined.len())
+		// Report errors for missing required arguments.
+		var missing []string
+		var i int
+		for i = n; i < m; i++ {
+			if locals[i] == nil {
+				missing = append(missing, paramIdents[i].Name)
 			}
 		}
 
-		// set default values
+		// Bind default values to parameters.
 		for ; i < nparams; i++ {
-			if !defined.get(i) {
-				locals[i] = fn.defaults[i-m]
+			if locals[i] == nil {
+				dflt := fn.defaults[i-m]
+				if _, ok := dflt.(mandatory); ok {
+					missing = append(missing, paramIdents[i].Name)
+					continue
+				}
+				locals[i] = dflt
 			}
 		}
+
+		if missing != nil {
+			return fmt.Errorf("function %s missing %d argument%s (%s)",
+				fn.Name(), len(missing), cond(len(missing) > 1, "s", ""), strings.Join(missing, ", "))
+		}
 	}
 	return nil
 }