gopls/internal/golang/completion: fix package clause completion suffix

CL 585275 introduced a call to completion.Surrounding.Suffix that
exposed a latent bug in package clause completions: the completion
content was not being correctly computed as containing the cursor.

Fix the heuristics for completing at "package<whitespace>|", so that the
surrounding content is correct.

As a result of this change the 'editRegexp' from some completion tests
now no longer includes the newline, which also seems more correct.

Fixes golang/go#68169

Change-Id: I32ea20a7f9f0096aef85ed77c64d3b4dfeedef45
Reviewed-on: https://go-review.googlesource.com/c/tools/+/594796
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Auto-Submit: Robert Findley <rfindley@google.com>

gopls/internal/golang/completion/package.go

@@ -106,39 +106,20 @@ func packageCompletionSurrounding(pgf *parsego.File, offset int) (*Selection, er
 
 	// 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(pgf.Src), "\n")
-	cursorLine := safetoken.Line(tok, cursor)
-	if cursorLine <= 0 || cursorLine > len(lines) {
-		return nil, fmt.Errorf("invalid line number")
+	// *ast.BadDecl since it is a keyword.
+	start, err := safetoken.Offset(tok, expr.Pos())
+	if err != nil {
+		return nil, err
 	}
-	if safetoken.StartPosition(fset, 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,
-					tokFile: tok,
-					start:   start,
-					end:     end,
-					mapper:  m,
-				}, nil
-			}
-		}
+	if offset > start && string(bytes.TrimRight(pgf.Src[start:offset], " ")) == PACKAGE {
+		return &Selection{
+			content: string(pgf.Src[start:offset]),
+			cursor:  cursor,
+			tokFile: tok,
+			start:   expr.Pos(),
+			end:     cursor,
+			mapper:  m,
+		}, nil
 	}
 
 	// If the cursor is after the start of the expression, no package

gopls/internal/test/integration/completion/completion_test.go

@@ -121,11 +121,11 @@ package
 			want:          nil,
 		},
 		{
-			name:          "package completion after keyword 'package'",
+			name:          "package completion after package keyword",
 			filename:      "fruits/testfile2.go",
 			triggerRegexp: "package()",
 			want:          []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"},
-			editRegexp:    "package\n",
+			editRegexp:    "package",
 		},
 		{
 			name:          "package completion with 'pac' prefix",
@@ -164,14 +164,14 @@ package
 			filename:      "123f_r.u~its-123/testfile.go",
 			triggerRegexp: "package()",
 			want:          []string{"package fruits123", "package fruits123_test", "package main"},
-			editRegexp:    "package\n",
+			editRegexp:    "package",
 		},
 		{
 			name:          "package completion for invalid dir name",
 			filename:      ".invalid-dir@-name/testfile.go",
 			triggerRegexp: "package()",
 			want:          []string{"package main"},
-			editRegexp:    "package\n",
+			editRegexp:    "package",
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {

gopls/internal/test/integration/completion/fixedbugs_test.go

@@ -0,0 +1,40 @@
+// Copyright 2024 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 completion
+
+import (
+	"testing"
+
+	. "golang.org/x/tools/gopls/internal/test/integration"
+)
+
+func TestPackageCompletionCrash_Issue68169(t *testing.T) {
+	// This test reproduces the scenario of golang/go#68169, a crash in
+	// completion.Selection.Suffix.
+	//
+	// The file content here is extracted from the issue.
+	const files = `
+-- go.mod --
+module example.com
+
+go 1.18
+-- playdos/play.go --
+package  
+`
+
+	Run(t, files, func(t *testing.T, env *Env) {
+		env.OpenFile("playdos/play.go")
+		// Previously, this call would crash gopls as it was incorrectly computing
+		// the surrounding completion suffix.
+		completions := env.Completion(env.RegexpSearch("playdos/play.go", "package  ()"))
+		if len(completions.Items) == 0 {
+			t.Fatal("Completion() returned empty results")
+		}
+		// Sanity check: we should get package clause compeltion.
+		if got, want := completions.Items[0].Label, "package playdos"; got != want {
+			t.Errorf("Completion()[0].Label == %s, want %s", got, want)
+		}
+	})
+}