internal/lsp: fix env setting type

We stored the user's environment settings as a string slice, but users
set it as a map. It turns out that we have code spread around to convert
it both back and forth, so rather than hack up the doc generator,
so centralize that code and change the native representation to a map.
We end up with slightly less code.

Fixes golang/go#41964.

Change-Id: I975c69524c7d79c7d09079f44cc44a27e72ba285
Reviewed-on: https://go-review.googlesource.com/c/tools/+/262351
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

gopls/doc/settings.md

@@ -28,11 +28,11 @@ The most common use is to set `-tags`.
 
 
 Default: `[]`.
-### **env** *[]string*
+### **env** *map[string]string*
 env adds environment variables to external commands run by `gopls`, most notably `go list`.
 
 
-Default: `[]`.
+Default: `{}`.
 ### **hoverKind** *enum*
 hoverKind controls the information that appears in the hover text.
 SingleLine and Structured are intended for use only by authors of editor plugins.

internal/lsp/cache/view.go

@@ -248,20 +248,7 @@ func (v *View) Options() *source.Options {
 
 func minorOptionsChange(a, b *source.Options) bool {
 	// Check if any of the settings that modify our understanding of files have been changed
-	mapEnv := func(env []string) map[string]string {
-		m := make(map[string]string, len(env))
-		for _, x := range env {
-			split := strings.SplitN(x, "=", 2)
-			if len(split) != 2 {
-				continue
-			}
-			m[split[0]] = split[1]
-		}
-		return m
-	}
-	aEnv := mapEnv(a.Env)
-	bEnv := mapEnv(b.Env)
-	if !reflect.DeepEqual(aEnv, bEnv) {
+	if !reflect.DeepEqual(a.Env, b.Env) {
 		return false
 	}
 	aBuildFlags := make([]string, len(a.BuildFlags))
@@ -346,7 +333,7 @@ func (s *snapshot) RunProcessEnvFunc(ctx context.Context, fn func(*imports.Optio
 // envLocked returns the environment and build flags for the current view.
 // It assumes that the caller is holding the view's optionsMu.
 func (v *View) envLocked() ([]string, []string) {
-	env := append(os.Environ(), v.options.Env...)
+	env := append(os.Environ(), v.options.EnvSlice()...)
 	buildFlags := append([]string{}, v.options.BuildFlags...)
 	return env, buildFlags
 }
@@ -640,20 +627,14 @@ func (s *Session) getWorkspaceInformation(ctx context.Context, folder span.URI,
 		return nil, errors.Errorf("invalid workspace configuration: %w", err)
 	}
 	var err error
-	goversion, err := s.goVersion(ctx, folder.Filename(), options.Env)
+	goversion, err := s.goVersion(ctx, folder.Filename(), options.EnvSlice())
 	if err != nil {
 		return nil, err
 	}
 
 	go111module := os.Getenv("GO111MODULE")
-	for _, kv := range options.Env {
-		split := strings.SplitN(kv, "=", 2)
-		if len(split) != 2 {
-			continue
-		}
-		if split[0] == "GO111MODULE" {
-			go111module = split[1]
-		}
+	if v, ok := options.Env["GO111MODULE"]; ok {
+		go111module = v
 	}
 	// If using 1.16, change the default back to auto. The primary effect of
 	// GO111MODULE=on is to break GOPATH, which we aren't too interested in.
@@ -662,7 +643,7 @@ func (s *Session) getWorkspaceInformation(ctx context.Context, folder span.URI,
 	}
 
 	// Make sure to get the `go env` before continuing with initialization.
-	envVars, env, err := s.getGoEnv(ctx, folder.Filename(), append(options.Env, "GO111MODULE="+go111module))
+	envVars, env, err := s.getGoEnv(ctx, folder.Filename(), append(options.EnvSlice(), "GO111MODULE="+go111module))
 	if err != nil {
 		return nil, err
 	}

internal/lsp/lsp_test.go

@@ -48,7 +48,7 @@ func testLSP(t *testing.T, datum *tests.Data) {
 	options := source.DefaultOptions().Clone()
 	tests.DefaultOptions(options)
 	session.SetOptions(options)
-	options.Env = datum.Config.Env
+	options.SetEnvSlice(datum.Config.Env)
 	view, snapshot, release, err := session.NewView(ctx, datum.Config.Dir, span.URIFromPath(datum.Config.Dir), options)
 	if err != nil {
 		t.Fatal(err)

internal/lsp/mod/mod_test.go

@@ -31,7 +31,7 @@ func TestModfileRemainsUnchanged(t *testing.T) {
 	options := source.DefaultOptions().Clone()
 	tests.DefaultOptions(options)
 	options.TempModfile = true
-	options.Env = []string{"GOPACKAGESDRIVER=off", "GOROOT="}
+	options.Env = map[string]string{"GOPACKAGESDRIVER": "off", "GOROOT": ""}
 
 	// Make sure to copy the test directory to a temporary directory so we do not
 	// modify the test code or add go.sum files when we run the tests.

internal/lsp/source/options.go

@@ -180,7 +180,7 @@ type UserOptions struct {
 	BuildFlags []string
 
 	// Env adds environment variables to external commands run by `gopls`, most notably `go list`.
-	Env []string
+	Env map[string]string
 
 	// HoverKind controls the information that appears in the hover text.
 	// SingleLine and Structured are intended for use only by authors of editor plugins.
@@ -206,6 +206,27 @@ type UserOptions struct {
 	Gofumpt bool
 }
 
+// EnvSlice returns Env as a slice of k=v strings.
+func (u *UserOptions) EnvSlice() []string {
+	var result []string
+	for k, v := range u.Env {
+		result = append(result, fmt.Sprintf("%v=%v", k, v))
+	}
+	return result
+}
+
+// SetEnvSlice sets Env from a slice of k=v strings.
+func (u *UserOptions) SetEnvSlice(env []string) {
+	u.Env = map[string]string{}
+	for _, kv := range env {
+		split := strings.SplitN(kv, "=", 2)
+		if len(split) != 2 {
+			continue
+		}
+		u.Env[split[0]] = split[1]
+	}
+}
+
 // Hooks contains configuration that is provided to the Gopls command by the
 // main package.
 type Hooks struct {
@@ -542,7 +563,7 @@ func (o *Options) Clone() *Options {
 		copy(dst, src)
 		return dst
 	}
-	result.Env = copySlice(o.Env)
+	result.SetEnvSlice(o.EnvSlice())
 	result.BuildFlags = copySlice(o.BuildFlags)
 
 	copyAnalyzerMap := func(src map[string]Analyzer) map[string]Analyzer {
@@ -586,7 +607,7 @@ func (o *Options) set(name string, value interface{}) OptionResult {
 			break
 		}
 		for k, v := range menv {
-			o.Env = append(o.Env, fmt.Sprintf("%s=%s", k, v))
+			o.Env[k] = fmt.Sprint(v)
 		}
 
 	case "buildFlags":

internal/lsp/source/source_test.go

@@ -50,7 +50,7 @@ func testSource(t *testing.T, datum *tests.Data) {
 	session := cache.NewSession(ctx)
 	options := source.DefaultOptions().Clone()
 	tests.DefaultOptions(options)
-	options.Env = datum.Config.Env
+	options.SetEnvSlice(datum.Config.Env)
 	view, _, release, err := session.NewView(ctx, "source_test", span.URIFromPath(datum.Config.Dir), options)
 	release()
 	if err != nil {