internal/lsp: refactor and clean up text synchronization
This change is the first step in reorganizing the logical flow of file
changes in the internal/lsp package. A new function,
(*server).didModifyFile does the bulk of the work, but we will be able
to push most of its details into the cache layer in a follow-up.
Also, some refactoring of the applyChanges function to flow more
logically. It was unnecessarily convoluted.
Change-Id: Icc1b8642a4cb04d309338b0f8840fe58133d3df1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209979
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index 162a4a5..6e7a140 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -193,12 +193,27 @@
SetOptions(Options)
}
+// FileModification represents a modification to a file.
+type FileModification struct {
+ URI span.URI
+ Action FileAction
+
+ // Version will be -1 and Text will be nil when they are not supplied,
+ // specifically on textDocument/didClose.
+ Version float64
+ Text []byte
+
+ // LanguageID is only sent from the language client on textDocument/didOpen.
+ LanguageID string
+}
+
type FileAction int
const (
Open = FileAction(iota)
- Close
Change
+ Close
+ Save
Create
Delete
UnknownFileAction
diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go
index 0b3563e..3f96e20 100644
--- a/internal/lsp/text_synchronization.go
+++ b/internal/lsp/text_synchronization.go
@@ -18,71 +18,93 @@
)
func (s *Server) didOpen(ctx context.Context, params *protocol.DidOpenTextDocumentParams) error {
- uri := span.NewURI(params.TextDocument.URI)
- text := []byte(params.TextDocument.Text)
- version := params.TextDocument.Version
-
// Confirm that the file's language ID is related to Go.
- fileKind := source.DetectLanguage(params.TextDocument.LanguageID, uri.Filename())
+ uri := span.NewURI(params.TextDocument.URI)
+ return s.didModifyFile(ctx, source.FileModification{
+ URI: uri,
+ Action: source.Open,
+ Version: params.TextDocument.Version,
+ Text: []byte(params.TextDocument.Text),
+ LanguageID: params.TextDocument.LanguageID,
+ })
+}
- // Open the file.
- s.session.DidOpen(ctx, uri, fileKind, version, text)
+func (s *Server) didSave(ctx context.Context, params *protocol.DidSaveTextDocumentParams) error {
+ return s.didModifyFile(ctx, source.FileModification{
+ URI: span.NewURI(params.TextDocument.URI),
+ Action: source.Save,
+ Version: params.TextDocument.Version,
+ Text: []byte(params.Text),
+ })
+}
- view, err := s.session.ViewOf(uri)
- if err != nil {
- return err
- }
- snapshot := view.Snapshot()
-
- // Run diagnostics on the newly-opened file.
- go s.diagnoseFile(snapshot, uri)
-
- return nil
+func (s *Server) didClose(ctx context.Context, params *protocol.DidCloseTextDocumentParams) error {
+ return s.didModifyFile(ctx, source.FileModification{
+ URI: span.NewURI(params.TextDocument.URI),
+ Action: source.Close,
+ Version: -1,
+ Text: nil,
+ })
}
func (s *Server) didChange(ctx context.Context, params *protocol.DidChangeTextDocumentParams) error {
- options := s.session.Options()
- if len(params.ContentChanges) < 1 {
- return jsonrpc2.NewErrorf(jsonrpc2.CodeInternalError, "no content changes provided")
- }
-
uri := span.NewURI(params.TextDocument.URI)
-
- // Check if the client sent the full content of the file.
- // We accept a full content change even if the server expected incremental changes.
- text, isFullChange := fullChange(params.ContentChanges)
-
- // We only accept an incremental change if the server expected it.
- if !isFullChange {
- switch options.TextDocumentSyncKind {
- case protocol.Full:
- return errors.Errorf("expected a full content change, received incremental changes for %s", uri)
- case protocol.Incremental:
- // Determine the new file content.
- var err error
- text, err = s.applyChanges(ctx, uri, params.ContentChanges)
- if err != nil {
- return err
- }
- }
- }
- // Cache the new file content and send fresh diagnostics.
- view, err := s.session.ViewOf(uri)
+ text, err := s.changedText(ctx, uri, params.ContentChanges)
if err != nil {
return err
}
- view.SetContent(ctx, uri, params.TextDocument.Version, []byte(text))
- // 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, view, uri) {
- s.client.ShowMessage(ctx, &protocol.ShowMessageParams{
- Message: fmt.Sprintf("Do not edit this file! %s is a generated file.", uri.Filename()),
- Type: protocol.Warning,
- })
- }
- // Run diagnostics on the newly-changed file.
- go s.diagnoseFile(view.Snapshot(), uri)
+ return s.didModifyFile(ctx, source.FileModification{
+ URI: uri,
+ Action: source.Change,
+ Version: params.TextDocument.Version,
+ Text: text,
+ })
+}
+// didModifyFile propagates the information about the file modification
+// to the cache layer and runs diagnostics.
+//
+// TODO(rstambler): This function should be mostly unnecessary once we unify the methods
+// for making changes to a file in internal/lsp/cache.
+func (s *Server) didModifyFile(ctx context.Context, c source.FileModification) error {
+ ctx = telemetry.URI.With(ctx, c.URI)
+
+ view, err := s.session.ViewOf(c.URI)
+ if err != nil {
+ return err
+ }
+ switch c.Action {
+ case source.Open:
+ kind := source.DetectLanguage(c.LanguageID, c.URI.Filename())
+ if kind == source.UnknownKind {
+ return errors.Errorf("didModifyFile: unknown file kind for %s", c.URI)
+ }
+ if err := s.session.DidOpen(ctx, c.URI, kind, c.Version, c.Text); err != nil {
+ return err
+ }
+ case source.Change:
+ view.SetContent(ctx, c.URI, c.Version, c.Text)
+
+ // 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(c.URI) && source.IsGenerated(ctx, view, c.URI) {
+ s.client.ShowMessage(ctx, &protocol.ShowMessageParams{
+ Message: fmt.Sprintf("Do not edit this file! %s is a generated file.", c.URI.Filename()),
+ Type: protocol.Warning,
+ })
+ }
+ case source.Save:
+ s.session.DidSave(c.URI, c.Version)
+ case source.Close:
+ s.session.DidClose(c.URI)
+ view.SetContent(ctx, c.URI, c.Version, c.Text)
+ }
+
+ // We should run diagnostics after opening or changing a file.
+ switch c.Action {
+ case source.Open, source.Change:
+ go s.diagnoseFile(view.Snapshot(), c.URI)
+ }
return nil
}
@@ -94,43 +116,51 @@
return ok
}
-func fullChange(changes []protocol.TextDocumentContentChangeEvent) (string, bool) {
- if len(changes) > 1 {
- return "", false
- }
- // The length of the changes must be 1 at this point.
- // TODO: This breaks if you insert a character at the beginning of the file.
- if changes[0].Range == nil && changes[0].RangeLength == 0 {
- return changes[0].Text, true
+func (s *Server) changedText(ctx context.Context, uri span.URI, changes []protocol.TextDocumentContentChangeEvent) ([]byte, error) {
+ if len(changes) == 0 {
+ return nil, jsonrpc2.NewErrorf(jsonrpc2.CodeInternalError, "no content changes provided")
}
- return "", false
+ // Check if the client sent the full content of the file.
+ // We accept a full content change even if the server expected incremental changes.
+ if len(changes) == 1 && changes[0].Range == nil && changes[0].RangeLength == 0 {
+ return []byte(changes[0].Text), nil
+ }
+
+ // We only accept an incremental change if that's what the server expects.
+ if s.session.Options().TextDocumentSyncKind == protocol.Full {
+ return nil, errors.Errorf("expected a full content change, received incremental changes for %s", uri)
+ }
+
+ return s.applyIncrementalChanges(ctx, uri, changes)
}
-func (s *Server) applyChanges(ctx context.Context, uri span.URI, changes []protocol.TextDocumentContentChangeEvent) (string, error) {
+func (s *Server) applyIncrementalChanges(ctx context.Context, uri span.URI, changes []protocol.TextDocumentContentChangeEvent) ([]byte, error) {
content, _, err := s.session.GetFile(uri, source.UnknownKind).Read(ctx)
if err != nil {
- return "", jsonrpc2.NewErrorf(jsonrpc2.CodeInternalError, "file not found (%v)", err)
+ return nil, jsonrpc2.NewErrorf(jsonrpc2.CodeInternalError, "file not found (%v)", err)
}
for _, change := range changes {
- // Update column mapper along with the content.
+ // Make sure to update column mapper along with the content.
converter := span.NewContentConverter(uri.Filename(), content)
m := &protocol.ColumnMapper{
URI: uri,
Converter: converter,
Content: content,
}
-
- spn, err := m.RangeSpan(*change.Range) // Could Range be nil here?
+ if change.Range == nil {
+ return nil, jsonrpc2.NewErrorf(jsonrpc2.CodeInternalError, "unexpected nil range for change")
+ }
+ spn, err := m.RangeSpan(*change.Range)
if err != nil {
- return "", err
+ return nil, err
}
if !spn.HasOffset() {
- return "", jsonrpc2.NewErrorf(jsonrpc2.CodeInternalError, "invalid range for content change")
+ return nil, jsonrpc2.NewErrorf(jsonrpc2.CodeInternalError, "invalid range for content change")
}
start, end := spn.Start().Offset(), spn.End().Offset()
if end < start {
- return "", jsonrpc2.NewErrorf(jsonrpc2.CodeInternalError, "invalid range for content change")
+ return nil, jsonrpc2.NewErrorf(jsonrpc2.CodeInternalError, "invalid range for content change")
}
var buf bytes.Buffer
buf.Write(content[:start])
@@ -138,22 +168,5 @@
buf.Write(content[end:])
content = buf.Bytes()
}
- return string(content), nil
-}
-
-func (s *Server) didSave(ctx context.Context, params *protocol.DidSaveTextDocumentParams) error {
- s.session.DidSave(span.NewURI(params.TextDocument.URI), params.TextDocument.Version)
- return nil
-}
-
-func (s *Server) didClose(ctx context.Context, params *protocol.DidCloseTextDocumentParams) error {
- uri := span.NewURI(params.TextDocument.URI)
- ctx = telemetry.URI.With(ctx, uri)
- s.session.DidClose(uri)
- view, err := s.session.ViewOf(uri)
- if err != nil {
- return err
- }
- view.SetContent(ctx, uri, -1, nil)
- return nil
+ return content, nil
}
