From eeaf4eb24c733638f3405f6ea6944361e4d99ad8 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Tue, 30 Aug 2022 11:56:37 -0400 Subject: [PATCH] internal/lsp/fake: add rename file support for testing This CL implements the fake.Editor.RenameFile method, which mimic's the behavior of VS Code when renaming files. Specifically: - open buffers affected by the renaming are closed, and then reopened with new URIs - files are moved on disk - didChangeWatchedFile notifications are sent Along the way, add missing comments and fix one place where the editor mutex was held while sending notifications (in Editor.CreateBuffer). Generally, the editor should not hold any mutex while making a remote call. For golang/go#41567 Change-Id: I2abfa846e923de566a21c096502a68f125e7e671 Reviewed-on: https://go-review.googlesource.com/c/tools/+/427903 Auto-Submit: Robert Findley Reviewed-by: Alan Donovan Run-TryBot: Robert Findley TryBot-Result: Gopher Robot --- gopls/internal/lsp/fake/client.go | 10 +- gopls/internal/lsp/fake/editor.go | 125 ++++++++++++++++++--- gopls/internal/lsp/fake/workdir.go | 28 ++++- gopls/internal/lsp/regtest/wrappers.go | 9 ++ gopls/internal/regtest/misc/rename_test.go | 68 +++++++++++ 5 files changed, 215 insertions(+), 25 deletions(-) diff --git a/gopls/internal/lsp/fake/client.go b/gopls/internal/lsp/fake/client.go index 037de8e3d1..5885c29c9f 100644 --- a/gopls/internal/lsp/fake/client.go +++ b/gopls/internal/lsp/fake/client.go @@ -121,14 +121,8 @@ func (c *Client) ApplyEdit(ctx context.Context, params *protocol.ApplyWorkspaceE return &protocol.ApplyWorkspaceEditResult{FailureReason: "Edit.Changes is unsupported"}, nil } for _, change := range params.Edit.DocumentChanges { - // Todo: Add a handler for RenameFile edits - if change.RenameFile != nil { - panic("Fake editor does not support the RenameFile edits.") - } - if change.TextDocumentEdit != nil { - if err := c.editor.applyProtocolEdit(ctx, *change.TextDocumentEdit); err != nil { - return nil, err - } + if err := c.editor.applyDocumentChange(ctx, change); err != nil { + return nil, err } } return &protocol.ApplyWorkspaceEditResult{Applied: true}, nil diff --git a/gopls/internal/lsp/fake/editor.go b/gopls/internal/lsp/fake/editor.go index ebdf1a891d..2c988ce666 100644 --- a/gopls/internal/lsp/fake/editor.go +++ b/gopls/internal/lsp/fake/editor.go @@ -16,10 +16,11 @@ import ( "strings" "sync" - "golang.org/x/tools/internal/jsonrpc2" - "golang.org/x/tools/internal/jsonrpc2/servertest" "golang.org/x/tools/gopls/internal/lsp/command" "golang.org/x/tools/gopls/internal/lsp/protocol" + "golang.org/x/tools/gopls/internal/lsp/source" + "golang.org/x/tools/internal/jsonrpc2" + "golang.org/x/tools/internal/jsonrpc2/servertest" "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/xcontext" ) @@ -39,7 +40,7 @@ type Editor struct { mu sync.Mutex // guards config, buffers, serverCapabilities config EditorConfig // editor configuration - buffers map[string]buffer // open buffers + buffers map[string]buffer // open buffers (relative path -> buffer content) serverCapabilities protocol.ServerCapabilities // capabilities / options // Call metrics for the purpose of expectations. This is done in an ad-hoc @@ -50,16 +51,18 @@ type Editor struct { calls CallCounts } +// CallCounts tracks the number of protocol notifications of different types. type CallCounts struct { DidOpen, DidChange, DidSave, DidChangeWatchedFiles, DidClose uint64 } +// buffer holds information about an open buffer in the editor. type buffer struct { - windowsLineEndings bool - version int - path string - lines []string - dirty bool + windowsLineEndings bool // use windows line endings when merging lines + version int // monotonic version; incremented on edits + path string // relative path in the workspace + lines []string // line content + dirty bool // if true, content is unsaved (TODO(rfindley): rename this field) } func (b buffer) text() string { @@ -374,7 +377,6 @@ func (e *Editor) CreateBuffer(ctx context.Context, path, content string) error { func (e *Editor) createBuffer(ctx context.Context, path string, dirty bool, content string) error { e.mu.Lock() - defer e.mu.Unlock() buf := buffer{ windowsLineEndings: e.config.WindowsLineEndings, @@ -385,13 +387,25 @@ func (e *Editor) createBuffer(ctx context.Context, path string, dirty bool, cont } e.buffers[path] = buf - item := protocol.TextDocumentItem{ + item := e.textDocumentItem(buf) + e.mu.Unlock() + + return e.sendDidOpen(ctx, item) +} + +// textDocumentItem builds a protocol.TextDocumentItem for the given buffer. +// +// Precondition: e.mu must be held. +func (e *Editor) textDocumentItem(buf buffer) protocol.TextDocumentItem { + return protocol.TextDocumentItem{ URI: e.sandbox.Workdir.URI(buf.path), LanguageID: languageID(buf.path, e.config.FileAssociations), Version: int32(buf.version), Text: buf.text(), } +} +func (e *Editor) sendDidOpen(ctx context.Context, item protocol.TextDocumentItem) error { if e.Server != nil { if err := e.Server.DidOpen(ctx, &protocol.DidOpenTextDocumentParams{ TextDocument: item, @@ -451,9 +465,13 @@ func (e *Editor) CloseBuffer(ctx context.Context, path string) error { delete(e.buffers, path) e.mu.Unlock() + return e.sendDidClose(ctx, e.TextDocumentIdentifier(path)) +} + +func (e *Editor) sendDidClose(ctx context.Context, doc protocol.TextDocumentIdentifier) error { if e.Server != nil { if err := e.Server.DidClose(ctx, &protocol.DidCloseTextDocumentParams{ - TextDocument: e.TextDocumentIdentifier(path), + TextDocument: doc, }); err != nil { return fmt.Errorf("DidClose: %w", err) } @@ -1157,16 +1175,91 @@ func (e *Editor) Rename(ctx context.Context, path string, pos Pos, newName strin return err } for _, change := range wsEdits.DocumentChanges { - if change.TextDocumentEdit != nil { - if err := e.applyProtocolEdit(ctx, *change.TextDocumentEdit); err != nil { - return err - } + if err := e.applyDocumentChange(ctx, change); err != nil { + return err } } return nil } -func (e *Editor) applyProtocolEdit(ctx context.Context, change protocol.TextDocumentEdit) error { +func (e *Editor) RenameFile(ctx context.Context, oldPath, newPath string) error { + closed, opened, err := e.renameBuffers(ctx, oldPath, newPath) + if err != nil { + return err + } + + for _, c := range closed { + if err := e.sendDidClose(ctx, c); err != nil { + return err + } + } + for _, o := range opened { + if err := e.sendDidOpen(ctx, o); err != nil { + return err + } + } + + // Finally, perform the renaming on disk. + return e.sandbox.Workdir.RenameFile(ctx, oldPath, newPath) +} + +// renameBuffers renames in-memory buffers affected by the renaming of +// oldPath->newPath, returning the resulting text documents that must be closed +// and opened over the LSP. +func (e *Editor) renameBuffers(ctx context.Context, oldPath, newPath string) (closed []protocol.TextDocumentIdentifier, opened []protocol.TextDocumentItem, _ error) { + e.mu.Lock() + defer e.mu.Unlock() + + // In case either oldPath or newPath is absolute, convert to absolute paths + // before checking for containment. + oldAbs := e.sandbox.Workdir.AbsPath(oldPath) + newAbs := e.sandbox.Workdir.AbsPath(newPath) + + // Collect buffers that are affected by the given file or directory renaming. + buffersToRename := make(map[string]string) // old path -> new path + + for path := range e.buffers { + abs := e.sandbox.Workdir.AbsPath(path) + if oldAbs == abs || source.InDirLex(oldAbs, abs) { + rel, err := filepath.Rel(oldAbs, abs) + if err != nil { + return nil, nil, fmt.Errorf("filepath.Rel(%q, %q): %v", oldAbs, abs, err) + } + nabs := filepath.Join(newAbs, rel) + newPath := e.sandbox.Workdir.RelPath(nabs) + buffersToRename[path] = newPath + } + } + + // Update buffers, and build protocol changes. + for old, new := range buffersToRename { + buf := e.buffers[old] + delete(e.buffers, old) + buf.version = 1 + buf.path = new + e.buffers[new] = buf + + closed = append(closed, e.TextDocumentIdentifier(old)) + opened = append(opened, e.textDocumentItem(buf)) + } + + return closed, opened, nil +} + +func (e *Editor) applyDocumentChange(ctx context.Context, change protocol.DocumentChanges) error { + if change.RenameFile != nil { + oldPath := e.sandbox.Workdir.URIToPath(change.RenameFile.OldURI) + newPath := e.sandbox.Workdir.URIToPath(change.RenameFile.NewURI) + + return e.RenameFile(ctx, oldPath, newPath) + } + if change.TextDocumentEdit != nil { + return e.applyTextDocumentEdit(ctx, *change.TextDocumentEdit) + } + panic("Internal error: one of RenameFile or TextDocumentEdit must be set") +} + +func (e *Editor) applyTextDocumentEdit(ctx context.Context, change protocol.TextDocumentEdit) error { path := e.sandbox.Workdir.URIToPath(change.TextDocument.URI) if ver := int32(e.BufferVersion(path)); ver != change.TextDocument.Version { return fmt.Errorf("buffer versions for %q do not match: have %d, editing %d", path, ver, change.TextDocument.Version) diff --git a/gopls/internal/lsp/fake/workdir.go b/gopls/internal/lsp/fake/workdir.go index 4b8c2f30fd..6530260609 100644 --- a/gopls/internal/lsp/fake/workdir.go +++ b/gopls/internal/lsp/fake/workdir.go @@ -315,13 +315,39 @@ func (w *Workdir) writeFile(ctx context.Context, path, content string) (FileEven if err := WriteFileData(path, []byte(content), w.RelativeTo); err != nil { return FileEvent{}, err } + return w.fileEvent(path, changeType), nil +} + +func (w *Workdir) fileEvent(path string, changeType protocol.FileChangeType) FileEvent { return FileEvent{ Path: path, ProtocolEvent: protocol.FileEvent{ URI: w.URI(path), Type: changeType, }, - }, nil + } +} + +// RenameFile performs an on disk-renaming of the workdir-relative oldPath to +// workdir-relative newPath. +func (w *Workdir) RenameFile(ctx context.Context, oldPath, newPath string) error { + oldAbs := w.AbsPath(oldPath) + newAbs := w.AbsPath(newPath) + + if err := os.Rename(oldAbs, newAbs); err != nil { + return err + } + + // Send synthetic file events for the renaming. Renamed files are handled as + // Delete+Create events: + // https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#fileChangeType + events := []FileEvent{ + w.fileEvent(oldPath, protocol.Deleted), + w.fileEvent(newPath, protocol.Created), + } + w.sendEvents(ctx, events) + + return nil } // listFiles lists files in the given directory, returning a map of relative diff --git a/gopls/internal/lsp/regtest/wrappers.go b/gopls/internal/lsp/regtest/wrappers.go index b5d1783e55..0f7cc9a1a6 100644 --- a/gopls/internal/lsp/regtest/wrappers.go +++ b/gopls/internal/lsp/regtest/wrappers.go @@ -400,6 +400,7 @@ func (e *Env) References(path string, pos fake.Pos) []protocol.Location { return locations } +// Rename wraps Editor.Rename, calling t.Fatal on any error. func (e *Env) Rename(path string, pos fake.Pos, newName string) { e.T.Helper() if err := e.Editor.Rename(e.Ctx, path, pos, newName); err != nil { @@ -407,6 +408,14 @@ func (e *Env) Rename(path string, pos fake.Pos, newName string) { } } +// RenameFile wraps Editor.RenameFile, calling t.Fatal on any error. +func (e *Env) RenameFile(oldPath, newPath string) { + e.T.Helper() + if err := e.Editor.RenameFile(e.Ctx, oldPath, newPath); err != nil { + e.T.Fatal(err) + } +} + // Completion executes a completion request on the server. func (e *Env) Completion(path string, pos fake.Pos) *protocol.CompletionList { e.T.Helper() diff --git a/gopls/internal/regtest/misc/rename_test.go b/gopls/internal/regtest/misc/rename_test.go index 70fc9c976d..a0447c5026 100644 --- a/gopls/internal/regtest/misc/rename_test.go +++ b/gopls/internal/regtest/misc/rename_test.go @@ -132,3 +132,71 @@ func main() { } }) } + +// This is a test that rename operation initiated by the editor function as expected. +func TestRenameFileFromEditor(t *testing.T) { + const files = ` +-- go.mod -- +module mod.com + +go 1.16 +-- a/a.go -- +package a + +const X = 1 +-- a/x.go -- +package a + +const X = 2 +-- b/b.go -- +package b +` + + Run(t, files, func(t *testing.T, env *Env) { + // Rename files and verify that diagnostics are affected accordingly. + + // Initially, we should have diagnostics on both X's, for their duplicate declaration. + env.Await( + OnceMet( + InitialWorkspaceLoad, + env.DiagnosticAtRegexp("a/a.go", "X"), + env.DiagnosticAtRegexp("a/x.go", "X"), + ), + ) + + // Moving x.go should make the diagnostic go away. + env.RenameFile("a/x.go", "b/x.go") + env.Await( + OnceMet( + env.DoneWithChangeWatchedFiles(), + EmptyDiagnostics("a/a.go"), // no more duplicate declarations + env.DiagnosticAtRegexp("b/b.go", "package"), // as package names mismatch + ), + ) + + // Renaming should also work on open buffers. + env.OpenFile("b/x.go") + + // Moving x.go back to a/ should cause the diagnostics to reappear. + env.RenameFile("b/x.go", "a/x.go") + // TODO(rfindley): enable using a OnceMet precondition here. We can't + // currently do this because DidClose, DidOpen and DidChangeWatchedFiles + // are sent, and it is not easy to use all as a precondition. + env.Await( + env.DiagnosticAtRegexp("a/a.go", "X"), + env.DiagnosticAtRegexp("a/x.go", "X"), + ) + + // Renaming the entire directory should move both the open and closed file. + env.RenameFile("a", "x") + env.Await( + env.DiagnosticAtRegexp("x/a.go", "X"), + env.DiagnosticAtRegexp("x/x.go", "X"), + ) + + // As a sanity check, verify that x/x.go is open. + if text := env.Editor.BufferText("x/x.go"); text == "" { + t.Fatal("got empty buffer for x/x.go") + } + }) +}