From 78b158585360beccadc3faac6e35759f491831f3 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Tue, 3 Nov 2020 15:56:12 -0500 Subject: [PATCH] internal/lsp/fake: reflect on-disk changes in clean buffers If an open buffer is unmodified from the on-disk version, and that on-disk content changes, VS Code will update the buffer with the change. This is relevant for our quick fixes that use the go command to make changes to go.mod/sum -- we want tests to pick up the changes without needing to close/reopen the buffer. For whatever reason, VS Code doesn't do this for deletions, and it made the implementation easier, so I followed suit. I also mimicked its behavior of sending both in-memory and on-disk change notifications. Change-Id: I838a64b2f48f3cbe1a86035293923951b53aecf3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/267577 Trust: Heschi Kreinick Run-TryBot: Heschi Kreinick gopls-CI: kokoro Reviewed-by: Robert Findley --- gopls/internal/regtest/codelens_test.go | 7 ++- gopls/internal/regtest/diagnostics_test.go | 1 - gopls/internal/regtest/modfile_test.go | 6 +- internal/lsp/fake/editor.go | 64 +++++++++++++++++----- 4 files changed, 57 insertions(+), 21 deletions(-) diff --git a/gopls/internal/regtest/codelens_test.go b/gopls/internal/regtest/codelens_test.go index 9dfafde359..a5fca844f5 100644 --- a/gopls/internal/regtest/codelens_test.go +++ b/gopls/internal/regtest/codelens_test.go @@ -9,6 +9,7 @@ import ( "strings" "testing" + "golang.org/x/tools/internal/lsp" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/lsp/tests" @@ -110,7 +111,8 @@ func main() { runner.Run(t, shouldUpdateDep, func(t *testing.T, env *Env) { env.OpenFile("go.mod") env.ExecuteCodeLensCommand("go.mod", source.CommandUpgradeDependency) - got := env.ReadWorkspaceFile("go.mod") + env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), 1)) + got := env.Editor.BufferText("go.mod") const wantGoMod = `module mod.com go 1.12 @@ -164,7 +166,8 @@ func main() { runner.Run(t, shouldRemoveDep, func(t *testing.T, env *Env) { env.OpenFile("go.mod") env.ExecuteCodeLensCommand("go.mod", source.CommandTidy) - got := env.ReadWorkspaceFile("go.mod") + env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), 1)) + got := env.Editor.BufferText("go.mod") const wantGoMod = `module mod.com go 1.14 diff --git a/gopls/internal/regtest/diagnostics_test.go b/gopls/internal/regtest/diagnostics_test.go index 83decca0f5..d3685ddac1 100644 --- a/gopls/internal/regtest/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics_test.go @@ -639,7 +639,6 @@ var ErrHelpWanted error // Test for golang/go#38211. func Test_Issue38211(t *testing.T) { - t.Skip("Requires CL 267577 to work without the save hook.") testenv.NeedsGo1Point(t, 14) const ardanLabs = ` -- go.mod -- diff --git a/gopls/internal/regtest/modfile_test.go b/gopls/internal/regtest/modfile_test.go index 69ff5e9441..2f897a2fa9 100644 --- a/gopls/internal/regtest/modfile_test.go +++ b/gopls/internal/regtest/modfile_test.go @@ -379,10 +379,8 @@ require ( example.com/blah/v2 v2.0.0 ) ` - // We need to read from disk even though go.mod is open -- the regtests - // currently don't apply on-disk changes to open but unmodified buffers - // like most editors would. - if got := env.ReadWorkspaceFile("go.mod"); got != want { + env.Await(EmptyDiagnostics("go.mod")) + if got := env.Editor.BufferText("go.mod"); got != want { t.Fatalf("suggested fixes failed:\n%s", tests.Diff(want, got)) } }) diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go index 9ef80fe1ff..3cc1c061c8 100644 --- a/internal/lsp/fake/editor.go +++ b/internal/lsp/fake/editor.go @@ -47,6 +47,7 @@ type buffer struct { version int path string content []string + dirty bool } func (b buffer) text() string { @@ -270,10 +271,28 @@ func (e *Editor) onFileChanges(ctx context.Context, evts []FileEvent) { if e.Server == nil { return } + e.mu.Lock() var lspevts []protocol.FileEvent for _, evt := range evts { + // Always send an on-disk change, even for events that seem useless + // because they're shadowed by an open buffer. lspevts = append(lspevts, evt.ProtocolEvent) + + if buf, ok := e.buffers[evt.Path]; ok { + // Following VS Code, don't honor deletions or changes to dirty buffers. + if buf.dirty || evt.ProtocolEvent.Type == protocol.Deleted { + continue + } + + content, err := e.sandbox.Workdir.ReadFile(evt.Path) + if err != nil { + continue // A race with some other operation. + } + // During shutdown, this call will fail. Ignore the error. + _ = e.setBufferContentLocked(ctx, evt.Path, false, strings.Split(content, "\n"), nil) + } } + e.mu.Unlock() e.Server.DidChangeWatchedFiles(ctx, &protocol.DidChangeWatchedFilesParams{ Changes: lspevts, }) @@ -285,15 +304,7 @@ func (e *Editor) OpenFile(ctx context.Context, path string) error { if err != nil { return err } - return e.CreateBuffer(ctx, path, content) -} - -func newBuffer(path, content string) buffer { - return buffer{ - version: 1, - path: path, - content: strings.Split(content, "\n"), - } + return e.createBuffer(ctx, path, false, content) } func textDocumentItem(wd *Workdir, buf buffer) protocol.TextDocumentItem { @@ -314,7 +325,16 @@ func textDocumentItem(wd *Workdir, buf buffer) protocol.TextDocumentItem { // CreateBuffer creates a new unsaved buffer corresponding to the workdir path, // containing the given textual content. func (e *Editor) CreateBuffer(ctx context.Context, path, content string) error { - buf := newBuffer(path, content) + return e.createBuffer(ctx, path, true, content) +} + +func (e *Editor) createBuffer(ctx context.Context, path string, dirty bool, content string) error { + buf := buffer{ + version: 1, + path: path, + content: strings.Split(content, "\n"), + dirty: dirty, + } e.mu.Lock() e.buffers[path] = buf item := textDocumentItem(e.sandbox.Workdir, buf) @@ -396,6 +416,12 @@ func (e *Editor) SaveBufferWithoutActions(ctx context.Context, path string) erro if err := e.sandbox.Workdir.WriteFile(ctx, path, content); err != nil { return errors.Errorf("writing %q: %w", path, err) } + + e.mu.Lock() + buf.dirty = false + e.buffers[path] = buf + e.mu.Unlock() + if e.Server != nil { params := &protocol.DidSaveTextDocumentParams{ TextDocument: protocol.VersionedTextDocumentIdentifier{ @@ -534,7 +560,7 @@ func (e *Editor) SetBufferContent(ctx context.Context, path, content string) err e.mu.Lock() defer e.mu.Unlock() lines := strings.Split(content, "\n") - return e.setBufferContentLocked(ctx, path, lines, nil) + return e.setBufferContentLocked(ctx, path, true, lines, nil) } // BufferText returns the content of the buffer with the given name. @@ -563,16 +589,17 @@ func (e *Editor) editBufferLocked(ctx context.Context, path string, edits []Edit if err != nil { return err } - return e.setBufferContentLocked(ctx, path, content, edits) + return e.setBufferContentLocked(ctx, path, true, content, edits) } -func (e *Editor) setBufferContentLocked(ctx context.Context, path string, content []string, fromEdits []Edit) error { +func (e *Editor) setBufferContentLocked(ctx context.Context, path string, dirty bool, content []string, fromEdits []Edit) error { buf, ok := e.buffers[path] if !ok { return fmt.Errorf("unknown buffer %q", path) } buf.content = content buf.version++ + buf.dirty = dirty e.buffers[path] = buf // A simple heuristic: if there is only one edit, send it incrementally. // Otherwise, send the entire content. @@ -739,7 +766,16 @@ func (e *Editor) ExecuteCommand(ctx context.Context, params *protocol.ExecuteCom if !match { return nil, fmt.Errorf("unsupported command %q", params.Command) } - return e.Server.ExecuteCommand(ctx, params) + result, err := e.Server.ExecuteCommand(ctx, params) + if err != nil { + return nil, err + } + // Some commands use the go command, which writes directly to disk. + // For convenience, check for those changes. + if err := e.sandbox.Workdir.CheckForFileChanges(ctx); err != nil { + return nil, err + } + return result, nil } func convertEdits(protocolEdits []protocol.TextEdit) []Edit {