gopls: unimported completion should use the completion matcher
Fix a bug where unimported completion was using strings.HasPrefix
directly, rather than using the configured completion matcher.
Fixes golang/go#60545
Change-Id: I96e8e0b2dbfd9f007b166d4a82399c591ffd823a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/499795
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
diff --git a/gopls/internal/lsp/source/completion/completion.go b/gopls/internal/lsp/source/completion/completion.go
index bc2b0c3..de54a3f 100644
--- a/gopls/internal/lsp/source/completion/completion.go
+++ b/gopls/internal/lsp/source/completion/completion.go
@@ -1210,8 +1210,10 @@
// quickParse does a quick parse of a single file of package m,
// extracts exported package members and adds candidates to c.items.
- var itemsMu sync.Mutex // guards c.items
- var enough int32 // atomic bool
+ // TODO(rfindley): synchronizing access to c here does not feel right.
+ // Consider adding a concurrency-safe API for completer.
+ var cMu sync.Mutex // guards c.items and c.matcher
+ var enough int32 // atomic bool
quickParse := func(uri span.URI, m *source.Metadata) error {
if atomic.LoadInt32(&enough) != 0 {
return nil
@@ -1231,13 +1233,22 @@
return
}
- if !id.IsExported() ||
- sel.Sel.Name != "_" && !strings.HasPrefix(id.Name, sel.Sel.Name) {
- return // not a match
+ if !id.IsExported() {
+ return
+ }
+
+ cMu.Lock()
+ score := c.matcher.Score(id.Name)
+ cMu.Unlock()
+
+ if sel.Sel.Name != "_" && score == 0 {
+ return // not a match; avoid constructing the completion item below
}
// The only detail is the kind and package: `var (from "example.com/foo")`
// TODO(adonovan): pretty-print FuncDecl.FuncType or TypeSpec.Type?
+ // TODO(adonovan): should this score consider the actual c.matcher.Score
+ // of the item? How does this compare with the deepState.enqueue path?
item := CompletionItem{
Label: id.Name,
Detail: fmt.Sprintf("%s (from %q)", strings.ToLower(tok.String()), m.PkgPath),
@@ -1298,12 +1309,12 @@
item.snippet = &sn
}
- itemsMu.Lock()
+ cMu.Lock()
c.items = append(c.items, item)
if len(c.items) >= unimportedMemberTarget {
atomic.StoreInt32(&enough, 1)
}
- itemsMu.Unlock()
+ cMu.Unlock()
})
return nil
}
diff --git a/gopls/internal/regtest/marker/testdata/completion/issue60545.txt b/gopls/internal/regtest/marker/testdata/completion/issue60545.txt
new file mode 100644
index 0000000..67221a6
--- /dev/null
+++ b/gopls/internal/regtest/marker/testdata/completion/issue60545.txt
@@ -0,0 +1,24 @@
+This test checks that unimported completion is case-insensitive.
+
+-- go.mod --
+module mod.test
+
+go 1.18
+
+-- main.go --
+package main
+
+func main() {
+ fmt.p //@complete(re"p()","Print", "Printf", "Println"), diag("fmt", re"(undefined|undeclared)")
+}
+
+-- other.go --
+package main
+
+// Including another package that imports "fmt" causes completion to use the
+// existing metadata, which is the codepath leading to golang/go#60545.
+import "fmt"
+
+func _() {
+ fmt.Println()
+}
