internal/lsp: support for package completion in all files

This change adds support for package completion in all files at valid
positions. By parsing expressions from an invalid file, we can check if
the cursor is at a position where package completion would be a valid
suggestion. These are positions above any other statements or
declaration or at prefix of the keyword pacakge above these
statements/declarations. This also introduces imporved end of file
handling in completion.

Change-Id: I2a865d018f58c3a98b69fb4100d186b507d123bd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/251618
Run-TryBot: Danish Dua <danishdua@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

gopls/internal/regtest/completion_test.go

@@ -24,29 +24,74 @@ fun apple() int {
 	return 0
 }
 
--- fruits/testfile.go --`
+-- fruits/testfile.go --
+// this is a comment
 
-	want := []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}
-	run(t, files, func(t *testing.T, env *Env) {
-		env.OpenFile("fruits/testfile.go")
-		content := env.ReadWorkspaceFile("fruits/testfile.go")
-		if content != "" {
-			t.Fatal("testfile.go should be empty to test completion on end of file without newline")
-		}
+import "fmt"
 
-		completions, err := env.Editor.Completion(env.Ctx, "fruits/testfile.go", fake.Pos{
-			Line:   0,
-			Column: 0,
+func test() {}
+
+-- fruits/testfile2.go --
+package
+
+-- fruits/testfile3.go --
+pac
+
+-- fruits/testfile4.go --`
+	for _, testcase := range []struct {
+		name      string
+		filename  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"},
+		},
+		{
+			"package completion in a comment",
+			"fruits/testfile.go", 0, 5,
+			nil,
+		},
+		{
+			"package completion at invalid position",
+			"fruits/testfile.go", 4, 0,
+			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"},
+		},
+		{
+			"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"},
+		},
+		{
+			"package completion at end of file",
+			"fruits/testfile4.go", 0, 0,
+			[]string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"},
+		},
+	} {
+		t.Run(testcase.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 err != nil {
+					t.Fatal(err)
+				}
+				diff := compareCompletionResults(testcase.want, completions.Items)
+				if diff != "" {
+					t.Error(diff)
+				}
+			})
 		})
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		diff := compareCompletionResults(want, completions.Items)
-		if diff != "" {
-			t.Fatal(diff)
-		}
-	})
+	}
 }
 
 func TestPackageNameCompletion(t *testing.T) {

internal/lsp/completion.go

@@ -13,6 +13,7 @@ import (
 	"golang.org/x/tools/internal/lsp/debug/tag"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/source"
+	"golang.org/x/tools/internal/span"
 )
 
 func (s *Server) completion(ctx context.Context, params *protocol.CompletionParams) (*protocol.CompletionList, error) {
@@ -42,15 +43,50 @@ func (s *Server) completion(ctx context.Context, params *protocol.CompletionPara
 	if err != nil {
 		return nil, err
 	}
-	// Span treats an end of file as the beginning of the next line, which for
-	// a final line ending without a newline is incorrect and leads to
-	// completions being ignored. We adjust the ending in case ange end is on a
-	// different line here.
-	// This should be removed after the resolution of golang/go#41029
-	if rng.Start.Line != rng.End.Line {
-		rng.End = protocol.Position{
-			Character: rng.Start.Character + float64(len(surrounding.Content())),
-			Line:      rng.Start.Line,
+
+	// internal/span treats end of file as the beginning of the next line, even
+	// when it's not newline-terminated. We correct for that behaviour here if
+	// end of file is not newline-terminated. See golang/go#41029.
+	src, err := fh.Read()
+	if err != nil {
+		return nil, err
+	}
+
+	// Get the actual number of lines in source.
+	numLines := len(strings.Split(string(src), "\n"))
+
+	tok := snapshot.FileSet().File(surrounding.Start())
+	endOfFile := 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 err != nil {
+			return nil, err
+		}
+		m := &protocol.ColumnMapper{
+			URI:       fh.URI(),
+			Converter: span.NewContentConverter(fh.URI().Filename(), []byte(src)),
+			Content:   []byte(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.
+			Character: eofRng.Start.Character + 1,
+		}
+		if surrounding.Start() == endOfFile {
+			rng.Start = eofPosition
+		}
+		if surrounding.End() == endOfFile {
+			rng.End = eofPosition
 		}
 	}
 

internal/lsp/source/completion.go

@@ -247,6 +247,14 @@ func (p Selection) Content() string {
 	return p.content
 }
 
+func (p Selection) Start() token.Pos {
+	return p.mappedRange.spanRange.Start
+}
+
+func (p Selection) End() token.Pos {
+	return p.mappedRange.spanRange.End
+}
+
 func (p Selection) Prefix() string {
 	return p.content[:p.cursor-p.spanRange.Start]
 }

internal/lsp/source/completion_package.go

@@ -8,6 +8,9 @@ import (
 	"context"
 	"fmt"
 	"go/ast"
+	"go/parser"
+	"go/scanner"
+	"go/token"
 	"go/types"
 	"path/filepath"
 	"strings"
@@ -23,18 +26,11 @@ import (
 func packageClauseCompletions(ctx context.Context, snapshot Snapshot, fh FileHandle, pos protocol.Position) ([]CompletionItem, *Selection, error) {
 	// We know that the AST for this file will be empty due to the missing
 	// package declaration, but parse it anyway to get a mapper.
-	pgf, err := snapshot.ParseGo(ctx, fh, ParseHeader)
+	pgf, err := snapshot.ParseGo(ctx, fh, ParseFull)
 	if err != nil {
 		return nil, nil, err
 	}
 
-	// Check that the file is completely empty, to avoid offering incorrect package
-	// clause completions.
-	// TODO: Support package clause completions in all files.
-	if pgf.Tok.Size() != 0 {
-		return nil, nil, errors.New("package clause completion is only offered for empty file")
-	}
-
 	cursorSpan, err := pgf.Mapper.PointSpan(pos)
 	if err != nil {
 		return nil, nil, err
@@ -44,10 +40,9 @@ func packageClauseCompletions(ctx context.Context, snapshot Snapshot, fh FileHan
 		return nil, nil, err
 	}
 
-	surrounding := &Selection{
-		content:     "",
-		cursor:      rng.Start,
-		mappedRange: newMappedRange(snapshot.FileSet(), pgf.Mapper, rng.Start, rng.Start),
+	surrounding, err := packageCompletionSurrounding(ctx, snapshot.FileSet(), fh, pgf, rng.Start)
+	if err != nil {
+		return nil, nil, fmt.Errorf("invalid position for package completion: %w", err)
 	}
 
 	packageSuggestions, err := packageSuggestions(ctx, snapshot, fh.URI(), "")
@@ -69,6 +64,123 @@ func packageClauseCompletions(ctx context.Context, snapshot Snapshot, fh FileHan
 	return items, surrounding, nil
 }
 
+// packageCompletionSurrounding returns surrounding for package completion if a
+// package completions can be suggested at a given position. A valid location
+// for package completion is above any declarations or import statements.
+func packageCompletionSurrounding(ctx context.Context, fset *token.FileSet, fh FileHandle, pgf *ParsedGoFile, pos token.Pos) (*Selection, error) {
+	src, err := fh.Read()
+	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))
+	if expr == nil {
+		return nil, fmt.Errorf("unparseable file (%s)", fh.URI())
+	}
+	tok := fset.File(expr.Pos())
+	cursor := tok.Pos(pgf.Tok.Offset(pos))
+	m := &protocol.ColumnMapper{
+		URI:       pgf.URI,
+		Content:   src,
+		Converter: span.NewContentConverter(fh.URI().Filename(), src),
+	}
+
+	// If we were able to parse out an identifier as the first expression from
+	// the file, it may be the beginning of a package declaration ("pack ").
+	// We can offer package completions if the cursor is in the identifier.
+	if name, ok := expr.(*ast.Ident); ok {
+		if cursor >= name.Pos() && cursor <= name.End() {
+			if !strings.HasPrefix(PACKAGE, name.Name) {
+				return nil, fmt.Errorf("cursor in non-matching ident")
+			}
+			return &Selection{
+				content:     name.Name,
+				cursor:      cursor,
+				mappedRange: newMappedRange(fset, m, name.Pos(), name.End()),
+			}, nil
+		}
+	}
+
+	// The file is invalid, but it contains an expression that we were able to
+	// parse. We will use this expression to construct the cursor's
+	// "surrounding".
+
+	// First, consider the possibility that we have a valid "package" keyword
+	// with an empty package name ("package "). "package" is parsed as an
+	// *ast.BadDecl since it is a keyword. This logic would allow "package" to
+	// appear on any line of the file as long as it's the first code expression
+	// in the file.
+	lines := strings.Split(string(src), "\n")
+	cursorLine := tok.Line(cursor)
+	if cursorLine <= 0 || cursorLine > len(lines) {
+		return nil, fmt.Errorf("invalid line number")
+	}
+	if fset.Position(expr.Pos()).Line == cursorLine {
+		words := strings.Fields(lines[cursorLine-1])
+		if len(words) > 0 && words[0] == PACKAGE {
+			content := PACKAGE
+			// Account for spaces if there are any.
+			if len(words) > 1 {
+				content += " "
+			}
+
+			start := expr.Pos()
+			end := token.Pos(int(expr.Pos()) + len(content) + 1)
+			// We have verified that we have a valid 'package' keyword as our
+			// first expression. Ensure that cursor is in this keyword or
+			// otherwise fallback to the general case.
+			if cursor >= start && cursor <= end {
+				return &Selection{
+					content:     content,
+					cursor:      cursor,
+					mappedRange: newMappedRange(fset, m, start, end),
+				}, nil
+			}
+		}
+	}
+
+	// If the cursor is after the start of the expression, no package
+	// declaration will be valid.
+	if cursor > expr.Pos() {
+		return nil, fmt.Errorf("cursor after expression")
+	}
+
+	// If the cursor is in a comment, don't offer any completions.
+	if cursorInComment(fset, cursor, src) {
+		return nil, fmt.Errorf("cursor in comment")
+	}
+
+	// The surrounding range in this case is the cursor except for empty file,
+	// in which case it's end of file - 1
+	start, end := cursor, cursor
+	if tok.Size() == 0 {
+		start, end = tok.Pos(0)-1, tok.Pos(0)-1
+	}
+
+	return &Selection{
+		content:     "",
+		cursor:      cursor,
+		mappedRange: newMappedRange(fset, m, start, end),
+	}, nil
+}
+
+func cursorInComment(fset *token.FileSet, cursor token.Pos, src []byte) bool {
+	var s scanner.Scanner
+	s.Init(fset.File(cursor), src, func(_ token.Position, _ string) {}, scanner.ScanComments)
+	for {
+		pos, tok, lit := s.Scan()
+		if pos <= cursor && cursor <= token.Pos(int(pos)+len(lit)) {
+			return tok == token.COMMENT
+		}
+		if tok == token.EOF {
+			break
+		}
+	}
+	return false
+}
+
 // packageNameCompletions returns name completions for a package clause using
 // the current name as prefix.
 func (c *completer) packageNameCompletions(ctx context.Context, fileURI span.URI, name *ast.Ident) error {