gopls/internal/lsp/source/completion: (unimported) add placeholders

The logic added in CL 472183 to provide completions of functions
from unimported packages, which lack type information, didn't
bother with placeholders. This change causes it to generate
placeholders ("name type") from the raw syntax.

Fixes golang/go#60269

Change-Id: I66340e18de90bdee471a0dfbb1e3fd5c77fec75f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/496596
gopls-CI: kokoro <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: adonovan

gopls/internal/lsp/source/completion/completion.go

@@ -12,6 +12,7 @@ import (
 	"go/ast"
 	"go/constant"
 	"go/parser"
+	"go/printer"
 	"go/scanner"
 	"go/token"
 	"go/types"
@@ -1268,19 +1269,30 @@ func (c *completer) selector(ctx context.Context, sel *ast.SelectorExpr) error {
 				var sn snippet.Builder
 				sn.WriteText(id.Name)
 				sn.WriteText("(")
+
+				var cfg printer.Config // slight overkill
 				var nparams int
+				param := func(name string, typ ast.Expr) {
+					if nparams > 0 {
+						sn.WriteText(", ")
+					}
+					nparams++
+					sn.WritePlaceholder(func(b *snippet.Builder) {
+						var buf strings.Builder
+						buf.WriteString(name)
+						buf.WriteByte(' ')
+						cfg.Fprint(&buf, token.NewFileSet(), typ)
+						b.WriteText(buf.String())
+					})
+				}
 				for _, field := range fn.Type.Params.List {
 					if field.Names != nil {
-						nparams += len(field.Names)
+						for _, name := range field.Names {
+							param(name.Name, field.Type)
+						}
 					} else {
-						nparams++
-					}
-				}
-				for i := 0; i < nparams; i++ {
-					if i > 0 {
-						sn.WriteText(", ")
+						param("_", field.Type)
 					}
-					sn.WritePlaceholder(nil)
 				}
 				sn.WriteText(")")
 				item.snippet = &sn

gopls/internal/regtest/completion/completion_test.go

@@ -527,13 +527,60 @@ func main() {
 		env.AcceptCompletion(loc, completions.Items[0])
 		env.Await(env.DoneWithChange())
 		got := env.BufferText("main.go")
-		want := "package main\r\n\r\nimport (\r\n\t\"fmt\"\r\n\t\"math\"\r\n)\r\n\r\nfunc main() {\r\n\tfmt.Println(\"a\")\r\n\tmath.Sqrt(${1:})\r\n}\r\n"
+		want := "package main\r\n\r\nimport (\r\n\t\"fmt\"\r\n\t\"math\"\r\n)\r\n\r\nfunc main() {\r\n\tfmt.Println(\"a\")\r\n\tmath.Sqrt(${1:x float64})\r\n}\r\n"
 		if diff := cmp.Diff(want, got); diff != "" {
 			t.Errorf("unimported completion (-want +got):\n%s", diff)
 		}
 	})
 }
 
+func TestUnimportedCompletionHasPlaceholders60269(t *testing.T) {
+	// We can't express this as a marker test because it doesn't support AcceptCompletion.
+	const src = `
+-- go.mod --
+module example.com
+go 1.12
+
+-- a/a.go --
+package a
+
+var _ = b.F
+
+-- b/b.go --
+package b
+
+func F0(a, b int, c float64) {}
+func F1(int, chan *string) {}
+`
+	WithOptions(
+		WindowsLineEndings(),
+	).Run(t, src, func(t *testing.T, env *Env) {
+		env.OpenFile("a/a.go")
+		env.Await(env.DoneWithOpen())
+
+		// The table lists the expected completions as they appear in Items.
+		const common = "package a\r\n\r\nimport \"example.com/b\"\r\n\r\nvar _ = "
+		for i, want := range []string{
+			common + "b.F0(${1:a int}, ${2:b int}, ${3:c float64})\r\n",
+			common + "b.F1(${1:_ int}, ${2:_ chan *string})\r\n",
+		} {
+			loc := env.RegexpSearch("a/a.go", "b.F()")
+			completions := env.Completion(loc)
+			if len(completions.Items) == 0 {
+				t.Fatalf("no completion items")
+			}
+			saved := env.BufferText("a/a.go")
+			env.AcceptCompletion(loc, completions.Items[i])
+			env.Await(env.DoneWithChange())
+			got := env.BufferText("a/a.go")
+			if diff := cmp.Diff(want, got); diff != "" {
+				t.Errorf("%d: unimported completion (-want +got):\n%s", i, diff)
+			}
+			env.SetBufferContent("a/a.go", saved) // restore
+		}
+	})
+}
+
 func TestPackageMemberCompletionAfterSyntaxError(t *testing.T) {
 	// This test documents the current broken behavior due to golang/go#58833.
 	const src = `