From 1ff154e68b8e2af17cb306147eae32639468784f Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 22 Jul 2020 14:03:48 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/fake/editor.go | 33 ++++++++++++++---------- internal/lsp/fake/sandbox.go | 22 +++++++++------- internal/lsp/regtest/diagnostics_test.go | 14 +++++----- internal/lsp/regtest/imports_test.go | 11 +++++--- internal/lsp/regtest/link_test.go | 2 +- internal/lsp/regtest/unix_test.go | 11 ++++---- internal/lsp/regtest/wrappers.go | 9 +++++-- 7 files changed, 61 insertions(+), 41 deletions(-) diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go index 53cb5b4cb2..a2fbd3c24d 100644 --- a/internal/lsp/fake/editor.go +++ b/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 } diff --git a/internal/lsp/fake/sandbox.go b/internal/lsp/fake/sandbox.go index 96257d843f..3b3b005e92 100644 --- a/internal/lsp/fake/sandbox.go +++ b/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) diff --git a/internal/lsp/regtest/diagnostics_test.go b/internal/lsp/regtest/diagnostics_test.go index 0398fa7f36..d9d3053290 100644 --- a/internal/lsp/regtest/diagnostics_test.go +++ b/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. diff --git a/internal/lsp/regtest/imports_test.go b/internal/lsp/regtest/imports_test.go index adde1d8660..a1427c6e7d 100644 --- a/internal/lsp/regtest/imports_test.go +++ b/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}})) + }) } diff --git a/internal/lsp/regtest/link_test.go b/internal/lsp/regtest/link_test.go index 913abf42fe..51282feb24 100644 --- a/internal/lsp/regtest/link_test.go +++ b/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")) diff --git a/internal/lsp/regtest/unix_test.go b/internal/lsp/regtest/unix_test.go index c75bc4e723..e67de38516 100644 --- a/internal/lsp/regtest/unix_test.go +++ b/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"}, - })) + }) } diff --git a/internal/lsp/regtest/wrappers.go b/internal/lsp/regtest/wrappers.go index fc3566e5ee..02d564a600 100644 --- a/internal/lsp/regtest/wrappers.go +++ b/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, ¶ms); err != nil { e.T.Fatal(err)