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>
diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go
index 5e5bcd1..9e4b85f 100644
--- a/gopls/internal/regtest/workspace/workspace_test.go
+++ b/gopls/internal/regtest/workspace/workspace_test.go
@@ -1305,3 +1305,67 @@
_, _ = 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"),
+ ),
+ )
+ })
+}
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 5f24d0f..5ce49f0 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -197,7 +197,7 @@
}
// 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 @@
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 @@
// 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 @@
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 @@
m.MissingDeps[importPkgPath] = struct{}{}
continue
}
- if s.noValidMetadataForIDLocked(importID) {
- if err := s.computeMetadataUpdates(ctx, importPkgPath, importPkg, cfg, query, updates, append(path, id)); err != nil {
+ if noValidMetadataForID(g, importID) {
+ 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 @@
return nil
}
-// computeWorkspacePackages computes workspace packages for the given metadata
-// graph.
-func computeWorkspacePackages(meta *metadataGraph) map[PackageID]PackagePath {
+// containsPackageLocked reports whether p is a workspace package for the
+// snapshot s.
+//
+// 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 @@
if allFilesHaveRealPackages(meta, m) {
continue
}
+
+ // We only care about command-line-arguments packages if they are still
+ // open.
+ if !containsOpenFileLocked(s, m) {
+ continue
+ }
}
switch {
diff --git a/internal/lsp/cache/metadata.go b/internal/lsp/cache/metadata.go
index 525a3e6..b4da713 100644
--- a/internal/lsp/cache/metadata.go
+++ b/internal/lsp/cache/metadata.go
@@ -67,13 +67,6 @@
// 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.
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index 286d8f1..0d3e944 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -323,6 +323,8 @@
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
}
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 3268173..b85b46c 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -768,6 +768,8 @@
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) 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 {
- m := s.meta.metadata[id]
+func noValidMetadataForID(g *metadataGraph, id PackageID) bool {
+ m := g.metadata[id]
return m == nil || !m.Valid
}
@@ -1789,8 +1791,10 @@
}
}
+ // 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 @@
// 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 @@
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 @@
}
}
+ // 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
}
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index 0ed9883..620efd8 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -397,16 +397,27 @@
}
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 !pathExcludedByFilter(strings.TrimPrefix(uri.Filename(), v.folder.Filename()), v.rootURI.Filename(), v.gomodcache, v.Options())
+
+ return !v.filters(uri)
+}
+
+// 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) {
diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go
index 0837b22..9648921 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -66,6 +66,8 @@
return "FromTypeChecking"
case orphanedSource:
return "FromOrphans"
+ case workSource:
+ return "FromGoWork"
default:
return fmt.Sprintf("From?%d?", d)
}
diff --git a/internal/lsp/regtest/expectation.go b/internal/lsp/regtest/expectation.go
index ab808f9..737f83d 100644
--- a/internal/lsp/regtest/expectation.go
+++ b/internal/lsp/regtest/expectation.go
@@ -613,7 +613,7 @@
}
return SimpleExpectation{
check: check,
- description: "no diagnostics",
+ description: fmt.Sprintf("no diagnostics for %q", name),
}
}
diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go
index 9cb2ee6..b8a7fc9 100644
--- a/internal/lsp/source/util.go
+++ b/internal/lsp/source/util.go
@@ -471,7 +471,7 @@
//
// 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 @@
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 @@
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):]
