From 1115af6fe6cfbcc9467f11ea249f1f25f6fb551e Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 6 Nov 2024 15:46:44 -0500 Subject: [PATCH] internal/expect: support named arguments f(a, b, c=d, e="f") + test Change-Id: I6c44e32d34bcdf1ca68e8989e99595f691c88329 Reviewed-on: https://go-review.googlesource.com/c/tools/+/626016 LUCI-TryBot-Result: Go LUCI Reviewed-by: Robert Findley Auto-Submit: Alan Donovan --- internal/expect/expect.go | 20 +++++---- internal/expect/expect_test.go | 39 ++++++++++++------ internal/expect/extract.go | 69 ++++++++++++++++++++++---------- internal/expect/testdata/test.go | 2 + 4 files changed, 85 insertions(+), 45 deletions(-) diff --git a/internal/expect/expect.go b/internal/expect/expect.go index fdc023c8924..d977ea4e262 100644 --- a/internal/expect/expect.go +++ b/internal/expect/expect.go @@ -42,7 +42,11 @@ comma-separated list of arguments. The empty parameter list and the missing parameter list are distinguishable if needed; they result in a nil or an empty list in the Args parameter respectively. -Arguments are either identifiers or literals. +Arguments may be positional, such as f(value), or named, such as f(name=value). +Positional arguments must appear before named arguments. +Names may not be repeated. + +Argument values may be either identifiers or literals. The literals supported are the basic value literals, of string, float, integer true, false or nil. All the literals match the standard go conventions, with all bases of integers, and both quote and backtick strings. @@ -62,9 +66,10 @@ import ( // It knows the position of the start of the comment, and the name and // arguments that make up the note. type Note struct { - Pos token.Pos // The position at which the note identifier appears - Name string // the name associated with the note - Args []interface{} // the arguments for the note + Pos token.Pos // The position at which the note identifier appears + Name string // the name associated with the note + Args []any // positional arguments (non-nil if parens were present) + NamedArgs map[string]any // named arguments (or nil if none) } // ReadFile is the type of a function that can provide file contents for a @@ -116,10 +121,3 @@ func MatchBefore(fset *token.FileSet, readFile ReadFile, end token.Pos, pattern } return f.Pos(startOffset + matchStart), f.Pos(startOffset + matchEnd), nil } - -func lineEnd(f *token.File, line int) token.Pos { - if line >= f.LineCount() { - return token.Pos(f.Base() + f.Size()) - } - return f.LineStart(line + 1) -} diff --git a/internal/expect/expect_test.go b/internal/expect/expect_test.go index 41cd2ac8512..3ad8d1a74fa 100644 --- a/internal/expect/expect_test.go +++ b/internal/expect/expect_test.go @@ -8,6 +8,8 @@ import ( "bytes" "go/token" "os" + "reflect" + "slices" "testing" "golang.org/x/tools/internal/expect" @@ -18,11 +20,13 @@ func TestMarker(t *testing.T) { filename string expectNotes int expectMarkers map[string]string - expectChecks map[string][]interface{} + expectChecks map[string][]any + // expectChecks holds {"id": values} for each call check(id, values...). + // Any named k=v arguments become a final map[string]any argument. }{ { filename: "testdata/test.go", - expectNotes: 13, + expectNotes: 14, expectMarkers: map[string]string{ "αSimpleMarker": "α", "OffsetMarker": "β", @@ -36,10 +40,15 @@ func TestMarker(t *testing.T) { "NonIdentifier": "+", "StringMarker": "\"hello\"", }, - expectChecks: map[string][]interface{}{ + expectChecks: map[string][]any{ "αSimpleMarker": nil, "StringAndInt": {"Number %d", int64(12)}, "Bool": {true}, + "NamedArgs": {int64(1), true, expect.Identifier("a"), map[string]any{ + "b": int64(1), + "c": "3", + "d": true, + }}, }, }, { @@ -79,7 +88,7 @@ func TestMarker(t *testing.T) { fset := token.NewFileSet() notes, err := expect.Parse(fset, tt.filename, content) if err != nil { - t.Fatalf("Failed to extract notes: %v", err) + t.Fatalf("Failed to extract notes:\n%v", err) } if len(notes) != tt.expectNotes { t.Errorf("Expected %v notes, got %v", tt.expectNotes, len(notes)) @@ -99,7 +108,7 @@ func TestMarker(t *testing.T) { } ident, ok := n.Args[0].(expect.Identifier) if !ok { - t.Errorf("%v: identifier, got %T", fset.Position(n.Pos), n.Args[0]) + t.Errorf("%v: got %v (%T), want identifier", fset.Position(n.Pos), n.Args[0], n.Args[0]) continue } checkMarker(t, fset, readFile, markers, n.Pos, string(ident), n.Args[1]) @@ -115,21 +124,27 @@ func TestMarker(t *testing.T) { } ident, ok := n.Args[0].(expect.Identifier) if !ok { - t.Errorf("%v: identifier, got %T", fset.Position(n.Pos), n.Args[0]) + t.Errorf("%v: got %v (%T), want identifier", fset.Position(n.Pos), n.Args[0], n.Args[0]) continue } - args, ok := tt.expectChecks[string(ident)] + wantArgs, ok := tt.expectChecks[string(ident)] if !ok { t.Errorf("%v: unexpected check %v", fset.Position(n.Pos), ident) continue } - if len(n.Args) != len(args)+1 { - t.Errorf("%v: expected %v args to check, got %v", fset.Position(n.Pos), len(args)+1, len(n.Args)) + gotArgs := n.Args[1:] + if n.NamedArgs != nil { + // Clip to avoid mutating Args' array. + gotArgs = append(slices.Clip(gotArgs), n.NamedArgs) + } + + if len(gotArgs) != len(wantArgs) { + t.Errorf("%v: expected %v args to check, got %v", fset.Position(n.Pos), len(wantArgs), len(gotArgs)) continue } - for i, got := range n.Args[1:] { - if args[i] != got { - t.Errorf("%v: arg %d expected %v, got %v", fset.Position(n.Pos), i, args[i], got) + for i := range gotArgs { + if !reflect.DeepEqual(wantArgs[i], gotArgs[i]) { + t.Errorf("%v: arg %d: expected %#v, got %#v", fset.Position(n.Pos), i+1, wantArgs[i], gotArgs[i]) } } default: diff --git a/internal/expect/extract.go b/internal/expect/extract.go index 1ca67d24958..db6b66aaf21 100644 --- a/internal/expect/extract.go +++ b/internal/expect/extract.go @@ -231,7 +231,7 @@ func parse(fset *token.FileSet, base token.Pos, text string) ([]*Note, error) { t := new(tokens).Init(base, text) notes := parseComment(t) if t.err != nil { - return nil, fmt.Errorf("%v:%s", fset.Position(t.Pos()), t.err) + return nil, fmt.Errorf("%v: %s", fset.Position(t.Pos()), t.err) } return notes, nil } @@ -272,7 +272,7 @@ func parseNote(t *tokens) *Note { // no argument list present return n case '(': - n.Args = parseArgumentList(t) + n.Args, n.NamedArgs = parseArgumentList(t) return n default: t.Errorf("unexpected %s parsing note", t.TokenString()) @@ -280,12 +280,30 @@ func parseNote(t *tokens) *Note { } } -func parseArgumentList(t *tokens) []interface{} { - args := []interface{}{} // @name() is represented by a non-nil empty slice. - t.Consume() // '(' +func parseArgumentList(t *tokens) (args []any, named map[string]any) { + args = []any{} // @name() is represented by a non-nil empty slice. + t.Consume() // '(' t.Skip('\n') for t.Token() != ')' { - args = append(args, parseArgument(t)) + name, arg := parseArgument(t) + if name != "" { + // f(k=v) + if named == nil { + named = make(map[string]any) + } + if _, dup := named[name]; dup { + t.Errorf("duplicate named argument %q", name) + return nil, nil + } + named[name] = arg + } else { + // f(v) + if named != nil { + t.Errorf("positional argument follows named argument") + return nil, nil + } + args = append(args, arg) + } if t.Token() != ',' { break } @@ -294,42 +312,50 @@ func parseArgumentList(t *tokens) []interface{} { } if t.Token() != ')' { t.Errorf("unexpected %s parsing argument list", t.TokenString()) - return nil + return nil, nil } t.Consume() // ')' - return args + return args, named } -func parseArgument(t *tokens) interface{} { +// parseArgument returns the value of the argument ("f(value)"), +// and its name if named "f(name=value)". +func parseArgument(t *tokens) (name string, value any) { +again: switch t.Token() { case scanner.Ident: v := t.Consume() switch v { case "true": - return true + value = true case "false": - return false + value = false case "nil": - return nil + value = nil case "re": if t.Token() != scanner.String && t.Token() != scanner.RawString { t.Errorf("re must be followed by string, got %s", t.TokenString()) - return nil + return } pattern, _ := strconv.Unquote(t.Consume()) // can't fail re, err := regexp.Compile(pattern) if err != nil { t.Errorf("invalid regular expression %s: %v", pattern, err) - return nil + return } - return re + value = re default: - return Identifier(v) + // f(name=value)? + if name == "" && t.Token() == '=' { + t.Consume() // '=' + name = v + goto again + } + value = Identifier(v) } case scanner.String, scanner.RawString: - v, _ := strconv.Unquote(t.Consume()) // can't fail - return v + value, _ = strconv.Unquote(t.Consume()) // can't fail case scanner.Int: s := t.Consume() @@ -337,7 +363,7 @@ func parseArgument(t *tokens) interface{} { if err != nil { t.Errorf("cannot convert %v to int: %v", s, err) } - return v + value = v case scanner.Float: s := t.Consume() @@ -345,14 +371,13 @@ func parseArgument(t *tokens) interface{} { if err != nil { t.Errorf("cannot convert %v to float: %v", s, err) } - return v + value = v case scanner.Char: t.Errorf("unexpected char literal %s", t.Consume()) - return nil default: t.Errorf("unexpected %s parsing argument", t.TokenString()) - return nil } + return } diff --git a/internal/expect/testdata/test.go b/internal/expect/testdata/test.go index f02a8f286a4..808864e7a91 100644 --- a/internal/expect/testdata/test.go +++ b/internal/expect/testdata/test.go @@ -37,4 +37,6 @@ check(StringAndInt, ) check(Bool, true) + +check(NamedArgs, 1, true, a, b=1, c="3", d=true) */