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>

internal/lsp/cache/analysis.go

@@ -157,11 +157,11 @@ func (act *actionHandle) analyze(ctx context.Context) ([]*source.Error, interfac
 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)

internal/lsp/cache/pkg.go

@@ -106,6 +106,9 @@ func (p *pkg) Imports() []source.Package {
 
 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 {

internal/lsp/cache/session.go

@@ -79,20 +79,20 @@ func (s *session) Cache() source.Cache {
 	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 @@ func (s *session) createView(ctx context.Context, name string, folder span.URI,
 	// 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 @@ func (s *session) removeView(ctx context.Context, view *view) error {
 	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 @@ func (s *session) updateView(ctx context.Context, view *view, options source.Opt
 	}
 	// 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) {

internal/lsp/cache/snapshot.go

@@ -107,15 +107,17 @@ func (s *snapshot) getPackages(uri source.FileURI, m source.ParseMode) (cphs []s
 // 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 @@ func (s *snapshot) clone(ctx context.Context, withoutURI *span.URI, withoutTypes
 	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{})

internal/lsp/cache/view.go

@@ -122,7 +122,7 @@ func (v *view) SetOptions(ctx context.Context, options source.Options) (source.V
 		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
 }
 

internal/lsp/cmd/test/imports.go

@@ -16,7 +16,7 @@ import (
 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)

internal/lsp/cmd/test/links.go

@@ -20,7 +20,7 @@ func (r *runner) Link(t *testing.T, uri span.URI, wantLinks []tests.Link) {
 	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)

internal/lsp/cmd/test/suggested_fix.go

@@ -15,7 +15,7 @@ import (
 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)

internal/lsp/diagnostics.go

@@ -16,6 +16,20 @@ import (
 	"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")

internal/lsp/general.go

@@ -158,9 +158,12 @@ func (s *Server) initialized(ctx context.Context, params *protocol.InitializedPa
 	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()))

internal/lsp/lsp_test.go

@@ -53,8 +53,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
 	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 {

internal/lsp/source/source_test.go

@@ -51,7 +51,7 @@ func testSource(t *testing.T, exporter packagestest.Exporter) {
 	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)
 	}

internal/lsp/source/view.go

@@ -156,7 +156,7 @@ type Cache interface {
 // 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 @@ type View interface {
 
 // 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
 

internal/lsp/workspace.go

@@ -24,19 +24,21 @@ func (s *Server) changeFolders(ctx context.Context, event protocol.WorkspaceFold
 	}
 
 	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()