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>

internal/lsp/cache/session.go

@@ -76,20 +76,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, []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 @@ func (s *session) createView(ctx context.Context, name string, folder span.URI,
 	// 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 @@ func (s *session) createView(ctx context.Context, name string, folder span.URI,
 		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 := v.snapshot.checkWorkspacePackages(ctx, m); err != nil {
-	if 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 @@ 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, []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 @@ 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, phs, nil
+	return v, snapshot, nil
 }
 
 func (s *session) dropView(ctx context.Context, view *view) (int, error) {

internal/lsp/cache/snapshot.go

@@ -250,6 +250,16 @@ func (s *snapshot) checkWorkspacePackages(ctx context.Context, m []*metadata) ([
 	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

internal/lsp/diagnostics.go

@@ -16,12 +16,17 @@ import (
 	"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
 		}

internal/lsp/general.go

@@ -165,12 +165,12 @@ func (s *Server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol
 
 	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)

internal/lsp/source/view.go

@@ -40,6 +40,10 @@ type Snapshot interface {
 	// 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 @@ type View interface {
 	// 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 @@ type View 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, []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

internal/lsp/workspace.go

@@ -26,7 +26,7 @@ func (s *Server) changeFolders(ctx context.Context, event protocol.WorkspaceFold
 	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 @@ func (s *Server) addView(ctx context.Context, name string, uri span.URI) (source
 	}
 
 	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 @@ func (s *Server) updateConfiguration(ctx context.Context, changed interface{}) e
 	// 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)
+		if err := s.fetchConfig(ctx, view.Name(), view.Folder(), &options); err != nil {
-		view.SetOptions(ctx, options)
+			return err
+		}
+		if _, err := view.SetOptions(ctx, options); err != nil {
+			return err
+		}
+		go s.diagnoseSnapshot(view.Snapshot())
 	}
 	return nil
 }