internal/lsp: return snapshot when creating a view
Previously, we returned CheckPackageHandles when creating a new view.
Now, return the view's snapshot. Also, add a WorkspacePackageIDs
function in order to run diagnostics on them.
Fixes golang/go#35548
Change-Id: Ica7e41f5a7aa3ee9413feb4de4ee16e1825db2e1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209418
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index 062df68..ccc24b6 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -76,20 +76,20 @@
return s.cache
}
-func (s *session) NewView(ctx context.Context, name string, folder span.URI, options source.Options) (source.View, []source.PackageHandle, error) {
+func (s *session) NewView(ctx context.Context, name string, folder span.URI, options source.Options) (source.View, source.Snapshot, error) {
s.viewMu.Lock()
defer s.viewMu.Unlock()
- v, phs, err := s.createView(ctx, name, folder, options)
+ v, snapshot, err := s.createView(ctx, name, folder, options)
if err != nil {
return nil, nil, err
}
s.views = append(s.views, v)
// we always need to drop the view map
s.viewMap = make(map[span.URI]source.View)
- return v, phs, nil
+ return v, snapshot, nil
}
-func (s *session) createView(ctx context.Context, name string, folder span.URI, options source.Options) (*view, []source.PackageHandle, error) {
+func (s *session) createView(ctx context.Context, name string, folder span.URI, options source.Options) (*view, *snapshot, error) {
index := atomic.AddInt64(&viewIndex, 1)
// We want a true background context and not a detached context here
// the spans need to be unrelated and no tag values should pollute it.
@@ -131,8 +131,6 @@
// Preemptively load everything in this directory.
// TODO(matloob): Determine if this can be done in parallel with something else.
// Perhaps different calls to NewView can be run in parallel?
- // TODO(matloob): By default when a new file is opened, its data is invalidated
- // and it's loaded again. Determine if the redundant reload can be avoided.
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))
@@ -141,19 +139,17 @@
log.Error(ctx, "failed to load snapshot", err, telemetry.Directory.Of(folder))
return v, nil, nil
}
-
// Prepare CheckPackageHandles for every package that's been loaded.
// (*snapshot).CheckPackageHandle makes the assumption that every package that's
// been loaded has an existing checkPackageHandle.
- phs, err := v.snapshot.checkWorkspacePackages(ctx, m)
- if err != nil {
+ if _, err := v.snapshot.checkWorkspacePackages(ctx, m); err != nil {
// Suppress all errors.
log.Error(ctx, "failed to check snapshot", err, telemetry.Directory.Of(folder))
return v, nil, nil
}
debug.AddView(debugView{v})
- return v, phs, nil
+ return v, v.snapshot, nil
}
// View returns the view by name.
@@ -248,14 +244,14 @@
return nil
}
-func (s *session) updateView(ctx context.Context, view *view, options source.Options) (*view, []source.PackageHandle, error) {
+func (s *session) updateView(ctx context.Context, view *view, options source.Options) (*view, *snapshot, error) {
s.viewMu.Lock()
defer s.viewMu.Unlock()
i, err := s.dropView(ctx, view)
if err != nil {
return nil, nil, err
}
- v, phs, err := s.createView(ctx, view.name, view.folder, options)
+ v, snapshot, err := s.createView(ctx, view.name, view.folder, options)
if err != nil {
// we have dropped the old view, but could not create the new one
// this should not happen and is very bad, but we still need to clean
@@ -266,7 +262,7 @@
}
// substitute the new view into the array where the old view was
s.views[i] = v
- return v, phs, nil
+ return v, snapshot, nil
}
func (s *session) dropView(ctx context.Context, view *view) (int, error) {
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 15cd0c3..0ebfc70 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -250,6 +250,16 @@
return phs, nil
}
+func (s *snapshot) WorkspacePackageIDs(ctx context.Context) (ids []string) {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+
+ for id := range s.workspacePackages {
+ ids = append(ids, string(id))
+ }
+ 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
diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go
index 62a0511..af7b59b 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -16,12 +16,17 @@
"golang.org/x/tools/internal/telemetry/trace"
)
-func (s *Server) diagnoseSnapshot(snapshot source.Snapshot, phs []source.PackageHandle) {
+func (s *Server) diagnoseSnapshot(snapshot source.Snapshot) {
ctx := snapshot.View().BackgroundContext()
ctx, done := trace.StartSpan(ctx, "lsp:background-worker")
defer done()
- for _, ph := range phs {
+ for _, id := range snapshot.WorkspacePackageIDs(ctx) {
+ ph, err := snapshot.PackageHandle(ctx, id)
+ if err != nil {
+ log.Error(ctx, "diagnoseSnapshot: no PackageHandle for workspace package", err, telemetry.Package.Of(id))
+ continue
+ }
if len(ph.CompiledGoFiles()) == 0 {
continue
}
diff --git a/internal/lsp/general.go b/internal/lsp/general.go
index c8c11ae..3da24d1 100644
--- a/internal/lsp/general.go
+++ b/internal/lsp/general.go
@@ -165,12 +165,12 @@
for _, folder := range folders {
uri := span.NewURI(folder.URI)
- view, workspacePackages, err := s.addView(ctx, folder.Name, span.NewURI(folder.URI))
+ _, snapshot, err := s.addView(ctx, folder.Name, span.NewURI(folder.URI))
if err != nil {
viewErrors[uri] = err
continue
}
- go s.diagnoseSnapshot(view.Snapshot(), workspacePackages)
+ go s.diagnoseSnapshot(snapshot)
}
if len(viewErrors) > 0 {
errMsg := fmt.Sprintf("Error loading workspace folders (expected %v, got %v)\n", len(folders), len(s.session.Views())-originalViews)
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index 56e2419..162a4a5 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -40,6 +40,10 @@
// that this file belongs to.
PackageHandles(ctx context.Context, fh FileHandle) ([]PackageHandle, error)
+ // WorkspacePackageIDs returns the ids of the packages at the top-level
+ // of the snapshot's view.
+ WorkspacePackageIDs(ctx context.Context) []string
+
// GetActiveReverseDeps returns the active files belonging to the reverse
// dependencies of this file's package.
GetReverseDependencies(id string) []string
@@ -128,6 +132,8 @@
// belong to or be part of a dependency of the given package.
FindPosInPackage(pkg Package, pos token.Pos) (*ast.File, Package, error)
+ // FindMapperInPackage returns the mapper associated with a file that may belong to
+ // the given package or one of its dependencies.
FindMapperInPackage(pkg Package, uri span.URI) (*protocol.ColumnMapper, error)
// Snapshot returns the current snapshot for the view.
@@ -140,7 +146,7 @@
// A session may have many active views at any given time.
type Session interface {
// NewView creates a new View and returns it.
- NewView(ctx context.Context, name string, folder span.URI, options Options) (View, []PackageHandle, error)
+ NewView(ctx context.Context, name string, folder span.URI, options Options) (View, Snapshot, error)
// Cache returns the cache that created this session.
Cache() Cache
diff --git a/internal/lsp/workspace.go b/internal/lsp/workspace.go
index 87e6bb9..0b23499 100644
--- a/internal/lsp/workspace.go
+++ b/internal/lsp/workspace.go
@@ -26,7 +26,7 @@
return nil
}
-func (s *Server) addView(ctx context.Context, name string, uri span.URI) (source.View, []source.PackageHandle, error) {
+func (s *Server) addView(ctx context.Context, name string, uri span.URI) (source.View, source.Snapshot, error) {
s.stateMu.Lock()
state := s.state
s.stateMu.Unlock()
@@ -35,8 +35,9 @@
}
options := s.session.Options()
- s.fetchConfig(ctx, name, uri, &options)
-
+ if err := s.fetchConfig(ctx, name, uri, &options); err != nil {
+ return nil, nil, err
+ }
return s.session.NewView(ctx, name, uri, options)
}
@@ -44,8 +45,13 @@
// go through all the views getting the config
for _, view := range s.session.Views() {
options := s.session.Options()
- s.fetchConfig(ctx, view.Name(), view.Folder(), &options)
- view.SetOptions(ctx, options)
+ if err := s.fetchConfig(ctx, view.Name(), view.Folder(), &options); err != nil {
+ return err
+ }
+ if _, err := view.SetOptions(ctx, options); err != nil {
+ return err
+ }
+ go s.diagnoseSnapshot(view.Snapshot())
}
return nil
}
