internal/lsp/source: factor out enum options pattern

Factor out processing of string enums when setting options results. In a
couple places this will result in errors being detected for invalid
values where they weren't before. Interestingly the default branch for
symbolMatcher was 'caseInsensitive', when in fact the default matcher in
the absence of any setting is 'fuzzy'. The command tests were implicitly
relying on this bug, passing 'default' to mean 'caseInsensitive'. Fix
this.

Also add a test, since option processing is not trivial.

Change-Id: Ib3ec4b1619add4c7315b6a689ca257e068da6027
Reviewed-on: https://go-review.googlesource.com/c/tools/+/258658
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Trust: Robert Findley <rfindley@google.com>

internal/lsp/cmd/cmd.go

@@ -263,7 +263,7 @@ func (app *Application) connectRemote(ctx context.Context, remote string) (*conn
 var matcherString = map[source.SymbolMatcher]string{
 	source.SymbolFuzzy:           "fuzzy",
 	source.SymbolCaseSensitive:   "caseSensitive",
-	source.SymbolCaseInsensitive: "default",
+	source.SymbolCaseInsensitive: "caseInsensitive",
 }
 
 func (c *connection) initialize(ctx context.Context, options func(*source.Options)) error {

internal/lsp/cmd/test/workspace_symbol.go

@@ -14,7 +14,7 @@ import (
 )
 
 func (r *runner) WorkspaceSymbols(t *testing.T, query string, expectedSymbols []protocol.SymbolInformation, dirs map[string]struct{}) {
-	r.runWorkspaceSymbols(t, "default", query, dirs)
+	r.runWorkspaceSymbols(t, "caseInsensitive", query, dirs)
 }
 
 func (r *runner) FuzzyWorkspaceSymbols(t *testing.T, query string, expectedSymbols []protocol.SymbolInformation, dirs map[string]struct{}) {
@@ -51,9 +51,11 @@ func (r *runner) runWorkspaceSymbols(t *testing.T, matcher, query string, dirs m
 }
 
 var workspaceSymbolsDir = map[string]string{
-	"default":       "",
-	"fuzzy":         "fuzzy",
-	"caseSensitive": "casesensitive",
+	// TODO: make caseInsensitive test cases consistent with
+	// other matcher.
+	"caseInsensitive": "",
+	"fuzzy":           "fuzzy",
+	"caseSensitive":   "casesensitive",
 }
 
 func workspaceSymbolsGolden(matcher, query string) string {

internal/lsp/source/options.go

@@ -574,67 +574,41 @@ func (o *Options) set(name string, value interface{}) OptionResult {
 	case "completionBudget":
 		result.setDuration(&o.CompletionBudget)
 	case "matcher":
-		matcher, ok := result.asString()
-		if !ok {
-			break
-		}
-		switch strings.ToLower(matcher) {
-		case "fuzzy":
-			o.Matcher = Fuzzy
-		case "casesensitive":
-			o.Matcher = CaseSensitive
-		default:
-			o.Matcher = CaseInsensitive
+		if s, ok := result.asOneOf(
+			string(Fuzzy),
+			string(CaseSensitive),
+			string(CaseInsensitive),
+		); ok {
+			o.Matcher = Matcher(s)
 		}
 
 	case "symbolMatcher":
-		matcher, ok := result.asString()
-		if !ok {
-			break
-		}
-		switch strings.ToLower(matcher) {
-		case "fuzzy":
-			o.SymbolMatcher = SymbolFuzzy
-		case "casesensitive":
-			o.SymbolMatcher = SymbolCaseSensitive
-		default:
-			o.SymbolMatcher = SymbolCaseInsensitive
+		if s, ok := result.asOneOf(
+			string(SymbolFuzzy),
+			string(SymbolCaseInsensitive),
+			string(SymbolCaseSensitive),
+		); ok {
+			o.SymbolMatcher = SymbolMatcher(s)
 		}
 
 	case "symbolStyle":
-		style, ok := result.asString()
-		if !ok {
-			break
-		}
-		switch strings.ToLower(style) {
-		case "full":
-			o.SymbolStyle = FullyQualifiedSymbols
-		case "dynamic":
-			o.SymbolStyle = DynamicSymbols
-		case "package":
-			o.SymbolStyle = PackageQualifiedSymbols
-		default:
-			result.errorf("Unsupported symbol style %q", style)
+		if s, ok := result.asOneOf(
+			string(FullyQualifiedSymbols),
+			string(PackageQualifiedSymbols),
+			string(DynamicSymbols),
+		); ok {
+			o.SymbolStyle = SymbolStyle(s)
 		}
 
 	case "hoverKind":
-		hoverKind, ok := result.asString()
-		if !ok {
-			break
-		}
-		switch strings.ToLower(hoverKind) {
-		case "nodocumentation":
-			o.HoverKind = NoDocumentation
-		case "singleline":
-			o.HoverKind = SingleLine
-		case "synopsisdocumentation":
-			o.HoverKind = SynopsisDocumentation
-		case "fulldocumentation":
-			o.HoverKind = FullDocumentation
-		case "structured":
-			o.HoverKind = Structured
-		default:
-			result.errorf("Unsupported hover kind %q", hoverKind)
+		if s, ok := result.asOneOf(
+			string(NoDocumentation),
+			string(SingleLine),
+			string(SynopsisDocumentation),
+			string(FullDocumentation),
+			string(Structured),
+		); ok {
+			o.HoverKind = HoverKind(s)
 		}
 
 	case "linkTarget":
@@ -644,15 +618,8 @@ func (o *Options) set(name string, value interface{}) OptionResult {
 		result.setBool(&o.LinksInHover)
 
 	case "importShortcut":
-		var s string
-		result.setString(&s)
-		switch strings.ToLower(s) {
-		case "both":
-			o.ImportShortcut = Both
-		case "link":
-			o.ImportShortcut = Link
-		case "definition":
-			o.ImportShortcut = Definition
+		if s, ok := result.asOneOf(string(Both), string(Link), string(Definition)); ok {
+			o.ImportShortcut = ImportShortcut(s)
 		}
 
 	case "analyses":
@@ -817,6 +784,21 @@ func (r *OptionResult) asString() (string, bool) {
 	return b, true
 }
 
+func (r *OptionResult) asOneOf(options ...string) (string, bool) {
+	s, ok := r.asString()
+	if !ok {
+		return "", false
+	}
+	lower := strings.ToLower(s)
+	for _, opt := range options {
+		if strings.ToLower(opt) == lower {
+			return opt, true
+		}
+	}
+	r.errorf("Invalid option %q for enum %q", r.Value, r.Name)
+	return "", false
+}
+
 func (r *OptionResult) setString(s *string) {
 	if v, ok := r.asString(); ok {
 		*s = v

internal/lsp/source/options_test.go

@@ -0,0 +1,70 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package source
+
+import (
+	"testing"
+	"time"
+)
+
+func TestSetOption(t *testing.T) {
+	tests := []struct {
+		name      string
+		value     interface{}
+		wantError bool
+		check     func(Options) bool
+	}{
+		{
+			name:  "symbolStyle",
+			value: "dynamic",
+			check: func(o Options) bool { return o.SymbolStyle == DynamicSymbols },
+		},
+		{
+			name:      "symbolStyle",
+			value:     "",
+			wantError: true,
+			check:     func(o Options) bool { return o.SymbolStyle == "" },
+		},
+		{
+			name:      "symbolStyle",
+			value:     false,
+			wantError: true,
+			check:     func(o Options) bool { return o.SymbolStyle == "" },
+		},
+		{
+			name:  "symbolMatcher",
+			value: "caseInsensitive",
+			check: func(o Options) bool { return o.SymbolMatcher == SymbolCaseInsensitive },
+		},
+		{
+			name:  "completionBudget",
+			value: "2s",
+			check: func(o Options) bool { return o.CompletionBudget == 2*time.Second },
+		},
+		{
+			name:  "staticcheck",
+			value: true,
+			check: func(o Options) bool { return o.Staticcheck == true },
+		},
+		{
+			name:  "codelens",
+			value: map[string]interface{}{"generate": true},
+			check: func(o Options) bool { return o.Codelens["generate"] },
+		},
+	}
+
+	for _, test := range tests {
+		var opts Options
+		result := opts.set(test.name, test.value)
+		if (result.Error != nil) != test.wantError {
+			t.Fatalf("Options.set(%q, %v): result.Error = %v, want error: %t", test.name, test.value, result.Error, test.wantError)
+		}
+		// TODO: this could be made much better using cmp.Diff, if that becomes
+		// available in this module.
+		if !test.check(opts) {
+			t.Errorf("Options.set(%q, %v): unexpected result %+v", test.name, test.value, opts)
+		}
+	}
+}