internal/lsp: only diagnose views affected by a change

For every file change, we want to diagnose the resulting snapshots.
However, we also want to diagnose the file in its "best possible" view
(for example, for a nested module, we should use the inner module rather
than the outer module).

Add a bestView function that operates on a set of views, which allows
us to a pick a view out of the *views affected* by a file modification,
not out of *all* of the views in the session. This means that we can
pick the best view, if any, for each file change and then only return
the snapshots that we should diagnose, as well as the changed URIs for
those snapshots.

Fixes golang/go#42890

Change-Id: Iad23d5ed4832dfd857f1424a7244cf3bd8900e3b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/274235
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

Author: stamblerre

internal/lsp/cache/load.go

@@ -459,7 +459,7 @@ func isTestMain(pkg *packages.Package, gocache string) bool {
 	if len(pkg.GoFiles) > 1 {
 		return false
 	}
-	if !strings.HasPrefix(pkg.GoFiles[0], gocache) {
+	if !source.InDir(gocache, pkg.GoFiles[0]) {
 		return false
 	}
 	return true

internal/lsp/cache/session.go

@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"os"
 	"strconv"
-	"strings"
 	"sync"
 	"sync/atomic"
 
@@ -288,12 +287,11 @@ func (s *Session) viewOf(uri span.URI) (*View, error) {
 		return v, nil
 	}
 	// Pick the best view for this file and memoize the result.
-	v, err := s.bestView(uri)
-	if err != nil {
-		return nil, err
+	if len(s.views) == 0 {
+		return nil, fmt.Errorf("no views in session")
 	}
-	s.viewMap[uri] = v
-	return v, nil
+	s.viewMap[uri] = bestViewForURI(uri, s.views)
+	return s.viewMap[uri], nil
 }
 
 func (s *Session) viewsOf(uri span.URI) []*View {
@@ -302,7 +300,7 @@ func (s *Session) viewsOf(uri span.URI) []*View {
 
 	var views []*View
 	for _, view := range s.views {
-		if strings.HasPrefix(string(uri), string(view.Folder())) {
+		if source.InDir(view.folder.Filename(), uri.Filename()) {
 			views = append(views, view)
 		}
 	}
@@ -319,15 +317,12 @@ func (s *Session) Views() []source.View {
 	return result
 }
 
-// bestView finds the best view to associate a given URI with.
-// viewMu must be held when calling this method.
-func (s *Session) bestView(uri span.URI) (*View, error) {
-	if len(s.views) == 0 {
-		return nil, errors.Errorf("no views in the session")
-	}
+// bestViewForURI returns the most closely matching view for the given URI
+// out of the given set of views.
+func bestViewForURI(uri span.URI, views []*View) *View {
 	// we need to find the best view for this file
 	var longest *View
-	for _, view := range s.views {
+	for _, view := range views {
 		if longest != nil && len(longest.Folder()) > len(view.Folder()) {
 			continue
 		}
@@ -336,16 +331,16 @@ func (s *Session) bestView(uri span.URI) (*View, error) {
 		}
 	}
 	if longest != nil {
-		return longest, nil
+		return longest
 	}
 	// Try our best to return a view that knows the file.
-	for _, view := range s.views {
+	for _, view := range views {
 		if view.knownFile(uri) {
-			return view, nil
+			return view
 		}
 	}
 	// TODO: are there any more heuristics we can use?
-	return s.views[0], nil
+	return views[0]
 }
 
 func (s *Session) removeView(ctx context.Context, view *View) error {
@@ -405,7 +400,7 @@ func (s *Session) dropView(ctx context.Context, v *View) (int, error) {
 }
 
 func (s *Session) ModifyFiles(ctx context.Context, changes []source.FileModification) error {
-	_, _, releases, err := s.DidModifyFiles(ctx, changes)
+	_, releases, err := s.DidModifyFiles(ctx, changes)
 	for _, release := range releases {
 		release()
 	}
@@ -418,13 +413,13 @@ type fileChange struct {
 	fileHandle source.VersionedFileHandle
 }
 
-func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModification) (map[span.URI]source.View, map[source.View]source.Snapshot, []func(), error) {
+func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModification) (map[source.Snapshot][]span.URI, []func(), error) {
 	views := make(map[*View]map[span.URI]*fileChange)
-	bestViews := map[span.URI]source.View{}
+	affectedViews := map[span.URI][]*View{}
 
 	overlays, err := s.updateOverlays(ctx, changes)
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, nil, err
 	}
 	var forceReloadMetadata bool
 	for _, c := range changes {
@@ -433,12 +428,6 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif
 		}
 
 		// Build the list of affected views.
-		bestView, err := s.viewOf(c.URI)
-		if err != nil {
-			return nil, nil, nil, err
-		}
-		bestViews[c.URI] = bestView
-
 		var changedViews []*View
 		for _, view := range s.views {
 			// Don't propagate changes that are outside of the view's scope
@@ -448,16 +437,25 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif
 			}
 			changedViews = append(changedViews, view)
 		}
-		// If no view matched the change, assign it to the best view.
+		// If the change is not relevant to any view, but the change is
+		// happening in the editor, assign it the most closely matching view.
 		if len(changedViews) == 0 {
+			if c.OnDisk {
+				continue
+			}
+			bestView, err := s.viewOf(c.URI)
+			if err != nil {
+				return nil, nil, err
+			}
 			changedViews = append(changedViews, bestView)
 		}
+		affectedViews[c.URI] = changedViews
 
 		// Apply the changes to all affected views.
 		for _, view := range changedViews {
 			// Make sure that the file is added to the view.
 			if _, err := view.getFile(c.URI); err != nil {
-				return nil, nil, nil, err
+				return nil, nil, err
 			}
 			if _, ok := views[view]; !ok {
 				views[view] = make(map[span.URI]*fileChange)
@@ -471,7 +469,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif
 			} else {
 				fsFile, err := s.cache.getFile(ctx, c.URI)
 				if err != nil {
-					return nil, nil, nil, err
+					return nil, nil, err
 				}
 				content, err := fsFile.Read()
 				fh := &closedFile{fsFile}
@@ -484,14 +482,32 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif
 		}
 	}
 
-	snapshots := map[source.View]source.Snapshot{}
 	var releases []func()
+	viewToSnapshot := map[*View]*snapshot{}
 	for view, changed := range views {
 		snapshot, release := view.invalidateContent(ctx, changed, forceReloadMetadata)
-		snapshots[view] = snapshot
 		releases = append(releases, release)
+		viewToSnapshot[view] = snapshot
+	}
+
+	// We only want to diagnose each changed file once, in the view to which
+	// it "most" belongs. We do this by picking the best view for each URI,
+	// and then aggregating the set of snapshots and their URIs (to avoid
+	// diagnosing the same snapshot multiple times).
+	snapshotURIs := map[source.Snapshot][]span.URI{}
+	for _, mod := range changes {
+		viewSlice, ok := affectedViews[mod.URI]
+		if !ok || len(viewSlice) == 0 {
+			continue
+		}
+		view := bestViewForURI(mod.URI, viewSlice)
+		snapshot, ok := viewToSnapshot[view]
+		if !ok {
+			panic(fmt.Sprintf("no snapshot for view %s", view.Folder()))
+		}
+		snapshotURIs[snapshot] = append(snapshotURIs[snapshot], mod.URI)
 	}
-	return bestViews, snapshots, releases, nil
+	return snapshotURIs, releases, nil
 }
 
 func (s *Session) ExpandModificationsToDirectories(ctx context.Context, changes []source.FileModification) []source.FileModification {

internal/lsp/cache/view.go

@@ -353,15 +353,23 @@ func basename(filename string) string {
 
 func (v *View) relevantChange(c source.FileModification) bool {
 	// If the file is known to the view, the change is relevant.
-	known := v.knownFile(c.URI)
-
+	if v.knownFile(c.URI) {
+		return true
+	}
+	// The gopls.mod may not be "known" because we first access it through the
+	// session. As a result, treat changes to the view's gopls.mod file as
+	// always relevant, even if they are only on-disk changes.
+	// TODO(rstambler): Make sure the gopls.mod is always known to the view.
+	if c.URI == goplsModURI(v.rootURI) {
+		return true
+	}
 	// If the file is not known to the view, and the change is only on-disk,
 	// we should not invalidate the snapshot. This is necessary because Emacs
 	// sends didChangeWatchedFiles events for temp files.
-	if !known && c.OnDisk && (c.Action == source.Change || c.Action == source.Delete) {
+	if c.OnDisk && (c.Action == source.Change || c.Action == source.Delete) {
 		return false
 	}
-	return v.contains(c.URI) || known
+	return v.contains(c.URI)
 }
 
 func (v *View) knownFile(uri span.URI) bool {
@@ -578,7 +586,7 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) {
 
 // invalidateContent invalidates the content of a Go file,
 // including any position and type information that depends on it.
-func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]*fileChange, forceReloadMetadata bool) (source.Snapshot, func()) {
+func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]*fileChange, forceReloadMetadata bool) (*snapshot, func()) {
 	// Detach the context so that content invalidation cannot be canceled.
 	ctx = xcontext.Detach(ctx)
 

internal/lsp/source/view.go

@@ -303,9 +303,10 @@ type Session interface {
 	// GetFile returns a handle for the specified file.
 	GetFile(ctx context.Context, uri span.URI) (FileHandle, error)
 
-	// DidModifyFile reports a file modification to the session. It returns the
-	// resulting snapshots, a guaranteed one per view.
-	DidModifyFiles(ctx context.Context, changes []FileModification) (map[span.URI]View, map[View]Snapshot, []func(), error)
+	// DidModifyFile reports a file modification to the session. It returns
+	// the new snapshots after the modifications have been applied, paired with
+	// the affected file URIs for those snapshots.
+	DidModifyFiles(ctx context.Context, changes []FileModification) (map[Snapshot][]span.URI, []func(), error)
 
 	// ExpandModificationsToDirectories returns the set of changes with the
 	// directory changes removed and expanded to include all of the files in

internal/lsp/text_synchronization.go

@@ -221,21 +221,12 @@ func (s *Server) didModifyFiles(ctx context.Context, modifications []source.File
 	// to their files.
 	modifications = s.session.ExpandModificationsToDirectories(ctx, modifications)
 
-	views, snapshots, releases, err := s.session.DidModifyFiles(ctx, modifications)
+	snapshots, releases, err := s.session.DidModifyFiles(ctx, modifications)
 	if err != nil {
 		return err
 	}
 
-	// Group files by best view and diagnose them.
-	viewURIs := map[source.View][]span.URI{}
-	for uri, view := range views {
-		viewURIs[view] = append(viewURIs[view], uri)
-	}
-	for view, uris := range viewURIs {
-		snapshot := snapshots[view]
-		if snapshot == nil {
-			panic(fmt.Sprintf("no snapshot assigned for files %v", uris))
-		}
+	for snapshot, uris := range snapshots {
 		diagnosticWG.Add(1)
 		go func(snapshot source.Snapshot, uris []span.URI) {
 			defer diagnosticWG.Done()