From cd0288ff8539478585b7c3bdba0a33b9d6c63266 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 1 Aug 2022 14:17:38 -0400 Subject: [PATCH] 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 TryBot-Result: Gopher Robot gopls-CI: kokoro Reviewed-by: Alan Donovan --- gopls/internal/lsp/cache/analysis.go | 8 ++--- gopls/internal/lsp/cache/graph.go | 8 ++--- gopls/internal/lsp/cache/load.go | 19 ++++++----- gopls/internal/lsp/cache/snapshot.go | 50 +++++++++++++++++----------- 4 files changed, 48 insertions(+), 37 deletions(-) diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go index 944485c3cd8..d41116c3400 100644 --- a/gopls/internal/lsp/cache/analysis.go +++ b/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: diff --git a/gopls/internal/lsp/cache/graph.go b/gopls/internal/lsp/cache/graph.go index a801c83a9ab..3d530c45ab1 100644 --- a/gopls/internal/lsp/cache/graph.go +++ b/gopls/internal/lsp/cache/graph.go @@ -134,12 +134,12 @@ 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] @@ -147,7 +147,7 @@ func (g *metadataGraph) reverseTransitiveClosure(includeInvalid bool, ids ...Pac if m == nil || !(m.Valid || includeInvalid) { continue } - seen[id] = struct{}{} + seen[id] = true visitAll(g.importedBy[id]) } } diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go index 2550dfba4be..3ef5301f83c 100644 --- a/gopls/internal/lsp/cache/load.go +++ b/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") diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index d7e39b31fb5..1587a5d178d 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/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