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: completions overwrite text after cursor on insert #34011

Open
zikaeroh opened this issue Sep 2, 2019 · 13 comments

Comments

@zikaeroh
Copy link

commented Sep 2, 2019

What did you do?

A constructed example:

package main

import "fmt"

type Tree struct {
	Left  *Tree
	Right *Tree
}

func (t *Tree) DoSomething() {
	fmt.Println(t)
}

func main() {
	t := &Tree{
		Left:  &Tree{},
		Right: &Tree{},
	}

	t.DoSomething()
}

I'd like to replace t.DoSomething() with t.Left.DoSomething(). I start typing Left after the . and accept the completion.

What did you expect to see?

Left gets inserted after I select the completion; everything to the right of my cursor stays.

What did you see instead?

DoSomething is deleted and replaced with Left. I have to retype DoSomething to put it back. This is a reduced example of this happening, and it gets a bit more annoying when you don't realize it was deleted and have to undo and type it all manually (especially if you don't remember what got deleted).

I don't think this behavior matches the completions of other languages, or VS Code's fallback word-level completions (which also just insert without touching anything to the right).

Build info

golang.org/x/tools/gopls v0.1.3
    golang.org/x/tools/gopls@v0.1.4-0.20190830223141-573d9926052a h1:GHSDcXHHvdapqqDYPriYzm7tvh64EQYFlmHI/MvS/yg=
    golang.org/x/sync@v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU=
    golang.org/x/tools@v0.0.0-20190830223141-573d9926052a h1:XAHT1kdPpnU8Hk+FPi42KZFhtNFEk4vBg1U4OmIeHTU=
    golang.org/x/xerrors@v0.0.0-20190717185122-a985d3407aa7 h1:9zdDQZ7Thm29KFXgAX/+yaf3eVbP7djjWp/dXAppNCc=

Built on master just to keep it recent, but happens on 0.1.3 and has on previous versions as well.

Go info

go version go1.12.9 linux/amd64

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jake/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/jake/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/jake/testproj/overwritebug/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build685890683=/tmp/go-build -gno-record-gcc-switches"
@gopherbot

This comment has been minimized.

Copy link

commented Sep 2, 2019

Thank you for filing a gopls issue! Please take a look at the Troubleshooting section of the gopls Wiki page, and make sure that you have provided all of the relevant information here.

@gopherbot gopherbot added the gopls label Sep 2, 2019

@zikaeroh

This comment has been minimized.

Copy link
Author

commented Sep 2, 2019

Not related, but I think it's a bit funny to have gopherbot yell at me for submitting an issue using gopls bug. 😄

@zikaeroh zikaeroh changed the title gopls: completions overwrite text after cursor on insert x/tools/gopls: completions overwrite text after cursor on insert Sep 2, 2019

@gopherbot gopherbot added this to the Unreleased milestone Sep 2, 2019

@stamblerre

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

Thanks for reporting. Filed #34042 for the GopherBot issue.

@stamblerre

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

/cc @muirrn

Edit: Sorry, wrong Muir again!

@muirrn

This comment has been minimized.

Copy link

commented Sep 5, 2019

There is definitely something to improve here, but I'm not sure changing it to never overwrite the postfix is the solution.

In general we can't be sure if your intent is to prepend something new or to replace entirely. In your example, you might instead want to replace t.DoSomething() with t.LeftyRighty(), in which case the current behavior of replacing the entire identifier is what you want.

It also would be a bit weird for gopls to leave an invalid identifier after performing a completion (e.g. leaving t.LeftDoSomething() per your expected behavior). Note that it currently leaves t.Left() which is also invalid.

On master, deep completions give you another option. In your example, I get a completion candidate for Left.DoSomething, which when selected results in t.Left.DoSomething().

A maybe good idea is to augment the deep completion matching to take into account the postfix. In your example t.L<>DoSomething() the prefix "L" matches "Left", but we could see the postfix "DoSomething" matches really well to "Left.DoSomething", so we present the deep candidate "Left.DoSomething" ranked above the non-deep candidate "Left". I feel like this approach is pretty flexible and could let people do something cool things once they get a feel for how it works.

One other workaround is to insert an extra "." (or space) before completing, i.e. t.L<>.DoSomething().

@zikaeroh

This comment has been minimized.

Copy link
Author

commented Sep 5, 2019

Personally, I don't find leaving invalid code after a completion "wrong", at least not in the course of editing. Every editor/IDE I've used has the same behavior, where a completion will insert what you asked it to, then you're left to finish it off (including Go completion, pre-gopls). Overwriting the whole token to the right is unexpected and I spend more time trying to undo things than the benefit I might get when it does the right thing. I get that gopls itself has no idea what the intent is when a user types something, but my guess says that deleting code is usually not what the user wants to do when accepting a completion (and LSP initially only supporting insertText with TextEdit later is an indicator).

