Skip to content

Commit

Permalink
gopls/internal/lsp/source/completion: support more postfix completion…
Browse files Browse the repository at this point in the history 
… for map and slice (len, for, forr)

Add some new postfix completions (len, for, forr) for map and slice,
with additional support for chan type (for). The previous
implementation of postfix completion did not support Placeholder, which
resulted in a poor user experience. This can be seen in go.dev/issue/59689.
This commit also adds support for Placeholder and modifies some existing places
where Placeholder should have been used.

Fixes golang/go#64037
Fixes golang/go#59689

Change-Id: I8d27e4c965ce1f8a5631dcdbe9242398fa1f8130
GitHub-Last-Rev: d61809f
GitHub-Pull-Request: #459
Reviewed-on: https://go-review.googlesource.com/c/tools/+/541277
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
  • Loading branch information
@rogeryk @gopherbot
rogeryk authored and gopherbot committed Dec 1, 2023
1 parent ca243bd commit b74ea80
Show file tree
Hide file tree
Showing 4 changed files with 351 additions and 19 deletions.
75 changes: 68 additions & 7 deletions gopls/internal/lsp/source/completion/postfix_snippets.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ type postfixTmplArgs struct {
edits []protocol.TextEdit
qf types.Qualifier
varNames map[string]bool
placeholders bool
}

