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 {