internal/lsp/cache: derive workspace packages from metadata

Now that we preserve stale metadata, we can derive workspace packages
entirely from known metadata and files. This consolidates the logic to
compute workspace packages into a single location, which can be invoked
whenever metadata changes (via load or invalidation in clone).

Additionally:
 - Precompute 'HasWorkspaceFiles' when loading metadata. This value
   should never change for a given Metadata, and our view.contains func
   is actually quite slow due to evaluating symlinks.
 - Track 'PkgFilesChanged' on KnownMetadata, since we don't include
   packages whose package name has changed in our workspace.

Also introduce a few debug helpers, so that we can leave some
instrumentation in critical functions.

For golang/go#45686

Change-Id: I2c994a1e8ca05c3c42f67bd2f4519bea5095c54c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/340735
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
gopls-CI: kokoro <[email protected]>

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)
+	}
+}

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
 		}
-
-		// 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.
+		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
 		}
 
+		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 {

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
 }

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
 }