var postfixTmpls = []postfixTmpl{{
Expand Down Expand Up @@ -103,7 +104,23 @@ for {{$i}}, {{$j}} := 0, len({{.X}})-1; {{$i}} < {{$j}}; {{$i}}, {{$j}} = {{$i}}
label: "range",
details: "range over slice",
body: `{{if and (eq .Kind "slice") .StmtOK -}}
for {{.VarName nil "i"}}, {{.VarName .ElemType "v"}} := range {{.X}} {
for {{.VarName nil "i" | .Placeholder }}, {{.VarName .ElemType "v" | .Placeholder}} := range {{.X}} {
{{.Cursor}}
}
{{- end}}`,
}, {
label: "for",
details: "range over slice by index",
body: `{{if and (eq .Kind "slice") .StmtOK -}}
for {{ .VarName nil "i" | .Placeholder }} := range {{.X}} {
{{.Cursor}}
}
{{- end}}`,
}, {
label: "forr",
details: "range over slice by index and value",
body: `{{if and (eq .Kind "slice") .StmtOK -}}
for {{.VarName nil "i" | .Placeholder }}, {{.VarName .ElemType "v" | .Placeholder }} := range {{.X}} {
{{.Cursor}}
}
{{- end}}`,
Expand All @@ -130,7 +147,23 @@ copy({{$v}}, {{.X}})
label: "range",
details: "range over map",
body: `{{if and (eq .Kind "map") .StmtOK -}}
for {{.VarName .KeyType "k"}}, {{.VarName .ElemType "v"}} := range {{.X}} {
for {{.VarName .KeyType "k" | .Placeholder}}, {{.VarName .ElemType "v" | .Placeholder}} := range {{.X}} {
{{.Cursor}}
}
{{- end}}`,
}, {
label: "for",
details: "range over map by key",
body: `{{if and (eq .Kind "map") .StmtOK -}}
for {{.VarName .KeyType "k" | .Placeholder}} := range {{.X}} {
{{.Cursor}}
}
{{- end}}`,
}, {
label: "forr",
details: "range over map by key and value",
body: `{{if and (eq .Kind "map") .StmtOK -}}
for {{.VarName .KeyType "k" | .Placeholder}}, {{.VarName .ElemType "v" | .Placeholder}} := range {{.X}} {
{{.Cursor}}
}
{{- end}}`,
Expand All @@ -155,21 +188,29 @@ for {{.VarName .KeyType "k"}}, {{.VarName .ElemType "v"}} := range {{.X}} {
label: "range",
details: "range over channel",
body: `{{if and (eq .Kind "chan") .StmtOK -}}
for {{.VarName .ElemType "e"}} := range {{.X}} {
for {{.VarName .ElemType "e" | .Placeholder}} := range {{.X}} {
{{.Cursor}}
}
{{- end}}`,
}, {
label: "for",
details: "range over channel",
body: `{{if and (eq .Kind "chan") .StmtOK -}}
for {{.VarName .ElemType "e" | .Placeholder}} := range {{.X}} {
{{.Cursor}}
}
{{- end}}`,
}, {
label: "var",
details: "assign to variables",
body: `{{if and (eq .Kind "tuple") .StmtOK -}}
{{$a := .}}{{range $i, $v := .Tuple}}{{if $i}}, {{end}}{{$a.VarName $v.Type $v.Name}}{{end}} := {{.X}}
{{$a := .}}{{range $i, $v := .Tuple}}{{if $i}}, {{end}}{{$a.VarName $v.Type $v.Name | $a.Placeholder }}{{end}} := {{.X}}
{{- end}}`,
}, {
label: "var",
details: "assign to variable",
body: `{{if and (ne .Kind "tuple") .StmtOK -}}
{{.VarName .Type ""}} := {{.X}}
{{.VarName .Type "" | .Placeholder }} := {{.X}}
{{- end}}`,
}, {
label: "print",
Expand Down Expand Up @@ -199,9 +240,15 @@ for {{.VarName .ElemType "e"}} := range {{.X}} {
label: "ifnotnil",
details: "if expr != nil",
body: `{{if and (or (eq .Kind "pointer") (eq .Kind "chan") (eq .Kind "signature") (eq .Kind "interface") (eq .Kind "map") (eq .Kind "slice")) .StmtOK -}}
if {{.X}} != nil {{"{"}}
if {{.X}} != nil {
{{.Cursor}}
{{"}"}}
}
{{- end}}`,
}, {
label: "len",
details: "len(s)",
body: `{{if (eq .Kind "slice" "map" "array" "chan") -}}
len({{.X}})
{{- end}}`,
}}

Expand All @@ -212,6 +259,19 @@ func (a *postfixTmplArgs) Cursor() string {
return ""
}

// Placeholder indicate a tab stops with the placeholder string, the order
// of tab stops is the same as the order of invocation
func (a *postfixTmplArgs) Placeholder(s string) string {
if a.placeholders {
a.snip.WritePlaceholder(func(b *snippet.Builder) {
b.WriteText(s)
})
} else {
a.snip.WritePlaceholder(nil)
}
return ""
}

// Import makes sure the package corresponding to path is imported,
// returning the identifier to use to refer to the package.
func (a *postfixTmplArgs) Import(path string) (string, error) {
Expand Down Expand Up @@ -399,6 +459,7 @@ func (c *completer) addPostfixSnippetCandidates(ctx context.Context, sel *ast.Se
importIfNeeded: c.importIfNeeded,
scope: scope,
varNames: make(map[string]bool),
placeholders: c.opts.placeholders,
}

// Feed the template straight into the snippet builder. This
Expand Down
189 changes: 180 additions & 9 deletions gopls/internal/test/integration/completion/postfix_snippet_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ go 1.12
`

cases := []struct {
name string
before, after string
name string
before, after string
allowMultipleItem bool
}{
{
name: "sort",
Expand Down Expand Up @@ -133,7 +134,7 @@ package foo
func _() {
type myThing struct{}
var foo []myThing
for i, mt := range foo {
for ${1:}, ${2:} := range foo {
$0
}
}
Expand Down Expand Up @@ -213,7 +214,7 @@ package foo
func _() {
var foo map[string]int
for k, v := range foo {
for ${1:}, ${2:} := range foo {
$0
}
}
Expand Down Expand Up @@ -279,7 +280,7 @@ package foo
func _() {
foo := make(chan int)
for e := range foo {
for ${1:} := range foo {
$0
}
}
Expand All @@ -302,7 +303,7 @@ package foo
func foo() (int, error) { return 0, nil }
func _() {
i, err := foo()
${1:}, ${2:} := foo()
}
`,
},
Expand All @@ -323,7 +324,7 @@ package foo
func foo() error { return nil }
func _() {
err := foo()
${1:} := foo()
}
`,
},
Expand All @@ -344,7 +345,7 @@ package foo
func foo() (int, int) { return 0, 0 }
func _() {
i, i2 := foo()
${1:}, ${2:} := foo()
}
`,
},
Expand Down Expand Up @@ -554,6 +555,173 @@ func _() {
$0
}
}
`,
},
{
name: "slice_len",
before: `
package foo
func _() {
var foo []int
foo.len
}
`,
after: `
package foo
func _() {
var foo []int
len(foo)
}
`,
},
{
name: "map_len",
before: `
package foo
func _() {
var foo map[string]int
foo.len
}
`,
after: `
package foo
func _() {
var foo map[string]int
len(foo)
}
`,
},
{
name: "slice_for",
allowMultipleItem: true,
before: `
package foo
func _() {
var foo []int
foo.for
}
`,
after: `
package foo
func _() {
var foo []int
for ${1:} := range foo {
$0
}
}
`,
},
{
name: "map_for",
allowMultipleItem: true,
before: `
package foo
func _() {
var foo map[string]int
foo.for
}
`,
after: `
package foo
func _() {
var foo map[string]int
for ${1:} := range foo {
$0
}
}
`,
},
{
name: "chan_for",
allowMultipleItem: true,
before: `
package foo
func _() {
var foo chan int
foo.for
}
`,
after: `
package foo
func _() {
var foo chan int
for ${1:} := range foo {
$0
}
}
`,
},
{
name: "slice_forr",
before: `
package foo
func _() {
var foo []int
foo.forr
}
`,
after: `
package foo
func _() {
var foo []int
for ${1:}, ${2:} := range foo {
$0
}
}
`,
},
{
name: "slice_forr",
before: `
package foo
func _() {
var foo []int
foo.forr
}
`,
after: `
package foo
func _() {
var foo []int
for ${1:}, ${2:} := range foo {
$0
}
}
`,
},
{
name: "map_forr",
before: `
package foo
func _() {
var foo map[string]int
foo.forr
}
`,
after: `
package foo
func _() {
var foo map[string]int
for ${1:}, ${2:} := range foo {
$0
}
}
`,
},
}
Expand All @@ -575,7 +743,10 @@ func _() {

loc := env.RegexpSearch("foo.go", "\n}")
completions := env.Completion(loc)
if len(completions.Items) != 1 {
if len(completions.Items) < 1 {
t.Fatalf("expected at least one completion, got %v", completions.Items)
}
if !c.allowMultipleItem && len(completions.Items) > 1 {
t.Fatalf("expected one completion, got %v", completions.Items)
}

Expand Down

0 comments on commit b74ea80

Please sign in to comment.