internal/lsp: load metadata for a single package ID, when needed

The metadata for the workspace packages may not be available when we
need it, so we should allow loading a single package ID. This can be
improved in follow-up CLs by consolidating the individual IDs into one
call to packages.Load. Some adjustments from CL 212102 were split out
into this CL.

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

internal/lsp/cache/check.go

@@ -58,8 +58,8 @@ type packageData struct {
 	err error
 }
 
-// packageHandle returns a source.CheckPackageHandle for a given package and config.
-func (s *snapshot) packageHandle(ctx context.Context, id packageID, mode source.ParseMode) (*packageHandle, error) {
+// buildPackageHandle returns a source.CheckPackageHandle for a given package and config.
+func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID, mode source.ParseMode) (*packageHandle, error) {
 	// Check if we already have this CheckPackageHandle cached.
 	if ph := s.getPackage(id, mode); ph != nil {
 		return ph, nil
@@ -140,7 +140,7 @@ func (s *snapshot) buildKey(ctx context.Context, id packageID, mode source.Parse
 		if s.workspacePackages[depID] {
 			mode = source.ParseFull
 		}
-		depHandle, err := s.packageHandle(ctx, depID, mode)
+		depHandle, err := s.buildPackageHandle(ctx, depID, mode)
 		if err != nil {
 			log.Error(ctx, "no dep handle", err, telemetry.Package.Of(depID))
 

internal/lsp/cache/load.go

@@ -34,24 +34,25 @@ type metadata struct {
 	config *packages.Config
 }
 
-func (s *snapshot) load(ctx context.Context, scope source.Scope) ([]*metadata, error) {
-	uri := scope.URI()
+func (s *snapshot) load(ctx context.Context, scope interface{}) ([]*metadata, error) {
 	var query string
-	switch scope.(type) {
-	case source.FileURI:
-		query = fmt.Sprintf("file=%s", scope.URI().Filename())
-	case source.DirectoryURI:
-		query = fmt.Sprintf("%s/...", scope.URI().Filename())
+	switch scope := scope.(type) {
+	case packageID:
+		query = string(scope)
+	case fileURI:
+		query = fmt.Sprintf("file=%s", span.URI(scope).Filename())
+	case directoryURI:
+		filename := span.URI(scope).Filename()
+		query = fmt.Sprintf("%s/...", filename)
 		// Simplify the query if it will be run in the requested directory.
 		// This ensures compatibility with Go 1.12 that doesn't allow
 		// <directory>/... in GOPATH mode.
-		if s.view.folder.Filename() == scope.URI().Filename() {
+		if s.view.folder.Filename() == filename {
 			query = "./..."
 		}
-	default:
-		panic(fmt.Errorf("unsupported scope type %T", scope))
 	}
-	ctx, done := trace.StartSpan(ctx, "cache.view.load", telemetry.URI.Of(uri))
+
+	ctx, done := trace.StartSpan(ctx, "cache.view.load", telemetry.Query.Of(query))
 	defer done()
 
 	cfg := s.view.Config(ctx)
@@ -60,8 +61,9 @@ func (s *snapshot) load(ctx context.Context, scope source.Scope) ([]*metadata, e
 	// If the context was canceled, return early.
 	// Otherwise, we might be type-checking an incomplete result.
 	if err == context.Canceled {
-		return nil, errors.Errorf("no metadata for %s: %v", uri, err)
+		return nil, err
 	}
+
 	log.Print(ctx, "go/packages.Load", tag.Of("query", query), tag.Of("packages", len(pkgs)))
 	if len(pkgs) == 0 {
 		if err == nil {
@@ -117,13 +119,16 @@ func (c *cache) shouldLoad(ctx context.Context, s *snapshot, originalFH, current
 	return false
 }
 
-func (s *snapshot) updateMetadata(ctx context.Context, uri source.Scope, pkgs []*packages.Package, cfg *packages.Config) ([]*metadata, error) {
+func (s *snapshot) updateMetadata(ctx context.Context, scope interface{}, pkgs []*packages.Package, cfg *packages.Config) ([]*metadata, error) {
 	var results []*metadata
 	for _, pkg := range pkgs {
-		if _, isDir := uri.(source.DirectoryURI); !isDir || s.view.Options().VerboseOutput {
+		if _, isDir := scope.(directoryURI); !isDir || s.view.Options().VerboseOutput {
 			log.Print(ctx, "go/packages.Load", tag.Of("package", pkg.PkgPath), tag.Of("files", pkg.CompiledGoFiles))
 		}
-
+		// Handle golang/go#36292 by ignoring packages with no sources and no errors.
+		if len(pkg.GoFiles) == 0 && len(pkg.CompiledGoFiles) == 0 && len(pkg.Errors) == 0 {
+			continue
+		}
 		// Set the metadata for this package.
 		if err := s.updateImports(ctx, packagePath(pkg.PkgPath), pkg, cfg, map[packageID]struct{}{}); err != nil {
 			return nil, err
@@ -138,7 +143,7 @@ func (s *snapshot) updateMetadata(ctx context.Context, uri source.Scope, pkgs []
 	s.clearAndRebuildImportGraph()
 
 	if len(results) == 0 {
-		return nil, errors.Errorf("no metadata for %s", uri)
+		return nil, errors.Errorf("no metadata for %s", scope)
 	}
 	return results, nil
 }

internal/lsp/cache/pkg.go

@@ -37,6 +37,10 @@ type pkg struct {
 type packageID string
 type packagePath string
 
+// Declare explicit types for files and directories to distinguish between the two.
+type fileURI span.URI
+type directoryURI span.URI
+
 func (p *pkg) ID() string {
 	return string(p.id)
 }

internal/lsp/cache/session.go

@@ -121,7 +121,7 @@ func (s *session) createView(ctx context.Context, name string, folder span.URI,
 	// Perhaps different calls to NewView can be run in parallel?
 	v.snapshotMu.Lock()
 	defer v.snapshotMu.Unlock() // The code after the snapshot is used isn't expensive.
-	m, err := v.snapshot.load(ctx, source.DirectoryURI(folder))
+	m, err := v.snapshot.load(ctx, directoryURI(folder))
 	if err != nil {
 		// Suppress all errors.
 		log.Error(ctx, "failed to load snapshot", err, telemetry.Directory.Of(folder))

internal/lsp/cache/snapshot.go

@@ -73,13 +73,39 @@ func (s *snapshot) PackageHandles(ctx context.Context, fh source.FileHandle) ([]
 
 	ctx = telemetry.File.With(ctx, fh.Identity().URI)
 	meta := s.getMetadataForURI(fh.Identity().URI)
-	// Determine if we need to type-check the package.
-	phs, load, check := s.shouldCheck(meta)
 
-	// We may need to re-load package metadata.
-	// We only need to this if it has been invalidated, and is therefore unvailable.
+	phs, err := s.packageHandles(ctx, fileURI(fh.Identity().URI), meta)
+	if err != nil {
+		return nil, err
+	}
+	var results []source.PackageHandle
+	for _, ph := range phs {
+		results = append(results, ph)
+	}
+	return results, nil
+}
+
+func (s *snapshot) PackageHandle(ctx context.Context, pkgID string) (source.PackageHandle, error) {
+	id := packageID(pkgID)
+	var meta []*metadata
+	if m := s.getMetadata(id); m != nil {
+		meta = append(meta, m)
+	}
+	phs, err := s.packageHandles(ctx, id, meta)
+	if err != nil {
+		return nil, err
+	}
+	if len(phs) > 1 {
+		return nil, errors.Errorf("more than one package for %s", id)
+	}
+	return phs[0], nil
+}
+
+func (s *snapshot) packageHandles(ctx context.Context, scope interface{}, meta []*metadata) ([]*packageHandle, error) {
+	// First, determine if we need to reload or recheck the package.
+	phs, load, check := s.shouldCheck(meta)
 	if load {
-		newMeta, err := s.load(ctx, source.FileURI(fh.Identity().URI))
+		newMeta, err := s.load(ctx, scope)
 		if err != nil {
 			return nil, err
 		}
@@ -91,22 +117,22 @@ func (s *snapshot) PackageHandles(ctx context.Context, fh source.FileHandle) ([]
 		}
 		meta = newMeta
 	}
+	var results []*packageHandle
 	if check {
-		var results []source.PackageHandle
 		for _, m := range meta {
-			ph, err := s.packageHandle(ctx, m.id, source.ParseFull)
+			ph, err := s.buildPackageHandle(ctx, m.id, source.ParseFull)
 			if err != nil {
 				return nil, err
 			}
 			results = append(results, ph)
 		}
-		phs = results
+	} else {
+		results = phs
 	}
-	if len(phs) == 0 {
-		return nil, errors.Errorf("no CheckPackageHandles for %s", fh.Identity().URI)
+	if len(results) == 0 {
+		return nil, errors.Errorf("packageHandles: no package handles for %v", scope)
 	}
-
-	return phs, nil
+	return results, nil
 }
 
 func missingImports(metadata []*metadata) map[packagePath]struct{} {
@@ -131,33 +157,9 @@ func sameSet(x, y map[packagePath]struct{}) bool {
 	return true
 }
 
-func (s *snapshot) PackageHandle(ctx context.Context, id string) (source.PackageHandle, error) {
-	ctx = telemetry.Package.With(ctx, id)
-
-	m := s.getMetadata(packageID(id))
-	if m == nil {
-		return nil, errors.Errorf("no known metadata for %s", id)
-	}
-	// Determine if we need to type-check the package.
-	phs, load, check := s.shouldCheck([]*metadata{m})
-	if load {
-		return nil, errors.Errorf("outdated metadata for %s, needs re-load", id)
-	}
-	if check {
-		return s.packageHandle(ctx, m.id, source.ParseFull)
-	}
-	if len(phs) == 0 {
-		return nil, errors.Errorf("no check package handle for %s", id)
-	}
-	if len(phs) > 1 {
-		return nil, errors.Errorf("multiple check package handles for a single id: %s", id)
-	}
-	return phs[0], nil
-}
-
 // shouldCheck determines if the packages provided by the metadata
 // need to be re-loaded or re-type-checked.
-func (s *snapshot) shouldCheck(m []*metadata) (phs []source.PackageHandle, load, check bool) {
+func (s *snapshot) shouldCheck(m []*metadata) (phs []*packageHandle, load, check bool) {
 	// No metadata. Re-load and re-check.
 	if len(m) == 0 {
 		return nil, true, true
@@ -252,7 +254,7 @@ func (s *snapshot) addPackage(ph *packageHandle) {
 func (s *snapshot) checkWorkspacePackages(ctx context.Context, m []*metadata) ([]source.PackageHandle, error) {
 	var phs []source.PackageHandle
 	for _, m := range m {
-		ph, err := s.packageHandle(ctx, m.id, source.ParseFull)
+		ph, err := s.buildPackageHandle(ctx, m.id, source.ParseFull)
 		if err != nil {
 			return nil, err
 		}
@@ -272,43 +274,46 @@ func (s *snapshot) WorkspacePackageIDs(ctx context.Context) (ids []string) {
 	return ids
 }
 
-func (s *snapshot) KnownPackages(ctx context.Context) []source.Package {
-	// TODO(matloob): This function exists because KnownImportPaths can't
-	// determine the import paths of all packages. Remove this function
-	// if KnownImportPaths gains that ability. That could happen if
-	// go list or go packages provide that information.
-	pkgIDs := make(map[packageID]bool)
+func (s *snapshot) KnownPackages(ctx context.Context) []source.PackageHandle {
+	// Collect PackageHandles for all of the workspace packages first.
+	// They may need to be reloaded if their metadata has been invalidated.
+	wsPackages := make(map[packageID]bool)
 	s.mu.Lock()
-	for _, m := range s.metadata {
-		pkgIDs[m.id] = true
-	}
-	// Add in all the workspacePackages in case the've been invalidated
-	// in the metadata since their initial load.
 	for id := range s.workspacePackages {
-		pkgIDs[id] = true
+		wsPackages[id] = true
+	}
+	s.mu.Unlock()
+
+	var results []source.PackageHandle
+	for pkgID := range wsPackages {
+		ph, err := s.PackageHandle(ctx, string(pkgID))
+		if err != nil {
+			log.Error(ctx, "KnownPackages: failed to create PackageHandle", err, telemetry.Package.Of(pkgID))
+			continue
+		}
+		results = append(results, ph)
+	}
+
+	// Once all workspace packages have been checked, the metadata will be up-to-date.
+	// Add all packages known in the workspace (that haven't already been added).
+	pkgIDs := make(map[packageID]bool)
+	s.mu.Lock()
+	for id := range s.metadata {
+		if !wsPackages[id] {
+			pkgIDs[id] = true
+		}
 	}
 	s.mu.Unlock()
 
-	var results []source.Package
 	for pkgID := range pkgIDs {
-		mode := source.ParseExported
-		if s.workspacePackages[pkgID] {
-			// Any package in our workspace should be loaded with ParseFull.
-			mode = source.ParseFull
-		}
-		ph, err := s.packageHandle(ctx, pkgID, mode)
+		// Metadata for these packages should already be up-to-date,
+		// so just build the package handle directly (without a reload).
+		ph, err := s.buildPackageHandle(ctx, pkgID, source.ParseExported)
 		if err != nil {
-			log.Error(ctx, "failed to create CheckPackageHandle", err, telemetry.Package.Of(pkgID))
+			log.Error(ctx, "KnownPackages: failed to create PackageHandle", err, telemetry.Package.Of(pkgID))
 			continue
 		}
-		// Check the package now if it's not checked yet.
-		// TODO(matloob): is this too slow?
-		pkg, err := ph.check(ctx)
-		if err != nil {
-			log.Error(ctx, "failed to check package", err, telemetry.Package.Of(pkgID))
-			continue
-		}
-		results = append(results, pkg)
+		results = append(results, ph)
 	}
 
 	return results

internal/lsp/source/implementation.go

@@ -97,7 +97,11 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol.
 			allNamed []*types.Named
 			pkgs     = make(map[*types.Package]Package)
 		)
-		for _, pkg := range s.KnownPackages(ctx) {
+		for _, ph := range s.KnownPackages(ctx) {
+			pkg, err := ph.Check(ctx)
+			if err != nil {
+				return nil, err
+			}
 			pkgs[pkg.GetTypes()] = pkg
 
 			info := pkg.GetTypesInfo()

internal/lsp/source/view.go

@@ -59,7 +59,7 @@ type Snapshot interface {
 	KnownImportPaths() map[string]Package
 
 	// KnownPackages returns all the packages loaded in this snapshot.
-	KnownPackages(ctx context.Context) []Package
+	KnownPackages(ctx context.Context) []PackageHandle
 }
 
 // PackageHandle represents a handle to a specific version of a package.
@@ -327,22 +327,6 @@ const (
 	UnknownKind
 )
 
-type FileURI span.URI
-
-func (f FileURI) URI() span.URI {
-	return span.URI(f)
-}
-
-type DirectoryURI span.URI
-
-func (d DirectoryURI) URI() span.URI {
-	return span.URI(d)
-}
-
-type Scope interface {
-	URI() span.URI
-}
-
 // Package represents a Go package that has been type-checked. It maintains
 // only the relevant fields of a *go/packages.Package.
 type Package interface {

internal/lsp/telemetry/telemetry.go

@@ -24,6 +24,7 @@ const (
 	URI           = tag.Key("URI")
 	Package       = tag.Key("package")
 	PackagePath   = tag.Key("package_path")
+	Query         = tag.Key("query")
 )
 
 var (