internal/lsp/regtest: allow sharing memoized results across regtests

Each regtest does a significant amount of extra work re-doing things like parsing and type-checking the runtime package. We can share this work across regtests by using a shared cache, significantly speeding them up at the cost of potentially hiding bugs related to timing. Sharing this work still retains most of the benefit of the regtests, so implement this in the default mode (formerly called "singleton" and now renamed to "default"). In a subsequent CL, modes will be cleaned up so that "default" is the only mode that runs with -short. Making this change actually revealed a caching bug: our cached package stores error messages extracted from go/packages errors, but does not include these errors in the cache key. Fix this by hashing all metadata errors into the package cache key. Updates golang/go#39384 Change-Id: I37ab9604149d34c9a79fc02b0e1bc23fcb17c454 Reviewed-on: https://go-review.googlesource.com/c/tools/+/417587 TryBot-Result: Gopher Robot <[email protected]> gopls-CI: kokoro <[email protected]> Run-TryBot: Robert Findley <[email protected]> Reviewed-by: Bryan Mills <[email protected]>
golang · Jul 26, 2022 · f157068 · f157068
1 parent 8ccb25c
commit f157068
Show file tree

Hide file tree

Showing 22 changed files with 182 additions and 80 deletions.
diff --git a/gopls/internal/regtest/bench/bench_test.go b/gopls/internal/regtest/bench/bench_test.go
@@ -33,7 +33,7 @@ func benchmarkOptions(dir string) []RunOption {
 		// Skip logs as they buffer up memory unnaturally.
 		SkipLogs(),
 		// The Debug server only makes sense if running in singleton mode.
-		Modes(Singleton),
+		Modes(Default),
 		// Remove the default timeout. Individual tests should control their
 		// own graceful termination.
 		NoDefaultTimeout(),

diff --git a/gopls/internal/regtest/debug/debug_test.go b/gopls/internal/regtest/debug/debug_test.go
@@ -20,7 +20,7 @@ func TestBugNotification(t *testing.T) {
 	// Verify that a properly configured session gets notified of a bug on the
 	// server.
 	WithOptions(
-		Modes(Singleton), // must be in-process to receive the bug report below
+		Modes(Default), // must be in-process to receive the bug report below
 		Settings{"showBugReports": true},
 	).Run(t, "", func(t *testing.T, env *Env) {
 		const desc = "got a bug"

diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go
@@ -298,7 +298,7 @@ func Hello() {
 
 	t.Run("without workspace module", func(t *testing.T) {
 		WithOptions(
-			Modes(Singleton),
+			Modes(Default),
 		).Run(t, noMod, func(t *testing.T, env *Env) {
 			env.Await(
 				env.DiagnosticAtRegexp("main.go", `"mod.com/bob"`),
@@ -1678,7 +1678,7 @@ import (
 		WithOptions(
 			InGOPATH(),
 			EnvVars{"GO111MODULE": "off"},
-			Modes(Singleton),
+			Modes(Default),
 		).Run(t, mod, func(t *testing.T, env *Env) {
 			env.Await(
 				env.DiagnosticAtRegexpWithMessage("main.go", `"nosuchpkg"`, `cannot find package "nosuchpkg" in any of`),
@@ -1705,7 +1705,7 @@ package b
 	for _, go111module := range []string{"on", "auto"} {
 		t.Run("GO111MODULE="+go111module, func(t *testing.T) {
 			WithOptions(
-				Modes(Singleton),
+				Modes(Default),
 				EnvVars{"GO111MODULE": go111module},
 			).Run(t, modules, func(t *testing.T, env *Env) {
 				env.OpenFile("a/a.go")
@@ -1722,7 +1722,7 @@ package b
 	// Expect no warning if GO111MODULE=auto in a directory in GOPATH.
 	t.Run("GOPATH_GO111MODULE_auto", func(t *testing.T) {
 		WithOptions(
-			Modes(Singleton),
+			Modes(Default),
 			EnvVars{"GO111MODULE": "auto"},
 			InGOPATH(),
 		).Run(t, modules, func(t *testing.T, env *Env) {
@@ -1784,7 +1784,7 @@ func helloHelper() {}
 `
 	WithOptions(
 		ProxyFiles(proxy),
-		Modes(Singleton),
+		Modes(Default),
 	).Run(t, nested, func(t *testing.T, env *Env) {
 		// Expect a diagnostic in a nested module.
 		env.OpenFile("nested/hello/hello.go")
@@ -1996,7 +1996,7 @@ func Hello() {}
 `
 	WithOptions(
 		Settings{"experimentalUseInvalidMetadata": true},
-		Modes(Singleton),
+		Modes(Default),
 	).Run(t, mod, func(t *testing.T, env *Env) {
 		env.OpenFile("go.mod")
 		env.RegexpReplace("go.mod", "module mod.com", "modul mod.com") // break the go.mod file
@@ -2052,7 +2052,7 @@ func _() {}
 		Settings{"experimentalUseInvalidMetadata": true},
 		// ExperimentalWorkspaceModule has a different failure mode for this
 		// case.
-		Modes(Singleton),
+		Modes(Default),
 	).Run(t, mod, func(t *testing.T, env *Env) {
 		env.Await(
 			OnceMet(

diff --git a/gopls/internal/regtest/misc/semantictokens_test.go b/gopls/internal/regtest/misc/semantictokens_test.go
@@ -25,7 +25,7 @@ func main() {}
 
 `
 	WithOptions(
-		Modes(Singleton),
+		Modes(Default),
 		Settings{"allExperiments": true},
 	).Run(t, src, func(t *testing.T, env *Env) {
 		params := &protocol.SemanticTokensParams{}

diff --git a/gopls/internal/regtest/misc/vendor_test.go b/gopls/internal/regtest/misc/vendor_test.go
@@ -49,7 +49,7 @@ func _() {
 }
 `
 	WithOptions(
-		Modes(Singleton),
+		Modes(Default),
 		ProxyFiles(basicProxy),
 	).Run(t, pkgThatUsesVendoring, func(t *testing.T, env *Env) {
 		env.OpenFile("a/a1.go")

diff --git a/gopls/internal/regtest/modfile/modfile_test.go b/gopls/internal/regtest/modfile/modfile_test.go
@@ -742,7 +742,7 @@ func main() {
 	WithOptions(
 		EnvVars{"GOFLAGS": "-mod=readonly"},
 		ProxyFiles(proxy),
-		Modes(Singleton),
+		Modes(Default),
 	).Run(t, mod, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 		original := env.ReadWorkspaceFile("go.mod")
@@ -922,7 +922,7 @@ func hello() {}
 		// TODO(rFindley) this doesn't work in multi-module workspace mode, because
 		// it keeps around the last parsing modfile. Update this test to also
 		// exercise the workspace module.
-		Modes(Singleton),
+		Modes(Default),
 	).Run(t, mod, func(t *testing.T, env *Env) {
 		env.OpenFile("go.mod")
 		env.Await(env.DoneWithOpen())
@@ -1090,7 +1090,7 @@ func main() {
 `
 	WithOptions(
 		ProxyFiles(workspaceProxy),
-		Modes(Singleton),
+		Modes(Default),
 	).Run(t, mod, func(t *testing.T, env *Env) {
 		env.OpenFile("go.mod")
 		params := &protocol.PublishDiagnosticsParams{}
@@ -1159,7 +1159,7 @@ func main() {
 `
 	WithOptions(
 		ProxyFiles(proxy),
-		Modes(Singleton),
+		Modes(Default),
 	).Run(t, mod, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 		d := &protocol.PublishDiagnosticsParams{}

diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go
@@ -1205,7 +1205,7 @@ package main
 `
 	WithOptions(
 		EnvVars{"GOPATH": filepath.FromSlash("$SANDBOX_WORKDIR/gopath")},
-		Modes(Singleton),
+		Modes(Default),
 	).Run(t, mod, func(t *testing.T, env *Env) {
 		env.Await(
 			// Confirm that the build configuration is seen as valid,
@@ -1236,7 +1236,7 @@ package main
 func main() {}
 `
 	WithOptions(
-		Modes(Singleton),
+		Modes(Default),
 	).Run(t, nomod, func(t *testing.T, env *Env) {
 		env.OpenFile("a/main.go")
 		env.OpenFile("b/main.go")

diff --git a/gopls/internal/vulncheck/command_test.go b/gopls/internal/vulncheck/command_test.go
@@ -293,7 +293,7 @@ func runTest(t *testing.T, workspaceData, proxyData string, test func(context.Co
 		t.Fatal(err)
 	}
 
-	cache := cache.New(nil)
+	cache := cache.New(nil, nil, nil)
 	session := cache.NewSession(ctx)
 	options := source.DefaultOptions().Clone()
 	tests.DefaultOptions(options)

diff --git a/internal/lsp/cache/cache.go b/internal/lsp/cache/cache.go
@@ -28,23 +28,46 @@ import (
 	"golang.org/x/tools/internal/span"
 )
 
-func New(options func(*source.Options)) *Cache {
+// New Creates a new cache for gopls operation results, using the given file
+// set, shared store, and session options.
+//
+// All of the fset, store and options may be nil, but if store is non-nil so
+// must be fset (and they must always be used together), otherwise it may be
+// possible to get cached data referencing token.Pos values not mapped by the
+// FileSet.
+func New(fset *token.FileSet, store *memoize.Store, options func(*source.Options)) *Cache {
 	index := atomic.AddInt64(&cacheIndex, 1)
+
+	if store != nil && fset == nil {
+		panic("non-nil store with nil fset")
+	}
+	if fset == nil {
+		fset = token.NewFileSet()
+	}
+	if store == nil {
+		store = &memoize.Store{}
+	}
+
 	c := &Cache{
 		id:          strconv.FormatInt(index, 10),
-		fset:        token.NewFileSet(),
+		fset:        fset,
 		options:     options,
+		store:       store,
 		fileContent: map[span.URI]*fileHandle{},
 	}
 	return c
 }
 
 type Cache struct {
-	id      string
-	fset    *token.FileSet
+	id   string
+	fset *token.FileSet
+
+	// TODO(rfindley): it doesn't make sense that cache accepts LSP options, just
+	// so that it can create a session: the cache does not (and should not)
+	// depend on options. Invert this relationship to remove options from Cache.
 	options func(*source.Options)
 
-	store memoize.Store
+	store *memoize.Store
 
 	fileMu      sync.Mutex
 	fileContent map[span.URI]*fileHandle

diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
@@ -249,6 +249,16 @@ func computePackageKey(id PackageID, files []source.FileHandle, m *KnownMetadata
 	for _, file := range files {
 		b.WriteString(file.FileIdentity().String())
 	}
+	// Metadata errors are interpreted and memoized on the computed package, so
+	// we must hash them into the key here.
+	//
+	// TODO(rfindley): handle metadata diagnostics independently from
+	// type-checking diagnostics.
+	for _, err := range m.Errors {
+		b.WriteString(err.Msg)
+		b.WriteString(err.Pos)
+		b.WriteRune(rune(err.Kind))
+	}
 	return packageHandleKey(source.HashOf(b.Bytes()))
 }
 

diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
@@ -231,7 +231,7 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI,
 		backgroundCtx:        backgroundCtx,
 		cancel:               cancel,
 		initializeOnce:       &sync.Once{},
-		store:                &s.cache.store,
+		store:                s.cache.store,
 		packages:             persistent.NewMap(packageKeyLessInterface),
 		meta:                 &metadataGraph{},
 		files:                newFilesMap(),

diff --git a/internal/lsp/cmd/capabilities_test.go b/internal/lsp/cmd/capabilities_test.go
@@ -43,7 +43,7 @@ func TestCapabilities(t *testing.T) {
 	params.Capabilities.Workspace.Configuration = true
 
 	// Send an initialize request to the server.
-	c.Server = lsp.NewServer(cache.New(app.options).NewSession(ctx), c.Client)
+	c.Server = lsp.NewServer(cache.New(nil, nil, app.options).NewSession(ctx), c.Client)
 	result, err := c.Server.Initialize(ctx, params)
 	if err != nil {
 		t.Fatal(err)

diff --git a/internal/lsp/cmd/cmd.go b/internal/lsp/cmd/cmd.go
@@ -286,7 +286,7 @@ func (app *Application) connect(ctx context.Context) (*connection, error) {
 	switch {
 	case app.Remote == "":
 		connection := newConnection(app)
-		connection.Server = lsp.NewServer(cache.New(app.options).NewSession(ctx), connection.Client)
+		connection.Server = lsp.NewServer(cache.New(nil, nil, app.options).NewSession(ctx), connection.Client)
 		ctx = protocol.WithClient(ctx, connection.Client)
 		return connection, connection.initialize(ctx, app.options)
 	case strings.HasPrefix(app.Remote, "internal@"):

diff --git a/internal/lsp/cmd/serve.go b/internal/lsp/cmd/serve.go
@@ -101,7 +101,7 @@ func (s *Serve) Run(ctx context.Context, args ...string) error {
 			return fmt.Errorf("creating forwarder: %w", err)
 		}
 	} else {
-		ss = lsprpc.NewStreamServer(cache.New(s.app.options), isDaemon)
+		ss = lsprpc.NewStreamServer(cache.New(nil, nil, s.app.options), isDaemon)
 	}
 
 	var network, addr string

diff --git a/internal/lsp/cmd/test/cmdtest.go b/internal/lsp/cmd/test/cmdtest.go
@@ -50,7 +50,7 @@ func TestCommandLine(t *testing.T, testdata string, options func(*source.Options
 
 func NewTestServer(ctx context.Context, options func(*source.Options)) *servertest.TCPServer {
 	ctx = debug.WithInstance(ctx, "", "")
-	cache := cache.New(options)
+	cache := cache.New(nil, nil, options)
 	ss := lsprpc.NewStreamServer(cache, false)
 	return servertest.NewTCPServer(ctx, ss, nil)
 }

diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go
@@ -49,7 +49,7 @@ type runner struct {
 func testLSP(t *testing.T, datum *tests.Data) {
 	ctx := tests.Context(t)
 
-	cache := cache.New(nil)
+	cache := cache.New(nil, nil, nil)
 	session := cache.NewSession(ctx)
 	options := source.DefaultOptions().Clone()
 	tests.DefaultOptions(options)

diff --git a/internal/lsp/lsprpc/lsprpc_test.go b/internal/lsp/lsprpc/lsprpc_test.go
@@ -58,7 +58,7 @@ func TestClientLogging(t *testing.T) {
 	client := FakeClient{Logs: make(chan string, 10)}
 
 	ctx = debug.WithInstance(ctx, "", "")
-	ss := NewStreamServer(cache.New(nil), false)
+	ss := NewStreamServer(cache.New(nil, nil, nil), false)
 	ss.serverForTest = server
 	ts := servertest.NewPipeServer(ss, nil)
 	defer checkClose(t, ts.Close)
@@ -121,7 +121,7 @@ func checkClose(t *testing.T, closer func() error) {
 func setupForwarding(ctx context.Context, t *testing.T, s protocol.Server) (direct, forwarded servertest.Connector, cleanup func()) {
 	t.Helper()
 	serveCtx := debug.WithInstance(ctx, "", "")
-	ss := NewStreamServer(cache.New(nil), false)
+	ss := NewStreamServer(cache.New(nil, nil, nil), false)
 	ss.serverForTest = s
 	tsDirect := servertest.NewTCPServer(serveCtx, ss, nil)
 
@@ -216,7 +216,7 @@ func TestDebugInfoLifecycle(t *testing.T) {
 	clientCtx := debug.WithInstance(baseCtx, "", "")
 	serverCtx := debug.WithInstance(baseCtx, "", "")
 
-	cache := cache.New(nil)
+	cache := cache.New(nil, nil, nil)
 	ss := NewStreamServer(cache, false)
 	tsBackend := servertest.NewTCPServer(serverCtx, ss, nil)
 

diff --git a/internal/lsp/mod/mod_test.go b/internal/lsp/mod/mod_test.go
@@ -26,7 +26,7 @@ func TestModfileRemainsUnchanged(t *testing.T) {
 	testenv.NeedsGo1Point(t, 14)
 
 	ctx := tests.Context(t)
-	cache := cache.New(nil)
+	cache := cache.New(nil, nil, nil)
 	session := cache.NewSession(ctx)
 	options := source.DefaultOptions().Clone()
 	tests.DefaultOptions(options)

diff --git a/internal/lsp/regtest/regtest.go b/internal/lsp/regtest/regtest.go
@@ -8,6 +8,7 @@ import (
 	"context"
 	"flag"
 	"fmt"
+	"go/token"
 	"io/ioutil"
 	"os"
 	"runtime"
@@ -16,6 +17,7 @@ import (
 
 	"golang.org/x/tools/internal/lsp/cmd"
 	"golang.org/x/tools/internal/lsp/source"
+	"golang.org/x/tools/internal/memoize"
 	"golang.org/x/tools/internal/testenv"
 	"golang.org/x/tools/internal/tool"
 )
@@ -87,9 +89,11 @@ var slowGOOS = map[string]bool{
 }
 
 func DefaultModes() Mode {
-	normal := Singleton | Experimental
+	// TODO(rfindley): these modes should *not* depend on GOOS. Depending on
+	// testing.Short() should be sufficient.
+	normal := Default | Experimental
 	if slowGOOS[runtime.GOOS] && testing.Short() {
-		normal = Singleton
+		normal = Default
 	}
 	if *runSubprocessTests {
 		return normal | SeparateProcess
@@ -116,6 +120,8 @@ func Main(m *testing.M, hook func(*source.Options)) {
 		PrintGoroutinesOnFailure: *printGoroutinesOnFailure,
 		SkipCleanup:              *skipCleanup,
 		OptionsHook:              hook,
+		fset:                     token.NewFileSet(),
+		store:                    memoize.NewStore(memoize.NeverEvict),
 	}
 	if *runSubprocessTests {
 		goplsPath := *goplsBinaryPath
@@ -126,13 +132,13 @@ func Main(m *testing.M, hook func(*source.Options)) {
 				panic(fmt.Sprintf("finding test binary path: %v", err))
 			}
 		}
-		runner.GoplsPath = goplsPath
+		runner.goplsPath = goplsPath
 	}
 	dir, err := ioutil.TempDir("", "gopls-regtest-")
 	if err != nil {
 		panic(fmt.Errorf("creating regtest temp directory: %v", err))
 	}
-	runner.TempDir = dir
+	runner.tempDir = dir
 
 	code := m.Run()
 	if err := runner.Close(); err != nil {