internal/lsp/cache: introduce a workspace abstraction

When incorporating the gopls.mod file, the invalidation logic for
workspace module information becomes quite complicated. For example:
 + if a modfile changes we should only invalidate if it is in the set of
   active modules
 + the set of active modules can be provided by either the filesystem or
   gopls.mod
 + if gopls.mod changes, we may gain or lose active modules in the
   workspace
 + if gopls.mod is *deleted*, we may need to walk the filesystem to
   actually find all active modules

Doing this with only concrete changes to the snapshot proved
prohibitively complex (at least for me), so a new workspace type is
introduced to manage the module state. This new abstraction is
responsible for tracking the set of active modules, the workspace
modfile, and the set of workspace directories. Its invalidation logic is
factored out of snapshot.clone, so that it can be tested and to
alleviate some of the growing complexity of snapshot.clone.

The workspace type is idempotent, allowing it to be shared across
snapshots without needing to use the cache. There is little benefit to
the cache in this case, since workspace module computation should be
infrequent, and the type itself consumes little memory.

This is made possible because the workspace type itself depends only on
file state, and therefore may be invalidated independently of the
snapshot. The new source.FileState interface is used in testing, and so
that the workspace module may be computed based on both the session file
state as well as the snapshot file state.

As a result of this change, the following improvements in functionality
are possible:
 + in the presence of a gopls.mod file, we avoid walking the filesystem
   to detect modules. This could be helpful for working in large
   monorepos or in GOPATH, when discovering all modules would be
   expensive.
 + The set of active modules should always be consistent with the
   gopls.mod file, likely fixing some bugs (for example, computing
   diagnostics for modfiles that shouldn't be loaded)

For golang/go#41837

Change-Id: I2da888c097748b659ee892ca2d6b3fbe29c1942e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/261237
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Trust: Robert Findley <rfindley@google.com>

gopls/internal/regtest/workspace_test.go

@@ -158,6 +158,16 @@ replace random.org => %s
 }
 
 const workspaceModuleProxy = `
+-- example.com@v1.2.3/go.mod --
+module example.com
+
+go 1.12
+-- example.com@v1.2.3/blah/blah.go --
+package blah
+
+func SaySomething() {
+	fmt.Println("something")
+}
 -- b.com@v1.2.3/go.mod --
 module b.com
 
@@ -364,6 +374,9 @@ func Hello() int {
 }
 
 func TestUseGoplsMod(t *testing.T) {
+	// This test validates certain functionality related to using a gopls.mod
+	// file to specify workspace modules.
+	testenv.NeedsGo1Point(t, 14)
 	const multiModule = `
 -- moda/a/go.mod --
 module a.com
