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 the issue golang/go#59689.
This commit also adds support for Placeholder and modifies
some existing places where Placeholder should have been used.

Fixes golang/go#64037, golang/go#59689
  • Loading branch information
@rogeryk
rogeryk committed Nov 30, 2023
1 parent 1b1e4da commit bed62f8
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/regtest/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
Loading

0 comments on commit bed62f8

Please sign in to comment.