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>

internal/lsp/source/completion/completion.go

@@ -8,6 +8,7 @@ package completion
 
 import (
  "context"
+ "fmt"
  "go/ast"
  "go/constant"
  "go/scanner"
@@ -683,33 +684,66 @@ func (c *completer) emptySwitchStmt() bool {
 // (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 {
+ importPath := searchImport.Path.Value
+
+ // Extract the text between the quotes (if any) in an import spec.
+ // prefix is the part of import path before the cursor.
+ 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: searchImport.Path.Value,
+ content: content,
  cursor: c.pos,
- MappedRange: source.NewMappedRange(c.snapshot.FileSet(), c.mapper, searchImport.Path.Pos(), searchImport.Path.End()),
+ 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 == searchImport.Path.Value {
+ if importSpec.Path.Value == importPath {
  continue
  }
- importPath, err := strconv.Unquote(importSpec.Path.Value)
+ seenImportPath, err := strconv.Unquote(importSpec.Path.Value)
  if err != nil {
  return err
  }
- seenImports[importPath] = struct{}{}
+ seenImports[seenImportPath] = 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], `"`)
-
- // The number of directories in the import path gives us the depth at
- // which to search.
- depth := len(strings.Split(prefix, "/")) - 1
-
  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 @@ func (c *completer) populateImportCompletions(ctx context.Context, searchImport
  }
  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 @@ func (c *completer) populateImportCompletions(ctx context.Context, searchImport
  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)
  }
  }

internal/lsp/testdata/lsp/primarymod/importedcomplit/imported_complit.go.in

@@ -10,6 +10,7 @@ import (
  "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 @@ func _() {
  _ = 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")

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