From 234df48a208cb23f778dd5a7c48315b528a1b0f3 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Fri, 27 Dec 2019 16:44:33 -0500 Subject: [PATCH] internal/lsp: load metadata for a single package ID, when needed The metadata for the workspace packages may not be available when we need it, so we should allow loading a single package ID. This can be improved in follow-up CLs by consolidating the individual IDs into one call to packages.Load. Some adjustments from CL 212102 were split out into this CL. Change-Id: I173a79a3cb136530bc99d093f1c2be189eac8ce2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/212628 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/check.go | 6 +- internal/lsp/cache/load.go | 37 ++++--- internal/lsp/cache/pkg.go | 4 + internal/lsp/cache/session.go | 2 +- internal/lsp/cache/snapshot.go | 137 +++++++++++++------------- internal/lsp/source/implementation.go | 6 +- internal/lsp/source/view.go | 18 +--- internal/lsp/telemetry/telemetry.go | 1 + 8 files changed, 107 insertions(+), 104 deletions(-) diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 2298fab4b0..87b7ee2102 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -58,8 +58,8 @@ type packageData struct { err error } -// packageHandle returns a source.CheckPackageHandle for a given package and config. -func (s *snapshot) packageHandle(ctx context.Context, id packageID, mode source.ParseMode) (*packageHandle, error) { +// buildPackageHandle returns a source.CheckPackageHandle for a given package and config. +func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID, mode source.ParseMode) (*packageHandle, error) { // Check if we already have this CheckPackageHandle cached. if ph := s.getPackage(id, mode); ph != nil { return ph, nil @@ -140,7 +140,7 @@ func (s *snapshot) buildKey(ctx context.Context, id packageID, mode source.Parse if s.workspacePackages[depID] { mode = source.ParseFull } - depHandle, err := s.packageHandle(ctx, depID, mode) + depHandle, err := s.buildPackageHandle(ctx, depID, mode) if err != nil { log.Error(ctx, "no dep handle", err, telemetry.Package.Of(depID)) diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 5b55f14744..2dfce37655 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -34,24 +34,25 @@ type metadata struct { config *packages.Config } -func (s *snapshot) load(ctx context.Context, scope source.Scope) ([]*metadata, error) { - uri := scope.URI() +func (s *snapshot) load(ctx context.Context, scope interface{}) ([]*metadata, error) { var query string - switch scope.(type) { - case source.FileURI: - query = fmt.Sprintf("file=%s", scope.URI().Filename()) - case source.DirectoryURI: - query = fmt.Sprintf("%s/...", scope.URI().Filename()) + switch scope := scope.(type) { + case packageID: + query = string(scope) + case fileURI: + query = fmt.Sprintf("file=%s", span.URI(scope).Filename()) + case directoryURI: + filename := span.URI(scope).Filename() + query = fmt.Sprintf("%s/...", filename) // Simplify the query if it will be run in the requested directory. // This ensures compatibility with Go 1.12 that doesn't allow // /... in GOPATH mode. - if s.view.folder.Filename() == scope.URI().Filename() { + if s.view.folder.Filename() == filename { query = "./..." } - default: - panic(fmt.Errorf("unsupported scope type %T", scope)) } - ctx, done := trace.StartSpan(ctx, "cache.view.load", telemetry.URI.Of(uri)) + + ctx, done := trace.StartSpan(ctx, "cache.view.load", telemetry.Query.Of(query)) defer done() cfg := s.view.Config(ctx) @@ -60,8 +61,9 @@ func (s *snapshot) load(ctx context.Context, scope source.Scope) ([]*metadata, e // If the context was canceled, return early. // Otherwise, we might be type-checking an incomplete result. if err == context.Canceled { - return nil, errors.Errorf("no metadata for %s: %v", uri, err) + return nil, err } + log.Print(ctx, "go/packages.Load", tag.Of("query", query), tag.Of("packages", len(pkgs))) if len(pkgs) == 0 { if err == nil { @@ -117,13 +119,16 @@ func (c *cache) shouldLoad(ctx context.Context, s *snapshot, originalFH, current return false } -func (s *snapshot) updateMetadata(ctx context.Context, uri source.Scope, pkgs []*packages.Package, cfg *packages.Config) ([]*metadata, error) { +func (s *snapshot) updateMetadata(ctx context.Context, scope interface{}, pkgs []*packages.Package, cfg *packages.Config) ([]*metadata, error) { var results []*metadata for _, pkg := range pkgs { - if _, isDir := uri.(source.DirectoryURI); !isDir || s.view.Options().VerboseOutput { + if _, isDir := scope.(directoryURI); !isDir || s.view.Options().VerboseOutput { log.Print(ctx, "go/packages.Load", tag.Of("package", pkg.PkgPath), tag.Of("files", pkg.CompiledGoFiles)) } - + // Handle golang/go#36292 by ignoring packages with no sources and no errors. + if len(pkg.GoFiles) == 0 && len(pkg.CompiledGoFiles) == 0 && len(pkg.Errors) == 0 { + continue + } // Set the metadata for this package. if err := s.updateImports(ctx, packagePath(pkg.PkgPath), pkg, cfg, map[packageID]struct{}{}); err != nil { return nil, err @@ -138,7 +143,7 @@ func (s *snapshot) updateMetadata(ctx context.Context, uri source.Scope, pkgs [] s.clearAndRebuildImportGraph() if len(results) == 0 { - return nil, errors.Errorf("no metadata for %s", uri) + return nil, errors.Errorf("no metadata for %s", scope) } return results, nil } diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index 915f0f501f..6b5c66a3e4 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -37,6 +37,10 @@ type pkg struct { type packageID string type packagePath string +// Declare explicit types for files and directories to distinguish between the two. +type fileURI span.URI +type directoryURI span.URI + func (p *pkg) ID() string { return string(p.id) } diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index e2e7767183..e31669bf86 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -121,7 +121,7 @@ func (s *session) createView(ctx context.Context, name string, folder span.URI, // Perhaps different calls to NewView can be run in parallel? v.snapshotMu.Lock() defer v.snapshotMu.Unlock() // The code after the snapshot is used isn't expensive. - m, err := v.snapshot.load(ctx, source.DirectoryURI(folder)) + m, err := v.snapshot.load(ctx, directoryURI(folder)) if err != nil { // Suppress all errors. log.Error(ctx, "failed to load snapshot", err, telemetry.Directory.Of(folder)) diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 3b5fe63c10..2d984fa3c8 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -73,13 +73,39 @@ func (s *snapshot) PackageHandles(ctx context.Context, fh source.FileHandle) ([] ctx = telemetry.File.With(ctx, fh.Identity().URI) meta := s.getMetadataForURI(fh.Identity().URI) - // Determine if we need to type-check the package. - phs, load, check := s.shouldCheck(meta) - // We may need to re-load package metadata. - // We only need to this if it has been invalidated, and is therefore unvailable. + phs, err := s.packageHandles(ctx, fileURI(fh.Identity().URI), meta) + if err != nil { + return nil, err + } + var results []source.PackageHandle + for _, ph := range phs { + results = append(results, ph) + } + return results, nil +} + +func (s *snapshot) PackageHandle(ctx context.Context, pkgID string) (source.PackageHandle, error) { + id := packageID(pkgID) + var meta []*metadata + if m := s.getMetadata(id); m != nil { + meta = append(meta, m) + } + phs, err := s.packageHandles(ctx, id, meta) + if err != nil { + return nil, err + } + if len(phs) > 1 { + return nil, errors.Errorf("more than one package for %s", id) + } + return phs[0], nil +} + +func (s *snapshot) packageHandles(ctx context.Context, scope interface{}, meta []*metadata) ([]*packageHandle, error) { + // First, determine if we need to reload or recheck the package. + phs, load, check := s.shouldCheck(meta) if load { - newMeta, err := s.load(ctx, source.FileURI(fh.Identity().URI)) + newMeta, err := s.load(ctx, scope) if err != nil { return nil, err } @@ -91,22 +117,22 @@ func (s *snapshot) PackageHandles(ctx context.Context, fh source.FileHandle) ([] } meta = newMeta } + var results []*packageHandle if check { - var results []source.PackageHandle for _, m := range meta { - ph, err := s.packageHandle(ctx, m.id, source.ParseFull) + ph, err := s.buildPackageHandle(ctx, m.id, source.ParseFull) if err != nil { return nil, err } results = append(results, ph) } - phs = results + } else { + results = phs } - if len(phs) == 0 { - return nil, errors.Errorf("no CheckPackageHandles for %s", fh.Identity().URI) + if len(results) == 0 { + return nil, errors.Errorf("packageHandles: no package handles for %v", scope) } - - return phs, nil + return results, nil } func missingImports(metadata []*metadata) map[packagePath]struct{} { @@ -131,33 +157,9 @@ func sameSet(x, y map[packagePath]struct{}) bool { return true } -func (s *snapshot) PackageHandle(ctx context.Context, id string) (source.PackageHandle, error) { - ctx = telemetry.Package.With(ctx, id) - - m := s.getMetadata(packageID(id)) - if m == nil { - return nil, errors.Errorf("no known metadata for %s", id) - } - // Determine if we need to type-check the package. - phs, load, check := s.shouldCheck([]*metadata{m}) - if load { - return nil, errors.Errorf("outdated metadata for %s, needs re-load", id) - } - if check { - return s.packageHandle(ctx, m.id, source.ParseFull) - } - if len(phs) == 0 { - return nil, errors.Errorf("no check package handle for %s", id) - } - if len(phs) > 1 { - return nil, errors.Errorf("multiple check package handles for a single id: %s", id) - } - return phs[0], nil -} - // shouldCheck determines if the packages provided by the metadata // need to be re-loaded or re-type-checked. -func (s *snapshot) shouldCheck(m []*metadata) (phs []source.PackageHandle, load, check bool) { +func (s *snapshot) shouldCheck(m []*metadata) (phs []*packageHandle, load, check bool) { // No metadata. Re-load and re-check. if len(m) == 0 { return nil, true, true @@ -252,7 +254,7 @@ func (s *snapshot) addPackage(ph *packageHandle) { func (s *snapshot) checkWorkspacePackages(ctx context.Context, m []*metadata) ([]source.PackageHandle, error) { var phs []source.PackageHandle for _, m := range m { - ph, err := s.packageHandle(ctx, m.id, source.ParseFull) + ph, err := s.buildPackageHandle(ctx, m.id, source.ParseFull) if err != nil { return nil, err } @@ -272,43 +274,46 @@ func (s *snapshot) WorkspacePackageIDs(ctx context.Context) (ids []string) { return ids } -func (s *snapshot) KnownPackages(ctx context.Context) []source.Package { - // TODO(matloob): This function exists because KnownImportPaths can't - // determine the import paths of all packages. Remove this function - // if KnownImportPaths gains that ability. That could happen if - // go list or go packages provide that information. - pkgIDs := make(map[packageID]bool) +func (s *snapshot) KnownPackages(ctx context.Context) []source.PackageHandle { + // Collect PackageHandles for all of the workspace packages first. + // They may need to be reloaded if their metadata has been invalidated. + wsPackages := make(map[packageID]bool) s.mu.Lock() - for _, m := range s.metadata { - pkgIDs[m.id] = true - } - // Add in all the workspacePackages in case the've been invalidated - // in the metadata since their initial load. for id := range s.workspacePackages { - pkgIDs[id] = true + wsPackages[id] = true + } + s.mu.Unlock() + + var results []source.PackageHandle + for pkgID := range wsPackages { + ph, err := s.PackageHandle(ctx, string(pkgID)) + if err != nil { + log.Error(ctx, "KnownPackages: failed to create PackageHandle", err, telemetry.Package.Of(pkgID)) + continue + } + results = append(results, ph) + } + + // Once all workspace packages have been checked, the metadata will be up-to-date. + // Add all packages known in the workspace (that haven't already been added). + pkgIDs := make(map[packageID]bool) + s.mu.Lock() + for id := range s.metadata { + if !wsPackages[id] { + pkgIDs[id] = true + } } s.mu.Unlock() - var results []source.Package for pkgID := range pkgIDs { - mode := source.ParseExported - if s.workspacePackages[pkgID] { - // Any package in our workspace should be loaded with ParseFull. - mode = source.ParseFull - } - ph, err := s.packageHandle(ctx, pkgID, mode) + // Metadata for these packages should already be up-to-date, + // so just build the package handle directly (without a reload). + ph, err := s.buildPackageHandle(ctx, pkgID, source.ParseExported) if err != nil { - log.Error(ctx, "failed to create CheckPackageHandle", err, telemetry.Package.Of(pkgID)) + log.Error(ctx, "KnownPackages: failed to create PackageHandle", err, telemetry.Package.Of(pkgID)) continue } - // Check the package now if it's not checked yet. - // TODO(matloob): is this too slow? - pkg, err := ph.check(ctx) - if err != nil { - log.Error(ctx, "failed to check package", err, telemetry.Package.Of(pkgID)) - continue - } - results = append(results, pkg) + results = append(results, ph) } return results diff --git a/internal/lsp/source/implementation.go b/internal/lsp/source/implementation.go index e405f1a33a..cab9e61f2d 100644 --- a/internal/lsp/source/implementation.go +++ b/internal/lsp/source/implementation.go @@ -97,7 +97,11 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol. allNamed []*types.Named pkgs = make(map[*types.Package]Package) ) - for _, pkg := range s.KnownPackages(ctx) { + for _, ph := range s.KnownPackages(ctx) { + pkg, err := ph.Check(ctx) + if err != nil { + return nil, err + } pkgs[pkg.GetTypes()] = pkg info := pkg.GetTypesInfo() diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 19a27a3808..0847f6c311 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -59,7 +59,7 @@ type Snapshot interface { KnownImportPaths() map[string]Package // KnownPackages returns all the packages loaded in this snapshot. - KnownPackages(ctx context.Context) []Package + KnownPackages(ctx context.Context) []PackageHandle } // PackageHandle represents a handle to a specific version of a package. @@ -327,22 +327,6 @@ const ( UnknownKind ) -type FileURI span.URI - -func (f FileURI) URI() span.URI { - return span.URI(f) -} - -type DirectoryURI span.URI - -func (d DirectoryURI) URI() span.URI { - return span.URI(d) -} - -type Scope interface { - URI() span.URI -} - // Package represents a Go package that has been type-checked. It maintains // only the relevant fields of a *go/packages.Package. type Package interface { diff --git a/internal/lsp/telemetry/telemetry.go b/internal/lsp/telemetry/telemetry.go index d0c0f2e11e..71c54a72a9 100644 --- a/internal/lsp/telemetry/telemetry.go +++ b/internal/lsp/telemetry/telemetry.go @@ -24,6 +24,7 @@ const ( URI = tag.Key("URI") Package = tag.Key("package") PackagePath = tag.Key("package_path") + Query = tag.Key("query") ) var (