diff --git a/internal/lsp/cache/debug.go b/internal/lsp/cache/debug.go new file mode 100644 index 0000000000..b8d207d83d --- /dev/null +++ b/internal/lsp/cache/debug.go @@ -0,0 +1,53 @@ +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package cache + +import ( + "fmt" + "os" + "sort" +) + +// This file contains helpers that can be used to instrument code while +// debugging. + +// debugEnabled toggles the helpers below. +const debugEnabled = false + +// If debugEnabled is true, debugf formats its arguments and prints to stderr. +// If debugEnabled is false, it is a no-op. +func debugf(format string, args ...interface{}) { + if !debugEnabled { + return + } + if false { + fmt.Sprintf(format, args...) // encourage vet to validate format strings + } + fmt.Fprintf(os.Stderr, ">>> "+format+"\n", args...) +} + +// If debugEnabled is true, dumpWorkspace prints a summary of workspace +// packages to stderr. If debugEnabled is false, it is a no-op. +func (s *snapshot) dumpWorkspace(context string) { + if !debugEnabled { + return + } + + debugf("workspace (after %s):", context) + var ids []PackageID + for id := range s.workspacePackages { + ids = append(ids, id) + } + + sort.Slice(ids, func(i, j int) bool { + return ids[i] < ids[j] + }) + + for _, id := range ids { + pkgPath := s.workspacePackages[id] + _, ok := s.meta.metadata[id] + debugf(" %s:%s (metadata: %t)", id, pkgPath, ok) + } +} diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 96c2a0733a..7b41d24482 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -149,6 +149,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf } moduleErrs := make(map[string][]packages.Error) // module path -> errors + var newMetadata []*Metadata for _, pkg := range pkgs { // The Go command returns synthetic list results for module queries that // encountered module errors. @@ -195,17 +196,28 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf } // Set the metadata for this package. s.mu.Lock() - m, err := s.setMetadataLocked(ctx, PackagePath(pkg.PkgPath), pkg, cfg, query, map[PackageID]struct{}{}) + seen := make(map[PackageID]struct{}) + m, err := s.setMetadataLocked(ctx, PackagePath(pkg.PkgPath), pkg, cfg, query, seen) s.mu.Unlock() if err != nil { return err } + newMetadata = append(newMetadata, m) + } + + // Rebuild package data when metadata is updated. + s.rebuildPackageData() + s.dumpWorkspace("load") + + // Now that the workspace has been rebuilt, verify that we can build package handles. + // + // TODO(rfindley): what's the point of returning an error here? Probably we + // can simply remove this step: The package handle will be rebuilt as needed. + for _, m := range newMetadata { if _, err := s.buildPackageHandle(ctx, m.ID, s.workspaceParseMode(m.ID)); err != nil { return err } } - // Rebuild the import graph when the metadata is updated. - s.clearAndRebuildImportGraph() if len(moduleErrs) > 0 { return &moduleErrorMap{moduleErrs} @@ -468,6 +480,19 @@ func (s *snapshot) setMetadataLocked(ctx context.Context, pkgPath PackagePath, p 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 + } + } + s.updateIDForURIsLocked(id, uris) // TODO(rstambler): is this still necessary? @@ -517,30 +542,65 @@ func (s *snapshot) setMetadataLocked(ctx context.Context, pkgPath PackagePath, p } } - // Set the workspace packages. If any of the package's files belong to the - // view, then the package may be a workspace package. - for _, uri := range append(m.CompiledGoFiles, m.GoFiles...) { - if !s.view.contains(uri) { + return m, nil +} + +// computeWorkspacePackages computes workspace packages for the given metadata +// graph. +func computeWorkspacePackages(meta *metadataGraph) map[PackageID]PackagePath { + workspacePackages := make(map[PackageID]PackagePath) + for _, m := range meta.metadata { + if !m.HasWorkspaceFiles { + continue + } + if m.PkgFilesChanged { + // If a package name has changed, it's possible that the package no + // longer exists. Leaving it as a workspace package can result in + // persistent stale diagnostics. + // + // If there are still valid files in the package, it will be reloaded. + // + // There may be more precise heuristics. continue } - // The package's files are in this view. It may be a workspace package. - if strings.Contains(string(uri), "/vendor/") { - // Vendored packages are not likely to be interesting to the user. - continue + if source.IsCommandLineArguments(string(m.ID)) { + // If all the files contained in m have a real package, we don't need to + // keep m as a workspace package. + if allFilesHaveRealPackages(meta, m) { + continue + } } switch { case m.ForTest == "": // A normal package. - s.workspacePackages[m.ID] = pkgPath + workspacePackages[m.ID] = m.PkgPath case m.ForTest == m.PkgPath, m.ForTest+"_test" == m.PkgPath: // The test variant of some workspace package or its x_test. // To load it, we need to load the non-test variant with -test. - s.workspacePackages[m.ID] = m.ForTest + workspacePackages[m.ID] = m.ForTest } } - return m, nil + return workspacePackages +} + +// allFilesHaveRealPackages reports whether all files referenced by m are +// contained in a "real" package (not command-line-arguments). +// +// If m is not a command-line-arguments package, this is trivially true. +func allFilesHaveRealPackages(g *metadataGraph, m *KnownMetadata) bool { + n := len(m.CompiledGoFiles) +checkURIs: + for _, uri := range append(m.CompiledGoFiles[0:n:n], m.GoFiles...) { + for _, id := range g.ids[uri] { + if !source.IsCommandLineArguments(string(id)) { + continue checkURIs + } + } + return false + } + return true } func isTestMain(pkg *packages.Package, gocache string) bool { diff --git a/internal/lsp/cache/metadata.go b/internal/lsp/cache/metadata.go index c2a21969d8..525a3e6549 100644 --- a/internal/lsp/cache/metadata.go +++ b/internal/lsp/cache/metadata.go @@ -67,6 +67,13 @@ 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. @@ -92,6 +99,14 @@ type KnownMetadata struct { // Invalid metadata can still be used if a metadata reload fails. Valid bool + // PkgFilesChanged reports whether the file set of this metadata has + // potentially changed. + PkgFilesChanged bool + // ShouldLoad is true if the given metadata should be reloaded. + // + // Note that ShouldLoad is different from !Valid: when we try to load a + // package, we mark ShouldLoad = false regardless of whether the load + // succeeded, to prevent endless loads. ShouldLoad bool } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 7b73f4b279..961a60f8aa 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -716,13 +716,14 @@ func (s *snapshot) getImportedByLocked(id PackageID) []PackageID { return s.meta.importedBy[id] } -func (s *snapshot) clearAndRebuildImportGraph() { +func (s *snapshot) rebuildPackageData() { s.mu.Lock() defer s.mu.Unlock() // Completely invalidate the original map. s.meta.importedBy = make(map[PackageID][]PackageID) s.rebuildImportGraph() + s.workspacePackages = computeWorkspacePackages(s.meta) } func (s *snapshot) rebuildImportGraph() { @@ -1337,6 +1338,7 @@ func (s *snapshot) updateIDForURIsLocked(id PackageID, uris map[span.URI]struct{ }) s.meta.ids[uri] = newIDs } + s.dumpWorkspace("updateIDs") } func (s *snapshot) isWorkspacePackage(id PackageID) bool { @@ -1857,7 +1859,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC } } - changedPkgFiles := map[PackageID]struct{}{} // packages whose file set may have changed + changedPkgFiles := map[PackageID]bool{} // packages whose file set may have changed anyImportDeleted := false for uri, change := range changes { // Maybe reinitialize the view if we see a change in the vendor @@ -1885,7 +1887,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC filePackageIDs := invalidatedPackageIDs(uri, s.meta.ids, pkgFileChanged) if pkgFileChanged { for id := range filePackageIDs { - changedPkgFiles[id] = struct{}{} + changedPkgFiles[id] = true } } for id := range filePackageIDs { @@ -2064,55 +2066,14 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC invalidateMetadata := idsToInvalidate[k] // Mark invalidated metadata rather than deleting it outright. result.meta.metadata[k] = &KnownMetadata{ - Metadata: v.Metadata, - Valid: v.Valid && !invalidateMetadata, - ShouldLoad: v.ShouldLoad || invalidateMetadata, + Metadata: v.Metadata, + Valid: v.Valid && !invalidateMetadata, + PkgFilesChanged: v.PkgFilesChanged || changedPkgFiles[k], + ShouldLoad: v.ShouldLoad || invalidateMetadata, } } - // Copy the set of initially loaded packages. - for id, pkgPath := range s.workspacePackages { - // Packages with the id "command-line-arguments" are generated by the - // go command when the user is outside of GOPATH and outside of a - // module. Do not cache them as workspace packages for longer than - // necessary. - if source.IsCommandLineArguments(string(id)) { - if invalidateMetadata, ok := idsToInvalidate[id]; invalidateMetadata && ok { - continue - } - } - - // If all the files we know about in a package have been deleted, - // the package is gone and we should no longer try to load it. - if m := s.meta.metadata[id]; m != nil { - hasFiles := false - for _, uri := range s.meta.metadata[id].GoFiles { - // For internal tests, we need _test files, not just the normal - // ones. External tests only have _test files, but we can check - // them anyway. - if m.ForTest != "" && !strings.HasSuffix(string(uri), "_test.go") { - continue - } - if _, ok := result.files[uri]; ok { - hasFiles = true - break - } - } - if !hasFiles { - continue - } - } - - // If the package name of a file in the package has changed, it's - // possible that the package ID may no longer exist. Delete it from - // the set of workspace packages, on the assumption that we will add it - // back when the relevant files are reloaded. - if _, ok := changedPkgFiles[id]; ok { - continue - } - - result.workspacePackages[id] = pkgPath - } + result.workspacePackages = computeWorkspacePackages(result.meta) // Inherit all of the go.mod-related handles. for _, v := range result.modTidyHandles { @@ -2142,6 +2103,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC result.initializeOnce = &sync.Once{} } } + result.dumpWorkspace("clone") return result }