internal/lsp: add more tests for package completion
This change adds one more test for package completions (and slightly
modifies the way we pass in file content without a terminal newline).
Also, a few tiny modifications to the package completion code.
Change-Id: I3041b5ad648873881b2b8df17df6f78fbd1898e5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/253177
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Danish Dua <danishdua@google.com>
diff --git a/gopls/internal/regtest/completion_test.go b/gopls/internal/regtest/completion_test.go
index e987e10..957a829 100644
--- a/gopls/internal/regtest/completion_test.go
+++ b/gopls/internal/regtest/completion_test.go
@@ -6,8 +6,10 @@
import (
"fmt"
+ "strings"
"testing"
+ "golang.org/x/tools/internal/lsp"
"golang.org/x/tools/internal/lsp/fake"
"golang.org/x/tools/internal/lsp/protocol"
)
@@ -36,56 +38,91 @@
-- fruits/testfile3.go --
pac
-
--- fruits/testfile4.go --`
- for _, testcase := range []struct {
+`
+ var (
+ testfile4 = ""
+ testfile5 = "/*a comment*/ "
+ )
+ for _, tc := range []struct {
name string
filename string
+ content *string
line, col int
want []string
}{
{
- "package completion at valid position",
- "fruits/testfile.go", 1, 0,
- []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"},
+ name: "package completion at valid position",
+ filename: "fruits/testfile.go",
+ line: 1, col: 0,
+ want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"},
},
{
- "package completion in a comment",
- "fruits/testfile.go", 0, 5,
- nil,
+ name: "package completion in a comment",
+ filename: "fruits/testfile.go",
+ line: 0, col: 5,
+ want: nil,
},
{
- "package completion at invalid position",
- "fruits/testfile.go", 4, 0,
- nil,
+ name: "package completion at invalid position",
+ filename: "fruits/testfile.go",
+ line: 4, col: 0,
+ want: nil,
},
{
- "package completion works after keyword 'package'",
- "fruits/testfile2.go", 0, 7,
- []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"},
+ name: "package completion after keyword 'package'",
+ filename: "fruits/testfile2.go",
+ line: 0, col: 7,
+ want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"},
},
{
- "package completion works with a prefix for keyword 'package'",
- "fruits/testfile3.go", 0, 3,
- []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"},
+ name: "package completion with 'pac' prefix",
+ filename: "fruits/testfile3.go",
+ line: 0, col: 3,
+ want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"},
},
{
- "package completion at end of file",
- "fruits/testfile4.go", 0, 0,
- []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"},
+ name: "package completion at end of file",
+ filename: "fruits/testfile4.go",
+ line: 0, col: 0,
+ content: &testfile4,
+ want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"},
+ },
+ {
+ name: "package completion without terminal newline",
+ filename: "fruits/testfile5.go",
+ line: 0, col: 14,
+ content: &testfile5,
+ want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"},
},
} {
- t.Run(testcase.name, func(t *testing.T) {
+ t.Run(tc.name, func(t *testing.T) {
run(t, files, func(t *testing.T, env *Env) {
- env.OpenFile(testcase.filename)
- completions, err := env.Editor.Completion(env.Ctx, testcase.filename, fake.Pos{
- Line: testcase.line,
- Column: testcase.col,
+ if tc.content != nil {
+ env.WriteWorkspaceFile(tc.filename, *tc.content)
+ env.Await(
+ CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), 1),
+ )
+ }
+ env.OpenFile(tc.filename)
+ completions, err := env.Editor.Completion(env.Ctx, tc.filename, fake.Pos{
+ Line: tc.line,
+ Column: tc.col,
})
if err != nil {
t.Fatal(err)
}
- diff := compareCompletionResults(testcase.want, completions.Items)
+ // Check that the completion item suggestions are in the range
+ // of the file.
+ lineCount := len(strings.Split(env.Editor.BufferText(tc.filename), "\n"))
+ for _, item := range completions.Items {
+ if start := int(item.TextEdit.Range.Start.Line); start >= lineCount {
+ t.Fatalf("unexpected text edit range start line number: got %d, want less than %d", start, lineCount)
+ }
+ if end := int(item.TextEdit.Range.End.Line); end >= lineCount {
+ t.Fatalf("unexpected text edit range end line number: got %d, want less than %d", end, lineCount)
+ }
+ }
+ diff := compareCompletionResults(tc.want, completions.Items)
if diff != "" {
t.Error(diff)
}
diff --git a/internal/lsp/completion.go b/internal/lsp/completion.go
index f02f915..9f12051 100644
--- a/internal/lsp/completion.go
+++ b/internal/lsp/completion.go
@@ -5,6 +5,7 @@
package lsp
import (
+ "bytes"
"context"
"fmt"
"strings"
@@ -52,42 +53,41 @@
if err != nil {
return nil, err
}
-
- // Get the actual number of lines in source.
- numLines := len(strings.Split(string(src), "\n"))
-
+ numLines := len(bytes.Split(src, []byte("\n")))
tok := snapshot.FileSet().File(surrounding.Start())
- endOfFile := tok.Pos(tok.Size())
+ eof := tok.Pos(tok.Size())
// For newline-terminated files, the line count reported by go/token should
// be lower than the actual number of lines we see when splitting by \n. If
// they're the same, the file isn't newline-terminated.
- if numLines == tok.LineCount() && tok.Size() != 0 {
- // Get span for character before end of file to bypass span's treatment of end
- // of file. We correct for this later.
- spn, err := span.NewRange(snapshot.FileSet(), endOfFile-1, endOfFile-1).Span()
+ if tok.Size() > 0 && tok.LineCount() == numLines {
+ // Get the span for the last character in the file-1. This is
+ // technically incorrect, but will get span to point to the previous
+ // line.
+ spn, err := span.NewRange(snapshot.FileSet(), eof-1, eof-1).Span()
if err != nil {
return nil, err
}
m := &protocol.ColumnMapper{
URI: fh.URI(),
- Converter: span.NewContentConverter(fh.URI().Filename(), []byte(src)),
- Content: []byte(src),
+ Converter: span.NewContentConverter(fh.URI().Filename(), src),
+ Content: src,
}
eofRng, err := m.Range(spn)
if err != nil {
return nil, err
}
- eofPosition := protocol.Position{
- Line: eofRng.Start.Line,
- // Correct for using endOfFile - 1 earlier.
+ // Instead of using the computed range, correct for our earlier
+ // position adjustment by adding 1 to the column, not the line number.
+ pos := protocol.Position{
+ Line: eofRng.Start.Line,
Character: eofRng.Start.Character + 1,
}
- if surrounding.Start() == endOfFile {
- rng.Start = eofPosition
+ if surrounding.Start() >= eof {
+ rng.Start = pos
}
- if surrounding.End() == endOfFile {
- rng.End = eofPosition
+ if surrounding.End() >= eof {
+ rng.End = pos
}
}
diff --git a/internal/lsp/source/completion/completion.go b/internal/lsp/source/completion/completion.go
index edaf6f0..9ae9e66 100644
--- a/internal/lsp/source/completion/completion.go
+++ b/internal/lsp/source/completion/completion.go
@@ -500,8 +500,8 @@
// present but no package name exists.
items, surrounding, innerErr := packageClauseCompletions(ctx, snapshot, fh, protoPos)
if innerErr != nil {
- // return the error for GetParsedFile since it's more relevant in this situation.
- return nil, nil, errors.Errorf("getting file for Completion: %w", err)
+ // return the error for getParsedFile since it's more relevant in this situation.
+ return nil, nil, errors.Errorf("getting file for Completion: %w (package completions: %v)", err, innerErr)
}
return items, surrounding, nil
diff --git a/internal/lsp/source/completion/completion_package.go b/internal/lsp/source/completion/completion_package.go
index a6ac7af..d18e2f8 100644
--- a/internal/lsp/source/completion/completion_package.go
+++ b/internal/lsp/source/completion/completion_package.go
@@ -73,7 +73,6 @@
if err != nil {
return nil, err
}
-
// If the file lacks a package declaration, the parser will return an empty
// AST. As a work-around, try to parse an expression from the file contents.
expr, _ := parser.ParseExprFrom(fset, fh.URI().Filename(), src, parser.Mode(0))
