internal/lsp/source/completion: improve import suggestion labels
This change improves the labels for import suggestions to only show the
last part of the path. Since VSCode fuzzy searches for labels in text
edit, we now return only the last part of path as text edit instead of
replacing the full import path. Just changing label while returning full
path leads to bad user experience.
Closes golang/go#35877
Change-Id: Ib10e7a3e030dc9b850ff1d9ec8d45240b75b64a3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/255837
Run-TryBot: Danish Dua <danishdua@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Trust: Danish Dua <danishdua@google.com>
diff --git a/internal/lsp/source/completion/completion.go b/internal/lsp/source/completion/completion.go
index 3bfa588..1cbb0a6 100644
--- a/internal/lsp/source/completion/completion.go
+++ b/internal/lsp/source/completion/completion.go
@@ -8,6 +8,7 @@
import (
"context"
+ "fmt"
"go/ast"
"go/constant"
"go/scanner"
@@ -683,33 +684,66 @@
// (i.e. "golang.org/x/"). The user is meant to accept completion suggestions
// until they reach a complete import path.
func (c *completer) populateImportCompletions(ctx context.Context, searchImport *ast.ImportSpec) error {
- c.surrounding = &Selection{
- content: searchImport.Path.Value,
- cursor: c.pos,
- MappedRange: source.NewMappedRange(c.snapshot.FileSet(), c.mapper, searchImport.Path.Pos(), searchImport.Path.End()),
- }
+ importPath := searchImport.Path.Value
- seenImports := make(map[string]struct{})
- for _, importSpec := range c.file.Imports {
- if importSpec.Path.Value == searchImport.Path.Value {
- continue
- }
- importPath, err := strconv.Unquote(importSpec.Path.Value)
- if err != nil {
- return err
- }
- seenImports[importPath] = struct{}{}
- }
-
- prefixEnd := c.pos - searchImport.Path.ValuePos
// Extract the text between the quotes (if any) in an import spec.
// prefix is the part of import path before the cursor.
- prefix := strings.Trim(searchImport.Path.Value[:prefixEnd], `"`)
+ prefixEnd := c.pos - searchImport.Path.Pos()
+ prefix := strings.Trim(importPath[:prefixEnd], `"`)
// The number of directories in the import path gives us the depth at
// which to search.
depth := len(strings.Split(prefix, "/")) - 1
+ content := importPath
+ start, end := searchImport.Path.Pos(), searchImport.Path.End()
+ namePrefix, nameSuffix := `"`, `"`
+ // If a starting quote is present, adjust surrounding to either after the
+ // cursor or after the first slash (/), except if cursor is at the starting
+ // quote. Otherwise we provide a completion including the starting quote.
+ if strings.HasPrefix(importPath, `"`) && c.pos > searchImport.Path.Pos() {
+ content = content[1:]
+ start++
+ if depth > 0 {
+ // Adjust textEdit start to replacement range. For ex: if current
+ // path was "golang.or/x/to<>ols/internal/", where <> is the cursor
+ // position, start of the replacement range would be after
+ // "golang.org/x/".
+ path := strings.SplitAfter(prefix, "/")
+ numChars := len(strings.Join(path[:len(path)-1], ""))
+ content = content[numChars:]
+ start += token.Pos(numChars)
+ }
+ namePrefix = ""
+ }
+
+ // We won't provide an ending quote if one is already present, except if
+ // cursor is after the ending quote but still in import spec. This is
+ // because cursor has to be in our textEdit range.
+ if strings.HasSuffix(importPath, `"`) && c.pos < searchImport.Path.End() {
+ end--
+ content = content[:len(content)-1]
+ nameSuffix = ""
+ }
+
+ c.surrounding = &Selection{
+ content: content,
+ cursor: c.pos,
+ MappedRange: source.NewMappedRange(c.snapshot.FileSet(), c.mapper, start, end),
+ }
+
+ seenImports := make(map[string]struct{})
+ for _, importSpec := range c.file.Imports {
+ if importSpec.Path.Value == importPath {
+ continue
+ }
+ seenImportPath, err := strconv.Unquote(importSpec.Path.Value)
+ if err != nil {
+ return err
+ }
+ seenImports[seenImportPath] = struct{}{}
+ }
+
var mu sync.Mutex // guard c.items locally, since searchImports is called in parallel
seen := make(map[string]struct{})
searchImports := func(pkg imports.ImportFix) {
@@ -726,12 +760,20 @@
}
pkgToConsider := strings.Join(pkgDirList[:depth+1], "/")
+ name := pkgDirList[depth]
+ // if we're adding an opening quote to completion too, set name to full
+ // package path since we'll need to overwrite that range.
+ if namePrefix == `"` {
+ name = pkgToConsider
+ }
+
score := float64(pkg.Relevance)
if len(pkgDirList)-1 == depth {
score *= highScore
} else {
// For incomplete package paths, add a terminal slash to indicate that the
// user should keep triggering completions.
+ name += "/"
pkgToConsider += "/"
}
@@ -744,11 +786,11 @@
defer mu.Unlock()
obj := types.NewPkgName(0, nil, pkg.IdentName, types.NewPackage(pkgToConsider, pkg.IdentName))
- // Running goimports logic in completions is expensive, and the
- // (*completer).found method imposes a 100ms budget. Work-around this
- // by adding to c.items directly.
- cand := candidate{obj: obj, name: `"` + pkgToConsider + `"`, score: score}
+ cand := candidate{obj: obj, name: namePrefix + name + nameSuffix, score: score}
+ // We use c.item here to be able to manually update the detail for a
+ // candidate. c.found doesn't give us access to the completion item.
if item, err := c.item(ctx, cand); err == nil {
+ item.Detail = fmt.Sprintf("%q", pkgToConsider)
c.items = append(c.items, item)
}
}
diff --git a/internal/lsp/testdata/lsp/primarymod/importedcomplit/imported_complit.go.in b/internal/lsp/testdata/lsp/primarymod/importedcomplit/imported_complit.go.in
index dddf20d..80d8524 100644
--- a/internal/lsp/testdata/lsp/primarymod/importedcomplit/imported_complit.go.in
+++ b/internal/lsp/testdata/lsp/primarymod/importedcomplit/imported_complit.go.in
@@ -10,6 +10,7 @@
"golang.org/x/too" //@complete("\" //", toolsImport)
"crypto/elli" //@complete("\" //", cryptoImport)
"golang.org/x/tools/internal/lsp/sign" //@complete("\" //", signatureImport)
+ "golang.org/x/tools/internal/lsp/sign" //@complete("ols", toolsImport)
namedParser "go/pars" //@complete("\" //", parserImport)
)
@@ -34,8 +35,8 @@
_ = foo.StructFoo{s.} //@complete("}", icFieldAB, icFieldAA)
}
-/* "fmt" */ //@item(fmtImport, "\"fmt\"", "\"fmt\"", "package")
-/* "go/parser" */ //@item(parserImport, "\"go/parser\"", "\"go/parser\"", "package")
-/* "golang.org/x/tools/internal/lsp/signature" */ //@item(signatureImport, "\"golang.org/x/tools/internal/lsp/signature\"", "\"golang.org/x/tools/internal/lsp/signature\"", "package")
-/* "golang.org/x/tools/" */ //@item(toolsImport, "\"golang.org/x/tools/\"", "\"golang.org/x/tools/\"", "package")
-/* "crypto/elliptic" */ //@item(cryptoImport, "\"crypto/elliptic\"", "\"crypto/elliptic\"", "package")
+/* "fmt" */ //@item(fmtImport, "fmt", "\"fmt\"", "package")
+/* "go/parser" */ //@item(parserImport, "parser", "\"go/parser\"", "package")
+/* "golang.org/x/tools/internal/lsp/signature" */ //@item(signatureImport, "signature", "\"golang.org/x/tools/internal/lsp/signature\"", "package")
+/* "golang.org/x/tools/" */ //@item(toolsImport, "tools/", "\"golang.org/x/tools/\"", "package")
+/* "crypto/elliptic" */ //@item(cryptoImport, "elliptic", "\"crypto/elliptic\"", "package")
diff --git a/internal/lsp/testdata/lsp/summary.txt.golden b/internal/lsp/testdata/lsp/summary.txt.golden
index e6e82d1..3161956 100644
--- a/internal/lsp/testdata/lsp/summary.txt.golden
+++ b/internal/lsp/testdata/lsp/summary.txt.golden
@@ -1,7 +1,7 @@
-- summary --
CallHierarchyCount = 1
CodeLensCount = 5
-CompletionsCount = 254
+CompletionsCount = 255
CompletionSnippetCount = 85
UnimportedCompletionsCount = 6
DeepCompletionsCount = 5
