From 5a9c9c7864a4cae5cd5dce9089c80234ab174d47 Mon Sep 17 00:00:00 2001 From: Simon Frei Date: Thu, 19 Aug 2021 10:00:53 +0200 Subject: [PATCH] Bubble errors instead of panicking (#194) --- build.go | 79 ++++++++++++++++++++++++++++++---------------- context.go | 2 -- global_test.go | 2 +- guesswidth_unix.go | 1 + kong.go | 34 ++++++-------------- model.go | 10 ------ tag.go | 47 +++++++++++++++------------ 7 files changed, 90 insertions(+), 85 deletions(-) diff --git a/build.go b/build.go index 36581e3..6f10a45 100644 --- a/build.go +++ b/build.go @@ -12,7 +12,6 @@ import ( type Plugins []interface{} func build(k *Kong, ast interface{}) (app *Application, err error) { - defer catch(&err) v := reflect.ValueOf(ast) iv := reflect.Indirect(v) if v.Kind() != reflect.Ptr || iv.Kind() != reflect.Struct { @@ -25,7 +24,10 @@ func build(k *Kong, ast interface{}) (app *Application, err error) { for _, flag := range extraFlags { seenFlags[flag.Name] = true } - node := buildNode(k, iv, ApplicationNode, seenFlags) + node, err := buildNode(k, iv, ApplicationNode, seenFlags) + if err != nil { + return nil, err + } if len(node.Positional) > 0 && len(node.Children) > 0 { return nil, fmt.Errorf("can't mix positional arguments and branching arguments on %T", ast) } @@ -46,12 +48,15 @@ type flattenedField struct { tag *Tag } -func flattenedFields(v reflect.Value) (out []flattenedField) { +func flattenedFields(v reflect.Value) (out []flattenedField, err error) { v = reflect.Indirect(v) for i := 0; i < v.NumField(); i++ { ft := v.Type().Field(i) fv := v.Field(i) - tag := parseTag(v, ft) + tag, err := parseTag(v, ft) + if err != nil { + return nil, err + } if tag.Ignored { continue } @@ -67,11 +72,18 @@ func flattenedFields(v reflect.Value) (out []flattenedField) { fv = fv.Elem() } else if fv.Type() == reflect.TypeOf(Plugins{}) { for i := 0; i < fv.Len(); i++ { - out = append(out, flattenedFields(fv.Index(i).Elem())...) + fields, ferr := flattenedFields(fv.Index(i).Elem()) + if ferr != nil { + return nil, ferr + } + out = append(out, fields...) } continue } - sub := flattenedFields(fv) + sub, err := flattenedFields(fv) + if err != nil { + return nil, err + } for _, subf := range sub { // Assign parent if it's not already set. if subf.tag.Group == "" { @@ -84,16 +96,20 @@ func flattenedFields(v reflect.Value) (out []flattenedField) { } out = append(out, sub...) } - return out + return out, nil } -func buildNode(k *Kong, v reflect.Value, typ NodeType, seenFlags map[string]bool) *Node { +func buildNode(k *Kong, v reflect.Value, typ NodeType, seenFlags map[string]bool) (*Node, error) { node := &Node{ Type: typ, Target: v, Tag: newEmptyTag(), } - for _, field := range flattenedFields(v) { + fields, err := flattenedFields(v) + if err != nil { + return nil, err + } + for _, field := range fields { ft := field.field fv := field.value @@ -111,9 +127,12 @@ func buildNode(k *Kong, v reflect.Value, typ NodeType, seenFlags map[string]bool if tag.Arg { typ = ArgumentNode } - buildChild(k, node, typ, v, ft, fv, tag, name, seenFlags) + err = buildChild(k, node, typ, v, ft, fv, tag, name, seenFlags) } else { - buildField(k, node, v, ft, fv, tag, name, seenFlags) + err = buildField(k, node, v, ft, fv, tag, name, seenFlags) + } + if err != nil { + return nil, err } } @@ -129,18 +148,21 @@ func buildNode(k *Kong, v reflect.Value, typ NodeType, seenFlags map[string]bool last := true for i, p := range node.Positional { if !last && p.Required { - fail("argument %q can not be required after an optional", p.Name) + return nil, fmt.Errorf("argument %q can not be required after an optional", p.Name) } last = p.Required p.Position = i } - return node + return node, nil } -func buildChild(k *Kong, node *Node, typ NodeType, v reflect.Value, ft reflect.StructField, fv reflect.Value, tag *Tag, name string, seenFlags map[string]bool) { - child := buildNode(k, fv, typ, seenFlags) +func buildChild(k *Kong, node *Node, typ NodeType, v reflect.Value, ft reflect.StructField, fv reflect.Value, tag *Tag, name string, seenFlags map[string]bool) error { + child, err := buildNode(k, fv, typ, seenFlags) + if err != nil { + return err + } child.Name = name child.Tag = tag child.Parent = node @@ -157,10 +179,10 @@ func buildChild(k *Kong, node *Node, typ NodeType, v reflect.Value, ft reflect.S // a positional argument is provided to the child, and move it to the branching argument field. if tag.Arg { if len(child.Positional) == 0 { - failField(v, ft, "positional branch must have at least one child positional argument named %q", name) + return failField(v, ft, "positional branch must have at least one child positional argument named %q", name) } if child.Positional[0].Name != name { - failField(v, ft, "first field in positional branch must have the same name as the parent field (%s).", child.Name) + return failField(v, ft, "first field in positional branch must have the same name as the parent field (%s).", child.Name) } child.Argument = child.Positional[0] @@ -170,24 +192,26 @@ func buildChild(k *Kong, node *Node, typ NodeType, v reflect.Value, ft reflect.S } } else if tag.Default != "" { if node.DefaultCmd != nil { - failField(v, ft, "can't have more than one default command under %s", node.Summary()) + return failField(v, ft, "can't have more than one default command under %s", node.Summary()) } if tag.Default != "withargs" && (len(child.Children) > 0 || len(child.Positional) > 0) { - failField(v, ft, "default command %s must not have subcommands or arguments", child.Summary()) + return failField(v, ft, "default command %s must not have subcommands or arguments", child.Summary()) } node.DefaultCmd = child } node.Children = append(node.Children, child) if len(child.Positional) > 0 && len(child.Children) > 0 { - failField(v, ft, "can't mix positional arguments and branching arguments") + return failField(v, ft, "can't mix positional arguments and branching arguments") } + + return nil } -func buildField(k *Kong, node *Node, v reflect.Value, ft reflect.StructField, fv reflect.Value, tag *Tag, name string, seenFlags map[string]bool) { +func buildField(k *Kong, node *Node, v reflect.Value, ft reflect.StructField, fv reflect.Value, tag *Tag, name string, seenFlags map[string]bool) error { mapper := k.registry.ForNamedValue(tag.Type, fv) if mapper == nil { - failField(v, ft, "unsupported field type %s, perhaps missing a cmd:\"\" tag?", ft.Type) + return failField(v, ft, "unsupported field type %s, perhaps missing a cmd:\"\" tag?", ft.Type) } value := &Value{ @@ -210,16 +234,14 @@ func buildField(k *Kong, node *Node, v reflect.Value, ft reflect.StructField, fv node.Positional = append(node.Positional, value) } else { if seenFlags["--"+value.Name] { - failField(v, ft, "duplicate flag --%s", value.Name) - } else { - seenFlags["--"+value.Name] = true + return failField(v, ft, "duplicate flag --%s", value.Name) } + seenFlags["--"+value.Name] = true if tag.Short != 0 { if seenFlags["-"+string(tag.Short)] { - failField(v, ft, "duplicate short flag -%c", tag.Short) - } else { - seenFlags["-"+string(tag.Short)] = true + return failField(v, ft, "duplicate short flag -%c", tag.Short) } + seenFlags["-"+string(tag.Short)] = true } flag := &Flag{ Value: value, @@ -233,6 +255,7 @@ func buildField(k *Kong, node *Node, v reflect.Value, ft reflect.StructField, fv value.Flag = flag node.Flags = append(node.Flags, flag) } + return nil } func buildGroupForKey(k *Kong, key string) *Group { diff --git a/context.go b/context.go index 0080b16..83368d1 100644 --- a/context.go +++ b/context.go @@ -653,7 +653,6 @@ func (c *Context) Apply() (string, error) { } func (c *Context) parseFlag(flags []*Flag, match string) (err error) { - defer catch(&err) candidates := []string{} for _, flag := range flags { long := "--" + flag.Name @@ -734,7 +733,6 @@ func (c *Context) RunNode(node *Node, binds ...interface{}) (err error) { // Any passed values will be bindable to arguments of the target Run() method. Additionally, // all parent nodes in the command structure will be bound. func (c *Context) Run(binds ...interface{}) (err error) { - defer catch(&err) node := c.Selected() if node == nil { return fmt.Errorf("no command selected") diff --git a/global_test.go b/global_test.go index ed0ee83..6f97302 100644 --- a/global_test.go +++ b/global_test.go @@ -21,7 +21,7 @@ func TestParseHandlingBadBuild(t *testing.T) { defer func() { if r := recover(); r != nil { - require.Equal(t, Error{msg: "fail=' is not quoted properly"}, r) + require.Equal(t, "fail=' is not quoted properly", r.(error).Error()) } }() diff --git a/guesswidth_unix.go b/guesswidth_unix.go index 41d54e6..db52595 100644 --- a/guesswidth_unix.go +++ b/guesswidth_unix.go @@ -1,3 +1,4 @@ +//go:build (!appengine && linux) || freebsd || darwin || dragonfly || netbsd || openbsd // +build !appengine,linux freebsd darwin dragonfly netbsd openbsd package kong diff --git a/kong.go b/kong.go index aa60c91..dd4b966 100644 --- a/kong.go +++ b/kong.go @@ -13,22 +13,12 @@ var ( callbackReturnSignature = reflect.TypeOf((*error)(nil)).Elem() ) -// Error reported by Kong. -type Error struct{ msg string } - -func (e Error) Error() string { return e.msg } - -func fail(format string, args ...interface{}) { - panic(Error{msg: fmt.Sprintf(format, args...)}) -} - -func failField(parent reflect.Value, field reflect.StructField, format string, args ...interface{}) { +func failField(parent reflect.Value, field reflect.StructField, format string, args ...interface{}) error { name := parent.Type().Name() if name == "" { name = "" } - msg := fmt.Sprintf("%s.%s: %s", name, field.Name, fmt.Sprintf(format, args...)) - panic(Error{msg: msg}) + return fmt.Errorf("%s.%s: %s", name, field.Name, fmt.Sprintf(format, args...)) } // Must creates a new Parser or panics if there is an error. @@ -118,16 +108,22 @@ func New(grammar interface{}, options ...Option) (*Kong, error) { // Synthesise command nodes. for _, dcmd := range k.dynamicCommands { - tag := parseTagString(strings.Join(dcmd.tags, " ")) + tag, terr := parseTagString(strings.Join(dcmd.tags, " ")) + if terr != nil { + return nil, terr + } tag.Name = dcmd.name tag.Help = dcmd.help tag.Group = dcmd.group tag.Cmd = true v := reflect.Indirect(reflect.ValueOf(dcmd.cmd)) - buildChild(k, k.Model.Node, CommandNode, reflect.Value{}, reflect.StructField{ + err = buildChild(k, k.Model.Node, CommandNode, reflect.Value{}, reflect.StructField{ Name: dcmd.name, Type: v.Type(), }, v, tag, dcmd.name, map[string]bool{}) + if err != nil { + return nil, err + } } for _, option := range k.postBuildOptions { @@ -231,7 +227,6 @@ func (k *Kong) extraFlags() []*Flag { // Will return a ParseError if a *semantically* invalid command-line is encountered (as opposed to a syntactically // invalid one, which will report a normal error). func (k *Kong) Parse(args []string) (ctx *Context, err error) { - defer catch(&err) ctx, err = Trace(k, args) if err != nil { return nil, err @@ -398,12 +393,3 @@ func (k *Kong) LoadConfig(path string) (Resolver, error) { return k.loader(r) } - -func catch(err *error) { - msg := recover() - if test, ok := msg.(Error); ok { - *err = test - } else if msg != nil { - panic(msg) - } -} diff --git a/model.go b/model.go index aed7c35..bc7f37e 100644 --- a/model.go +++ b/model.go @@ -317,16 +317,6 @@ func (v *Value) IsCounter() bool { // Parse tokens into value, parse, and validate, but do not write to the field. func (v *Value) Parse(scan *Scanner, target reflect.Value) (err error) { - defer func() { - if rerr := recover(); rerr != nil { - switch rerr := rerr.(type) { - case Error: - err = errors.Wrap(rerr, v.ShortSummary()) - default: - panic(fmt.Sprintf("mapper %T failed to apply to %s: %s", v.Mapper, v.Summary(), rerr)) - } - } - }() err = v.Mapper.Decode(&DecodeContext{Value: v, Scan: scan}, target) if err != nil { return errors.Wrap(err, v.ShortSummary()) diff --git a/tag.go b/tag.go index c30052b..84c068d 100644 --- a/tag.go +++ b/tag.go @@ -48,7 +48,7 @@ type tagChars struct { var kongChars = tagChars{sep: ',', quote: '\'', assign: '='} var bareChars = tagChars{sep: ' ', quote: '"', assign: ':'} -func parseTagItems(tagString string, chr tagChars) map[string][]string { +func parseTagItems(tagString string, chr tagChars) (map[string][]string, error) { d := map[string][]string{} key := []rune{} value := []rune{} @@ -91,11 +91,10 @@ func parseTagItems(tagString string, chr tagChars) map[string][]string { if next == chr.sep || eof { continue } - fail("%v has an unexpected char at pos %v", tagString, idx) - } else { - quotes = true - continue + return nil, fmt.Errorf("%v has an unexpected char at pos %v", tagString, idx) } + quotes = true + continue } if inKey { key = append(key, r) @@ -104,12 +103,12 @@ func parseTagItems(tagString string, chr tagChars) map[string][]string { } } if quotes { - fail("%v is not quoted properly", tagString) + return nil, fmt.Errorf("%v is not quoted properly", tagString) } add() - return d + return d, nil } func getTagInfo(ft reflect.StructField) (string, tagChars) { @@ -129,31 +128,39 @@ func tagSplitFn(r rune) bool { return r == ',' || r == ' ' } -func parseTagString(s string) *Tag { - t := &Tag{ - items: parseTagItems(s, bareChars), - } - err := hydrateTag(t, "", false) +func parseTagString(s string) (*Tag, error) { + items, err := parseTagItems(s, bareChars) if err != nil { - fail("%s: %s", s, err) + return nil, err } - return t + t := &Tag{ + items: items, + } + err = hydrateTag(t, "", false) + if err != nil { + return nil, fmt.Errorf("%s: %s", s, err) + } + return t, nil } -func parseTag(parent reflect.Value, ft reflect.StructField) *Tag { +func parseTag(parent reflect.Value, ft reflect.StructField) (*Tag, error) { if ft.Tag.Get("kong") == "-" { t := newEmptyTag() t.Ignored = true - return t + return t, nil + } + items, err := parseTagItems(getTagInfo(ft)) + if err != nil { + return nil, err } t := &Tag{ - items: parseTagItems(getTagInfo(ft)), + items: items, } - err := hydrateTag(t, ft.Type.Name(), ft.Type.Kind() == reflect.Bool) + err = hydrateTag(t, ft.Type.Name(), ft.Type.Kind() == reflect.Bool) if err != nil { - failField(parent, ft, "%s", err) + return nil, failField(parent, ft, "%s", err) } - return t + return t, nil } func hydrateTag(t *Tag, typeName string, isBool bool) error {