internal/lsp/cache: honor the go.work for computing workspace packages

When using Go workspaces, the go.work file should be used to determine
which packages are workspace packages.

For golang/go#48929

Change-Id: I1a8753ab7887daf193e093fca5070b4cc250a245
Reviewed-on: https://go-review.googlesource.com/c/tools/+/400822
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>

gopls/internal/regtest/workspace/workspace_test.go

@@ -1305,3 +1305,67 @@ func (Server) Foo() {}
 		_, _ = env.GoToDefinition("other_test.go", env.RegexpSearch("other_test.go", "Server"))
 	})
 }
+
+// Test for golang/go#48929.
+func TestClearNonWorkspaceDiagnostics(t *testing.T) {
+	testenv.NeedsGo1Point(t, 18) // uses go.work
+
+	const ws = `
+-- go.work --
+go 1.18
+
+use (
+        ./b
+)
+-- a/go.mod --
+module a
+
+go 1.17
+-- a/main.go --
+package main
+
+func main() {
+   var V string
+}
+-- b/go.mod --
+module b
+
+go 1.17
+-- b/main.go --
+package b
+
+import (
+        _ "fmt"
+)
+`
+	Run(t, ws, func(t *testing.T, env *Env) {
+		env.OpenFile("b/main.go")
+		env.Await(
+			OnceMet(
+				env.DoneWithOpen(),
+				NoDiagnostics("a/main.go"),
+			),
+		)
+		env.OpenFile("a/main.go")
+		env.Await(
+			OnceMet(
+				env.DoneWithOpen(),
+				env.DiagnosticAtRegexpWithMessage("a/main.go", "V", "declared but not used"),
+			),
+		)
+		env.CloseBuffer("a/main.go")
+
+		// Make an arbitrary edit because gopls explicitly diagnoses a/main.go
+		// whenever it is "changed".
+		//
+		// TODO(rfindley): it should not be necessary to make another edit here.
+		// Gopls should be smart enough to avoid diagnosing a.
+		env.RegexpReplace("b/main.go", "package b", "package b // a package")
+		env.Await(
+			OnceMet(
+				env.DoneWithChange(),
+				EmptyDiagnostics("a/main.go"),
+			),
+		)
+	})
+}

internal/lsp/cache/load.go

@@ -197,7 +197,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 		}
 		// TODO: once metadata is immutable, we shouldn't have to lock here.
 		s.mu.Lock()
-		err := s.computeMetadataUpdates(ctx, PackagePath(pkg.PkgPath), pkg, cfg, query, updates, nil)
+		err := computeMetadataUpdates(ctx, s.meta, PackagePath(pkg.PkgPath), pkg, cfg, query, updates, nil)
 		s.mu.Unlock()
 		if err != nil {
 			return err
@@ -216,7 +216,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 			delete(s.packages, key)
 		}
 	}
-	s.workspacePackages = computeWorkspacePackages(s.meta)
+	s.workspacePackages = computeWorkspacePackagesLocked(s, s.meta)
 	s.dumpWorkspace("load")
 	s.mu.Unlock()
 
