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>

internal/lsp/source/completion_literal.go

@@ -6,6 +6,7 @@ package source
 
 import (
 	"context"
+	"fmt"
 	"go/ast"
 	"go/token"
 	"go/types"
@@ -161,7 +162,7 @@ func (c *completer) literal(ctx context.Context, literalType types.Type, imp *im
 	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 @@ const literalCandidateScore = highScore / 2
 
 // 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 @@ func (c *completer) functionLiteral(sig *types.Signature, matchScore float64) {
 		// 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 @@ func (c *completer) functionLiteral(sig *types.Signature, matchScore float64) {
 		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 @@ func (c *completer) functionLiteral(sig *types.Signature, matchScore float64) {
 	})
 }
 
-// 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))
 		}

internal/lsp/source/types_format.go

@@ -14,7 +14,6 @@ import (
 	"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 @@ func formatVarType(ctx context.Context, snapshot Snapshot, srcpkg Package, srcfi
 
 	// 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 @@ func varType(ctx context.Context, snapshot Snapshot, pgf *ParsedGoFile, obj *typ
 }
 
 // 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 @@ func qualifyExpr(fset *token.FileSet, expr ast.Expr, srcpkg, pkg Package, file *
 			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 @@ func qualifyExpr(fset *token.FileSet, expr ast.Expr, srcpkg, pkg Package, file *
 			}
 			// 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 @@ func qualifyExpr(fset *token.FileSet, expr ast.Expr, srcpkg, pkg Package, file *
 	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:

internal/lsp/testdata/lsp/primarymod/snippets/literal_snippets.go.in

@@ -131,14 +131,14 @@ func _() {
 
 	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 @@ func _() {
 	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 _() {

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