internal/lsp/cache: invalidate analysis results on packages.Load

Most invalidation happens in snapshot.clone, but to be safe we were also
invalidating package data when new metadata is set via packages.Load. At
the time, I wasn't sure why this was necessary.

Now I understand: with experimentalUseInvalidMetadata it is possible
that we re-compute invalidated data using stale metadata in between the
initial clone and the subsequent reload. I noticed that it was also
possible to have stale analysis results after the Load results arrive.

Fix this by invalidating analysis results on Load, in addition to
packages. Factor out invalidation into a new helper method.

Since we believe this may fix analyzer panics, re-enable strict handling
of analysis panics during tests.

For golang/go#56035
For golang/go#54762
For golang/go#42857

Change-Id: I8c28e0700f8c16c58df4ecf60f6127b1c05d6dc5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/420538
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

gopls/internal/lsp/cache/analysis.go

@@ -365,10 +365,10 @@ func actionImpl(ctx context.Context, snapshot *snapshot, deps []*actionHandle, a
 			if r := recover(); r != nil {
 				// An Analyzer crashed. This is often merely a symptom
 				// of a problem in package loading.
-				// Now that we have a theory of these crashes,
-				// we disable the check to stop flakes from being a nuisance.
-				// TODO(adonovan): re-enable it when plausibly fixed.
-				const strict = false
+				//
+				// We believe that CL 420538 may have fixed these crashes, so enable
+				// strict checks in tests.
+				const strict = true
 				if strict && bug.PanicOnBugs && analyzer.Name != "fact_purity" {
 					// During testing, crash. See issues 54762, 56035.
 					// But ignore analyzers with known crash bugs:

gopls/internal/lsp/cache/graph.go

@@ -134,20 +134,20 @@ func (g *metadataGraph) build() {
 //
 // If includeInvalid is false, the algorithm ignores packages with invalid
 // metadata (including those in the given list of ids).
-func (g *metadataGraph) reverseTransitiveClosure(includeInvalid bool, ids ...PackageID) map[PackageID]struct{} {
-	seen := make(map[PackageID]struct{})
+func (g *metadataGraph) reverseTransitiveClosure(includeInvalid bool, ids ...PackageID) map[PackageID]bool {
+	seen := make(map[PackageID]bool)
 	var visitAll func([]PackageID)
 	visitAll = func(ids []PackageID) {
 		for _, id := range ids {
-			if _, ok := seen[id]; ok {
+			if seen[id] {
 				continue
 			}
 			m := g.metadata[id]
 			// Only use invalid metadata if we support it.
 			if m == nil || !(m.Valid || includeInvalid) {
 				continue
 			}
-			seen[id] = struct{}{}
+			seen[id] = true
 			visitAll(g.importedBy[id])
 		}
 	}

gopls/internal/lsp/cache/load.go

@@ -228,16 +228,17 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 	s.meta = s.meta.Clone(updates)
 	s.resetIsActivePackageLocked()
 
-	// Invalidate any packages we may have associated with this metadata.
+	// Invalidate any packages and analysis results we may have associated with
+	// this metadata.
 	//
-	// TODO(rfindley): this should not be necessary, as we should have already
-	// invalidated in snapshot.clone.
-	for id := range invalidatedPackages {
-		for _, mode := range source.AllParseModes {
-			key := packageKey{mode, id}
-			s.packages.Delete(key)
-		}
-	}
+	// Generally speaking we should have already invalidated these results in
+	// snapshot.clone, but with experimentalUseInvalidMetadata is may be possible
+	// that we have re-computed stale results before the reload completes. In
+	// this case, we must re-invalidate here.
+	//
+	// TODO(golang/go#54180): if we decide to make experimentalUseInvalidMetadata
+	// obsolete, we should avoid this invalidation.
+	s.invalidatePackagesLocked(invalidatedPackages)
 
 	s.workspacePackages = computeWorkspacePackagesLocked(s, s.meta)
 	s.dumpWorkspace("load")

gopls/internal/lsp/cache/snapshot.go

@@ -1856,26 +1856,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 		addRevDeps(id, invalidateMetadata)
 	}
 
-	// Delete invalidated package type information.
-	for id := range idsToInvalidate {
-		for _, mode := range source.AllParseModes {
-			key := packageKey{mode, id}
-			result.packages.Delete(key)
-		}
-	}
-
-	// Copy actions.
-	// TODO(adonovan): opt: avoid iteration over s.actions.
-	var actionsToDelete []actionKey
-	s.actions.Range(func(k, _ interface{}) {
-		key := k.(actionKey)
-		if _, ok := idsToInvalidate[key.pkgid]; ok {
-			actionsToDelete = append(actionsToDelete, key)
-		}
-	})
-	for _, key := range actionsToDelete {
-		result.actions.Delete(key)
-	}
+	result.invalidatePackagesLocked(idsToInvalidate)
 
 	// If a file has been deleted, we must delete metadata for all packages
 	// containing that file.
@@ -2050,6 +2031,35 @@ func invalidatedPackageIDs(uri span.URI, known map[span.URI][]PackageID, package
 	return invalidated
 }
 
+// invalidatePackagesLocked deletes data associated with the given package IDs.
+//
+// Note: all keys in the ids map are invalidated, regardless of the
+// corresponding value.
+//
+// s.mu must be held while calling this function.
+func (s *snapshot) invalidatePackagesLocked(ids map[PackageID]bool) {
+	// Delete invalidated package type information.
+	for id := range ids {
+		for _, mode := range source.AllParseModes {
+			key := packageKey{mode, id}
+			s.packages.Delete(key)
+		}
+	}
+
+	// Copy actions.
+	// TODO(adonovan): opt: avoid iteration over s.actions.
+	var actionsToDelete []actionKey
+	s.actions.Range(func(k, _ interface{}) {
+		key := k.(actionKey)
+		if _, ok := ids[key.pkgid]; ok {
+			actionsToDelete = append(actionsToDelete, key)
+		}
+	})
+	for _, key := range actionsToDelete {
+		s.actions.Delete(key)
+	}
+}
+
 // fileWasSaved reports whether the FileHandle passed in has been saved. It
 // accomplishes this by checking to see if the original and current FileHandles
 // are both overlays, and if the current FileHandle is saved while the original