@@ -442,7 +442,7 @@ func getWorkspaceDir(ctx context.Context, h *memoize.Handle, g *memoize.Generati
 // computeMetadataUpdates populates the updates map with metadata updates to
 // apply, based on the given pkg. It recurs through pkg.Imports to ensure that
 // metadata exists for all dependencies.
-func (s *snapshot) computeMetadataUpdates(ctx context.Context, pkgPath PackagePath, pkg *packages.Package, cfg *packages.Config, query []string, updates map[PackageID]*KnownMetadata, path []PackageID) error {
+func computeMetadataUpdates(ctx context.Context, g *metadataGraph, pkgPath PackagePath, pkg *packages.Package, cfg *packages.Config, query []string, updates map[PackageID]*KnownMetadata, path []PackageID) error {
 	id := PackageID(pkg.ID)
 	if new := updates[id]; new != nil {
 		return nil
@@ -494,28 +494,13 @@ func (s *snapshot) computeMetadataUpdates(ctx context.Context, pkgPath PackagePa
 		m.Errors = append(m.Errors, err)
 	}
 
-	uris := map[span.URI]struct{}{}
 	for _, filename := range pkg.CompiledGoFiles {
 		uri := span.URIFromPath(filename)
 		m.CompiledGoFiles = append(m.CompiledGoFiles, uri)
-		uris[uri] = struct{}{}
 	}
 	for _, filename := range pkg.GoFiles {
 		uri := span.URIFromPath(filename)
 		m.GoFiles = append(m.GoFiles, uri)
-		uris[uri] = struct{}{}
-	}
-
-	for uri := range uris {
-		// In order for a package to be considered for the workspace, at least one
-		// file must be contained in the workspace and not vendored.
-
-		// The package's files are in this view. It may be a workspace package.
-		// Vendored packages are not likely to be interesting to the user.
-		if !strings.Contains(string(uri), "/vendor/") && s.view.contains(uri) {
-			m.HasWorkspaceFiles = true
-			break
-		}
 	}
 
 	for importPath, importPkg := range pkg.Imports {
@@ -532,8 +517,8 @@ func (s *snapshot) computeMetadataUpdates(ctx context.Context, pkgPath PackagePa
 			m.MissingDeps[importPkgPath] = struct{}{}
 			continue
 		}
-		if s.noValidMetadataForIDLocked(importID) {
+		if noValidMetadataForID(g, importID) {
-			if err := s.computeMetadataUpdates(ctx, importPkgPath, importPkg, cfg, query, updates, append(path, id)); err != nil {
+			if err := computeMetadataUpdates(ctx, g, importPkgPath, importPkg, cfg, query, updates, append(path, id)); err != nil {
 				event.Error(ctx, "error in dependency", err)
 			}
 		}
@@ -542,12 +527,101 @@ func (s *snapshot) computeMetadataUpdates(ctx context.Context, pkgPath PackagePa
 	return nil
 }
 
-// computeWorkspacePackages computes workspace packages for the given metadata
+// containsPackageLocked reports whether p is a workspace package for the
-// graph.
+// snapshot s.
-func computeWorkspacePackages(meta *metadataGraph) map[PackageID]PackagePath {
+//
+// s.mu must be held while calling this function.
+func containsPackageLocked(s *snapshot, m *Metadata) bool {
+	// In legacy workspace mode, or if a package does not have an associated
+	// module, a package is considered inside the workspace if any of its files
+	// are under the workspace root (and not excluded).
+	//
+	// Otherwise if the package has a module it must be an active module (as
+	// defined by the module root or go.work file) and at least one file must not
+	// be filtered out by directoryFilters.
+	if m.Module != nil && s.workspace.moduleSource != legacyWorkspace {
+		modURI := span.URIFromPath(m.Module.GoMod)
+		_, ok := s.workspace.activeModFiles[modURI]
+		if !ok {
+			return false
+		}
+
+		uris := map[span.URI]struct{}{}
+		for _, uri := range m.CompiledGoFiles {
+			uris[uri] = struct{}{}
+		}
+		for _, uri := range m.GoFiles {
+			uris[uri] = struct{}{}
+		}
+
+		for uri := range uris {
+			// Don't use view.contains here. go.work files may include modules
+			// outside of the workspace folder.
+			if !strings.Contains(string(uri), "/vendor/") && !s.view.filters(uri) {
+				return true
+			}
+		}
+		return false
+	}
+
+	return containsFileInWorkspaceLocked(s, m)
+}
+
+// containsOpenFileLocked reports whether any file referenced by m is open in
+// the snapshot s.
+//
+// s.mu must be held while calling this function.
+func containsOpenFileLocked(s *snapshot, m *KnownMetadata) bool {
+	uris := map[span.URI]struct{}{}
+	for _, uri := range m.CompiledGoFiles {
+		uris[uri] = struct{}{}
+	}
+	for _, uri := range m.GoFiles {
+		uris[uri] = struct{}{}
+	}
+
+	for uri := range uris {
+		if s.isOpenLocked(uri) {
+			return true
+		}
+	}
+	return false
+}
+
+// containsFileInWorkspace reports whether m contains any file inside the
+// workspace of the snapshot s.
+//
+// s.mu must be held while calling this function.
+func containsFileInWorkspaceLocked(s *snapshot, m *Metadata) bool {
+	uris := map[span.URI]struct{}{}
+	for _, uri := range m.CompiledGoFiles {
+		uris[uri] = struct{}{}
+	}
+	for _, uri := range m.GoFiles {
+		uris[uri] = struct{}{}
+	}
+
+	for uri := range uris {
+		// In order for a package to be considered for the workspace, at least one
+		// file must be contained in the workspace and not vendored.
+
+		// The package's files are in this view. It may be a workspace package.
+		// Vendored packages are not likely to be interesting to the user.
+		if !strings.Contains(string(uri), "/vendor/") && s.view.contains(uri) {
+			return true
+		}
+	}
+	return false
+}
+
+// computeWorkspacePackagesLocked computes workspace packages in the snapshot s
+// for the given metadata graph.
+//
+// s.mu must be held while calling this function.
+func computeWorkspacePackagesLocked(s *snapshot, meta *metadataGraph) map[PackageID]PackagePath {
 	workspacePackages := make(map[PackageID]PackagePath)
 	for _, m := range meta.metadata {
-		if !m.HasWorkspaceFiles {
+		if !containsPackageLocked(s, m.Metadata) {
 			continue
 		}
 		if m.PkgFilesChanged {
@@ -567,6 +641,12 @@ func computeWorkspacePackages(meta *metadataGraph) map[PackageID]PackagePath {
 			if allFilesHaveRealPackages(meta, m) {
 				continue
 			}
+
+			// We only care about command-line-arguments packages if they are still
+			// open.
+			if !containsOpenFileLocked(s, m) {
+				continue
+			}
 		}
 
 		switch {

internal/lsp/cache/metadata.go

@@ -67,13 +67,6 @@ type Metadata struct {
 	// TODO(rfindley): this can probably just be a method, since it is derived
 	// from other fields.
 	IsIntermediateTestVariant bool
-
-	// HasWorkspaceFiles reports whether m contains any files that are considered
-	// part of the workspace.
-	//
-	// TODO(golang/go#48929): this should be a property of the workspace
-	// (the go.work file), not a constant.
-	HasWorkspaceFiles bool
 }
 
 // Name implements the source.Metadata interface.

internal/lsp/cache/session.go

@@ -323,6 +323,8 @@ func bestViewForURI(uri span.URI, views []*View) *View {
 		if longest != nil && len(longest.Folder()) > len(view.Folder()) {
 			continue
 		}
+		// TODO(rfindley): this should consider the workspace layout (i.e.
+		// go.work).
 		if view.contains(uri) {
 			longest = view
 		}

internal/lsp/cache/snapshot.go

@@ -768,6 +768,8 @@ func (s *snapshot) isActiveLocked(id PackageID, seen map[PackageID]bool) (active
 			return true
 		}
 	}
+	// TODO(rfindley): it looks incorrect that we don't also check GoFiles here.
+	// If a CGo file is open, we want to consider the package active.
 	for _, dep := range m.Deps {
 		if s.isActiveLocked(dep, seen) {
 			return true
@@ -1289,11 +1291,11 @@ func (s *snapshot) noValidMetadataForURILocked(uri span.URI) bool {
 func (s *snapshot) noValidMetadataForID(id PackageID) bool {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	return s.noValidMetadataForIDLocked(id)
+	return noValidMetadataForID(s.meta, id)
 }
 
-func (s *snapshot) noValidMetadataForIDLocked(id PackageID) bool {
+func noValidMetadataForID(g *metadataGraph, id PackageID) bool {
-	m := s.meta.metadata[id]
+	m := g.metadata[id]
 	return m == nil || !m.Valid
 }
 
@@ -1789,8 +1791,10 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 		}
 	}
 
+	// Compute invalidations based on file changes.
 	changedPkgFiles := map[PackageID]bool{} // packages whose file set may have changed
 	anyImportDeleted := false
+	anyFileOpenedOrClosed := false
 	for uri, change := range changes {
 		// Maybe reinitialize the view if we see a change in the vendor
 		// directory.
@@ -1800,6 +1804,10 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 
 		// The original FileHandle for this URI is cached on the snapshot.
 		originalFH := s.files[uri]
+		var originalOpen, newOpen bool
+		_, originalOpen = originalFH.(*overlay)
+		_, newOpen = change.fileHandle.(*overlay)
+		anyFileOpenedOrClosed = originalOpen != newOpen
 
 		// If uri is a Go file, check if it has changed in a way that would
 		// invalidate metadata. Note that we can't use s.view.FileKind here,
@@ -1903,6 +1911,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 		newGen.Inherit(v.handle)
 		result.packages[k] = v
 	}
+
 	// Copy the package analysis information.
 	for k, v := range s.actions {
 		if _, ok := idsToInvalidate[k.pkg.id]; ok {
@@ -1988,13 +1997,19 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 		}
 	}
 
+	// Update metadata, if necessary.
 	if len(metadataUpdates) > 0 {
 		result.meta = s.meta.Clone(metadataUpdates)
-		result.workspacePackages = computeWorkspacePackages(result.meta)
 	} else {
 		// No metadata changes. Since metadata is only updated by cloning, it is
 		// safe to re-use the existing metadata here.
 		result.meta = s.meta
+	}
+
+	// Update workspace packages, if necessary.
+	if result.meta != s.meta || anyFileOpenedOrClosed {
+		result.workspacePackages = computeWorkspacePackagesLocked(result, result.meta)
+	} else {
 		result.workspacePackages = s.workspacePackages
 	}
 

internal/lsp/cache/view.go

@@ -397,16 +397,27 @@ func (s *snapshot) locateTemplateFiles(ctx context.Context) {
 }
 
 func (v *View) contains(uri span.URI) bool {
+	// TODO(rfindley): should we ignore the root here? It is not provided by the
+	// user, and is undefined when go.work is outside the workspace. It would be
+	// better to explicitly consider the set of active modules wherever relevant.
 	inRoot := source.InDir(v.rootURI.Filename(), uri.Filename())
 	inFolder := source.InDir(v.folder.Filename(), uri.Filename())
+
 	if !inRoot && !inFolder {
 		return false
 	}
-	// Filters are applied relative to the workspace folder.
+
-	if inFolder {
+	return !v.filters(uri)
-		return !pathExcludedByFilter(strings.TrimPrefix(uri.Filename(), v.folder.Filename()), v.rootURI.Filename(), v.gomodcache, v.Options())
+}
+
+// filters reports whether uri is filtered by the currently configured
+// directoryFilters.
+func (v *View) filters(uri span.URI) bool {
+	// Only filter relative to the configured root directory.
+	if source.InDirLex(v.folder.Filename(), uri.Filename()) {
+		return pathExcludedByFilter(strings.TrimPrefix(uri.Filename(), v.folder.Filename()), v.rootURI.Filename(), v.gomodcache, v.Options())
 	}
-	return true
+	return false
 }
 
 func (v *View) mapFile(uri span.URI, f *fileBase) {

internal/lsp/diagnostics.go

@@ -66,6 +66,8 @@ func (d diagnosticSource) String() string {
 		return "FromTypeChecking"
 	case orphanedSource:
 		return "FromOrphans"
+	case workSource:
+		return "FromGoWork"
 	default:
 		return fmt.Sprintf("From?%d?", d)
 	}

internal/lsp/regtest/expectation.go

@@ -613,7 +613,7 @@ func NoDiagnostics(name string) Expectation {
 	}
 	return SimpleExpectation{
 		check:       check,
-		description: "no diagnostics",
+		description: fmt.Sprintf("no diagnostics for %q", name),
 	}
 }
 

internal/lsp/source/util.go

@@ -471,7 +471,7 @@ func CompareURI(left, right span.URI) int {
 //
 // Copied and slightly adjusted from go/src/cmd/go/internal/search/search.go.
 func InDir(dir, path string) bool {
-	if inDirLex(dir, path) {
+	if InDirLex(dir, path) {
 		return true
 	}
 	if !honorSymlinks {
@@ -481,18 +481,18 @@ func InDir(dir, path string) bool {
 	if err != nil || xpath == path {
 		xpath = ""
 	} else {
-		if inDirLex(dir, xpath) {
+		if InDirLex(dir, xpath) {
 			return true
 		}
 	}
 
 	xdir, err := filepath.EvalSymlinks(dir)
 	if err == nil && xdir != dir {
-		if inDirLex(xdir, path) {
+		if InDirLex(xdir, path) {
 			return true
 		}
 		if xpath != "" {
-			if inDirLex(xdir, xpath) {
+			if InDirLex(xdir, xpath) {
 				return true
 			}
 		}
@@ -500,11 +500,11 @@ func InDir(dir, path string) bool {
 	return false
 }
 
-// inDirLex is like inDir but only checks the lexical form of the file names.
+// InDirLex is like inDir but only checks the lexical form of the file names.
 // It does not consider symbolic links.
 //
 // Copied from go/src/cmd/go/internal/search/search.go.
-func inDirLex(dir, path string) bool {
+func InDirLex(dir, path string) bool {
 	pv := strings.ToUpper(filepath.VolumeName(path))
 	dv := strings.ToUpper(filepath.VolumeName(dir))
 	path = path[len(pv):]