Deep completions are something I've never seen until gopls (and they're interesting, if not sometimes offputting; I've been using master for a while now), but I feel like using them as the solution to this problem is not entirely correct, since there are cases where there wouldn't be a match. I constructed this case to be short enough to show the overwrite, but it doesn't really represent a piece of code I've ever written.

Inserting an extra . is certainly a workaround that prevents the issue. I'll see if I can remember to make a note the next time this happens to me to show it in a non-constructed light.

@zikaeroh

This comment has been minimized.

Copy link
Author

commented Sep 5, 2019

At the least, it'd be nice to have an option to force gopls to use insert-only edits, just to have it match everything else my editor is doing and not trip me up all the time...

@muirrn

This comment has been minimized.

Copy link

commented Sep 5, 2019

Every editor/IDE I've used has the same behavior, where a completion will insert what you asked it to

We may very well end up just going with this behavior, but I'd like see if there is a smarter approach that does what the user wants and saves time/keystrokes vs the status quo.

using [deep completions] ... is not entirely correct, since there are cases where there wouldn't be a match

Can you give an example where you don't want to overwrite but the postfix doesn't match a deep completion?

Deep completions are .. sometimes offputting

Can you elaborate on this? I'm very interested in feedback regarding deep completions.

@zikaeroh

This comment has been minimized.

Copy link
Author

commented Sep 6, 2019

Can you give an example where you don't want to overwrite but the postfix doesn't match a deep completion?

The most obvious case is when there's a method call in the middle. The example I gave really only works because it's a single extra thing and is a member access. I'd have to go looking for a better example, as I wrote down this bug to report (and then waited a bit too long before sending it in...)

Can you elaborate on this? I'm very interested in feedback regarding deep completions.

I'd have to sit down and code and see if I can produce any specific examples, but my main issue has been that they've almost never been what I wanted, and seemed to appear at the top in some cases.

An odd case I can come up with from playing around is when you're just starting to type the package name, and it's suggesting stuff inside the package before you've finished it at random. For example, if I start to type opentracing, I'll get a completion item for opentracing the package, but then 3 random members of opentracing that change depending on the number of characters of opentracing I've finished typing... It's weird.

Another thing is that it seems like gopls will try to pick the "best" completion item based on an expected type, so the deep completions will often show up first, even though I'm much less likely to want to access something that's a deep completion away.

@muirrn

This comment has been minimized.

Copy link

commented Sep 6, 2019

The most obvious case is when there's a method call in the middle

Not sure I understand, exactly. Hopefully you come across a good example you can share.

if I start to type opentracing, I'll get a completion item for opentracing the package, but then 3 random members of opentracing that change depending on the number of characters of opentracing I've finished typing

It is true that the deep completions in that case are a bit arbitrary, but it is convenient if, for example, you are trying to call opentracing.DoSomething() you can probably just type "DoSo" or "otds" or maybe just "ds" and complete straight to what you want. If you don't know what you want, "opentracing" the package is still the first candidate, like you said. But, it sounds like the deep candidates are still distracting to some degree. We can probably do a better job filtering out low quality candidates.

gopls will try to pick the "best" completion item based on an expected type, so the deep completions will often show up first, even though I'm much less likely to want to access something that's a deep completion away.

Deep candidates will always be ranked below non-deep candidates, so if deep candidates show up first it means either there are no non-deep candidates that match the type, or your search prefix matches the deep candidates much better than the non-deep candidates. Do you have an example where a deep candidate you did not want was ranked higher than a non-deep candidate you did want?

The general goal is deep candidates never get in your way and often save a lot of typing. If you have examples where they slowed you down I'd love to see so I can improve things.

@stamblerre stamblerre added the Tools label Sep 9, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Sep 18, 2019

Change https://golang.org/cl/196119 mentions this issue: internal/lsp: don't overwrite suffix when completion prefix empty

@zikaeroh

This comment has been minimized.

Copy link
Author

commented Sep 18, 2019

@muirrn Forgive me for not commenting on gerrit (haven't used it yet), but wouldn't that CL not fix this issue per se? It says that it only works if the prefix is empty, which to me seems like will never be the case in normal usage, as presumably one would want to type a few characters to select the right completion rather than using no prefix and arrowing through until the right selection is found. I get that it's a middle ground, but it's not exactly what I thought would happen.

Forgive me for not giving more feedback; I've been busy and haven't had any interesting thoughts about deep completion to share.

@muirrn

This comment has been minimized.

Copy link

commented Sep 18, 2019

Yes, you are right of course. Let me try again later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.