internal/lsp: use -modfile for import organization
Also add a regression test.
Fixes golang/go#39681.
Change-Id: I685fbe4255b9e8e067dacd84a6d464d0a2bbe1f1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/238737
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 99c3910..6b4d847 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -84,7 +84,7 @@
defer done()
cfg := s.config(ctx)
- var cleanup func()
+ cleanup := func() {}
if s.view.tmpMod {
modFH, err := s.GetFile(ctx, s.view.modURI)
if err != nil {
@@ -97,17 +97,15 @@
return err
}
}
- var uri span.URI
- uri, cleanup, err = tempModFile(modFH, sumFH)
+ var tmpURI span.URI
+ tmpURI, cleanup, err = tempModFile(modFH, sumFH)
if err != nil {
return err
}
- cfg.BuildFlags = append(cfg.BuildFlags, fmt.Sprintf("-modfile=%s", uri.Filename()))
+ cfg.BuildFlags = append(cfg.BuildFlags, fmt.Sprintf("-modfile=%s", tmpURI.Filename()))
}
pkgs, err := packages.Load(cfg, query...)
- if cleanup != nil {
- cleanup()
- }
+ cleanup()
// If the context was canceled, return early. Otherwise, we might be
// type-checking an incomplete result. Check the context directly,
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index fef1c31..4c27c0c 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -62,7 +62,10 @@
// importsMu guards imports-related state, particularly the ProcessEnv.
importsMu sync.Mutex
- // process is the process env for this view.
+
+ // processEnv is the process env for this view.
+ // Some of its fields can be changed dynamically by modifications to
+ // the view's options. These fields are repopulated for every use.
// Note: this contains cached module and filesystem state.
//
// TODO(suzmue): the state cached in the process env is specific to each view,
@@ -361,22 +364,45 @@
v.importsMu.Lock()
defer v.importsMu.Unlock()
+ // The resolver cached in the process env is reused, but some fields need
+ // to be repopulated for each use.
if v.processEnv == nil {
- var err error
- if v.processEnv, err = v.buildProcessEnv(ctx); err != nil {
- return err
- }
+ v.processEnv = &imports.ProcessEnv{}
}
- // In module mode, check if the mod file has changed.
- if v.modURI != "" {
- mod, err := v.session.cache.GetFile(ctx, v.modURI)
+ var modFH, sumFH source.FileHandle
+ if v.tmpMod {
+ var err error
+ // Use temporary go.mod files, but always go to disk for the contents.
+ // Rebuilding the cache is expensive, and we don't want to do it for
+ // transient changes.
+ modFH, err = v.session.cache.GetFile(ctx, v.modURI)
if err != nil {
return err
}
- if mod.Identity() != v.cachedModFileVersion {
+ if v.sumURI != "" {
+ sumFH, err = v.session.cache.GetFile(ctx, v.sumURI)
+ if err != nil {
+ return err
+ }
+ }
+ }
+
+ cleanup, err := v.populateProcessEnv(ctx, modFH, sumFH)
+ if err != nil {
+ return err
+ }
+ defer cleanup()
+
+ // If the go.mod file has changed, clear the cache.
+ if v.modURI != "" {
+ modFH, err := v.session.cache.GetFile(ctx, v.modURI)
+ if err != nil {
+ return err
+ }
+ if modFH.Identity() != v.cachedModFileVersion {
v.processEnv.GetResolver().(*imports.ModuleResolver).ClearForNewMod()
- v.cachedModFileVersion = mod.Identity()
+ v.cachedModFileVersion = modFH.Identity()
}
}
@@ -431,20 +457,37 @@
v.importsMu.Unlock()
}
-func (v *View) buildProcessEnv(ctx context.Context) (*imports.ProcessEnv, error) {
+// populateProcessEnv sets the dynamically configurable fields for the view's
+// process environment. It operates on a snapshot because it needs to access
+// file contents. Assumes that the caller is holding the s.view.importsMu.
+func (v *View) populateProcessEnv(ctx context.Context, modFH, sumFH source.FileHandle) (cleanup func(), err error) {
+ cleanup = func() {}
+
v.optionsMu.Lock()
env, buildFlags := v.envLocked()
localPrefix, verboseOutput := v.options.LocalPrefix, v.options.VerboseOutput
v.optionsMu.Unlock()
- processEnv := &imports.ProcessEnv{
- WorkingDir: v.folder.Filename(),
- BuildFlags: buildFlags,
- LocalPrefix: localPrefix,
- GocmdRunner: v.gocmdRunner,
+ pe := v.processEnv
+
+ pe.BuildFlags = buildFlags
+
+ // Add -modfile to the build flags, if we are using it.
+ if modFH != nil {
+ var tmpURI span.URI
+ tmpURI, cleanup, err = tempModFile(modFH, sumFH)
+ if err != nil {
+ return nil, err
+ }
+ pe.BuildFlags = append(pe.BuildFlags, fmt.Sprintf("-modfile=%s", tmpURI.Filename()))
}
+
+ pe.WorkingDir = v.folder.Filename()
+ pe.LocalPrefix = localPrefix
+ pe.GocmdRunner = v.gocmdRunner
+
if verboseOutput {
- processEnv.Logf = func(format string, args ...interface{}) {
+ pe.Logf = func(format string, args ...interface{}) {
event.Log(ctx, fmt.Sprintf(format, args...))
}
}
@@ -455,23 +498,23 @@
}
switch split[0] {
case "GOPATH":
- processEnv.GOPATH = split[1]
+ pe.GOPATH = split[1]
case "GOROOT":
- processEnv.GOROOT = split[1]
+ pe.GOROOT = split[1]
case "GO111MODULE":
- processEnv.GO111MODULE = split[1]
+ pe.GO111MODULE = split[1]
case "GOPROXY":
- processEnv.GOPROXY = split[1]
+ pe.GOPROXY = split[1]
case "GOFLAGS":
- processEnv.GOFLAGS = split[1]
+ pe.GOFLAGS = split[1]
case "GOSUMDB":
- processEnv.GOSUMDB = split[1]
+ pe.GOSUMDB = split[1]
}
}
- if processEnv.GOPATH == "" {
+ if pe.GOPATH == "" {
return nil, fmt.Errorf("no GOPATH for view %s", v.folder)
}
- return processEnv, nil
+ return cleanup, nil
}
// envLocked returns the environment and build flags for the current view.
diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go
index a2285be..6dc9aed 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -70,12 +70,12 @@
// Ensure that the reports returned from mod.Diagnostics are only related
// to the go.mod file for the module.
if len(reports) > 1 {
- panic("unexpected reports from mod.Diagnostics")
+ panic(fmt.Sprintf("expected 1 report from mod.Diagnostics, got %v: %v", len(reports), reports))
}
modURI := snapshot.View().ModFile()
for id, diags := range reports {
if id.URI != modURI {
- panic("unexpected reports from mod.Diagnostics")
+ panic(fmt.Sprintf("expected module diagnostics report for %q, got %q", modURI, id.URI))
}
key := diagnosticKey{
id: id,
diff --git a/internal/lsp/regtest/modfile_test.go b/internal/lsp/regtest/modfile_test.go
new file mode 100644
index 0000000..47a0ed2
--- /dev/null
+++ b/internal/lsp/regtest/modfile_test.go
@@ -0,0 +1,60 @@
+// 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 regtest
+
+import (
+ "testing"
+
+ "golang.org/x/tools/internal/lsp/tests"
+ "golang.org/x/tools/internal/testenv"
+)
+
+func TestModFileModification(t *testing.T) {
+ testenv.NeedsGo1Point(t, 14)
+
+ const proxy = `
+-- example.com@v1.2.3/go.mod --
+module example.com
+
+go 1.12
+-- example.com@v1.2.3/blah/blah.go --
+package blah
+
+const Name = "Blah"
+`
+ const untidyModule = `
+-- go.mod --
+module mod.com
+
+-- main.go --
+package main
+
+import "example.com/blah"
+
+func main() {
+ fmt.Println(blah.Name)
+}`
+ runner.Run(t, untidyModule, func(t *testing.T, env *Env) {
+ // Open the file and make sure that the initial workspace load does not
+ // modify the go.mod file.
+ goModContent := env.ReadWorkspaceFile("go.mod")
+ env.OpenFile("main.go")
+ env.Await(
+ env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
+ )
+ if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
+ t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(want, got))
+ }
+ // Save the buffer, which will format and organize imports.
+ // Confirm that the go.mod file still does not change.
+ env.SaveBuffer("main.go")
+ env.Await(
+ env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
+ )
+ if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
+ t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(want, got))
+ }
+ }, WithProxy(proxy))
+}
diff --git a/internal/lsp/regtest/wrappers.go b/internal/lsp/regtest/wrappers.go
index f1c7502..827dcdb 100644
--- a/internal/lsp/regtest/wrappers.go
+++ b/internal/lsp/regtest/wrappers.go
@@ -34,8 +34,8 @@
return content
}
-// RemoveFileFromWorkspace deletes a file on disk but does nothing in the
-// editor. It calls t.Fatal on any error.
+// WriteWorkspaceFile writes a file to disk but does nothing in the editor.
+// It calls t.Fatal on any error.
func (e *Env) WriteWorkspaceFile(name, content string) {
e.T.Helper()
if err := e.Sandbox.Workdir.WriteFile(e.Ctx, name, content); err != nil {
