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: completion for struct literal inserts package name even if already in prefix #34872

Closed
zikaeroh opened this issue Oct 13, 2019 · 14 comments
Labels
Milestone

Comments

@zikaeroh
Copy link

@zikaeroh zikaeroh commented Oct 13, 2019

What did you do?

In one package:

package some

type Struct struct{}

In another:

package main

import "some"

func main() {
	var s *some.Struct
	_ = s

	s = &some.
}

There will be a completion for some.Struct{} in the list.

What did you expect to see?

When selected, the completion expands to give the final code s = &some.Struct{}

What did you see instead?

Accepting the completion results in s = &some.some.Struct{}.

Build info

golang.org/x/tools/gopls v0.1.6
    golang.org/x/tools/gopls@v0.0.0-20191012152004-8de300cfc20a h1:DJK0vkrEsRdxY21k0L5kHOoZZMhkg6kzdSLADjKmflM=
    github.com/BurntSushi/toml@v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
    github.com/sergi/go-diff@v1.0.0 h1:Kpca3qRNrduNnOQeazBd0ysaKrUJiIuISHxogkT9RPQ=
    golang.org/x/sync@v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU=
    golang.org/x/tools@v0.0.0-20191012152004-8de300cfc20a h1:TwMENskLwU2NnWBzrJGEWHqSiGUkO/B4rfyhwqDxDYQ=
    golang.org/x/xerrors@v0.0.0-20190717185122-a985d3407aa7 h1:9zdDQZ7Thm29KFXgAX/+yaf3eVbP7djjWp/dXAppNCc=
    honnef.co/go/tools@v0.0.1-2019.2.3 h1:3JgtbtFHMiCmsznwGVTUWbgGov+pVqnlf1dEJTNAXeM=

Go info

go version go1.13.1 linux/amd64

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jake/.cache/go-build"
GOENV="/home/jake/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/jake/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/jake/zikaeroh/hortbot/hortbot/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-build070793326=/tmp/go-build -gno-record-gcc-switches"
@gopherbot gopherbot added this to the Unreleased milestone Oct 13, 2019
@zikaeroh zikaeroh changed the title x/tools/gopls: completion for deep literal inserts package name even if already specified x/tools/gopls: completion for struct literal inserts package name even if already in prefix Oct 13, 2019
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Oct 15, 2019

/cc @muirrn

@golang golang deleted a comment from gopherbot Oct 15, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 15, 2019

Change https://golang.org/cl/201200 mentions this issue: internal/lsp: don't qualify literal candidates in *ast.SelectorExpr

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Oct 15, 2019

Thanks! Let me know if the above CL doesn't resolve it for you.

@zikaeroh

This comment has been minimized.

Copy link
Author

@zikaeroh zikaeroh commented Oct 15, 2019

Not quite. A corner case using my example above is s = some. which lists &Struct{} and completes to some.&Struct{}. Would be nicer if the completion edit prepended a & to the selector expression to correct the code, but not including a & after a dot and leaving it to the coder is also fine.

I realized that I worded my original issue in the form of "this completion inserts the wrong thing". However, I'm noticing that s = &some. lists both completions for Struct{} and Struct. I'm assuming that this duplication is intended, which is fine, I only mention it as this issue could also be framed as a completion showing up more than once where one doesn't make sense in the context.

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Oct 15, 2019

Just accept the completion before you type the dot! 😛

Would be nicer if the completion edit prepended a & to the selector expression to correct the code

That would be cool, but we don't do anything like that yet. I'll play with it a bit.

not including a & after a dot and leaving it to the coder is also fine

I'd prefer not to do that since it amounts to inserting an "invalid" completion. Maybe better than nothing, though.

s = &some. lists both completions for Struct{} and Struct

That is intentional for now. The "literal" completions are in addition to the normal completions. There isn't much of a downside to including maybe useless candidates, so we err on the side of including things.

@zikaeroh

This comment has been minimized.

Copy link
Author

