diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go index 5e5bcd13b5..9e4b85fced 100644 --- a/gopls/internal/regtest/workspace/workspace_test.go +++ b/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"), + ), + ) + }) +} diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 5f24d0f08e..5ce49f00d4 100644 --- a/internal/lsp/cache/load.go +++ b/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 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 @@ func (s *snapshot) computeMetadataUpdates(ctx context.Context, pkgPath PackagePa 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 @@ 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 { diff --git a/internal/lsp/cache/metadata.go b/internal/lsp/cache/metadata.go index 525a3e6549..b4da7130c2 100644 --- a/internal/lsp/cache/metadata.go +++ b/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. diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 286d8f12c4..0d3e944b98 100644 --- a/internal/lsp/cache/session.go +++ b/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 } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 32681735b2..b85b46c64c 100644 --- a/internal/lsp/cache/snapshot.go +++ b/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 { - 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 @@ 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 } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 0ed9883451..620efd8b96 100644 --- a/internal/lsp/cache/view.go +++ b/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 !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 0837b22cc2..9648921ef5 100644 --- a/internal/lsp/diagnostics.go +++ b/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) } diff --git a/internal/lsp/regtest/expectation.go b/internal/lsp/regtest/expectation.go index ab808f9e8c..737f83da89 100644 --- a/internal/lsp/regtest/expectation.go +++ b/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), } } diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index 9cb2ee6948..b8a7fc9135 100644 --- a/internal/lsp/source/util.go +++ b/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):]