internal/lsp/source: compute imports text edits from scratch
Fix the imports text edits by computing it on first principles. This
fixes at least a couple bugs:
- Incorrect handling of positions between \r and \n with windows line
endings.
- Incorrect computation of edits when the imports source prefix is
synthetically terminated with just \n, but the actual source has
\r\n.
This is an unsatisfying solution, necessary because of the interaction
of token.File with file termination (token.File does not capture this
information).
Efforts to fix token.File proved complicated, and this is causing
problems for our users, so I think we should do this for now.
For golang/vscode-go#1489
Change-Id: I235caf3960c7201af93800d65546fbab5c6e3f4b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/319129
Trust: Rebecca Stambler <rstambler@golang.org>
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/gopls/internal/hooks/diff.go b/gopls/internal/hooks/diff.go
index 46d7dd7..a307ba7 100644
--- a/gopls/internal/hooks/diff.go
+++ b/gopls/internal/hooks/diff.go
@@ -14,7 +14,7 @@
func ComputeEdits(uri span.URI, before, after string) (edits []diff.TextEdit, err error) {
// The go-diff library has an unresolved panic (see golang/go#278774).
- // TOOD(rstambler): Remove the recover once the issue has been fixed
+ // TODO(rstambler): Remove the recover once the issue has been fixed
// upstream.
defer func() {
if r := recover(); r != nil {
diff --git a/gopls/internal/regtest/completion/completion_test.go b/gopls/internal/regtest/completion/completion_test.go
index cd70cca..795f7ae 100644
--- a/gopls/internal/regtest/completion/completion_test.go
+++ b/gopls/internal/regtest/completion/completion_test.go
@@ -504,7 +504,6 @@
}
func TestUnimportedCompletion_VSCodeIssue1489(t *testing.T) {
- t.Skip("broken due to golang/vscode-go#1489")
testenv.NeedsGo1Point(t, 14)
const src = `
@@ -524,8 +523,7 @@
}
`
WithOptions(
- WindowsLineEndings,
- ProxyFiles(proxy),
+ EditorConfig{WindowsLineEndings: true},
).Run(t, src, func(t *testing.T, env *Env) {
// Trigger unimported completions for the example.com/blah package.
env.OpenFile("main.go")
@@ -537,6 +535,10 @@
}
env.AcceptCompletion("main.go", pos, completions.Items[0])
env.Await(env.DoneWithChange())
- t.Log(env.Editor.BufferText("main.go"))
+ got := env.Editor.BufferText("main.go")
+ want := "package main\r\n\r\nimport (\r\n\t\"fmt\"\r\n\t\"math\"\r\n)\r\n\r\nfunc main() {\r\n\tfmt.Println(\"a\")\r\n\tmath.Sqrt(${1:})\r\n}\r\n"
+ if got != want {
+ t.Errorf("unimported completion: got %q, want %q", got, want)
+ }
})
}
diff --git a/internal/lsp/lsppos/lsppos.go b/internal/lsp/lsppos/lsppos.go
new file mode 100644
index 0000000..f27bde5
--- /dev/null
+++ b/internal/lsp/lsppos/lsppos.go
@@ -0,0 +1,89 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package lsppos provides utilities for working with LSP positions.
+//
+// See https://microsoft.github.io/language-server-protocol/specification#textDocuments
+// for a description of LSP positions. Notably:
+// - Positions are specified by a 0-based line count and 0-based utf-16
+// character offset.
+// - Positions are line-ending agnostic: there is no way to specify \r|\n or
+// \n|. Instead the former maps to the end of the current line, and the
+// latter to the start of the next line.
+package lsppos
+
+import (
+ "sort"
+ "unicode/utf8"
+)
+
+type Mapper struct {
+ nonASCII bool
+ src []byte
+
+ // Start-of-line positions. If src is newline-terminated, the final entry will be empty.
+ lines []int
+}
+
+func NewMapper(src []byte) *Mapper {
+ m := &Mapper{src: src}
+ if len(src) == 0 {
+ return m
+ }
+ m.lines = []int{0}
+ for offset, b := range src {
+ if b == '\n' {
+ m.lines = append(m.lines, offset+1)
+ }
+ if b >= utf8.RuneSelf {
+ m.nonASCII = true
+ }
+ }
+ return m
+}
+
+func (m *Mapper) Position(offset int) (line, char int) {
+ if offset < 0 || offset > len(m.src) {
+ return -1, -1
+ }
+ nextLine := sort.Search(len(m.lines), func(i int) bool {
+ return offset < m.lines[i]
+ })
+ if nextLine == 0 {
+ return -1, -1
+ }
+ line = nextLine - 1
+ start := m.lines[line]
+ var charOffset int
+ if m.nonASCII {
+ charOffset = UTF16len(m.src[start:offset])
+ } else {
+ charOffset = offset - start
+ }
+
+ var eol int
+ if line == len(m.lines)-1 {
+ eol = len(m.src)
+ } else {
+ eol = m.lines[line+1] - 1
+ }
+
+ // Adjustment for line-endings: \r|\n is the same as |\r\n.
+ if offset == eol && offset > 0 && m.src[offset-1] == '\r' {
+ charOffset--
+ }
+
+ return line, charOffset
+}
+
+func UTF16len(buf []byte) int {
+ cnt := 0
+ for _, r := range string(buf) {
+ cnt++
+ if r >= 1<<16 {
+ cnt++
+ }
+ }
+ return cnt
+}
diff --git a/internal/lsp/regtest/runner.go b/internal/lsp/regtest/runner.go
index 6b3501c..05867c4 100644
--- a/internal/lsp/regtest/runner.go
+++ b/internal/lsp/regtest/runner.go
@@ -178,10 +178,6 @@
})
}
-var WindowsLineEndings = optionSetter(func(opts *runConfig) {
- opts.editor.WindowsLineEndings = true
-})
-
// SkipLogs skips the buffering of logs during test execution. It is intended
// for long-running stress tests.
func SkipLogs() RunOption {
diff --git a/internal/lsp/source/completion/format.go b/internal/lsp/source/completion/format.go
index 5a20633..166ba55 100644
--- a/internal/lsp/source/completion/format.go
+++ b/internal/lsp/source/completion/format.go
@@ -143,6 +143,7 @@
// add the additional text edits needed.
if cand.imp != nil {
addlEdits, err := c.importEdits(cand.imp)
+
if err != nil {
return CompletionItem{}, err
}
diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go
index ca76d20..0d61172 100644
--- a/internal/lsp/source/format.go
+++ b/internal/lsp/source/format.go
@@ -19,7 +19,9 @@
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/imports"
"golang.org/x/tools/internal/lsp/diff"
+ "golang.org/x/tools/internal/lsp/lsppos"
"golang.org/x/tools/internal/lsp/protocol"
+ "golang.org/x/tools/internal/span"
)
// Format formats a file with a given range.
@@ -177,7 +179,7 @@
if err != nil {
return nil, err
}
- return ToProtocolEdits(pgf.Mapper, edits)
+ return ProtocolEditsFromSource([]byte(left), edits, pgf.Mapper.Converter)
}
// importPrefix returns the prefix of the given file content through the final
@@ -280,6 +282,37 @@
return ToProtocolEdits(pgf.Mapper, edits)
}
+// ProtocolEditsFromSource converts text edits to LSP edits using the original
+// source.
+func ProtocolEditsFromSource(src []byte, edits []diff.TextEdit, converter span.Converter) ([]protocol.TextEdit, error) {
+ m := lsppos.NewMapper(src)
+ var result []protocol.TextEdit
+ for _, edit := range edits {
+ spn, err := edit.Span.WithOffset(converter)
+ if err != nil {
+ return nil, fmt.Errorf("computing offsets: %v", err)
+ }
+ startLine, startChar := m.Position(spn.Start().Offset())
+ endLine, endChar := m.Position(spn.End().Offset())
+ if startLine < 0 || endLine < 0 {
+ return nil, fmt.Errorf("out of bound span: %v", spn)
+ }
+
+ pstart := protocol.Position{Line: uint32(startLine), Character: uint32(startChar)}
+ pend := protocol.Position{Line: uint32(endLine), Character: uint32(endChar)}
+ if pstart == pend && edit.NewText == "" {
+ // Degenerate case, which may result from a diff tool wanting to delete
+ // '\r' in line endings. Filter it out.
+ continue
+ }
+ result = append(result, protocol.TextEdit{
+ Range: protocol.Range{Start: pstart, End: pend},
+ NewText: edit.NewText,
+ })
+ }
+ return result, nil
+}
+
func ToProtocolEdits(m *protocol.ColumnMapper, edits []diff.TextEdit) ([]protocol.TextEdit, error) {
if edits == nil {
return nil, nil
