Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/gopls: add location-aware snippets for append and other utility methods #39507

Open
A-UNDERSCORE-D opened this issue Jun 10, 2020 · 36 comments
Labels
FeatureRequest gopls/completion Issues related to auto-completion in gopls. gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@A-UNDERSCORE-D
Copy link

Is your feature request related to a problem? Please describe.
After using GoLand for some time I'm missing a small feature or two. Specifically I'm missing type aware snippets for append, sprintf, and a few other utility methods (like things from slicetricks).

Describe the solution you'd like
I'd like a snippet under the name .aapend (auto-append) or similar to construct an append-assign statement for me.

Either from the start by starting with append and getting toAppend = append(toAppend, <>)

Or from the middle of a line using something like .aapend which would add the correct = append(name, <>) to the end of the line.

The sprintf and slicetricks I mentioned above would work similarly, sprintf simply going from thing.sprint to fmt.Sprintf("<>", thing), and delete applying a (preferably pointer aware delete implementation from the slicetricks wiki page

Describe alternatives you've considered
I've written my own snippets for this, but they get suggested in comments and other places where they are not useful, cluttering any autocomplete I may have had. And these rely on somewhat brittle regexps, which do not always get it right.

	"assign append": {
		"description": "appends to a slice and assigns it to itself",
		"prefix": "aappend",
		"body": "${1:toAppend} = append(${1:toappend}, $2)"
	},
	"assign append inplace": {
		"description": "appends to a slice and assigns it to itself",
		"prefix": ".aappend",
		"body": " = append(${TM_CURRENT_LINE/\\s*(\\S+)\\..*/$1/}, $0)"
	},
	"slice delete": {
		"description": "cuts a value from a slice",
		"prefix": ".delete",
		"body": " = append(${TM_CURRENT_LINE/\\s*(\\S+)\\..*/$1/}[:${1:pos}], ${TM_CURRENT_LINE/\\s*(\\S+)\\..*/$1/}[${1:pos}+1:]...)"
	}

The other option here is to type things manually, though that has the annoyance that for some things, you're going to end up checking a wiki page EVERY time (slicetricks things, delete et al.) or are just repetitive--typing thing = append(thing, newStuff) gets old. Not to mention that if its type aware it also suggest thing = append(thing, newStuff...) where appropriate.

This may be more applicable to gopls itself, rather than here, but I thought I'd ask about it here first.

@hyangah
Copy link
Contributor

hyangah commented Jun 10, 2020

@A-UNDERSCORE-D Yeah, Goland's context-aware, custom postfix completion support is nice.

As you mentioned, we want to delegate most completion logic to the language server and this is the tracking issue #39354.

The completion snippets around append, or common standard functions, slice tricks are good completion candidates to be offered by default. It would be nice if customization and template support can be offered in the feature, too. @stamblerre, thought?

@stamblerre
Copy link
Contributor

Thanks for the suggestion, @A-UNDERSCORE-D. In general, this does sound like a gopls feature request so I'll transfer it to the golang/go repo.

We actually should have some similar functionality around append already: https://go-review.googlesource.com/c/tools/+/221777. Maybe there are ways we can improve it, because it doesn't trigger when you write append, only when you are completing the slice variable. #39354 is definitely also relevant here.

@stamblerre stamblerre transferred this issue from golang/vscode-go Jun 10, 2020
@A-UNDERSCORE-D
Copy link
Author

Thanks for the transfer! And yeah Ive noticed append popping up like that on occasion, it works quite well. I guess my real want here is a more direct way to tell gopls "please give me this specific autocompletion" as shown in #39354 already. Not sure if I should keep this open or defer to the #39354? I think mine provides a bit more abstract around expected behavior for postfix style completions.

That said #39354 may be an opening for something more abstract where there can be a predefined set of postfixes that can be expanded on via config if a user desires.

@stamblerre stamblerre changed the title Add location aware snippets for append and other utility methods x/tools/gopls: add location-aware snippets for append and other utility methods Jun 11, 2020
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jun 11, 2020
@gopherbot gopherbot added this to the Unreleased milestone Jun 11, 2020
@stamblerre
Copy link
Contributor

Yeah, I agree this can be a more general issue. Let's start with seeing how we can improve append and then think of more cases from there.

@golang golang deleted a comment from gopherbot Jun 11, 2020
@hyangah
Copy link
Contributor

hyangah commented Jul 1, 2020

FYI the CL (https://go-review.googlesource.com/c/tools/+/221777) that implements autocompletion for append is merged and available in gopls 0.4.2.

@A-UNDERSCORE-D when is the second command 'assign append inplace' rule useful? (We are trying to understand use cases and how to improve the autocompletion logic)

@A-UNDERSCORE-D
Copy link
Author

@hyangah Its mostly just an ease of use thing I picked up from my time using goland. Its a part of the whole postfix completion request. For us with gopls the main thing I can think of its its an unambiguous way to ask gopls to provide the completion we want. Meaning that instead of typing the partial name of a slice and using arrow keys to grab the append (I have not tried 4.2 yet, so the arrow keys comment is purely speculative) meaning that one's right hand needs to move to the right for arrow keys, one can use the progression partial name -> .aa (suggested append at this point) to get the same result. Or that is the intent, anyway

@segevfiner
Copy link
Contributor

I'm not getting completions for the case foo = app<> // offer "append(foo, <>)", but only for fo<> // offer "foo = append(foo, <>)" with gopls 0.4.3 and VS Code 1.47.2.

@stamblerre stamblerre removed this from the Unreleased milestone Jul 23, 2020
@muirdm
Copy link

muirdm commented Jul 30, 2020

it doesn't trigger when you write append, only when you are completing the slice variable

I'm not getting completions for the case foo = app<> // offer "append(foo, <>)"

Strange, it works great in Emacs 😛. I have no idea why VSCode isn't showing the candidate. In fact, it doesn't show any candidates in this case:

var foo []int
foo = app<>

Candidates from gopls:

[Trace - 20:54:09.159 PM] Received response 'textDocument/completion - (7)' in 100ms.
Result: {"isIncomplete":true,"items":[{"label":"append(foo, )","kind":3,"preselect":true,"sortText":"00000","insertTextFormat":2,"textEdit":{"range":{"start":{"line":4,"character":7},"end":{"line":4,"character":10}},"newText":"append(foo, ${1:})"}},{"label":"append","kind":3,"detail":"func(slice []Type, elems ...Type) []Type","sortText":"00001","filterText":"append","insertTextFormat":2,"textEdit":{"range":{"start":{"line":4,"character":7},"end":{"line":4,"character":10}},"newText":"append(${1:slice []Type}, ${2:elems ...Type})"}}]}

Should I open a separate (VSCode?) issue @stamblerre?

@stamblerre
Copy link
Contributor

Oh, I actually didn't realize that was supposed to work - thanks for clarifying. I just opened golang/vscode-go#441 - we should probably investigate it in VS Code Go first, but you're right in thinking it may be a VS Code issue.

@hyangah
Copy link
Contributor

hyangah commented Jul 31, 2020

@muirdm What's the vscode version? The candidate appears in my vscode. Maybe I am missing something.

@stamblerre stamblerre added this to the gopls/unplanned milestone Oct 21, 2020
@muirdm
Copy link

muirdm commented Nov 23, 2020

I'm playing around with postfix completions if anyone wants to try it and give feedback: https://go-review.googlesource.com/c/tools/+/272586

You can see the list of postfix thingies available here: https://go-review.googlesource.com/c/tools/+/272586/1/internal/lsp/source/completion/postfix.go#82

The list is not necessarily complete, and the things in the list are not necessarily useful enough to be in the list. I was just trying a variety of things to see how they would work.

@A-UNDERSCORE-D
Copy link
Author

Playing around with it a bit now. I really like this. the ! indicates its a little snippet, which is very helpful. And what you have is quite nice, I'd probably use them all over time.

Only complaint is that there arent docs sent with it. Not sure if that can be changed or not.

@A-UNDERSCORE-D
Copy link
Author

is there a way I can add suggestions or otherwise to your CL @muirdm? Ive been considering implementing some extra ones

@muirdm
Copy link

muirdm commented Nov 24, 2020

You can add things to the list in https://go-review.googlesource.com/c/tools/+/272586/1/internal/lsp/source/completion/postfix.go#82 and then rebuild gopls.

If you mean push changes to my branch, then I don't know. I assume there is some way to collaborate like that. My branch is rough and likely to change a lot, so I wouldn't put in too much time.

Only complaint is that there arent docs sent with it.

Do you mean more detail showing what the snippet will do, or actual textual documentation explaining it what it will do?

@A-UNDERSCORE-D
Copy link
Author

yeah, a "This is what you'll get if you use this" kind of idea. IIRC vscode can show a little box next to the completion option? Im not 100% sure but I think that its a thing

@muirdm
Copy link

muirdm commented Nov 25, 2020

This is what you'll get if you use this

Yes, gopls can fill in the completion item "detail" for that I think.

@A-UNDERSCORE-D
Copy link
Author

This is what you'll get if you use this

Yes, gopls can fill in the completion item "detail" for that I think.

Poked around a bit in that, was pretty easy. Still getting used to how gopls works internally. I think this is reasonable though. Also your code appeared to rebase fine to the latest release, but not to master -- I definitely dont know enough to fix the conflict though -- Not sure how to resolve the two different edit methods.

diff --git a/internal/lsp/source/completion/postfix.go b/internal/lsp/source/completion/postfix.go
index 1f95cff2..3eb62e67 100644
--- a/internal/lsp/source/completion/postfix.go
+++ b/internal/lsp/source/completion/postfix.go
@@ -71,6 +71,7 @@ func and(filters ...postfixFilter) postfixFilter {
 
 type postfixRule struct {
 	name    string
+	details string
 	filter  postfixFilter
 	label   string
 	imports []string
@@ -84,52 +85,60 @@ var postfixRules = []postfixRule{{
 	filter:  isSLice,
 	label:   "sort",
 	imports: []string{"sort"},
+	details: "Insert a sort.Slice implementation",
 	body: `{{.Import "sort"}}.Slice({{.Obj}}, func(i, j int) bool {
 	{{.Cursor}}
 })`,
 }, {
-	name:   "slice_last",
-	filter: isSLice,
-	label:  "last",
-	body:   "{{.Obj}}[len({{.Obj}})-1]",
+	name:    "slice_last",
+	filter:  isSLice,
+	label:   "last",
+	body:    "{{.Obj}}[len({{.Obj}})-1]",
+	details: "Access the last item in this slice",
 }, {
-	name:   "slice_reverse",
-	filter: isSLice,
-	label:  "reverse",
+	name:    "slice_reverse",
+	filter:  isSLice,
+	label:   "reverse",
+	details: "Reverse this slice (in-place)",
 	body: `for i, j := 0, len({{.Obj}})-1; i < j; i, j = i+1, j-1 {
 	{{.Obj}}[i], {{.Obj}}[j] = {{.Obj}}[j], {{.Obj}}[i]
 }`,
 }, {
-	name:   "slice_range",
-	filter: isSLice,
-	label:  "range",
+	name:    "slice_range",
+	filter:  isSLice,
+	label:   "range",
+	details: "Iterate over this slice",
 	body: `for i, {{index .Vars 0}} := range {{.Obj}} {
 	{{.Cursor}}
 }`,
 }, {
-	name:   "slice_append",
-	filter: and(isSLice, isAssignable),
-	label:  "append",
-	body:   `{{.Obj}} = append({{.Obj}}, {{.Cursor}})`,
+	name:    "slice_append",
+	filter:  and(isSLice, isAssignable),
+	label:   "append",
+	details: "Append to this slice",
+	body:    `{{.Obj}} = append({{.Obj}}, {{.Cursor}})`,
 }, {
-	name:   "map_range",
-	filter: isMap,
-	label:  "range",
+	name:    "map_range",
+	filter:  isMap,
+	label:   "range",
+	details: "Iterate over this map",
 	body: `{{$v := .Vars}}for {{index $v 0}}, {{index $v 1}} := range {{.Obj}} {
 	{{.Cursor}}
 }`,
 }, {
-	name:   "map_clear",
-	filter: isMap,
-	label:  "clear",
+	name:    "map_clear",
+	filter:  isMap,
+	label:   "clear",
+	details: "Clear this map",
 	body: `{{$k := (index .Vars 0)}}for {{$k}} := range {{.Obj}} {
 	delete({{.Obj}}, {{$k}})
 }
 	`,
 }, {
-	name:   "map_keys",
-	filter: isMap,
-	label:  "keys",
+	name:    "map_keys",
+	filter:  isMap,
+	label:   "keys",
+	details: "Create a slice of all the keys in this map",
 	body: `keys := make([]{{index .Types 0}}, 0, len({{.Obj}}))
 {{$k := (index .Vars 0)}}for {{$k}} := range {{.Obj}} {
 	keys = append(keys, {{$k}})
@@ -145,24 +154,28 @@ var postfixRules = []postfixRule{{
 	name:    "print_scalar",
 	filter:  isScalar,
 	label:   "print",
+	details: "Print the %v of this variable",
 	imports: []string{"fmt"},
 	body:    `{{.Import "fmt"}}.Printf("%v\n", {{.Obj}})`,
 }, {
 	name:    "print_multi",
 	filter:  isMulti,
 	label:   "print",
+	details: "Print all of these",
 	imports: []string{"fmt"},
 	body:    `{{.Import "fmt"}}.Println({{.Obj}})`,
 }, {
 	name:    "warn_scalar",
 	filter:  isScalar,
 	label:   "warn",
+	details: "Print this to stderr",
 	imports: []string{"fmt", "os"},
 	body:    `{{.Import "fmt"}}.Fprintf({{.Import "os"}}.Stderr, "%v\n", {{.Obj}})`,
 }, {
 	name:    "warn_multi",
 	filter:  isMulti,
 	label:   "warn",
+	details: "Print all these to stderr",
 	imports: []string{"fmt", "os"},
 	body:    `{{.Import "fmt"}}.Fprintln({{.Import "os"}}.Stderr, {{.Obj}})`,
 }}
@@ -341,6 +354,8 @@ Rules:
 			Kind:                protocol.SnippetCompletion,
 			snippet:             snip,
 			AdditionalTextEdits: edits,
+			Documentation:       snip.String(),
+			Detail:              rule.details,
 		})
 	}
 }

@muirdm
Copy link

muirdm commented Dec 20, 2020

I've updated the cl to push more logic into the completion templates. This way gopls can theoretically support dynamic user supplied postfix completion templates specific to the user's code base. The template framework probably needs a few more facilities to make this actually useful (for example a way to test for a specific type, e.g. {{if eq .Type "myproject/somepkg.SomeType"}}).

I also added basic "details" description for each completion. I don't think including the snippet text as the documentation is a good idea since that would include inscrutable snippet meta information, depending on the snippet.

TODOs:

  • duplicate object names in "vars"
  • inconsistent indent insertion in different editors
  • type aliases probably don't do the right thing
  • tests

@stamblerre can you take a preliminary look and either bless or condemn the general approach?

@gopherbot
Copy link

Change https://golang.org/cl/272586 mentions this issue: internal/lsp/source/completion: add postfix snippet completions

@A-UNDERSCORE-D
Copy link
Author

Built, I'll play with it sometime when I next write go.

And yeah its very much not for everyone, its just how my thought process works, I'm typing as I think, so "use x and do ..." is where I'm going

@muirdm
Copy link

muirdm commented Feb 24, 2021

@stamblerre This is what I see in VSCode:

Screen Shot 2021-02-24 at 8 54 20 AM

These are my settings
{
    "window.zoomLevel": 0,
    "go.useLanguageServer": true,
    "[go]": {
        "editor.suggest.snippetsPreventQuickSuggestions": false,
        "editor.formatOnSave": true,
        "editor.codeActionsOnSave": {
         "source.organizeImports": true,
        }
    },
    "gopls": {
        "usePlaceholders": true, // add parameter placeholders when completing a function

        // Experimental settings
        "completeUnimported": true, // autocomplete unimported packages
    },

    "go.languageServerFlags": [
        "-rpc.trace", // for more detailed debug logging

        "-logfile", "/tmp/vscode.log",
    ],
}

VSCode 1.53.2
Go extension 0.22.1

@stamblerre
Copy link
Contributor

stamblerre commented Feb 24, 2021

Welp, it would help if I removed "editor.snippetSuggestions": "none" from my config. 😬 Thank you for following up though. This is a pretty awesome feature!

@stamblerre
Copy link
Contributor

From golang/vscode-go#1278, there are some more possibilities in https://github.com/yokoe/vscode-postfix-go.

@A-UNDERSCORE-D
Copy link
Author

Yep! ive used those for a bit but as they don't have much needed context of what they're working around, it adds noise to your completion options. So you end up with being offered len, append, and so on on an int, or some other non collection type. I even built some of my own before as I mentioned in considered alternatives.

gopherbot pushed a commit to golang/tools that referenced this issue Mar 29, 2021
Postfix snippets are artificial methods that allow the user to compose
common operations in an "argument oriented" fashion. For example,
instead of "sort.Slice(someSlice, ...)" a user can expand
"someSlice.sort!". The snippet labels end in "!" to make it clearer
they do something potentially unexpected. The postfix snippets have
low scores so they should not interfere with normal completions.

The snippets are represented (almost) entirely as Go text/template
templates. This way the user can create custom snippets to match their
general preferences or to capture common patterns in their codebase.
There is currently no way for the user to create snippets, but it
could be accomplished with a configuration file, custom LSP command,
or similar.

I started by implementing a variety of snippets to help flesh out the
various facilities needed by the templates. The most interesting
template capabilities are:
 - The ability to import packages as necessary (e.g. "sort" must be
   imported to call sort.Slice()).
 - The ability to generate unique variable names to avoid accidental
   shadowing issues.
 - The ability to weave LSP snippets into the template. Currently,
   only {{.Cursor}} is exposed, which corresponds to the snippet's
   final tab stop.

Briefly, these are the postfix snippets in this commit:
 - foo.sort => sort.Slice(foo, func(...){}) (slices)
 - foo.last => foo[len(foo)-1] (slices)
 - foo.reverse (slices)
 - foo.range => for i, v := range foo {} (slices/maps)
 - foo.append
     This snippet inserts a self-assignment append statement when
     appropriate, otherwise just an append expression.
 - foo.copy creates a copy of a slice
 - foo.clear empties out a map
 - foo.keys creates slice of keys
 - foo().var assigns result value(s) to variables
 - foo.print prints foo to stdout

Some of these are probably not very useful in practice, and I'm sure
there are lots of great ones I didn't think of.

Updates golang/go#39507.

Change-Id: I9ecc748aa79c0d47fa6ff72d4ea671e917a2d5d6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/272586
Run-TryBot: Muir Manders <muir@mnd.rs>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@muirdm
Copy link

muirdm commented Mar 30, 2021

Experimental postfix snippet support is now merged to master. If interested, please update gopls to master, enable the config flag experimentalPostfixCompletions: true and try it out.

@gopherbot
Copy link

Change https://golang.org/cl/317389 mentions this issue: lsp/source: enable experimentalPostfixCompletions by default

gopherbot pushed a commit to golang/tools that referenced this issue May 5, 2021
Leave the options flag so people can disable it for now if needed.

Updates golang/go#39507.

Change-Id: I78bbac157caa18c5d9a8e2ffe1a5c5eba4c6c30f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/317389
Run-TryBot: Muir Manders <muir@mnd.rs>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Trust: Rebecca Stambler <rstambler@golang.org>
Trust: Peter Weinberger <pjw@google.com>
@findleyr
Copy link
Contributor

Since this has been enabled by default for a long time, let's rename the option to "postfixCompletions". We can continue to honor the old option though. I'll send a CL.

At that point, can we close this issue?

@muirdm
Copy link

muirdm commented Jan 10, 2022

let's rename the option to "postfixCompletions"

I've realized in retrospect that isn't a very good name since all completions are postfix. Maybe "postfixSnippets" is better?

@findleyr
Copy link
Contributor

I've realized in retrospect that isn't a very good name since all completions are postfix. Maybe "postfixSnippets" is better?

SGTM. (I was about to suggest "completionSnippets", because it's clearer that that relates to completion, but then it is too easily confused with completionItem.snippetSupport in the LSP).

@findleyr findleyr self-assigned this Jan 10, 2022
@A-UNDERSCORE-D
Copy link
Author

Yeah as in the original message its a name I swiped from my use of them; possibly just contextAwareSnippets?

@findleyr
Copy link
Contributor

It sounds like we're just blocked on a name :)

I think I prefer postfixSnippets: its not immediately clear what it means for a snippet to be "context aware", but a snippet being postfix is evocative. I don't think either is a complete description of the feature (because I don't think that's possible), but postfixSnippets is easier to remember.

I don't feel strongly though.

@muirdm
Copy link

muirdm commented Jan 11, 2022

possibly just contextAwareSnippets?

All gopls completions/snippets are context aware, so I don't think that name sets these snippets apart. The essence of these snippets is that they give you psuedo methods that then do some sort of non-postfix transformation. Another idea might be "snippetMethods".

@findleyr
Copy link
Contributor

Ok, in the interest of not being blocked on a name: does anyone have a (strong) objection to postfixSnippets? If not, we'll go ahead and make the configuration change for v0.8.0. #42107 might be helpful here, to ease the switch to a new name.

@A-UNDERSCORE-D
Copy link
Author

Works for me

@findleyr findleyr modified the milestones: gopls/v0.8.0, gopls/v0.8.1 Feb 21, 2022
@findleyr findleyr modified the milestones: gopls/v0.8.1, gopls/v0.8.2 Mar 7, 2022
@findleyr findleyr modified the milestones: gopls/v0.8.2, gopls/on-deck Mar 24, 2022
@hyangah hyangah added the gopls/completion Issues related to auto-completion in gopls. label May 10, 2022
@arpitjp
Copy link

arpitjp commented Aug 4, 2023

Can autocomplete be added for len as well?

@findleyr findleyr removed their assignment Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest gopls/completion Issues related to auto-completion in gopls. gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

8 participants