internal/lsp/source: improve func literal completions
When a signature doesn't name its params, we make up param
names when generating a corresponding func literal:
var f func(myType) // no param name
f = fun<> // completes to "func(mt myType) {}"
Previously we would abbreviate named types and fall back to "_" for
builtins or repeated names. We would require placeholders to be
enabled when using "_" so the user could name the param easily. That
left users that don't use placeholders with no completion at all in
this case.
I made the following improvements:
- Generate a name for all params. For builtin types we use the first
letter, e.g. "i" for "int", "s" for "[]string". If a name is
repeated, we append incrementing numeric suffixes. For example,
"func(int, int32)" becomes "func(i1 int, i2 int32").
- No longer require placeholders to be enabled in any case.
- Fix handling of alias types so the param name and type name are
based on the alias, not the aliasee.
I also tweaked formatVarType to qualify packages using a
types.Qualifier. I needed it to respect a qualifier that doesn't
qualify anything so I could format e.g. "http.Response" as just
"Response" to come up with param names.
Fixes golang/go#38416.
Change-Id: I0ce8a0a4e2485dda41a0aa696d9fd48bea595869
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246262
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/internal/lsp/source/completion_literal.go b/internal/lsp/source/completion_literal.go
index b3a50ab..a115e95 100644
--- a/internal/lsp/source/completion_literal.go
+++ b/internal/lsp/source/completion_literal.go
@@ -6,6 +6,7 @@
import (
"context"
+ "fmt"
"go/ast"
"go/token"
"go/types"
@@ -161,7 +162,7 @@
if score := c.matcher.Score("func"); !cand.takeAddress && score >= 0 && !isInterface(expType) {
switch t := literalType.Underlying().(type) {
case *types.Signature:
- c.functionLiteral(t, float64(score))
+ c.functionLiteral(ctx, t, float64(score))
}
}
}
@@ -188,42 +189,69 @@
// functionLiteral adds a function literal completion item for the
// given signature.
-func (c *completer) functionLiteral(sig *types.Signature, matchScore float64) {
+func (c *completer) functionLiteral(ctx context.Context, sig *types.Signature, matchScore float64) {
snip := &snippet.Builder{}
snip.WriteText("func(")
- seenParamNames := make(map[string]bool)
+
+ // First we generate names for each param and keep a seen count so
+ // we know if we need to uniquify param names. For example,
+ // "func(int)" will become "func(i int)", but "func(int, int64)"
+ // will become "func(i1 int, i2 int64)".
+ var (
+ paramNames = make([]string, sig.Params().Len())
+ paramNameCount = make(map[string]int)
+ )
+ for i := 0; i < sig.Params().Len(); i++ {
+ var (
+ p = sig.Params().At(i)
+ name = p.Name()
+ )
+ if name == "" {
+ // If the param has no name in the signature, guess a name based
+ // on the type. Use an empty qualifier to ignore the package.
+ // For example, we want to name "http.Request" "r", not "hr".
+ name = formatVarType(ctx, c.snapshot, c.pkg, c.file, p, func(p *types.Package) string {
+ return ""
+ })
+ name = abbreviateTypeName(name)
+ }
+ paramNames[i] = name
+ if name != "_" {
+ paramNameCount[name]++
+ }
+ }
+
+ for n, c := range paramNameCount {
+ // Any names we saw more than once will need a unique suffix added
+ // on. Reset the count to 1 to act as the suffix for the first
+ // name.
+ if c >= 2 {
+ paramNameCount[n] = 1
+ } else {
+ delete(paramNameCount, n)
+ }
+ }
+
for i := 0; i < sig.Params().Len(); i++ {
if i > 0 {
snip.WriteText(", ")
}
- p := sig.Params().At(i)
- name := p.Name()
+ var (
+ p = sig.Params().At(i)
+ name = paramNames[i]
+ )
- // If the parameter has no name in the signature, we need to try
- // come up with a parameter name.
- if name == "" {
- // Our parameter names are guesses, so they must be placeholders
- // for easy correction. If placeholders are disabled, don't
- // offer the completion.
- if !c.opts.placeholders {
- return
- }
+ // Uniquify names by adding on an incrementing numeric suffix.
+ if idx, found := paramNameCount[name]; found {
+ paramNameCount[name]++
+ name = fmt.Sprintf("%s%d", name, idx)
+ }
- // Try abbreviating named types. If the type isn't named, or the
- // abbreviation duplicates a previous name, give up and use
- // "_". The user will have to provide a name for this parameter
- // in order to use it.
- if named, ok := deref(p.Type()).(*types.Named); ok {
- name = abbreviateCamel(named.Obj().Name())
- if seenParamNames[name] {
- name = "_"
- } else {
- seenParamNames[name] = true
- }
- } else {
- name = "_"
- }
+ if name != p.Name() && c.opts.placeholders {
+ // If we didn't use the signature's param name verbatim then we
+ // may have chosen a poor name. Give the user a placeholder so
+ // they can easily fix the name.
snip.WritePlaceholder(func(b *snippet.Builder) {
b.WriteText(name)
})
@@ -236,7 +264,7 @@
// of "i int, j int".
if i == sig.Params().Len()-1 || !types.Identical(p.Type(), sig.Params().At(i+1).Type()) {
snip.WriteText(" ")
- typeStr := types.TypeString(p.Type(), c.qf)
+ typeStr := formatVarType(ctx, c.snapshot, c.pkg, c.file, p, c.qf)
if sig.Variadic() && i == sig.Params().Len()-1 {
typeStr = strings.Replace(typeStr, "[]", "...", 1)
}
@@ -264,7 +292,7 @@
if name := r.Name(); name != "" {
snip.WriteText(name + " ")
}
- snip.WriteText(types.TypeString(r.Type(), c.qf))
+ snip.WriteText(formatVarType(ctx, c.snapshot, c.pkg, c.file, r, c.qf))
}
if resultsNeedParens {
snip.WriteText(")")
@@ -282,14 +310,38 @@
})
}
-// abbreviateCamel abbreviates camel case identifiers into
-// abbreviations. For example, "fooBar" is abbreviated "fb".
-func abbreviateCamel(s string) string {
+// abbreviateTypeName abbreviates type names into acronyms. For
+// example, "fooBar" is abbreviated "fb". Care is taken to ignore
+// non-identifier runes. For example, "[]int" becomes "i", and
+// "struct { i int }" becomes "s".
+func abbreviateTypeName(s string) string {
var (
b strings.Builder
useNextUpper bool
)
+
+ // Trim off leading non-letters. We trim everything between "[" and
+ // "]" to handle array types like "[someConst]int".
+ var inBracket bool
+ s = strings.TrimFunc(s, func(r rune) bool {
+ if inBracket {
+ inBracket = r != ']'
+ return true
+ }
+
+ if r == '[' {
+ inBracket = true
+ }
+
+ return !unicode.IsLetter(r)
+ })
+
for i, r := range s {
+ // Stop if we encounter a non-identifier rune.
+ if !unicode.IsLetter(r) && !unicode.IsNumber(r) {
+ break
+ }
+
if i == 0 {
b.WriteRune(unicode.ToLower(r))
}
diff --git a/internal/lsp/source/types_format.go b/internal/lsp/source/types_format.go
index 9fee430..2bf9f26 100644
--- a/internal/lsp/source/types_format.go
+++ b/internal/lsp/source/types_format.go
@@ -14,7 +14,6 @@
"go/types"
"strings"
- "golang.org/x/tools/go/ast/astutil"
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/lsp/debug/tag"
"golang.org/x/tools/internal/lsp/protocol"
@@ -218,7 +217,7 @@
// If the request came from a different package than the one in which the
// types are defined, we may need to modify the qualifiers.
- qualified = qualifyExpr(snapshot.FileSet(), qualified, srcpkg, pkg, srcfile, clonedInfo)
+ qualified = qualifyExpr(snapshot.FileSet(), qualified, srcpkg, pkg, srcfile, clonedInfo, qf)
fmted := formatNode(snapshot.FileSet(), qualified)
return fmted
}
@@ -241,7 +240,7 @@
}
// qualifyExpr applies the "pkgName." prefix to any *ast.Ident in the expr.
-func qualifyExpr(fset *token.FileSet, expr ast.Expr, srcpkg, pkg Package, file *ast.File, clonedInfo map[token.Pos]*types.PkgName) ast.Expr {
+func qualifyExpr(fset *token.FileSet, expr ast.Expr, srcpkg, pkg Package, file *ast.File, clonedInfo map[token.Pos]*types.PkgName, qf types.Qualifier) ast.Expr {
ast.Inspect(expr, func(n ast.Node) bool {
switch n := n.(type) {
case *ast.ArrayType, *ast.ChanType, *ast.Ellipsis,
@@ -262,7 +261,7 @@
if !ok {
return false
}
- pkgName := importedPkgName(fset, srcpkg.GetTypes(), obj.Imported(), file)
+ pkgName := qf(obj.Imported())
if pkgName != "" {
x.Name = pkgName
}
@@ -273,7 +272,7 @@
}
// Only add the qualifier if the identifier is exported.
if ast.IsExported(n.Name) {
- pkgName := importedPkgName(fset, srcpkg.GetTypes(), pkg.GetTypes(), file)
+ pkgName := qf(pkg.GetTypes())
n.Name = pkgName + "." + n.Name
}
}
@@ -282,24 +281,6 @@
return expr
}
-// importedPkgName returns the package name used for pkg in srcpkg.
-func importedPkgName(fset *token.FileSet, srcpkg, pkg *types.Package, file *ast.File) string {
- if srcpkg == pkg {
- return ""
- }
- // If the file already imports the package under another name, use that.
- for _, group := range astutil.Imports(fset, file) {
- for _, cand := range group {
- if strings.Trim(cand.Path.Value, `"`) == pkg.Path() {
- if cand.Name != nil && cand.Name.Name != "" {
- return cand.Name.Name
- }
- }
- }
- }
- return pkg.Name()
-}
-
// cloneExpr only clones expressions that appear in the parameters or return
// values of a function declaration. The original expression may be returned
// to the caller in 2 cases:
diff --git a/internal/lsp/testdata/lsp/primarymod/snippets/literal_snippets.go.in b/internal/lsp/testdata/lsp/primarymod/snippets/literal_snippets.go.in
index 69b8cdf..d970bf1 100644
--- a/internal/lsp/testdata/lsp/primarymod/snippets/literal_snippets.go.in
+++ b/internal/lsp/testdata/lsp/primarymod/snippets/literal_snippets.go.in
@@ -131,14 +131,14 @@
sort.Slice(nil, fun) //@complete(")", litFunc),snippet(")", litFunc, "func(i, j int) bool {$0\\}", "func(i, j int) bool {$0\\}")
- http.HandleFunc("", f) //@snippet(")", litFunc, "", "func(${1:rw} http.ResponseWriter, ${2:r} *http.Request) {$0\\}")
+ http.HandleFunc("", f) //@snippet(")", litFunc, "func(rw http.ResponseWriter, r *http.Request) {$0\\}", "func(${1:rw} http.ResponseWriter, ${2:r} *http.Request) {$0\\}")
// no literal "func" completions
http.Handle("", fun) //@complete(")")
http.HandlerFunc() //@item(handlerFunc, "http.HandlerFunc()", "", "var")
http.Handle("", h) //@snippet(")", handlerFunc, "http.HandlerFunc($0)", "http.HandlerFunc($0)")
- http.Handle("", http.HandlerFunc()) //@snippet("))", litFunc, "", "func(${1:rw} http.ResponseWriter, ${2:r} *http.Request) {$0\\}")
+ http.Handle("", http.HandlerFunc()) //@snippet("))", litFunc, "func(rw http.ResponseWriter, r *http.Request) {$0\\}", "func(${1:rw} http.ResponseWriter, ${2:r} *http.Request) {$0\\}")
var namedReturn func(s string) (b bool)
namedReturn = f //@snippet(" //", litFunc, "func(s string) (b bool) {$0\\}", "func(s string) (b bool) {$0\\}")
@@ -150,7 +150,15 @@
multiNamedReturn = f //@snippet(" //", litFunc, "func() (b bool, i int) {$0\\}", "func() (b bool, i int) {$0\\}")
var duplicateParams func(myImpl, int, myImpl)
- duplicateParams = f //@snippet(" //", litFunc, "", "func(${1:mi} myImpl, ${2:_} int, ${3:_} myImpl) {$0\\}")
+ duplicateParams = f //@snippet(" //", litFunc, "func(mi1 myImpl, i int, mi2 myImpl) {$0\\}", "func(${1:mi1} myImpl, ${2:i} int, ${3:mi2} myImpl) {$0\\}")
+
+ type aliasImpl = myImpl
+ var aliasParams func(aliasImpl) aliasImpl
+ aliasParams = f //@snippet(" //", litFunc, "func(ai aliasImpl) aliasImpl {$0\\}", "func(${1:ai} aliasImpl) aliasImpl {$0\\}")
+
+ const two = 2
+ var builtinTypes func([]int, [two]bool, map[string]string, struct{ i int }, interface{ foo() }, <-chan int)
+ builtinTypes = f //@snippet(" //", litFunc, "func(i1 []int, b [two]bool, m map[string]string, s struct{ i int \\}, i2 interface{ foo() \\}, c <-chan int) {$0\\}", "func(${1:i1} []int, ${2:b} [two]bool, ${3:m} map[string]string, ${4:s} struct{ i int \\}, ${5:i2} interface{ foo() \\}, ${6:c} <-chan int) {$0\\}")
}
func _() {
diff --git a/internal/lsp/testdata/lsp/summary.txt.golden b/internal/lsp/testdata/lsp/summary.txt.golden
index 6a2ca71..2ea9450 100644
--- a/internal/lsp/testdata/lsp/summary.txt.golden
+++ b/internal/lsp/testdata/lsp/summary.txt.golden
@@ -2,7 +2,7 @@
CallHierarchyCount = 1
CodeLensCount = 5
CompletionsCount = 239
-CompletionSnippetCount = 81
+CompletionSnippetCount = 83
UnimportedCompletionsCount = 6
DeepCompletionsCount = 5
FuzzyCompletionsCount = 8
