internal/lsp: reload metadata for orphaned files

go/packages overlay handling only really works for contains queries
(file=), so our approach of reloading packages by package path (for
workspace packages) wasn't handling newly created packages that need to
be handled through overlays. Workaround this by reloading metadata for
individual files that are missing it by running extra contains queries
(only after the first metadata load for package paths). Be careful not
to reload the same file multiple times if the first load did not succeed.

Somewhat related, clear out `go list` errors in packages that go
through overlay handling, since they will often be rendered irrelevant.
I'm not sure if this is the right move, but if it's not, then we will
have to do extra work to disregard those errors in gopls.

Fixes golang/go#36661.
Fixes golang/go#36635.

Change-Id: Ib83cffcdf8a3e07da0f30e734d5e2c89691e1aba
Reviewed-on: https://go-review.googlesource.com/c/tools/+/216141
Run-TryBot: Rebecca Stambler <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>

Author: stamblerre

go/packages/golist_overlay.go

@@ -132,6 +132,12 @@ func (state *golistState) processGolistOverlay(response *responseDeduper) (modif
 			pkg.CompiledGoFiles = append(pkg.CompiledGoFiles, opath)
 			modifiedPkgsSet[pkg.ID] = true
 		}
+
+		// Clear out the package's errors, since we've probably corrected
+		// them by adding the overlay. This may eliminate some legitimate
+		// errors, but that's a risk with overlays in general.
+		pkg.Errors = nil
+
 		imports, err := extractImports(opath, contents)
 		if err != nil {
 			// Let the parser or type checker report errors later.

go/packages/packages114_test.go

@@ -15,6 +15,9 @@ import (
 	"golang.org/x/tools/go/packages/packagestest"
 )
 
+// These tests check fixes that are only available in Go 1.14.
+// They can be moved into packages_test.go when we no longer support 1.13.
+// See golang/go#35973 for more information.
 func TestInvalidFilesInOverlay(t *testing.T) { packagestest.TestAll(t, testInvalidFilesInOverlay) }
 func testInvalidFilesInOverlay(t *testing.T, exporter packagestest.Exporter) {
 	exported := packagestest.Export(t, exporter, []packagestest.Module{
@@ -61,6 +64,19 @@ func testInvalidFilesInOverlay(t *testing.T, exporter packagestest.Exporter) {
 					t.Fatal(err)
 				}
 				d := initial[0]
+				var containsFile bool
+				for _, goFile := range d.CompiledGoFiles {
+					if f == goFile {
+						containsFile = true
+						break
+					}
+				}
+				if !containsFile {
+					t.Fatalf("expected %s in CompiledGoFiles, got %v", f, d.CompiledGoFiles)
+				}
+				if len(d.Errors) > 0 {
+					t.Fatalf("expected no errors in package, got %v", d.Errors)
+				}
 				// Check value of d.D.
 				dD := constant(d, "D")
 				if dD == nil {

internal/lsp/cache/load.go

@@ -81,18 +81,14 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) ([]*metadata
 
 	log.Print(ctx, "go/packages.Load", tag.Of("snapshot", s.ID()), tag.Of("query", query), tag.Of("packages", len(pkgs)))
 	if len(pkgs) == 0 {
-		if err == nil {
-			err = errors.Errorf("no packages found for query %s", query)
-		}
 		return nil, err
 	}
 	return s.updateMetadata(ctx, scopes, pkgs, cfg)
 }
 
 // shouldLoad reparses a file's package and import declarations to
-// determine if they have changed.
+// determine if the file requires a metadata reload.
 func (c *cache) shouldLoad(ctx context.Context, s *snapshot, originalFH, currentFH source.FileHandle) bool {
-	// TODO(rstambler): go.mod files should be tracked in the snapshot.
 	if originalFH == nil {
 		return currentFH.Identity().Kind == source.Go
 	}
@@ -112,10 +108,8 @@ func (c *cache) shouldLoad(ctx context.Context, s *snapshot, originalFH, current
 	}
 
 	// Check if the package's metadata has changed. The cases handled are:
-	//
 	//    1. A package's name has changed
 	//    2. A file's imports have changed
-	//
 	if original.Name.Name != current.Name.Name {
 		return true
 	}

internal/lsp/cache/session.go

@@ -96,6 +96,7 @@ func (s *session) createView(ctx context.Context, name string, folder span.URI,
 			importedBy:        make(map[packageID][]packageID),
 			actions:           make(map[actionKey]*actionHandle),
 			workspacePackages: make(map[packageID]packagePath),
+			unloadableFiles:   make(map[span.URI]struct{}),
 		},
 		ignoredURIs: make(map[span.URI]struct{}),
 	}

internal/lsp/cache/snapshot.go

@@ -49,6 +49,9 @@ type snapshot struct {
 	// workspacePackages contains the workspace's packages, which are loaded
 	// when the view is created.
 	workspacePackages map[packageID]packagePath
+
+	// unloadableFiles keeps track of files that we've failed to load.
+	unloadableFiles map[span.URI]struct{}
 }
 
 type packageKey struct {
@@ -425,12 +428,17 @@ func (s *snapshot) addActionHandle(ah *actionHandle) {
 	s.actions[key] = ah
 }
 
-func (s *snapshot) getMetadataForURI(uri span.URI) (metadata []*metadata) {
-	// TODO(matloob): uri can be a file or directory. Should we update the mappings
-	// to map directories to their contained packages?
+func (s *snapshot) getMetadataForURI(uri span.URI) []*metadata {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
+	return s.getMetadataForURILocked(uri)
+}
+
+func (s *snapshot) getMetadataForURILocked(uri span.URI) (metadata []*metadata) {
+	// TODO(matloob): uri can be a file or directory. Should we update the mappings
+	// to map directories to their contained packages?
+
 	for _, id := range s.ids[uri] {
 		if m, ok := s.metadata[id]; ok {
 			metadata = append(metadata, m)
@@ -489,13 +497,6 @@ func (s *snapshot) isWorkspacePackage(id packageID) (packagePath, bool) {
 	return scope, ok
 }
 
-func (s *snapshot) setWorkspacePackage(id packageID, pkgPath packagePath) {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	s.workspacePackages[id] = pkgPath
-}
-
 func (s *snapshot) getFileURIs() []span.URI {
 	s.mu.Lock()
 	defer s.mu.Unlock()
@@ -545,32 +546,115 @@ func (s *snapshot) awaitLoaded(ctx context.Context) error {
 
 // reloadWorkspace reloads the metadata for all invalidated workspace packages.
 func (s *snapshot) reloadWorkspace(ctx context.Context) error {
-	scope := s.workspaceScope(ctx)
-	if scope == nil {
-		return nil
+	// See which of the workspace packages are missing metadata.
+	s.mu.Lock()
+	var pkgPaths []interface{}
+	for id, pkgPath := range s.workspacePackages {
+		if s.metadata[id] == nil {
+			pkgPaths = append(pkgPaths, pkgPath)
+		}
+	}
+	s.mu.Unlock()
+
+	if len(pkgPaths) > 0 {
+		if m, err := s.load(ctx, pkgPaths...); err == nil {
+			for _, m := range m {
+				s.setWorkspacePackage(ctx, m)
+			}
+		}
+	}
+
+	// When we load ./... or a package path directly, we may not get packages
+	// that exist only in overlays. As a workaround, we search all of the files
+	// available in the snapshot and reload their metadata individually using a
+	// file= query if the metadata is unavailable.
+	if scopes := s.orphanedFileScopes(); len(scopes) > 0 {
+		m, err := s.load(ctx, scopes...)
+
+		// If we failed to load some files, i.e. they have no metadata,
+		// mark the failures so we don't bother retrying until the file's
+		// content changes.
+		//
+		// TODO(rstambler): This may be an overestimate if the load stopped
+		// early for an unrelated errors. Add a fallback?
+		//
+		// Check for context cancellation so that we don't incorrectly mark files
+		// as unloadable, but don't return before setting all workspace packages.
+		if ctx.Err() == nil && err != nil {
+			s.mu.Lock()
+			for _, scope := range scopes {
+				uri := span.URI(scope.(fileURI))
+				if s.getMetadataForURILocked(uri) == nil {
+					s.unloadableFiles[uri] = struct{}{}
+				}
+			}
+			s.mu.Unlock()
+		}
+		for _, m := range m {
+			// If a package's files belong to this view, it is a workspace package
+			// and should be added to the set of workspace packages.
+			for _, uri := range m.compiledGoFiles {
+				if !contains(s.view.session.viewsOf(uri), s.view) {
+					continue
+				}
+				s.setWorkspacePackage(ctx, m)
+			}
+		}
 	}
-	_, err := s.load(ctx, scope)
-	return err
+	// Create package handles for all of the workspace packages.
+	for _, id := range s.workspacePackageIDs() {
+		if _, err := s.packageHandle(ctx, id); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
-func (s *snapshot) workspaceScope(ctx context.Context) interface{} {
+func (s *snapshot) orphanedFileScopes() []interface{} {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	var pkgPaths []packagePath
-	for id, pkgPath := range s.workspacePackages {
-		if s.metadata[id] == nil {
-			pkgPaths = append(pkgPaths, pkgPath)
+	scopeSet := make(map[span.URI]struct{})
+	for uri, fh := range s.files {
+		// Don't try to reload metadata for go.mod files.
+		if fh.Identity().Kind != source.Go {
+			continue
+		}
+		// Don't reload metadata for files we've already deemed unloadable.
+		if _, ok := s.unloadableFiles[uri]; ok {
+			continue
+		}
+		if s.getMetadataForURILocked(uri) == nil {
+			scopeSet[uri] = struct{}{}
 		}
 	}
-	switch len(pkgPaths) {
-	case 0:
-		return nil
-	case len(s.workspacePackages):
-		return directoryURI(s.view.folder)
-	default:
-		return pkgPaths
+	var scopes []interface{}
+	for uri := range scopeSet {
+		scopes = append(scopes, fileURI(uri))
 	}
+	return scopes
+}
+
+func contains(views []*view, view *view) bool {
+	for _, v := range views {
+		if v == view {
+			return true
+		}
+	}
+	return false
+}
+
+func (s *snapshot) setWorkspacePackage(ctx context.Context, m *metadata) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	// A test variant of a package can only be loaded directly by loading
+	// the non-test variant with -test. Track the import path of the non-test variant.
+	pkgPath := m.pkgPath
+	if m.forTest != "" {
+		pkgPath = m.forTest
+	}
+	s.workspacePackages[m.id] = pkgPath
 }
 
 func (s *snapshot) clone(ctx context.Context, withoutURIs []span.URI) *snapshot {
@@ -587,12 +671,17 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs []span.URI) *snapshot
 		actions:           make(map[actionKey]*actionHandle),
 		files:             make(map[span.URI]source.FileHandle),
 		workspacePackages: make(map[packageID]packagePath),
+		unloadableFiles:   make(map[span.URI]struct{}),
 	}
 
 	// Copy all of the FileHandles.
 	for k, v := range s.files {
 		result.files[k] = v
 	}
+	// Copy the set of unloadable files.
+	for k, v := range s.unloadableFiles {
+		result.unloadableFiles[k] = v
+	}
 
 	// transitiveIDs keeps track of transitive reverse dependencies.
 	// If an ID is present in the map, invalidate its types.
@@ -664,6 +753,8 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs []span.URI) *snapshot
 		} else {
 			result.files[withoutURI] = currentFH
 		}
+		// Make sure to remove the changed file from the unloadable set.
+		delete(result.unloadableFiles, withoutURI)
 	}
 
 	// Collect the IDs for the packages associated with the excluded URIs.

internal/lsp/cache/view.go

@@ -557,13 +557,7 @@ func (v *view) initialize(ctx context.Context, s *snapshot) {
 					}
 					continue
 				}
-				// A test variant of a package can only be loaded directly by loading
-				// the non-test variant with -test. Track the import path of the non-test variant.
-				pkgPath := m.pkgPath
-				if m.forTest != "" {
-					pkgPath = m.forTest
-				}
-				s.setWorkspacePackage(m.id, pkgPath)
+				s.setWorkspacePackage(ctx, m)
 				if _, err := s.packageHandle(ctx, m.id); err != nil {
 					return err
 				}