From 251092de1b93f5800df813ff1af2d470f818034b Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 12 May 2021 00:21:14 -0400 Subject: [PATCH] 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 Trust: Robert Findley Run-TryBot: Rebecca Stambler Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- gopls/internal/hooks/diff.go | 2 +- .../regtest/completion/completion_test.go | 10 ++- internal/lsp/lsppos/lsppos.go | 89 +++++++++++++++++++ internal/lsp/regtest/runner.go | 4 - internal/lsp/source/completion/format.go | 1 + internal/lsp/source/format.go | 35 +++++++- 6 files changed, 131 insertions(+), 10 deletions(-) create mode 100644 internal/lsp/lsppos/lsppos.go diff --git a/gopls/internal/hooks/diff.go b/gopls/internal/hooks/diff.go index 46d7dd74bd..a307ba77fd 100644 --- a/gopls/internal/hooks/diff.go +++ b/gopls/internal/hooks/diff.go @@ -14,7 +14,7 @@ import ( 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 cd70ccabb9..795f7ae1fb 100644 --- a/gopls/internal/regtest/completion/completion_test.go +++ b/gopls/internal/regtest/completion/completion_test.go @@ -504,7 +504,6 @@ func doit() { } 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 @@ func main() { } ` 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 @@ func main() { } 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 0000000000..f27bde5737 --- /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 6b3501c1e1..05867c4f97 100644 --- a/internal/lsp/regtest/runner.go +++ b/internal/lsp/regtest/runner.go @@ -178,10 +178,6 @@ func DebugAddress(addr string) RunOption { }) } -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 5a20633f7c..166ba553bb 100644 --- a/internal/lsp/source/completion/format.go +++ b/internal/lsp/source/completion/format.go @@ -143,6 +143,7 @@ Suffixes: // 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 ca76d2080e..0d61172a24 100644 --- a/internal/lsp/source/format.go +++ b/internal/lsp/source/format.go @@ -19,7 +19,9 @@ import ( "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 @@ func computeFixEdits(snapshot Snapshot, pgf *ParsedGoFile, options *imports.Opti 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 @@ func computeTextEdits(ctx context.Context, snapshot Snapshot, pgf *ParsedGoFile, 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