@zikaeroh zikaeroh commented Oct 15, 2019

Just accept the completion before you type the dot! 😛

I don't get this sort of literal completion without fully typing the package name and dot, at least not immediately. Basically, I can do the equivalent of s = & and start typing, and see loads of completions which wouldn't typecheck, then if I return to do it again I'll see the literal at the top. I'd screen-record this, but it's not simple to do on Wayland... I already can't get screenshots of tooltips due to focus stealing. 🙁

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Oct 15, 2019

I return to do it again I'll see the literal at the top

Ah, sounds like the package wasn't imported the first time. I was playing with something that would deep search into unimported packages, but that resulted in too many junk candidates. However, it seems like we could at least search into the expected type's package, even if it isn't imported yet.

@zikaeroh

This comment has been minimized.

Copy link
Author

@zikaeroh zikaeroh commented Oct 15, 2019

No, the package is imported. I haven't been taking about unimported completions, just ones solely from existing imports. I'm busy at the moment, but I'll try and show you how to reproduce it in my real project.

@zikaeroh

This comment has been minimized.

Copy link
Author

@zikaeroh zikaeroh commented Oct 15, 2019

What I've been doing to test this is by going to this part of the code: https://github.com/hortbot/hortbot/blob/ec47ba4e9d9a5fefddf74c681ce4f003d2feac18/internal/bot/cmd_repeat.go#L99

then adding in repeat = &models.RepeatedCommand{} in various forms. What I see is that I type out repeat = &m, then what I see in the completion list (at least at the top) is not &models.RepeatedCommand{} (which is the only thing that fits type-wise). However, if I come back later, I can start typing again (or Ctrl-Space to trigger a completion), and then the literal will have been popped to the top.

It's a big project, but you shouldn't need to do anything other than the above. Hopefully that helps, though I think I've sorta derailed this from the original prefix issue... 😃

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Oct 15, 2019

I updated https://go-review.googlesource.com/c/tools/+/201200/, can you try it again?

What I see is that I type out repeat = &m, then what I see in the completion list (at least at the top) is not &models.RepeatedCommand{}

Seems to work fine for me in Emacs :)

repeat

I think you might be hitting microsoft/vscode#64367 (comment)

@zikaeroh

This comment has been minimized.

Copy link
Author

@zikaeroh zikaeroh commented Oct 16, 2019

Had the chance to check, and I think the CL is working well.

Agreed on the VS Code issue (it wasn't related anyway).

gopherbot pushed a commit to golang/tools that referenced this issue Oct 16, 2019
Previously we unconditionally qualified literal candidate types with
their package. For example:

var buf *bytes.Buffer
buf = &bytes.Bu<>

would complete to:

buf = &bytes.bytes.Buffer{}

Now we don't qualify the type if the cursor position is in the
selector of an *ast.SelectorExpr. We only generate literal candidates
for type names, so if we are in a selector then we can assume it is a
package qualified type (as opposed to an object field).

We also handle the insertion of "&" for literal pointers better. If you are in
the selector of an *ast.SelectorExpr, we prepend the "&" to the beginning of the
expression rather than the selector. For example, you will end up with
"&bytes.Buffer{}" instead of "bytes.&Buffer{}".

Updates golang/go#34872.

Change-Id: I812aa809cd4e649a429853386789f80033412814
Reviewed-on: https://go-review.googlesource.com/c/tools/+/201200
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@zikaeroh

This comment has been minimized.

Copy link
Author

@zikaeroh zikaeroh commented Oct 17, 2019

Any reason that CL was an "Updates" rather than "Fixes"?

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Oct 17, 2019

I didn't want to jump to conclusions since I keep missing things. If everything looks good, please close it out.

@zikaeroh

This comment has been minimized.

Copy link
Author

@zikaeroh zikaeroh commented Oct 17, 2019

As far as I can tell from my usage, this is fixed (and the & TextEdit is very neat). Thanks for the fix!

@zikaeroh zikaeroh closed this Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.