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>
2024-11-18 19:54:44 -07:00 · 2019-12-04 18:45:25 -05:00 · 2019-12-04 18:45:25 -05:00 · d1f10d1c4e
commit d1f10d1c4e
parent a588733072
2 changed files with 118 additions and 90 deletions
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@ -193,12 +193,27 @@ type Session interface {
 	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
--- a/internal/lsp/text_synchronization.go
+++ b/internal/lsp/text_synchronization.go
@ -18,71 +18,93 @@ import (
 )

 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 @@ func (s *Server) wasFirstChange(uri span.URI) bool {
 	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 @@ func (s *Server) applyChanges(ctx context.Context, uri span.URI, changes []proto
 		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
 }