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>
diff --git a/gopls/internal/golang/completion/package.go b/gopls/internal/golang/completion/package.go
index ba9ddc2..e71f5c9 100644
--- a/gopls/internal/golang/completion/package.go
+++ b/gopls/internal/golang/completion/package.go
@@ -106,39 +106,20 @@
// 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
diff --git a/gopls/internal/test/integration/completion/completion_test.go b/gopls/internal/test/integration/completion/completion_test.go
index 0ca46da..c96e569 100644
--- a/gopls/internal/test/integration/completion/completion_test.go
+++ b/gopls/internal/test/integration/completion/completion_test.go
@@ -121,11 +121,11 @@
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 @@
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) {
diff --git a/gopls/internal/test/integration/completion/fixedbugs_test.go b/gopls/internal/test/integration/completion/fixedbugs_test.go
new file mode 100644
index 0000000..5269584
--- /dev/null
+++ b/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)
+ }
+ })
+}
