internal/lsp: reload metadata for orphaned files go/packages overlay handling only really works for contains queries (file=), so our approach of reloading packages by package path (for workspace packages) wasn't handling newly created packages that need to be handled through overlays. Workaround this by reloading metadata for individual files that are missing it by running extra contains queries (only after the first metadata load for package paths). Be careful not to reload the same file multiple times if the first load did not succeed. Somewhat related, clear out `go list` errors in packages that go through overlay handling, since they will often be rendered irrelevant. I'm not sure if this is the right move, but if it's not, then we will have to do extra work to disregard those errors in gopls. Fixes golang/go#36661. Fixes golang/go#36635. Change-Id: Ib83cffcdf8a3e07da0f30e734d5e2c89691e1aba Reviewed-on: https://go-review.googlesource.com/c/tools/+/216141 Run-TryBot: Rebecca Stambler <rstambler@golang.org> Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/go/packages/golist_overlay.go b/go/packages/golist_overlay.go index c6925c8..5d088cb 100644 --- a/go/packages/golist_overlay.go +++ b/go/packages/golist_overlay.go
@@ -132,6 +132,12 @@ pkg.CompiledGoFiles = append(pkg.CompiledGoFiles, opath) modifiedPkgsSet[pkg.ID] = true } + + // Clear out the package's errors, since we've probably corrected + // them by adding the overlay. This may eliminate some legitimate + // errors, but that's a risk with overlays in general. + pkg.Errors = nil + imports, err := extractImports(opath, contents) if err != nil { // Let the parser or type checker report errors later.
diff --git a/go/packages/packages114_test.go b/go/packages/packages114_test.go index 1b0a824..eb2fa29 100644 --- a/go/packages/packages114_test.go +++ b/go/packages/packages114_test.go
@@ -15,6 +15,9 @@ "golang.org/x/tools/go/packages/packagestest" ) +// These tests check fixes that are only available in Go 1.14. +// They can be moved into packages_test.go when we no longer support 1.13. +// See golang/go#35973 for more information. func TestInvalidFilesInOverlay(t *testing.T) { packagestest.TestAll(t, testInvalidFilesInOverlay) } func testInvalidFilesInOverlay(t *testing.T, exporter packagestest.Exporter) { exported := packagestest.Export(t, exporter, []packagestest.Module{ @@ -61,6 +64,19 @@ t.Fatal(err) } d := initial[0] + var containsFile bool + for _, goFile := range d.CompiledGoFiles { + if f == goFile { + containsFile = true + break + } + } + if !containsFile { + t.Fatalf("expected %s in CompiledGoFiles, got %v", f, d.CompiledGoFiles) + } + if len(d.Errors) > 0 { + t.Fatalf("expected no errors in package, got %v", d.Errors) + } // Check value of d.D. dD := constant(d, "D") if dD == nil {
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 12839d6..3b6841f 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go
@@ -81,18 +81,14 @@ log.Print(ctx, "go/packages.Load", tag.Of("snapshot", s.ID()), tag.Of("query", query), tag.Of("packages", len(pkgs))) if len(pkgs) == 0 { - if err == nil { - err = errors.Errorf("no packages found for query %s", query) - } return nil, err } return s.updateMetadata(ctx, scopes, pkgs, cfg) } // shouldLoad reparses a file's package and import declarations to -// determine if they have changed. +// determine if the file requires a metadata reload. func (c *cache) shouldLoad(ctx context.Context, s *snapshot, originalFH, currentFH source.FileHandle) bool { - // TODO(rstambler): go.mod files should be tracked in the snapshot. if originalFH == nil { return currentFH.Identity().Kind == source.Go } @@ -112,10 +108,8 @@ } // Check if the package's metadata has changed. The cases handled are: - // // 1. A package's name has changed // 2. A file's imports have changed - // if original.Name.Name != current.Name.Name { return true }
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 03f1404..5bc236a 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go
@@ -96,6 +96,7 @@ importedBy: make(map[packageID][]packageID), actions: make(map[actionKey]*actionHandle), workspacePackages: make(map[packageID]packagePath), + unloadableFiles: make(map[span.URI]struct{}), }, ignoredURIs: make(map[span.URI]struct{}), }
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 7695025..bf31342 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go
@@ -49,6 +49,9 @@ // workspacePackages contains the workspace's packages, which are loaded // when the view is created. workspacePackages map[packageID]packagePath + + // unloadableFiles keeps track of files that we've failed to load. + unloadableFiles map[span.URI]struct{} } type packageKey struct { @@ -425,12 +428,17 @@ s.actions[key] = ah } -func (s *snapshot) getMetadataForURI(uri span.URI) (metadata []*metadata) { - // TODO(matloob): uri can be a file or directory. Should we update the mappings - // to map directories to their contained packages? +func (s *snapshot) getMetadataForURI(uri span.URI) []*metadata { s.mu.Lock() defer s.mu.Unlock() + return s.getMetadataForURILocked(uri) +} + +func (s *snapshot) getMetadataForURILocked(uri span.URI) (metadata []*metadata) { + // TODO(matloob): uri can be a file or directory. Should we update the mappings + // to map directories to their contained packages? + for _, id := range s.ids[uri] { if m, ok := s.metadata[id]; ok { metadata = append(metadata, m) @@ -489,13 +497,6 @@ return scope, ok } -func (s *snapshot) setWorkspacePackage(id packageID, pkgPath packagePath) { - s.mu.Lock() - defer s.mu.Unlock() - - s.workspacePackages[id] = pkgPath -} - func (s *snapshot) getFileURIs() []span.URI { s.mu.Lock() defer s.mu.Unlock() @@ -545,32 +546,115 @@ // reloadWorkspace reloads the metadata for all invalidated workspace packages. func (s *snapshot) reloadWorkspace(ctx context.Context) error { - scope := s.workspaceScope(ctx) - if scope == nil { - return nil - } - _, err := s.load(ctx, scope) - return err -} - -func (s *snapshot) workspaceScope(ctx context.Context) interface{} { + // See which of the workspace packages are missing metadata. s.mu.Lock() - defer s.mu.Unlock() - - var pkgPaths []packagePath + var pkgPaths []interface{} for id, pkgPath := range s.workspacePackages { if s.metadata[id] == nil { pkgPaths = append(pkgPaths, pkgPath) } } - switch len(pkgPaths) { - case 0: - return nil - case len(s.workspacePackages): - return directoryURI(s.view.folder) - default: - return pkgPaths + s.mu.Unlock() + + if len(pkgPaths) > 0 { + if m, err := s.load(ctx, pkgPaths...); err == nil { + for _, m := range m { + s.setWorkspacePackage(ctx, m) + } + } } + + // When we load ./... or a package path directly, we may not get packages + // that exist only in overlays. As a workaround, we search all of the files + // available in the snapshot and reload their metadata individually using a + // file= query if the metadata is unavailable. + if scopes := s.orphanedFileScopes(); len(scopes) > 0 { + m, err := s.load(ctx, scopes...) + + // If we failed to load some files, i.e. they have no metadata, + // mark the failures so we don't bother retrying until the file's + // content changes. + // + // TODO(rstambler): This may be an overestimate if the load stopped + // early for an unrelated errors. Add a fallback? + // + // Check for context cancellation so that we don't incorrectly mark files + // as unloadable, but don't return before setting all workspace packages. + if ctx.Err() == nil && err != nil { + s.mu.Lock() + for _, scope := range scopes { + uri := span.URI(scope.(fileURI)) + if s.getMetadataForURILocked(uri) == nil { + s.unloadableFiles[uri] = struct{}{} + } + } + s.mu.Unlock() + } + for _, m := range m { + // If a package's files belong to this view, it is a workspace package + // and should be added to the set of workspace packages. + for _, uri := range m.compiledGoFiles { + if !contains(s.view.session.viewsOf(uri), s.view) { + continue + } + s.setWorkspacePackage(ctx, m) + } + } + } + // Create package handles for all of the workspace packages. + for _, id := range s.workspacePackageIDs() { + if _, err := s.packageHandle(ctx, id); err != nil { + return err + } + } + return nil +} + +func (s *snapshot) orphanedFileScopes() []interface{} { + s.mu.Lock() + defer s.mu.Unlock() + + scopeSet := make(map[span.URI]struct{}) + for uri, fh := range s.files { + // Don't try to reload metadata for go.mod files. + if fh.Identity().Kind != source.Go { + continue + } + // Don't reload metadata for files we've already deemed unloadable. + if _, ok := s.unloadableFiles[uri]; ok { + continue + } + if s.getMetadataForURILocked(uri) == nil { + scopeSet[uri] = struct{}{} + } + } + var scopes []interface{} + for uri := range scopeSet { + scopes = append(scopes, fileURI(uri)) + } + return scopes +} + +func contains(views []*view, view *view) bool { + for _, v := range views { + if v == view { + return true + } + } + return false +} + +func (s *snapshot) setWorkspacePackage(ctx context.Context, m *metadata) { + s.mu.Lock() + defer s.mu.Unlock() + + // A test variant of a package can only be loaded directly by loading + // the non-test variant with -test. Track the import path of the non-test variant. + pkgPath := m.pkgPath + if m.forTest != "" { + pkgPath = m.forTest + } + s.workspacePackages[m.id] = pkgPath } func (s *snapshot) clone(ctx context.Context, withoutURIs []span.URI) *snapshot { @@ -587,12 +671,17 @@ actions: make(map[actionKey]*actionHandle), files: make(map[span.URI]source.FileHandle), workspacePackages: make(map[packageID]packagePath), + unloadableFiles: make(map[span.URI]struct{}), } // Copy all of the FileHandles. for k, v := range s.files { result.files[k] = v } + // Copy the set of unloadable files. + for k, v := range s.unloadableFiles { + result.unloadableFiles[k] = v + } // transitiveIDs keeps track of transitive reverse dependencies. // If an ID is present in the map, invalidate its types. @@ -664,6 +753,8 @@ } else { result.files[withoutURI] = currentFH } + // Make sure to remove the changed file from the unloadable set. + delete(result.unloadableFiles, withoutURI) } // Collect the IDs for the packages associated with the excluded URIs.
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 9a48249..b73a571 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go
@@ -557,13 +557,7 @@ } continue } - // A test variant of a package can only be loaded directly by loading - // the non-test variant with -test. Track the import path of the non-test variant. - pkgPath := m.pkgPath - if m.forTest != "" { - pkgPath = m.forTest - } - s.setWorkspacePackage(m.id, pkgPath) + s.setWorkspacePackage(ctx, m) if _, err := s.packageHandle(ctx, m.id); err != nil { return err }