internal/lsp: batch file changes in didChangeWatchedFiles

Remove the special handling for go.mod file saves. This was only really
added to be extra careful, but our cancellation logic should cope with
this.

Fixes golang/go#31553

Change-Id: I0a69bcdeaf6369697e79aba4689a7b714484ccc2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215908
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

Author: stamblerre

internal/lsp/cache/session.go

@@ -254,7 +254,6 @@ func (s *session) dropView(ctx context.Context, v *view) (int, error) {
 
 func (s *session) DidModifyFiles(ctx context.Context, changes []source.FileModification) ([]source.Snapshot, error) {
 	views := make(map[*view][]span.URI)
-	saves := make(map[*view]bool)
 
 	for _, c := range changes {
 		// Only update overlays for in-editor changes.
@@ -275,25 +274,18 @@ func (s *session) DidModifyFiles(ctx context.Context, changes []source.FileModif
 					if !view.knownFile(c.URI) {
 						continue
 					}
-				case source.Save:
-					panic("save considered an on-disk change")
 				}
 			}
 			// Make sure that the file is added to the view.
 			if _, err := view.getFile(c.URI); err != nil {
 				return nil, err
 			}
 			views[view] = append(views[view], c.URI)
-			saves[view] = len(changes) == 1 && !changes[0].OnDisk && changes[0].Action == source.Save
 		}
 	}
 	var snapshots []source.Snapshot
 	for view, uris := range views {
-		containsFileSave, ok := saves[view]
-		if !ok {
-			panic("unknown view")
-		}
-		snapshots = append(snapshots, view.invalidateContent(ctx, uris, containsFileSave))
+		snapshots = append(snapshots, view.invalidateContent(ctx, uris))
 	}
 	return snapshots, nil
 }

internal/lsp/cache/view.go

