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 }