internal/lsp/cache: tweaks to metadata graph

- ignore error from buildPackageHandle, with rationale.
- remove unnessary optimization in call to Clone(updates={}):
  Clone does the same check internally.
- more comments.

Change-Id: I4551cf560aea722d972fb6da404aed71a79f4037
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416217
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>

4 files changed, 12 insertions, 24 deletions

diff --git a/internal/lsp/cache/graph.go b/internal/lsp/cache/graph.go
index c1beff828..d0f80cc40 100644
--- a/internal/lsp/cache/graph.go
+++ b/internal/lsp/cache/graph.go
@@ -11,12 +11,8 @@ import (
 	"golang.org/x/tools/internal/span"
 )
 
-// A metadataGraph holds information about a transtively closed import graph of
-// Go packages, as obtained from go/packages.
-//
-// Currently a new metadata graph is created for each snapshot.
-// TODO(rfindley): make this type immutable, so that it may be shared across
-// snapshots.
+// A metadataGraph is an immutable and transitively closed import
+// graph of Go packages, as obtained from go/packages.
 type metadataGraph struct {
 	// metadata maps package IDs to their associated metadata.
 	metadata map[PackageID]*KnownMetadata
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index b15c5374e..6d9c59860 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -93,9 +93,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 
 	if s.view.Options().VerboseWorkDoneProgress {
 		work := s.view.session.progress.Start(ctx, "Load", fmt.Sprintf("Loading query=%s", query), nil, nil)
-		defer func() {
-			work.End(ctx, "Done.")
-		}()
+		defer work.End(ctx, "Done.")
 	}
 
 	ctx, done := event.Start(ctx, "cache.view.load", tag.Query.Of(query))
@@ -241,14 +239,13 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 	s.dumpWorkspace("load")
 	s.mu.Unlock()
 
-	// Rebuild the workspace package handle for any packages we invalidated.
+	// Recompute the workspace package handle for any packages we invalidated.
 	//
-	// 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.
+	// This is (putatively) an optimization since handle
+	// construction prefetches the content of all Go source files.
+	// It is safe to ignore errors, or omit this step entirely.
 	for _, m := range updates {
-		if _, err := s.buildPackageHandle(ctx, m.ID, s.workspaceParseMode(m.ID)); err != nil {
-			return err
-		}
+		s.buildPackageHandle(ctx, m.ID, s.workspaceParseMode(m.ID)) // ignore error
 	}
 
 	if len(moduleErrs) > 0 {
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 0fa670cf2..87ef595c3 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -1986,13 +1986,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 	}
 
 	// Update metadata, if necessary.
-	if len(metadataUpdates) > 0 {
-		result.meta = s.meta.Clone(metadataUpdates)
-	} else {
-		// No metadata changes. Since metadata is only updated by cloning, it is
-		// safe to re-use the existing metadata here.
-		result.meta = s.meta
-	}
+	result.meta = s.meta.Clone(metadataUpdates)
 
 	// Update workspace and active packages, if necessary.
 	if result.meta != s.meta || anyFileOpenedOrClosed {
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index b23ed614f..9d1ee5581 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -710,8 +710,9 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) {
 		scopes = append(scopes, viewLoadScope("LOAD_VIEW"))
 	}
 
-	// If we're loading anything, ensure we also load builtin.
-	// TODO(rstambler): explain the rationale for this.
+	// If we're loading anything, ensure we also load builtin,
+	// since it provides fake definitions (and documentation)
+	// for types like int that are used everywhere.
 	if len(scopes) > 0 {
 		scopes = append(scopes, PackagePath("builtin"))
 	}