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 <[email protected]> Reviewed-by: Robert Findley <[email protected]> Auto-Submit: Alan Donovan <[email protected]>
golang · Nov 6, 2024 · 1115af6 · 1115af6
1 parent 0b9e499
commit 1115af6
Show file tree

Hide file tree

Showing 4 changed files with 85 additions and 45 deletions.
diff --git 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
@@ -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
@@ -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,20 +272,38 @@ 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())
 		return nil
 	}
 }
 
-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,65 +312,72 @@ 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()
 		v, err := strconv.ParseInt(s, 0, 0)
 		if err != nil {
 			t.Errorf("cannot convert %v to int: %v", s, err)
 		}
-		return v
+		value = v
 
 	case scanner.Float:
 		s := t.Consume()
 		v, err := strconv.ParseFloat(s, 64)
 		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
@@ -37,4 +37,6 @@ check(StringAndInt,
 )
 
 check(Bool, true)
+
+check(NamedArgs, 1, true, a, b=1, c="3", d=true)
 */