From 219d3418f548f07d1c2bb552ce3b5553edec04d5 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Wed, 22 Jan 2020 21:58:50 -0500 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/session.go | 10 +-- internal/lsp/cache/view.go | 13 +-- internal/lsp/lsp_test.go | 22 ++--- internal/lsp/source/source_test.go | 22 ++--- internal/lsp/text_synchronization.go | 115 ++++++++++++--------------- 5 files changed, 75 insertions(+), 107 deletions(-) diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index ff423a1baa..25d490a504 100644 --- a/internal/lsp/cache/session.go +++ b/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,8 +274,6 @@ 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. @@ -284,16 +281,11 @@ func (s *session) DidModifyFiles(ctx context.Context, changes []source.FileModif 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 } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 3c2b2ad0e8..31678fb26d 100644 --- a/internal/lsp/cache/view.go +++ b/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) diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index be6cef54ae..2a9c502f9b 100644 --- a/internal/lsp/lsp_test.go +++ b/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{ diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 0db99477f1..e561a409f9 100644 --- a/internal/lsp/source/source_test.go +++ b/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) } diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go index 8e38f478a1..f11ccb6be4 100644 --- a/internal/lsp/text_synchronization.go +++ b/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: + uris[uri] = snapshot + snapshotSet[snapshot] = struct{}{} + } + for snapshot := range snapshotSet { go s.diagnoseSnapshot(snapshot) } - - 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 - } - } - return nil, nil, errors.Errorf("bestSnapshot: no snapshot for %s", uri) + return uris, nil } func (s *Server) wasFirstChange(uri span.URI) bool {