internal/lsp: run diagnostics on the entire workspace
This change runs diagnostics on all packages in the workspace, instead
of just open files. We also want to avoid invalidating the type
information for a newly-opened file (since we should have it be default
now), so handle that case.
This causes a large increase in memory usage in the
internal/lsp/cmd tests, so to handle that, share an app between all of
the tests, rather than creating one per-test type.
Change-Id: Ifba18d77a700cda79ec79f66174de0e7f13fe319
Reviewed-on: https://go-review.googlesource.com/c/tools/+/207353
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go
index 0649937..5c206fa 100644
--- a/internal/lsp/cache/analysis.go
+++ b/internal/lsp/cache/analysis.go
@@ -157,11 +157,11 @@
func (act *actionHandle) cached() ([]*source.Error, interface{}, error) {
v := act.handle.Cached()
if v == nil {
- return nil, nil, errors.Errorf("no analyses for %s", act.pkg.ID())
+ return nil, nil, errors.Errorf("no cached analyses for %s", act.pkg.ID())
}
data, ok := v.(*actionData)
if !ok {
- return nil, nil, errors.Errorf("unexpected type for %s:%s", act.pkg.ID(), act.analyzer.Name)
+ return nil, nil, errors.Errorf("unexpected type for cached analysis %s:%s", act.pkg.ID(), act.analyzer.Name)
}
if data == nil {
return nil, nil, errors.Errorf("unexpected nil cached analysis for %s:%s", act.pkg.ID(), act.analyzer.Name)
diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go
index b5281ed..316b98a 100644
--- a/internal/lsp/cache/pkg.go
+++ b/internal/lsp/cache/pkg.go
@@ -106,6 +106,9 @@
func (s *snapshot) FindAnalysisError(ctx context.Context, id string, diag protocol.Diagnostic) (*source.Error, error) {
acts := s.getActionHandles(packageID(id), source.ParseFull)
+ if len(acts) == 0 {
+ return nil, errors.Errorf("no action handles for %v", id)
+ }
for _, act := range acts {
errors, _, err := act.analyze(ctx)
if err != nil {
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index 8f1158f..bbbcd9c 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -79,20 +79,20 @@
return s.cache
}
-func (s *session) NewView(ctx context.Context, name string, folder span.URI, options source.Options) (source.View, error) {
+func (s *session) NewView(ctx context.Context, name string, folder span.URI, options source.Options) (source.View, []source.CheckPackageHandle, error) {
s.viewMu.Lock()
defer s.viewMu.Unlock()
- v, err := s.createView(ctx, name, folder, options)
+ v, cphs, err := s.createView(ctx, name, folder, options)
if err != nil {
- return nil, err
+ 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, nil
+ return v, cphs, nil
}
-func (s *session) createView(ctx context.Context, name string, folder span.URI, options source.Options) (*view, error) {
+func (s *session) createView(ctx context.Context, name string, folder span.URI, options source.Options) (*view, []source.CheckPackageHandle, 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.
@@ -147,12 +147,13 @@
// 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.
- if err := v.snapshot.checkWorkspacePackages(ctx, m); err != nil {
- return nil, err
+ cphs, err := v.snapshot.checkWorkspacePackages(ctx, m)
+ if err != nil {
+ return nil, nil, err
}
debug.AddView(debugView{v})
- return v, loadErr
+ return v, cphs, loadErr
}
// View returns the view by name.
@@ -247,14 +248,14 @@
return nil
}
-func (s *session) updateView(ctx context.Context, view *view, options source.Options) (*view, error) {
+func (s *session) updateView(ctx context.Context, view *view, options source.Options) (*view, []source.CheckPackageHandle, error) {
s.viewMu.Lock()
defer s.viewMu.Unlock()
i, err := s.dropView(ctx, view)
if err != nil {
- return nil, err
+ return nil, nil, err
}
- v, err := s.createView(ctx, view.name, view.folder, options)
+ v, cphs, 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
@@ -265,7 +266,7 @@
}
// substitute the new view into the array where the old view was
s.views[i] = v
- return v, nil
+ return v, cphs, 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 575e864..6b8da33 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -107,15 +107,17 @@
// the view is created. This is needed because
// (*snapshot).CheckPackageHandle makes the assumption that every package that's
// been loaded has an existing checkPackageHandle.
-func (s *snapshot) checkWorkspacePackages(ctx context.Context, m []*metadata) error {
+func (s *snapshot) checkWorkspacePackages(ctx context.Context, m []*metadata) ([]source.CheckPackageHandle, error) {
+ var cphs []source.CheckPackageHandle
for _, m := range m {
- _, err := s.checkPackageHandle(ctx, m.id, source.ParseFull)
+ cph, err := s.checkPackageHandle(ctx, m.id, source.ParseFull)
if err != nil {
- return err
+ return nil, err
}
s.workspacePackages[m.id] = true
+ cphs = append(cphs, cph)
}
- return nil
+ return cphs, nil
}
func (s *snapshot) KnownPackages(ctx context.Context) []source.Package {
@@ -392,9 +394,20 @@
return result
}
+func (s *snapshot) ID() uint64 {
+ return s.id
+}
+
// invalidateContent invalidates the content of a Go file,
// including any position and type information that depends on it.
+// It returns true if we were already tracking the given file, false otherwise.
func (v *view) invalidateContent(ctx context.Context, f source.File, kind source.FileKind, action source.FileAction) bool {
+ // TODO: Handle the possibility of opening a file outside of the current view.
+ // For now, return early if we open a file.
+ // We assume that we are already tracking any files within the given view.
+ if action == source.Open {
+ return true
+ }
var (
withoutTypes = make(map[span.URI]struct{})
withoutMetadata = make(map[span.URI]struct{})
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index e2a4236..4ff2fcb 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -122,7 +122,7 @@
v.options = options
return v, nil
}
- newView, err := v.session.updateView(ctx, v, options)
+ newView, _, err := v.session.updateView(ctx, v, options)
return newView, err
}
diff --git a/internal/lsp/cmd/test/imports.go b/internal/lsp/cmd/test/imports.go
index 9432a6c..c51897a 100644
--- a/internal/lsp/cmd/test/imports.go
+++ b/internal/lsp/cmd/test/imports.go
@@ -16,7 +16,7 @@
func (r *runner) Import(t *testing.T, spn span.Span) {
uri := spn.URI()
filename := uri.Filename()
- args := []string{"imports", filename}
+ args := []string{"-remote=internal", "imports", filename}
app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env, r.options)
got := CaptureStdOut(t, func() {
_ = tool.Run(r.ctx, app, args)
diff --git a/internal/lsp/cmd/test/links.go b/internal/lsp/cmd/test/links.go
index 79a6799..c74ea6a 100644
--- a/internal/lsp/cmd/test/links.go
+++ b/internal/lsp/cmd/test/links.go
@@ -20,7 +20,7 @@
if err != nil {
t.Fatal(err)
}
- args := []string{"links", "-json", uri.Filename()}
+ args := []string{"-remote=internal", "links", "-json", uri.Filename()}
app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env, r.options)
out := CaptureStdOut(t, func() {
_ = tool.Run(r.ctx, app, args)
diff --git a/internal/lsp/cmd/test/suggested_fix.go b/internal/lsp/cmd/test/suggested_fix.go
index 9be0f8e..d41c590 100644
--- a/internal/lsp/cmd/test/suggested_fix.go
+++ b/internal/lsp/cmd/test/suggested_fix.go
@@ -15,7 +15,7 @@
func (r *runner) SuggestedFix(t *testing.T, spn span.Span) {
uri := spn.URI()
filename := uri.Filename()
- args := []string{"fix", "-a", filename}
+ args := []string{"-remote=internal", "fix", "-a", filename}
app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env, r.options)
got := CaptureStdOut(t, func() {
_ = tool.Run(r.ctx, app, args)
diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go
index 2b68f7c..206cc89 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -16,6 +16,20 @@
"golang.org/x/tools/internal/telemetry/trace"
)
+func (s *Server) diagnoseView(view source.View, cphs []source.CheckPackageHandle) {
+ for _, cph := range cphs {
+ if len(cph.Files()) == 0 {
+ continue
+ }
+ f := cph.Files()[0]
+
+ // Run diagnostics on the workspace package.
+ go func(view source.View, uri span.URI) {
+ s.diagnostics(view, uri)
+ }(view, f.File().Identity().URI)
+ }
+}
+
func (s *Server) diagnostics(view source.View, uri span.URI) error {
ctx := view.BackgroundContext()
ctx, done := trace.StartSpan(ctx, "lsp:background-worker")
diff --git a/internal/lsp/general.go b/internal/lsp/general.go
index 8e9922b..a210aee 100644
--- a/internal/lsp/general.go
+++ b/internal/lsp/general.go
@@ -158,9 +158,12 @@
viewErrors := make(map[span.URI]error)
for _, folder := range s.pendingFolders {
uri := span.NewURI(folder.URI)
- if _, err := s.addView(ctx, folder.Name, span.NewURI(folder.URI)); err != nil {
+ view, workspacePackages, err := s.addView(ctx, folder.Name, span.NewURI(folder.URI))
+ if err != nil {
viewErrors[uri] = err
+ continue
}
+ s.diagnoseView(view, workspacePackages)
}
if len(viewErrors) > 0 {
errMsg := fmt.Sprintf("Error loading workspace folders (expected %v, got %v)\n", len(s.pendingFolders), len(s.session.Views()))
diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go
index 23d3927..1feddb1 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -53,8 +53,7 @@
options := tests.DefaultOptions()
session.SetOptions(options)
options.Env = data.Config.Env
- _, err := session.NewView(ctx, viewName, span.FileURI(data.Config.Dir), options)
- if err != nil {
+ if _, _, err := session.NewView(ctx, viewName, span.FileURI(data.Config.Dir), options); err != nil {
t.Fatal(err)
}
for filename, content := range data.Config.Overlay {
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index 2e2b0fd..8f84a26 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -51,7 +51,7 @@
session := cache.NewSession(ctx)
options := tests.DefaultOptions()
options.Env = data.Config.Env
- view, err := session.NewView(ctx, "source_test", span.FileURI(data.Config.Dir), options)
+ view, _, err := session.NewView(ctx, "source_test", span.FileURI(data.Config.Dir), options)
if err != nil {
t.Fatal(err)
}
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index 2e433fd..1087514 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -156,7 +156,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, error)
+ NewView(ctx context.Context, name string, folder span.URI, options Options) (View, []CheckPackageHandle, error)
// Cache returns the cache that created this session.
Cache() Cache
@@ -285,6 +285,8 @@
// Snapshot represents the current state for the given view.
type Snapshot interface {
+ ID() uint64
+
// Handle returns the FileHandle for the given file.
Handle(ctx context.Context, f File) FileHandle
diff --git a/internal/lsp/workspace.go b/internal/lsp/workspace.go
index 1e4fa66..d78f37c 100644
--- a/internal/lsp/workspace.go
+++ b/internal/lsp/workspace.go
@@ -24,19 +24,21 @@
}
for _, folder := range event.Added {
- if _, err := s.addView(ctx, folder.Name, span.NewURI(folder.URI)); err != nil {
+ view, cphs, err := s.addView(ctx, folder.Name, span.NewURI(folder.URI))
+ if err != nil {
return err
}
+ go s.diagnoseView(view, cphs)
}
return nil
}
-func (s *Server) addView(ctx context.Context, name string, uri span.URI) (source.View, error) {
+func (s *Server) addView(ctx context.Context, name string, uri span.URI) (source.View, []source.CheckPackageHandle, error) {
s.stateMu.Lock()
state := s.state
s.stateMu.Unlock()
if state < serverInitialized {
- return nil, errors.Errorf("addView called before server initialized")
+ return nil, nil, errors.Errorf("addView called before server initialized")
}
options := s.session.Options()
