From c36379be2b347665c06d85bea46f9d4b977ec3ce Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 24 Jun 2022 11:31:51 -0400 Subject: [PATCH] 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 TryBot-Result: Gopher Robot gopls-CI: kokoro Reviewed-by: Alan Donovan --- internal/lsp/cache/graph.go | 1 - internal/lsp/cache/load.go | 38 +++++++++++++++++++++++++++------- internal/lsp/cache/snapshot.go | 5 +---- 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/internal/lsp/cache/graph.go b/internal/lsp/cache/graph.go index 36e658b3a8..3f247739fd 100644 --- a/internal/lsp/cache/graph.go +++ b/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 diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 3c6795370d..bdafdcd115 100644 --- a/internal/lsp/cache/load.go +++ b/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) } } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index c8d6085333..05f892808f 100644 --- a/internal/lsp/cache/snapshot.go +++ b/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 }