internal/lsp/cache: delay transitive invalidation in snapshot.clone

Currently, walking the transitive closure of invalidated packages
happens eagerly while we invalidate each file. Delay this until after
looping through invalidated files, so that we may more easily consider
invalidations not directly associated with an individual file change.

Change-Id: Id6a95098ce6d5d3ec70143c029ac7d2c9b8c7d28
Reviewed-on: https://go-review.googlesource.com/c/tools/+/263205
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/cache/snapshot.go

@@ -1065,19 +1065,10 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Ve
 
 	var modulesChanged, shouldReinitializeView bool
 
-	// transitiveIDs keeps track of transitive reverse dependencies.
-	// If an ID is present in the map, invalidate its types.
-	// If an ID's value is true, invalidate its metadata too.
-	transitiveIDs := make(map[packageID]bool)
+	// directIDs keeps track of package IDs that have directly changed.
+	// It maps id->invalidateMetadata.
+	directIDs := map[packageID]bool{}
 	for withoutURI, currentFH := range withoutURIs {
-		directIDs := map[packageID]struct{}{}
-
-		filePackages := guessPackagesForURI(withoutURI, s.ids)
-		// Collect all of the package IDs that correspond to the given file.
-		// TODO: if the file has moved into a new package, we should invalidate that too.
-		for _, id := range filePackages {
-			directIDs[id] = struct{}{}
-		}
 
 		// The original FileHandle for this URI is cached on the snapshot.
 		originalFH := s.files[withoutURI]
@@ -1086,6 +1077,13 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Ve
 		// and if so, invalidate this file's packages' metadata.
 		invalidateMetadata := forceReloadMetadata || s.shouldInvalidateMetadata(ctx, result, originalFH, currentFH)
 
+		// Mark all of the package IDs containing the given file.
+		// TODO: if the file has moved into a new package, we should invalidate that too.
+		filePackages := guessPackagesForURI(withoutURI, s.ids)
+		for _, id := range filePackages {
+			directIDs[id] = directIDs[id] || invalidateMetadata
+		}
+
 		// Invalidate the previous modTidyHandle if any of the files have been
 		// saved or if any of the metadata has been invalidated.
 		if invalidateMetadata || fileWasSaved(originalFH, currentFH) {
@@ -1116,7 +1114,7 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Ve
 			// the metadata for every known package in the snapshot.
 			if invalidateMetadata {
 				for k := range s.metadata {
-					directIDs[k] = struct{}{}
+					directIDs[k] = true
 				}
 				// If a go.mod file in the workspace has changed, we need to
 				// rebuild the workspace module.
@@ -1162,27 +1160,6 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Ve
 			}
 		}
 
-		// Invalidate reverse dependencies too.
-		// TODO(heschi): figure out the locking model and use transitiveReverseDeps?
-		var addRevDeps func(packageID)
-		addRevDeps = func(id packageID) {
-			current, seen := transitiveIDs[id]
-			newInvalidateMetadata := current || invalidateMetadata
-
-			// If we've already seen this ID, and the value of invalidate
-			// metadata has not changed, we can return early.
-			if seen && current == newInvalidateMetadata {
-				return
-			}
-			transitiveIDs[id] = newInvalidateMetadata
-			for _, rid := range s.getImportedByLocked(id) {
-				addRevDeps(rid)
-			}
-		}
-		for id := range directIDs {
-			addRevDeps(id)
-		}
-
 		// Handle the invalidated file; it may have new contents or not exist.
 		if !currentExists {
 			delete(result.files, withoutURI)
@@ -1193,6 +1170,31 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Ve
 		delete(result.unloadableFiles, withoutURI)
 	}
 
+	// Invalidate reverse dependencies too.
+	// TODO(heschi): figure out the locking model and use transitiveReverseDeps?
+	// transitiveIDs keeps track of transitive reverse dependencies.
+	// If an ID is present in the map, invalidate its types.
+	// If an ID's value is true, invalidate its metadata too.
+	transitiveIDs := make(map[packageID]bool)
+	var addRevDeps func(packageID, bool)
+	addRevDeps = func(id packageID, invalidateMetadata bool) {
+		current, seen := transitiveIDs[id]
+		newInvalidateMetadata := current || invalidateMetadata
+
+		// If we've already seen this ID, and the value of invalidate
+		// metadata has not changed, we can return early.
+		if seen && current == newInvalidateMetadata {
+			return
+		}
+		transitiveIDs[id] = newInvalidateMetadata
+		for _, rid := range s.getImportedByLocked(id) {
+			addRevDeps(rid, invalidateMetadata)
+		}
+	}
+	for id, invalidateMetadata := range directIDs {
+		addRevDeps(id, invalidateMetadata)
+	}
+
 	// When modules change, we need to recompute their workspace directories,
 	// as replace directives may have changed.
 	if modulesChanged {