internal/lsp: set workspace packages during (*snapshot).load

We can assume that any package we load directly will be a package in the
workspace, so it's reasonable to set workspace packages at that point.
We're guaranteed not to directly load dependencies, or we may end up
with indirect dependencies in the go.mod file.

Change-Id: I5d406e54da2bc3278b139c75b436d111b5564418
Reviewed-on: https://go-review.googlesource.com/c/tools/+/216726
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

internal/lsp/cache/load.go

@@ -39,6 +39,7 @@ type metadata struct {
 
 func (s *snapshot) load(ctx context.Context, scopes ...interface{}) ([]*metadata, error) {
 	var query []string
+	var containsDir bool // for logging
 	for _, scope := range scopes {
 		switch scope := scope.(type) {
 		case packagePath:
@@ -69,6 +70,10 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) ([]*metadata
 		default:
 			panic(fmt.Sprintf("unknown scope type %T", scope))
 		}
+		switch scope.(type) {
+		case directoryURI, viewLoadScope:
+			containsDir = true
+		}
 	}
 	sort.Strings(query) // for determinism
 
@@ -89,20 +94,8 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) ([]*metadata
 	if len(pkgs) == 0 {
 		return nil, err
 	}
-	return s.updateMetadata(ctx, scopes, pkgs, cfg)
-}
-
-func (s *snapshot) updateMetadata(ctx context.Context, scopes []interface{}, pkgs []*packages.Package, cfg *packages.Config) ([]*metadata, error) {
 	var results []*metadata
 	for _, pkg := range pkgs {
-		// Don't log output for full workspace packages.Loads.
-		var containsDir bool
-		for _, scope := range scopes {
-			switch scope.(type) {
-			case directoryURI, viewLoadScope:
-				containsDir = true
-			}
-		}
 		if !containsDir || s.view.Options().VerboseOutput {
 			log.Print(ctx, "go/packages.Load", tag.Of("snapshot", s.ID()), tag.Of("package", pkg.PkgPath), tag.Of("files", pkg.CompiledGoFiles))
 		}
@@ -116,12 +109,17 @@ func (s *snapshot) updateMetadata(ctx context.Context, scopes []interface{}, pkg
 			continue
 		}
 		// Set the metadata for this package.
-		if err := s.updateImports(ctx, packagePath(pkg.PkgPath), pkg, cfg, map[packageID]struct{}{}); err != nil {
+		m, err := s.setMetadata(ctx, packagePath(pkg.PkgPath), pkg, cfg, map[packageID]struct{}{})
+		if err != nil {
 			return nil, err
 		}
-		if m := s.getMetadata(packageID(pkg.ID)); m != nil {
-			results = append(results, m)
+		// All packages returned by packages.Load will be top-level packages,
+		// with dependencies in the Imports field. Therefore, we can assume that
+		// they are all workspace packages and mark them as such.
+		if err := s.setWorkspacePackage(ctx, m); err != nil {
+			return nil, err
 		}
+		results = append(results, m)
 	}
 
 	// Rebuild the import graph when the metadata is updated.
@@ -133,10 +131,30 @@ func (s *snapshot) updateMetadata(ctx context.Context, scopes []interface{}, pkg
 	return results, nil
 }
 
-func (s *snapshot) updateImports(ctx context.Context, pkgPath packagePath, pkg *packages.Package, cfg *packages.Config, seen map[packageID]struct{}) error {
+func (s *snapshot) setWorkspacePackage(ctx context.Context, m *metadata) error {
+	// Make sure that the builtin package doesn't get marked a workspace package.
+	if m.pkgPath == "builtin" {
+		return nil
+	}
+	// 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.mu.Lock()
+	s.workspacePackages[m.id] = pkgPath
+	s.mu.Unlock()
+
+	_, err := s.packageHandle(ctx, m.id)
+	return err
+}
+
+func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *packages.Package, cfg *packages.Config, seen map[packageID]struct{}) (*metadata, error) {
 	id := packageID(pkg.ID)
 	if _, ok := seen[id]; ok {
-		return errors.Errorf("import cycle detected: %q", id)
+		return nil, errors.Errorf("import cycle detected: %q", id)
 	}
 	// Recreate the metadata rather than reusing it to avoid locking.
 	m := &metadata{
@@ -160,8 +178,9 @@ func (s *snapshot) updateImports(ctx context.Context, pkgPath packagePath, pkg *
 		s.addID(uri, m.id)
 	}
 
-	seen[id] = struct{}{}
-	copied := make(map[packageID]struct{})
+	copied := map[packageID]struct{}{
+		id: struct{}{},
+	}
 	for k, v := range seen {
 		copied[k] = v
 	}
@@ -180,14 +199,24 @@ func (s *snapshot) updateImports(ctx context.Context, pkgPath packagePath, pkg *
 			continue
 		}
 		if s.getMetadata(importID) == nil {
-			if err := s.updateImports(ctx, importPkgPath, importPkg, cfg, copied); err != nil {
+			if _, err := s.setMetadata(ctx, importPkgPath, importPkg, cfg, copied); err != nil {
 				log.Error(ctx, "error in dependency", err)
 			}
 		}
 	}
 	// Add the metadata to the cache.
-	s.setMetadata(m)
-	return nil
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	// TODO: We should make sure not to set duplicate metadata,
+	// and instead panic here. This can be done by making sure not to
+	// reset metadata information for packages we've already seen.
+	if orig, ok := s.metadata[m.id]; ok {
+		return orig, nil
+	} else {
+		s.metadata[m.id] = m
+		return m, nil
+	}
 }
 
 func isTestMain(ctx context.Context, pkg *packages.Package, gocache string) bool {

internal/lsp/cache/snapshot.go

@@ -449,19 +449,6 @@ func (s *snapshot) getMetadataForURILocked(uri span.URI) (metadata []*metadata)
 	return metadata
 }
 
-func (s *snapshot) setMetadata(m *metadata) {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	// TODO: We should make sure not to set duplicate metadata,
-	// and instead panic here. This can be done by making sure not to
-	// reset metadata information for packages we've already seen.
-	if _, ok := s.metadata[m.id]; ok {
-		return
-	}
-	s.metadata[m.id] = m
-}
-
 func (s *snapshot) getMetadata(id packageID) *metadata {
 	s.mu.Lock()
 	defer s.mu.Unlock()
@@ -512,31 +499,19 @@ func (s *snapshot) awaitLoaded(ctx context.Context) error {
 	// Do not return results until the snapshot's view has been initialized.
 	s.view.awaitInitialized(ctx)
 
-	m, err := s.reloadWorkspace(ctx)
-	if err != nil {
+	if err := s.reloadWorkspace(ctx); err != nil {
 		return err
 	}
-	for _, m := range m {
-		s.setWorkspacePackage(ctx, m)
-	}
-	if err := s.reloadOrphanedFiles(ctx); err != nil {
-		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
+	return s.reloadOrphanedFiles(ctx)
 }
 
 // reloadWorkspace reloads the metadata for all invalidated workspace packages.
-func (s *snapshot) reloadWorkspace(ctx context.Context) ([]*metadata, error) {
+func (s *snapshot) reloadWorkspace(ctx context.Context) error {
 	// If the view's build configuration is invalid, we cannot reload by package path.
 	// Just reload the directory instead.
 	if !s.view.hasValidBuildConfiguration {
-		return s.load(ctx, viewLoadScope("LOAD_INVALID_VIEW"))
+		_, err := s.load(ctx, viewLoadScope("LOAD_INVALID_VIEW"))
+		return err
 	}
 
 	// See which of the workspace packages are missing metadata.
@@ -550,10 +525,10 @@ func (s *snapshot) reloadWorkspace(ctx context.Context) ([]*metadata, error) {
 	s.mu.Unlock()
 
 	if len(pkgPaths) == 0 {
-		return nil, nil
+		return nil
 	}
-
-	return s.load(ctx, pkgPaths...)
+	_, err := s.load(ctx, pkgPaths...)
+	return err
 }
 
 func (s *snapshot) reloadOrphanedFiles(ctx context.Context) error {
@@ -566,7 +541,7 @@ func (s *snapshot) reloadOrphanedFiles(ctx context.Context) error {
 		return nil
 	}
 
-	m, err := s.load(ctx, scopes...)
+	_, 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
@@ -587,9 +562,6 @@ func (s *snapshot) reloadOrphanedFiles(ctx context.Context) error {
 		}
 		s.mu.Unlock()
 	}
-	for _, m := range m {
-		s.setWorkspacePackage(ctx, m)
-	}
 	return nil
 }
 
@@ -632,19 +604,6 @@ func contains(views []*view, view *view) bool {
 	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 {
 	s.mu.Lock()
 	defer s.mu.Unlock()

internal/lsp/cache/view.go

@@ -551,22 +551,13 @@ func (v *view) initialize(ctx context.Context, s *snapshot) {
 			if err != nil {
 				return err
 			}
-			// Keep track of the workspace packages.
+			// Find the builtin package in order to handle it separately.
 			for _, m := range meta {
-				// Make sure to handle the builtin package separately
-				// Don't set it as a workspace package.
 				if m.pkgPath == "builtin" {
-					if err := s.view.buildBuiltinPackage(ctx, m); err != nil {
-						return err
-					}
-					continue
-				}
-				s.setWorkspacePackage(ctx, m)
-				if _, err := s.packageHandle(ctx, m.id); err != nil {
-					return err
+					return s.view.buildBuiltinPackage(ctx, m)
 				}
 			}
-			return nil
+			return errors.Errorf("failed to load the builtin package")
 		}()
 		if err != nil {
 			log.Error(ctx, "initial workspace load failed", err)