@@ -384,6 +397,7 @@ func main() {
 -- modb/go.mod --
 module b.com
 
+require example.com v1.2.3
 -- modb/b/b.go --
 package b
 
@@ -404,12 +418,18 @@ replace a.com => $SANDBOX_WORKDIR/moda/a
 		WithProxyFiles(workspaceModuleProxy),
 		WithModes(Experimental),
 	).run(t, multiModule, func(t *testing.T, env *Env) {
+		// Initially, the gopls.mod should cause only the a.com module to be
+		// loaded. Validate this by jumping to a definition in b.com and ensuring
+		// that we go to the module cache.
 		env.OpenFile("moda/a/a.go")
-		original, _ := env.GoToDefinition("moda/a/a.go", env.RegexpSearch("moda/a/a.go", "Hello"))
-		if want := "b.com@v1.2.3/b/b.go"; !strings.HasSuffix(original, want) {
-			t.Errorf("expected %s, got %v", want, original)
+		location, _ := env.GoToDefinition("moda/a/a.go", env.RegexpSearch("moda/a/a.go", "Hello"))
+		if want := "b.com@v1.2.3/b/b.go"; !strings.HasSuffix(location, want) {
+			t.Errorf("expected %s, got %v", want, location)
 		}
 		workdir := env.Sandbox.Workdir.RootURI().SpanURI().Filename()
+
+		// Now, modify the gopls.mod file on disk to activate the b.com module in
+		// the workspace.
 		env.WriteWorkspaceFile("gopls.mod", fmt.Sprintf(`module gopls-workspace
 
 require (
@@ -426,9 +446,41 @@ replace b.com => %s/modb
 				env.DiagnosticAtRegexp("modb/b/b.go", "x"),
 			),
 		)
-		newLocation, _ := env.GoToDefinition("moda/a/a.go", env.RegexpSearch("moda/a/a.go", "Hello"))
-		if want := "modb/b/b.go"; !strings.HasSuffix(newLocation, want) {
-			t.Errorf("expected %s, got %v", want, newLocation)
+		env.OpenFile("modb/go.mod")
+		// Check that go.mod diagnostics picked up the newly active mod file.
+		env.Await(env.DiagnosticAtRegexp("modb/go.mod", `require example.com v1.2.3`))
+		// ...and that jumping to definition now goes to b.com in the workspace.
+		location, _ = env.GoToDefinition("moda/a/a.go", env.RegexpSearch("moda/a/a.go", "Hello"))
+		if want := "modb/b/b.go"; !strings.HasSuffix(location, want) {
+			t.Errorf("expected %s, got %v", want, location)
+		}
+
+		// Now, let's modify the gopls.mod *overlay* (not on disk), and verify that
+		// this change is also picked up.
+		env.OpenFile("gopls.mod")
+		env.SetBufferContent("gopls.mod", fmt.Sprintf(`module gopls-workspace
+
+require (
+	a.com v0.0.0-goplsworkspace
+)
+
+replace a.com => %s/moda/a
+`, workdir))
+		env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), 1))
+		// TODO: diagnostics are not being cleared from the old go.mod location,
+		// because it's not treated as a 'deleted' file. Uncomment this after
+		// fixing.
+		/*
+			env.Await(OnceMet(
+				CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), 1),
+				EmptyDiagnostics("modb/go.mod"),
+			))
+		*/
+
+		// Just as before, check that we now jump to the module cache.
+		location, _ = env.GoToDefinition("moda/a/a.go", env.RegexpSearch("moda/a/a.go", "Hello"))
+		if want := "b.com@v1.2.3/b/b.go"; !strings.HasSuffix(location, want) {
+			t.Errorf("expected %s, got %v", want, location)
 		}
 	})
 }

gopls/internal/regtest/wrappers.go

@@ -92,6 +92,13 @@ func (e *Env) EditBuffer(name string, edits ...fake.Edit) {
 	}
 }
 
+func (e *Env) SetBufferContent(name string, content string) {
+	e.T.Helper()
+	if err := e.Editor.SetBufferContent(e.Ctx, name, content); err != nil {
+		e.T.Fatal(err)
+	}
+}
+
 // RegexpRange returns the range of the first match for re in the buffer
 // specified by name, calling t.Fatal on any error. It first searches for the
 // position in open buffers, then in workspace files.

internal/lsp/cache/cache.go

@@ -79,14 +79,10 @@ func (c *Cache) getFile(ctx context.Context, uri span.URI) (*fileHandle, error)
 		return fh, nil
 	}
 
-	select {
-	case ioLimit <- struct{}{}:
-	case <-ctx.Done():
-		return nil, ctx.Err()
+	fh, err := readFile(ctx, uri, fi.ModTime())
+	if err != nil {
+		return nil, err
 	}
-	defer func() { <-ioLimit }()
-
-	fh = readFile(ctx, uri, fi.ModTime())
 	c.fileMu.Lock()
 	c.fileContent[uri] = fh
 	c.fileMu.Unlock()
@@ -96,7 +92,14 @@ func (c *Cache) getFile(ctx context.Context, uri span.URI) (*fileHandle, error)
 // ioLimit limits the number of parallel file reads per process.
 var ioLimit = make(chan struct{}, 128)
 
-func readFile(ctx context.Context, uri span.URI, modTime time.Time) *fileHandle {
+func readFile(ctx context.Context, uri span.URI, modTime time.Time) (*fileHandle, error) {
+	select {
+	case ioLimit <- struct{}{}:
+	case <-ctx.Done():
+		return nil, ctx.Err()
+	}
+	defer func() { <-ioLimit }()
+
 	ctx, done := event.Start(ctx, "cache.readFile", tag.File.Of(uri.Filename()))
 	_ = ctx
 	defer done()
@@ -106,14 +109,14 @@ func readFile(ctx context.Context, uri span.URI, modTime time.Time) *fileHandle
 		return &fileHandle{
 			modTime: modTime,
 			err:     err,
-		}
+		}, nil
 	}
 	return &fileHandle{
 		modTime: modTime,
 		uri:     uri,
 		bytes:   data,
 		hash:    hashContents(data),
-	}
+	}, nil
 }
 
 func (c *Cache) NewSession(ctx context.Context) *Session {

internal/lsp/cache/imports.go

@@ -33,34 +33,31 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot
 	// 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.
-	var modFH, sumFH source.FileHandle
+	var modFH source.FileHandle
+	var gosum []byte
 	var modFileIdentifier string
 	var err error
-	// TODO(heschik): Change the goimports logic to use a persistent workspace
+	// TODO(rfindley): Change the goimports logic to use a persistent workspace
 	// module for workspace module mode.
 	//
 	// Get the go.mod file that corresponds to this view's root URI. This is
 	// broken because it assumes that the view's root is a module, but this is
 	// not more broken than the previous state--it is a temporary hack that
 	// should be removed ASAP.
-	var match *moduleRoot
-	for _, m := range snapshot.modules {
-		if m.rootURI == snapshot.view.rootURI {
-			match = m
+	var matchURI span.URI
+	for modURI := range snapshot.workspace.activeModFiles() {
+		if dirURI(modURI) == snapshot.view.rootURI {
+			matchURI = modURI
 		}
 	}
-	if match != nil {
-		modFH, err = snapshot.GetFile(ctx, match.modURI)
+	// TODO(rFindley): should it be an error if matchURI is empty?
+	if matchURI != "" {
+		modFH, err = snapshot.GetFile(ctx, matchURI)
 		if err != nil {
 			return err
 		}
 		modFileIdentifier = modFH.FileIdentity().Hash
-		if match.sumURI != "" {
-			sumFH, err = snapshot.GetFile(ctx, match.sumURI)
-			if err != nil {
-				return err
-			}
-		}
+		gosum = snapshot.goSum(ctx, matchURI)
 	}
 	// v.goEnv is immutable -- changes make a new view. Options can change.
 	// We can't compare build flags directly because we may add -modfile.
@@ -87,7 +84,7 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot
 		}
 		s.cachedModFileIdentifier = modFileIdentifier
 		s.cachedBuildFlags = currentBuildFlags
-		s.cleanupProcessEnv, err = s.populateProcessEnv(ctx, snapshot, modFH, sumFH)
+		s.cleanupProcessEnv, err = s.populateProcessEnv(ctx, snapshot, modFH, gosum)
 		if err != nil {
 			return err
 		}
@@ -125,7 +122,7 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot
 
 // populateProcessEnv sets the dynamically configurable fields for the view's
 // process environment. Assumes that the caller is holding the s.view.importsMu.
-func (s *importsState) populateProcessEnv(ctx context.Context, snapshot *snapshot, modFH, sumFH source.FileHandle) (cleanup func(), err error) {
+func (s *importsState) populateProcessEnv(ctx context.Context, snapshot *snapshot, modFH source.FileHandle, gosum []byte) (cleanup func(), err error) {
 	cleanup = func() {}
 	pe := s.processEnv
 
@@ -166,7 +163,7 @@ func (s *importsState) populateProcessEnv(ctx context.Context, snapshot *snapsho
 	// Add -modfile to the build flags, if we are using it.
 	if snapshot.workspaceMode()&tempModfile != 0 && modFH != nil {
 		var tmpURI span.URI
-		tmpURI, cleanup, err = tempModFile(modFH, sumFH)
+		tmpURI, cleanup, err = tempModFile(modFH, gosum)
 		if err != nil {
 			return nil, err
 		}

internal/lsp/cache/load.go

@@ -191,14 +191,11 @@ func (s *snapshot) tempWorkspaceModule(ctx context.Context) (_ span.URI, cleanup
 	if s.workspaceMode()&usesWorkspaceModule == 0 {
 		return "", cleanup, nil
 	}
-	wsModuleHandle, err := s.getWorkspaceModuleHandle(ctx)
-	if err != nil {
-		return "", nil, err
-	}
-	file, err := wsModuleHandle.build(ctx, s)
+	file, err := s.workspace.modFile(ctx, s)
 	if err != nil {
 		return "", nil, err
 	}
+
 	content, err := file.Format()
 	if err != nil {
 		return "", cleanup, err

internal/lsp/cache/mod.go

@@ -98,24 +98,26 @@ func (s *snapshot) ParseMod(ctx context.Context, modFH source.FileHandle) (*sour
 	return pmh.parse(ctx, s)
 }
 
-func (s *snapshot) sumFH(ctx context.Context, modFH source.FileHandle) (source.FileHandle, error) {
+// goSum reads the go.sum file for the go.mod file at modURI, if it exists. If
+// it doesn't exist, it returns nil.
+func (s *snapshot) goSum(ctx context.Context, modURI span.URI) []byte {
 	// Get the go.sum file, either from the snapshot or directly from the
 	// cache. Avoid (*snapshot).GetFile here, as we don't want to add
 	// nonexistent file handles to the snapshot if the file does not exist.
-	sumURI := span.URIFromPath(sumFilename(modFH.URI()))
+	sumURI := span.URIFromPath(sumFilename(modURI))
 	var sumFH source.FileHandle = s.FindFile(sumURI)
 	if sumFH == nil {
 		var err error
 		sumFH, err = s.view.session.cache.getFile(ctx, sumURI)
 		if err != nil {
-			return nil, err
+			return nil
 		}
 	}
-	_, err := sumFH.Read()
+	content, err := sumFH.Read()
 	if err != nil {
-		return nil, err
+		return nil
 	}
-	return sumFH, nil
+	return content
 }
 
 func sumFilename(modURI span.URI) string {