From 201d438bced1e430169c2aca60ef39df08c26a7f Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Fri, 4 Sep 2020 13:36:45 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Danish Dua --- gopls/internal/regtest/completion_test.go | 91 +++++++++++++------ internal/lsp/completion.go | 36 ++++---- internal/lsp/source/completion/completion.go | 4 +- .../source/completion/completion_package.go | 1 - 4 files changed, 84 insertions(+), 48 deletions(-) diff --git a/gopls/internal/regtest/completion_test.go b/gopls/internal/regtest/completion_test.go index e987e10988..957a829997 100644 --- a/gopls/internal/regtest/completion_test.go +++ b/gopls/internal/regtest/completion_test.go @@ -6,8 +6,10 @@ package regtest 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 @@ package -- 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 f02f91559b..9f12051a09 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 @@ func (s *Server) completion(ctx context.Context, params *protocol.CompletionPara 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 edaf6f0ca9..9ae9e666d1 100644 --- a/internal/lsp/source/completion/completion.go +++ b/internal/lsp/source/completion/completion.go @@ -500,8 +500,8 @@ func Completion(ctx context.Context, snapshot source.Snapshot, fh source.FileHan // 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 a6ac7aff3c..d18e2f83ea 100644 --- a/internal/lsp/source/completion/completion_package.go +++ b/internal/lsp/source/completion/completion_package.go @@ -73,7 +73,6 @@ func packageCompletionSurrounding(ctx context.Context, fset *token.FileSet, fh s 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))