internal/lsp/cache: don't set new metadata if existing is valid

If gopls believes it has valid metadata for a package, don't set new
metadata. This is consistent with previous behavior that was changed in
CL 340851.

In principle this shouldn't matter, but in practice there are places
where gopls doesn't yet want to invalidate packages, *even though* their
metadata may have changed (such as while editing a go.mod file before
saving). In the future we should eliminate these places, but for now we
should let snapshot.clone control this invalidation.

This also reduces the number of type-checked packages we invalidate on
load.

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

internal/lsp/cache/graph.go

@@ -18,7 +18,6 @@ import (
 // TODO(rfindley): make this type immutable, so that it may be shared across
 // snapshots.
 type metadataGraph struct {
-
 	// metadata maps package IDs to their associated metadata.
 	metadata map[PackageID]*KnownMetadata
 

internal/lsp/cache/load.go

@@ -444,24 +444,42 @@ func getWorkspaceDir(ctx context.Context, h *memoize.Handle, g *memoize.Generati
 // metadata exists for all dependencies.
 func computeMetadataUpdates(ctx context.Context, g *metadataGraph, pkgPath PackagePath, pkg *packages.Package, cfg *packages.Config, query []string, updates map[PackageID]*KnownMetadata, path []PackageID) error {
 	id := PackageID(pkg.ID)
-	if new := updates[id]; new != nil {
-		return nil
-	}
 	if source.IsCommandLineArguments(pkg.ID) {
 		suffix := ":" + strings.Join(query, ",")
 		id = PackageID(string(id) + suffix)
 		pkgPath = PackagePath(string(pkgPath) + suffix)
 	}
+
+	// If we have valid metadata for this package, don't update. This minimizes
+	// the amount of subsequent invalidation.
+	//
+	// TODO(rfindley): perform a sanity check that metadata matches here. If not,
+	// we have an invalidation bug elsewhere.
+	if existing := g.metadata[id]; existing != nil && existing.Valid {
+		return nil
+	}
+
 	if _, ok := updates[id]; ok {
 		// If we've already seen this dependency, there may be an import cycle, or
 		// we may have reached the same package transitively via distinct paths.
 		// Check the path to confirm.
+
+		// TODO(rfindley): this doesn't look right. Any single piece of new
+		// metadata could theoretically introduce import cycles in the metadata
+		// graph. What's the point of this limited check here (and is it even
+		// possible to get an import cycle in data from go/packages)? Consider
+		// simply returning, so that this function need not return an error.
+		//
+		// We should consider doing a more complete guard against import cycles
+		// elsewhere.
 		for _, prev := range path {
 			if prev == id {
 				return fmt.Errorf("import cycle detected: %q", id)
 			}
 		}
+		return nil
 	}
+
 	// Recreate the metadata rather than reusing it to avoid locking.
 	m := &KnownMetadata{
 		Metadata: &Metadata{
@@ -504,6 +522,14 @@ func computeMetadataUpdates(ctx context.Context, g *metadataGraph, pkgPath Packa
 	}
 
 	for importPath, importPkg := range pkg.Imports {
+		// TODO(rfindley): in rare cases it is possible that the import package
+		// path is not the same as the package path of the import. That is to say
+		// (quoting adonovan):
+		// "The importPath string is the path by which one package is imported from
+		// another, but that needn't be the same as its internal name (sometimes
+		// called the "package path") used to prefix its linker symbols"
+		//
+		// We should not set this package path on the metadata of the dep.
 		importPkgPath := PackagePath(importPath)
 		importID := PackageID(importPkg.ID)
 
@@ -517,10 +543,8 @@ func computeMetadataUpdates(ctx context.Context, g *metadataGraph, pkgPath Packa
 			m.MissingDeps[importPkgPath] = struct{}{}
 			continue
 		}
-		if noValidMetadataForID(g, importID) {
-			if err := computeMetadataUpdates(ctx, g, importPkgPath, importPkg, cfg, query, updates, append(path, id)); err != nil {
-				event.Error(ctx, "error in dependency", err)
-			}
+		if err := computeMetadataUpdates(ctx, g, importPkgPath, importPkg, cfg, query, updates, append(path, id)); err != nil {
+			event.Error(ctx, "error in dependency", err)
 		}
 	}
 

internal/lsp/cache/snapshot.go

@@ -1304,11 +1304,8 @@ func (s *snapshot) noValidMetadataForURILocked(uri span.URI) bool {
 func (s *snapshot) noValidMetadataForID(id PackageID) bool {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	return noValidMetadataForID(s.meta, id)
-}
 
-func noValidMetadataForID(g *metadataGraph, id PackageID) bool {
-	m := g.metadata[id]
+	m := s.meta.metadata[id]
 	return m == nil || !m.Valid
 }