@@ -577,22 +577,13 @@ func (v *view) awaitInitialized(ctx context.Context) error {
 // 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, uris []span.URI, containsFileSave bool) source.Snapshot {
+func (v *view) invalidateContent(ctx context.Context, uris []span.URI) source.Snapshot {
 	// Detach the context so that content invalidation cannot be canceled.
 	ctx = xcontext.Detach(ctx)
 
-	if containsFileSave && len(uris) > 1 {
-		panic("file save among multiple content invalidations")
-	}
-
 	// Cancel all still-running previous requests, since they would be
 	// operating on stale data.
-	//
-	// TODO(rstambler): File saves should also lead to cancellation,
-	// but this will only be possible when they trigger workspace-level diagnostics.
-	if !containsFileSave {
-		v.cancelBackground()
-	}
+	v.cancelBackground()
 
 	// Do not clone a snapshot until its view has finished initializing.
 	_ = v.awaitInitialized(ctx)

internal/lsp/lsp_test.go

@@ -59,22 +59,22 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
 	if _, _, err := session.NewView(ctx, viewName, span.FileURI(data.Config.Dir), options); err != nil {
 		t.Fatal(err)
 	}
+	var modifications []source.FileModification
 	for filename, content := range data.Config.Overlay {
 		kind := source.DetectLanguage("", filename)
 		if kind != source.Go {
 			continue
 		}
-		if _, err := session.DidModifyFiles(ctx, []source.FileModification{
-			{
-				URI:        span.FileURI(filename),
-				Action:     source.Open,
-				Version:    -1,
-				Text:       content,
-				LanguageID: "go",
-			},
-		}); err != nil {
-			t.Fatal(err)
-		}
+		modifications = append(modifications, source.FileModification{
+			URI:        span.FileURI(filename),
+			Action:     source.Open,
+			Version:    -1,
+			Text:       content,
+			LanguageID: "go",
+		})
+	}
+	if _, err := session.DidModifyFiles(ctx, modifications); err != nil {
+		t.Fatal(err)
 	}
 	r := &runner{
 		server: &Server{

internal/lsp/source/source_test.go

@@ -60,22 +60,22 @@ func testSource(t *testing.T, exporter packagestest.Exporter) {
 		data: data,
 		ctx:  ctx,
 	}
+	var modifications []source.FileModification
 	for filename, content := range data.Config.Overlay {
 		kind := source.DetectLanguage("", filename)
 		if kind != source.Go {
 			continue
 		}
-		if _, err := session.DidModifyFiles(ctx, []source.FileModification{
-			{
-				URI:        span.FileURI(filename),
-				Action:     source.Open,
-				Version:    -1,
-				Text:       content,
-				LanguageID: "go",
-			},
-		}); err != nil {
-			t.Fatal(err)
-		}
+		modifications = append(modifications, source.FileModification{
+			URI:        span.FileURI(filename),
+			Action:     source.Open,
+			Version:    -1,
+			Text:       content,
+			LanguageID: "go",
+		})
+	}
+	if _, err := session.DidModifyFiles(ctx, modifications); err != nil {
+		t.Fatal(err)
 	}
 	tests.Run(t, r, data)
 }

internal/lsp/text_synchronization.go

@@ -12,18 +12,19 @@ import (
 	"golang.org/x/tools/internal/jsonrpc2"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/source"
-	"golang.org/x/tools/internal/lsp/telemetry"
 	"golang.org/x/tools/internal/span"
 	errors "golang.org/x/xerrors"
 )
 
 func (s *Server) didOpen(ctx context.Context, params *protocol.DidOpenTextDocumentParams) error {
-	_, err := s.didModifyFile(ctx, source.FileModification{
-		URI:        span.NewURI(params.TextDocument.URI),
-		Action:     source.Open,
-		Version:    params.TextDocument.Version,
-		Text:       []byte(params.TextDocument.Text),
-		LanguageID: params.TextDocument.LanguageID,
+	_, err := s.didModifyFiles(ctx, []source.FileModification{
+		{
+			URI:        span.NewURI(params.TextDocument.URI),
+			Action:     source.Open,
+			Version:    params.TextDocument.Version,
+			Text:       []byte(params.TextDocument.Text),
+			LanguageID: params.TextDocument.LanguageID,
+		},
 	})
 	return err
 }
@@ -40,10 +41,14 @@ func (s *Server) didChange(ctx context.Context, params *protocol.DidChangeTextDo
 		Version: params.TextDocument.Version,
 		Text:    text,
 	}
-	snapshot, err := s.didModifyFile(ctx, c)
+	snapshots, err := s.didModifyFiles(ctx, []source.FileModification{c})
 	if err != nil {
 		return err
 	}
+	snapshot := snapshots[uri]
+	if snapshot == nil {
+		return errors.Errorf("no snapshot for %s", uri)
+	}
 	// Ideally, we should be able to specify that a generated file should be opened as read-only.
 	// Tell the user that they should not be editing a generated file.
 	if s.wasFirstChange(uri) && source.IsGenerated(ctx, snapshot, uri) {
@@ -58,38 +63,23 @@ func (s *Server) didChange(ctx context.Context, params *protocol.DidChangeTextDo
 }
 
 func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.DidChangeWatchedFilesParams) error {
-	// Keep track of each change's view and final snapshot.
-	views := make(map[source.View]source.Snapshot)
+	var modifications []source.FileModification
 	for _, change := range params.Changes {
 		uri := span.NewURI(change.URI)
-		ctx := telemetry.File.With(ctx, uri)
 
 		// Do nothing if the file is open in the editor.
 		// The editor is the source of truth.
 		if s.session.IsOpen(uri) {
 			continue
 		}
-		snapshots, err := s.session.DidModifyFiles(ctx, []source.FileModification{
-			{
-				URI:    uri,
-				Action: changeTypeToFileAction(change.Type),
-				OnDisk: true,
-			},
+		modifications = append(modifications, source.FileModification{
+			URI:    uri,
+			Action: changeTypeToFileAction(change.Type),
+			OnDisk: true,
 		})
-		if err != nil {
-			return err
-		}
-		snapshot, _, err := snapshotOf(s.session, uri, snapshots)
-		if err != nil {
-			return err
-		}
-		views[snapshot.View()] = snapshot
-	}
-	// Diagnose all resulting snapshots.
-	for _, snapshot := range views {
-		go s.diagnoseSnapshot(snapshot)
 	}
-	return nil
+	_, err := s.didModifyFiles(ctx, modifications)
+	return err
 }
 
 func (s *Server) didSave(ctx context.Context, params *protocol.DidSaveTextDocumentParams) error {
@@ -101,59 +91,54 @@ func (s *Server) didSave(ctx context.Context, params *protocol.DidSaveTextDocume
 	if params.Text != nil {
 		c.Text = []byte(*params.Text)
 	}
-	_, err := s.didModifyFile(ctx, c)
+	_, err := s.didModifyFiles(ctx, []source.FileModification{c})
 	return err
 }
 
 func (s *Server) didClose(ctx context.Context, params *protocol.DidCloseTextDocumentParams) error {
-	_, err := s.didModifyFile(ctx, source.FileModification{
-		URI:     span.NewURI(params.TextDocument.URI),
-		Action:  source.Close,
-		Version: -1,
-		Text:    nil,
+	_, err := s.didModifyFiles(ctx, []source.FileModification{
+		{
+			URI:     span.NewURI(params.TextDocument.URI),
+			Action:  source.Close,
+			Version: -1,
+			Text:    nil,
+		},
 	})
 	return err
 }
 
-func (s *Server) didModifyFile(ctx context.Context, c source.FileModification) (source.Snapshot, error) {
-	snapshots, err := s.session.DidModifyFiles(ctx, []source.FileModification{c})
+func (s *Server) didModifyFiles(ctx context.Context, modifications []source.FileModification) (map[span.URI]source.Snapshot, error) {
+	snapshots, err := s.session.DidModifyFiles(ctx, modifications)
 	if err != nil {
 		return nil, err
 	}
-	snapshot, _, err := snapshotOf(s.session, c.URI, snapshots)
-	if err != nil {
-		return nil, err
+	uris := make(map[span.URI]source.Snapshot)
+	for _, c := range modifications {
+		uris[c.URI] = nil
 	}
-	switch c.Action {
-	case source.Save:
-		// If we're saving a go.mod file, all of the metadata has been invalidated,
-		// so we need to run diagnostics and make sure that they cannot be canceled.
-		fh, err := snapshot.GetFile(c.URI)
+	// Avoid diagnosing the same snapshot twice.
+	snapshotSet := make(map[source.Snapshot]struct{})
+	for uri := range uris {
+		view, err := s.session.ViewOf(uri)
 		if err != nil {
 			return nil, err
 		}
-		if fh.Identity().Kind == source.Mod {
-			go s.diagnoseDetached(snapshot)
+		var snapshot source.Snapshot
+		for _, s := range snapshots {
+			if s.View() == view {
+				if snapshot != nil {
+					return nil, errors.Errorf("duplicate snapshots for the same view")
+				}
+				snapshot = s
+			}
 		}
-	default:
-		go s.diagnoseSnapshot(snapshot)
+		uris[uri] = snapshot
+		snapshotSet[snapshot] = struct{}{}
 	}
-
-	return snapshot, nil
-}
-
-// snapshotOf returns the snapshot corresponding to the view for the given file URI.
-func snapshotOf(session source.Session, uri span.URI, snapshots []source.Snapshot) (source.Snapshot, source.View, error) {
-	view, err := session.ViewOf(uri)
-	if err != nil {
-		return nil, nil, err
-	}
-	for _, s := range snapshots {
-		if s.View() == view {
-			return s, view, nil
-		}
+	for snapshot := range snapshotSet {
+		go s.diagnoseSnapshot(snapshot)
 	}
-	return nil, nil, errors.Errorf("bestSnapshot: no snapshot for %s", uri)
+	return uris, nil
 }
 
 func (s *Server) wasFirstChange(uri span.URI) bool {