internal/lsp: lift env out of the sandbox into the editor

This removes some unncessary mangling of strings, and allows the editor
to fully own its configuration.

Change-Id: I0fde206824964124182c9138ee06fb670461d486
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244360
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

internal/lsp/fake/editor.go

@@ -31,6 +31,7 @@ type Editor struct {
 	serverConn jsonrpc2.Conn
 	client     *Client
 	sandbox    *Sandbox
+	defaultEnv map[string]string
 
 	// Since this editor is intended just for testing, we use very coarse
 	// locking.
@@ -57,7 +58,7 @@ func (b buffer) text() string {
 //
 // The zero value for EditorConfig should correspond to its defaults.
 type EditorConfig struct {
-	Env []string
+	Env map[string]string
 
 	// CodeLens is a map defining whether codelens are enabled, keyed by the
 	// codeLens command. CodeLens which are not present in this map are left in
@@ -70,11 +71,12 @@ type EditorConfig struct {
 }
 
 // NewEditor Creates a new Editor.
-func NewEditor(ws *Sandbox, config EditorConfig) *Editor {
+func NewEditor(sandbox *Sandbox, config EditorConfig) *Editor {
 	return &Editor{
-		buffers: make(map[string]buffer),
-		sandbox: ws,
-		Config:  config,
+		buffers:    make(map[string]buffer),
+		sandbox:    sandbox,
+		defaultEnv: sandbox.GoEnv(),
+		Config:     config,
 	}
 }
 
@@ -144,20 +146,23 @@ func (e *Editor) Client() *Client {
 	return e.client
 }
 
+func (e *Editor) overlayEnv() map[string]string {
+	env := make(map[string]string)
+	for k, v := range e.defaultEnv {
+		env[k] = v
+	}
+	for k, v := range e.Config.Env {
+		env[k] = v
+	}
+	return env
+}
+
 func (e *Editor) configuration() map[string]interface{} {
 	config := map[string]interface{}{
 		"verboseWorkDoneProgress": true,
+		"env":                     e.overlayEnv(),
 	}
 
-	envvars := e.sandbox.GoEnv()
-	envvars = append(envvars, e.Config.Env...)
-	env := map[string]interface{}{}
-	for _, value := range envvars {
-		kv := strings.SplitN(value, "=", 2)
-		env[kv[0]] = kv[1]
-	}
-	config["env"] = env
-
 	if e.Config.CodeLens != nil {
 		config["codelens"] = e.Config.CodeLens
 	}

internal/lsp/fake/sandbox.go

@@ -114,27 +114,31 @@ func (sb *Sandbox) GOPATH() string {
 
 // GoEnv returns the default environment variables that can be used for
 // invoking Go commands in the sandbox.
-func (sb *Sandbox) GoEnv() []string {
-	vars := []string{
-		"GOPATH=" + sb.GOPATH(),
-		"GOPROXY=" + sb.Proxy.GOPROXY(),
-		"GO111MODULE=",
-		"GOSUMDB=off",
-		"GOPACKAGESDRIVER=off",
+func (sb *Sandbox) GoEnv() map[string]string {
+	vars := map[string]string{
+		"GOPATH":           sb.GOPATH(),
+		"GOPROXY":          sb.Proxy.GOPROXY(),
+		"GO111MODULE":      "",
+		"GOSUMDB":          "off",
+		"GOPACKAGESDRIVER": "off",
 	}
 	if testenv.Go1Point() >= 5 {
-		vars = append(vars, "GOMODCACHE=")
+		vars["GOMODCACHE"] = ""
 	}
 	return vars
 }
 
 // RunGoCommand executes a go command in the sandbox.
 func (sb *Sandbox) RunGoCommand(ctx context.Context, verb string, args ...string) error {
+	var vars []string
+	for k, v := range sb.GoEnv() {
+		vars = append(vars, fmt.Sprintf("%s=%s", k, v))
+	}
 	inv := gocommand.Invocation{
 		Verb:       verb,
 		Args:       args,
 		WorkingDir: sb.Workdir.workdir,
-		Env:        sb.GoEnv(),
+		Env:        vars,
 	}
 	gocmdRunner := &gocommand.Runner{}
 	_, _, _, err := gocmdRunner.RunRaw(ctx, inv)

internal/lsp/regtest/diagnostics_test.go

@@ -430,7 +430,7 @@ func Hello() {
 
 // Tests golang/go#37984: GOPATH should be read from the go command.
 func TestNoGOPATH_Issue37984(t *testing.T) {
-	const missingImport = `
+	const files = `
 -- main.go --
 package main
 
@@ -438,17 +438,18 @@ func _() {
 	fmt.Println("Hello World")
 }
 `
-	runner.Run(t, missingImport, func(t *testing.T, env *Env) {
+	editorConfig := fake.EditorConfig{Env: map[string]string{"GOPATH": ""}}
+	withOptions(WithEditorConfig(editorConfig)).run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 		env.Await(env.DiagnosticAtRegexp("main.go", "fmt"))
 		env.SaveBuffer("main.go")
 		env.Await(EmptyDiagnostics("main.go"))
-	}, WithEditorConfig(fake.EditorConfig{Env: []string{"GOPATH="}}))
+	})
 }
 
 // Tests golang/go#38669.
 func TestEqualInEnv_Issue38669(t *testing.T) {
-	const missingImport = `
+	const files = `
 -- go.mod --
 module mod.com
 
@@ -461,11 +462,12 @@ package x
 
 var X = 0
 `
-	runner.Run(t, missingImport, func(t *testing.T, env *Env) {
+	editorConfig := fake.EditorConfig{Env: map[string]string{"GOFLAGS": "-tags=foo"}}
+	withOptions(WithEditorConfig(editorConfig)).run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 		env.OrganizeImports("main.go")
 		env.Await(EmptyDiagnostics("main.go"))
-	}, WithEditorConfig(fake.EditorConfig{Env: []string{"GOFLAGS=-tags=foo"}}))
+	})
 }
 
 // Tests golang/go#38467.

internal/lsp/regtest/imports_test.go

@@ -121,7 +121,7 @@ package y
 
 const Y = 2
 `
-	const ws = `
+	const files = `
 -- go.mod --
 module mod.com
 
@@ -141,8 +141,11 @@ var _, _ = x.X, y.Y
 		t.Fatal(err)
 	}
 	defer os.RemoveAll(modcache)
-
-	runner.Run(t, ws, func(t *testing.T, env *Env) {
+	editorConfig := fake.EditorConfig{Env: map[string]string{"GOMODCACHE": modcache}}
+	withOptions(
+		WithEditorConfig(editorConfig),
+		WithProxy(proxy),
+	).run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 		env.Await(env.DiagnosticAtRegexp("main.go", `y.Y`))
 		env.SaveBuffer("main.go")
@@ -151,5 +154,5 @@ var _, _ = x.X, y.Y
 		if !strings.HasPrefix(path, filepath.ToSlash(modcache)) {
 			t.Errorf("found module dependency outside of GOMODCACHE: got %v, wanted subdir of %v", path, filepath.ToSlash(modcache))
 		}
-	}, WithProxy(proxy), WithEditorConfig(fake.EditorConfig{Env: []string{"GOMODCACHE=" + modcache}}))
+	})
 }

internal/lsp/regtest/link_test.go

@@ -62,7 +62,7 @@ const Hello = "Hello"
 		}
 
 		// Then change the environment to make these links private.
-		env.ChangeEnv("GOPRIVATE=import.test")
+		env.ChangeEnv(map[string]string{"GOPRIVATE": "import.test"})
 
 		// Finally, verify that the links are gone.
 		content, _ = env.Hover("main.go", env.RegexpSearch("main.go", "pkg.Hello"))

internal/lsp/regtest/unix_test.go

@@ -13,7 +13,7 @@ import (
 )
 
 func TestBadGOPATH(t *testing.T) {
-	const missingImport = `
+	const files = `
 -- main.go --
 package main
 
@@ -21,15 +21,16 @@ func _() {
 	fmt.Println("Hello World")
 }
 `
+	editorConfig := fake.EditorConfig{
+		Env: map[string]string{"GOPATH": ":/path/to/gopath"},
+	}
 	// Test the case given in
 	// https://github.com/fatih/vim-go/issues/2673#issuecomment-622307211.
-	runner.Run(t, missingImport, func(t *testing.T, env *Env) {
+	withOptions(WithEditorConfig(editorConfig)).run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 		env.Await(env.DiagnosticAtRegexp("main.go", "fmt"))
 		if err := env.Editor.OrganizeImports(env.Ctx, "main.go"); err != nil {
 			t.Fatal(err)
 		}
-	}, WithEditorConfig(fake.EditorConfig{
-		Env: []string{"GOPATH=:/path/to/gopath"},
-	}))
+	})
 }

internal/lsp/regtest/wrappers.go

@@ -264,12 +264,17 @@ func (e *Env) CodeAction(path string) []protocol.CodeAction {
 // ChangeEnv modifies the editor environment and reconfigures the LSP client.
 // TODO: extend this to "ChangeConfiguration", once we refactor the way editor
 // configuration is defined.
-func (e *Env) ChangeEnv(envvars ...string) {
+func (e *Env) ChangeEnv(overlay map[string]string) {
 	e.T.Helper()
 	// TODO: to be correct, this should probably be synchronized, but right now
 	// configuration is only ever modified synchronously in a regtest, so this
 	// correctness can wait for the previously mentioned refactoring.
-	e.Editor.Config.Env = append(e.Editor.Config.Env, envvars...)
+	if e.Editor.Config.Env == nil {
+		e.Editor.Config.Env = make(map[string]string)
+	}
+	for k, v := range overlay {
+		e.Editor.Config.Env[k] = v
+	}
 	var params protocol.DidChangeConfigurationParams
 	if err := e.Editor.Server.DidChangeConfiguration(e.Ctx, &params); err != nil {
 		e.T.Fatal(err)