Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

starlark: tweak how UnpackArgs enforces optional args #545

Merged
merged 1 commit into from
May 7, 2024
Merged

starlark: tweak how UnpackArgs enforces optional args #545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions starlark/eval_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,3 +1109,36 @@ f(1)
t.Errorf("env() returned %s, want %s", got, want)
}
}

func TestUnpackArgsOptionalInference(t *testing.T) {
// success
kwargs := []starlark.Tuple{
{starlark.String("x"), starlark.MakeInt(1)},
{starlark.String("y"), starlark.MakeInt(2)},
}
var x, y, z int
if err := starlark.UnpackArgs("unpack", nil, kwargs,
"x", &x, "y?", &y, "z", &z); err != nil {
t.Errorf("UnpackArgs failed: %v", err)
}
got := fmt.Sprintf("x=%d y=%d z=%d", x, y, z)
want := "x=1 y=2 z=0"
if got != want {
t.Errorf("got %s, want %s", got, want)
}

// success
args := starlark.Tuple{starlark.MakeInt(1), starlark.MakeInt(2)}
x = 0
y = 0
z = 0
if err := starlark.UnpackArgs("unpack", args, nil,
"x", &x, "y?", &y, "z", &z); err != nil {
t.Errorf("UnpackArgs failed: %v", err)
}
got = fmt.Sprintf("x=%d y=%d z=%d", x, y, z)
want = "x=1 y=2 z=0"
if got != want {
t.Errorf("got %s, want %s", got, want)
}
}
68 changes: 35 additions & 33 deletions starlark/unpack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@ type Unpacker interface {
// If the variable is a bool, integer, string, *List, *Dict, Callable,
// Iterable, or user-defined implementation of Value,
// UnpackArgs performs the appropriate type check.
// Predeclared Go integer types uses the AsInt check.
// Predeclared Go integer types use the AsInt check.
//
// If the parameter name ends with "?", it is optional.
//
// If the parameter name ends with "??", it is optional and treats the None value
// as if the argument was absent.
//
// If a parameter is marked optional, then all following parameters are
// implicitly optional where or not they are marked.
// implicitly optional whether or not they are marked.
//
// If the variable implements Unpacker, its Unpack argument
// is called with the argument value, allowing an application
Expand All @@ -44,23 +44,23 @@ type Unpacker interface {
//
// Examples:
//
// var (
// a Value
// b = MakeInt(42)
// c Value = starlark.None
// )
// var (
nicks marked this conversation as resolved.
Show resolved Hide resolved
// a Value
// b = MakeInt(42)
// c Value = starlark.None
// )
//
// // 1. mixed parameters, like def f(a, b=42, c=None).
// err := UnpackArgs("f", args, kwargs, "a", &a, "b?", &b, "c?", &c)
// // 1. mixed parameters, like def f(a, b=42, c=None).
// err := UnpackArgs("f", args, kwargs, "a", &a, "b?", &b, "c?", &c)
//
// // 2. keyword parameters only, like def f(*, a, b, c=None).
// if len(args) > 0 {
// return fmt.Errorf("f: unexpected positional arguments")
// }
// err := UnpackArgs("f", args, kwargs, "a", &a, "b?", &b, "c?", &c)
// // 2. keyword parameters only, like def f(*, a, b, c=None).
// if len(args) > 0 {
// return fmt.Errorf("f: unexpected positional arguments")
// }
// err := UnpackArgs("f", args, kwargs, "a", &a, "b?", &b, "c?", &c)
//
// // 3. positional parameters only, like def f(a, b=42, c=None, /) in Python 3.8.
// err := UnpackPositionalArgs("f", args, kwargs, 1, &a, &b, &c)
// // 3. positional parameters only, like def f(a, b=42, c=None, /) in Python 3.8.
// err := UnpackPositionalArgs("f", args, kwargs, 1, &a, &b, &c)
//
// More complex forms such as def f(a, b=42, *args, c, d=123, **kwargs)
// require additional logic, but their need in built-ins is exceedingly rare.
Expand All @@ -79,23 +79,22 @@ type Unpacker interface {
// for the zero values of variables of type *List, *Dict, Callable, or
// Iterable. For example:
//
// // def myfunc(d=None, e=[], f={})
// var (
// d Value
// e *List
// f *Dict
// )
// err := UnpackArgs("myfunc", args, kwargs, "d?", &d, "e?", &e, "f?", &f)
// if d == nil { d = None; }
// if e == nil { e = new(List); }
// if f == nil { f = new(Dict); }
//
func UnpackArgs(fnname string, args Tuple, kwargs []Tuple, pairs ...interface{}) error {
// // def myfunc(d=None, e=[], f={})
// var (
// d Value
// e *List
// f *Dict
// )
// err := UnpackArgs("myfunc", args, kwargs, "d?", &d, "e?", &e, "f?", &f)
// if d == nil { d = None; }
// if e == nil { e = new(List); }
// if f == nil { f = new(Dict); }
func UnpackArgs(fnname string, args Tuple, kwargs []Tuple, pairs ...any) error {
nparams := len(pairs) / 2
var defined intset
defined.init(nparams)

paramName := func(x interface{}) (name string, skipNone bool) { // (no free variables)
paramName := func(x any) (name string, skipNone bool) { // (no free variables)
name = x.(string)
if strings.HasSuffix(name, "??") {
name = strings.TrimSuffix(name, "??")
Expand Down Expand Up @@ -164,12 +163,15 @@ kwloop:
}

// Check that all non-optional parameters are defined.
// (We needn't check the first len(args).)
for i := len(args); i < nparams; i++ {
for i := 0; i < nparams; i++ {
name := pairs[2*i].(string)
if strings.HasSuffix(name, "?") {
break // optional
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc comment says "If a parameter is marked optional, then all following parameters are implicitly optional whether or not they are marked." In hindsight we should probably have disallowed non-optionals after optionals, but that would be a breaking change now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

}
// (We needn't check the first len(args).)
if i < len(args) {
continue
}
if !defined.get(i) {
return fmt.Errorf("%s: missing argument for %s", fnname, name)
}
Expand All @@ -187,7 +189,7 @@ kwloop:
// any conversion fails.
//
// See UnpackArgs for general comments.
func UnpackPositionalArgs(fnname string, args Tuple, kwargs []Tuple, min int, vars ...interface{}) error {
func UnpackPositionalArgs(fnname string, args Tuple, kwargs []Tuple, min int, vars ...any) error {
if len(kwargs) > 0 {
return fmt.Errorf("%s: unexpected keyword arguments", fnname)
}
Expand All @@ -214,7 +216,7 @@ func UnpackPositionalArgs(fnname string, args Tuple, kwargs []Tuple, min int, va
return nil
}

func unpackOneArg(v Value, ptr interface{}) error {
func unpackOneArg(v Value, ptr any) error {
// On failure, don't clobber *ptr.
switch ptr := ptr.(type) {
case Unpacker:
Expand Down
Loading