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>

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)
 				}

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
 		}
 	}
 

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

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))