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: superfluous {} added to first completion result for named struct type #36655

Closed
myitcv opened this issue Jan 20, 2020 · 12 comments
Closed

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Jan 20, 2020

What version of Go are you using (go version)?

$ go version
go version devel +8e0be05ec7 Sun Jan 19 14:04:09 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200117220505-0cba7a3a9ee9 => github.com/myitcvforks/tools v0.0.0-20200119092928-0fd5434cd1ba
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.8-0.20200117220505-0cba7a3a9ee9 => github.com/myitcvforks/tools/gopls v0.0.0-20200119092928-0fd5434cd1ba

Note that per govim/govim@c7a6cc3, github.com/myitcvforks/tools/gopls@v0.0.0-20200119092928-0fd5434cd1ba is 0cba7a3a with a partial revert of CL214586 on top (the result of which can be seen in https://go-review.googlesource.com/c/tools/+/215239)

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/.vim/plugged/govim/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-build657644987=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Attempted to complete the following type:

https://github.com/govim/govim/blob/0876ba34ac78adec927e745f119759215fac3406/cmd/govim/internal/golang_org_x_tools/lsp/protocol/tsprotocol.go#L2655

i.e. had the following text:

// ...
v.server.SignatureHelp(context.Background(), &protocol.SignatureHelp_)
// ...

with the cursor at the position indicated by _ and then requested Completion.

Got the following results back:

&protocol.CompletionList{
    IsIncomplete: false,
    Items:        {
        {
            Label:            "SignatureHelpParams{}",
            Kind:             6,
            Tags:             nil,
            Detail:           "",
            Documentation:    "",
            Deprecated:       false,
            Preselect:        true,
            SortText:         "00000",
            FilterText:       "SignatureHelpParams{}",
            InsertText:       "",
            InsertTextFormat: 1,
            TextEdit:         &protocol.TextEdit{
                Range: protocol.Range{
                    Start: protocol.Position{Line:93, Character:57},
                    End:   protocol.Position{Line:93, Character:70},
                },
                NewText: "SignatureHelpParams{}",
            },
            AdditionalTextEdits: nil,
            CommitCharacters:    nil,
            Command:             (*protocol.Command)(nil),
            Data:                nil,
        },
        {
            Label:            "SignatureHelp",
            Kind:             22,
            Tags:             nil,
            Detail:           "struct{...}",
            Documentation:    "*\n * Signature help represents the signature of something\n * callable. There can be multiple signature but only one\n * active and only one active parameter.\n",
            Deprecated:       false,
            Preselect:        false,
            SortText:         "00001",
            FilterText:       "SignatureHelp",
            InsertText:       "",
            InsertTextFormat: 1,
            TextEdit:         &protocol.TextEdit{
                Range: protocol.Range{
                    Start: protocol.Position{Line:93, Character:57},
                    End:   protocol.Position{Line:93, Character:70},
                },
                NewText: "SignatureHelp",
            },
            AdditionalTextEdits: nil,
            CommitCharacters:    nil,
            Command:             (*protocol.Command)(nil),
            Data:                nil,
        },
        {
            Label:            "SignatureHelpClientCapabilities",
            Kind:             22,
            Tags:             nil,
            Detail:           "struct{...}",
            Documentation:    "*\n * Client Capabilities for a [SignatureHelpRequest](#SignatureHelpRequest).\n",
            Deprecated:       false,
            Preselect:        false,
            SortText:         "00002",
            FilterText:       "SignatureHelpClientCapabilities",
            InsertText:       "",
            InsertTextFormat: 1,
            TextEdit:         &protocol.TextEdit{
                Range: protocol.Range{
                    Start: protocol.Position{Line:93, Character:57},
                    End:   protocol.Position{Line:93, Character:70},
                },
                NewText: "SignatureHelpClientCapabilities",
            },
            AdditionalTextEdits: nil,
            CommitCharacters:    nil,
            Command:             (*protocol.Command)(nil),
            Data:                nil,
        },
        {
            Label:            "SignatureHelpContext",
            Kind:             22,
            Tags:             nil,
            Detail:           "struct{...}",
            Documentation:    "*\n * Additional information about the context in which a signature help request was triggered.\n *\n * @since 3.15.0\n",
            Deprecated:       false,
            Preselect:        false,
            SortText:         "00003",
            FilterText:       "SignatureHelpContext",
            InsertText:       "",
            InsertTextFormat: 1,
            TextEdit:         &protocol.TextEdit{
                Range: protocol.Range{
                    Start: protocol.Position{Line:93, Character:57},
                    End:   protocol.Position{Line:93, Character:70},
                },
                NewText: "SignatureHelpContext",
            },
            AdditionalTextEdits: nil,
            CommitCharacters:    nil,
            Command:             (*protocol.Command)(nil),
            Data:                nil,
        },
        {
            Label:            "SignatureHelpOptions",
            Kind:             22,
            Tags:             nil,
            Detail:           "struct{...}",
            Documentation:    "*\n * Server Capabilities for a [SignatureHelpRequest](#SignatureHelpRequest).\n",
            Deprecated:       false,
            Preselect:        false,
            SortText:         "00004",
            FilterText:       "SignatureHelpOptions",
            InsertText:       "",
            InsertTextFormat: 1,
            TextEdit:         &protocol.TextEdit{
                Range: protocol.Range{
                    Start: protocol.Position{Line:93, Character:57},
                    End:   protocol.Position{Line:93, Character:70},
                },
                NewText: "SignatureHelpOptions",
            },
            AdditionalTextEdits: nil,
            CommitCharacters:    nil,
            Command:             (*protocol.Command)(nil),
            Data:                nil,
        },
        {
            Label:            "SignatureHelpParams",
            Kind:             22,
            Tags:             nil,
            Detail:           "struct{...}",
            Documentation:    "*\n * Parameters for a [SignatureHelpRequest](#SignatureHelpRequest).\n",
            Deprecated:       false,
            Preselect:        false,
            SortText:         "00005",
            FilterText:       "SignatureHelpParams",
            InsertText:       "",
            InsertTextFormat: 1,
            TextEdit:         &protocol.TextEdit{
                Range: protocol.Range{
                    Start: protocol.Position{Line:93, Character:57},
                    End:   protocol.Position{Line:93, Character:70},
                },
                NewText: "SignatureHelpParams",
            },
            AdditionalTextEdits: nil,
            CommitCharacters:    nil,
            Command:             (*protocol.Command)(nil),
            Data:                nil,
        },
        {
            Label:            "SignatureHelpTriggerKind",
            Kind:             7,
            Tags:             nil,
            Detail:           "float64",
            Documentation:    "*\n * How a signature help was triggered.\n *\n * @since 3.15.0\n",
            Deprecated:       false,
            Preselect:        false,
            SortText:         "00006",
            FilterText:       "SignatureHelpTriggerKind",
            InsertText:       "",
            InsertTextFormat: 1,
            TextEdit:         &protocol.TextEdit{
                Range: protocol.Range{
                    Start: protocol.Position{Line:93, Character:57},
                    End:   protocol.Position{Line:93, Character:70},
                },
                NewText: "SignatureHelpTriggerKind",
            },
            AdditionalTextEdits: nil,
            CommitCharacters:    nil,
            Command:             (*protocol.Command)(nil),
            Data:                nil,
        },
    },
}

Selected the first entry, labelled SignatureHelpParams{}, to complete completion.

What did you expect to see?

&protocol.SignatureHelpParams

What did you see instead?

&protocol.SignatureHelpParams{}

Note the additional {}. I was expecting to have to type a { to then fill out some fields.

Perhaps this is intentional, so that the completed value is then valid for the context? If so, I wonder whether we can do better here by specifying two edits to leave the cursor in between the { and }?


cc @stamblerre @muirdm

FYI @leitzler

@myitcv myitcv added this to the gopls/v0.3.0 milestone Jan 20, 2020
@gopherbot gopherbot added the Tools label Jan 20, 2020
@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Jan 20, 2020

Do you have snippets turned on? It should be leaving the cursor in the curlies unless the struct has no visible fields.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 20, 2020

Do you have snippets turned on

No.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 20, 2020

Oops, that sent too early.

It should be leaving the cursor in the curlies unless the struct has no visible fields.

I'm not sure I see how that's possible given the results sent back because there's just a single text edit.

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Jan 20, 2020

Snippets are required to place the cursor in different spots. With snippets on, you will get insert text something like SignatureHelpParams{$0}, where $0 indicates the final position of the cursor.

Perhaps literal completions that use snippets should be disabled if the client doesn't have snippet support enabled. It is certainly counter productive to leave the cursor in an annoying position.

Out of curiosity, do you have snippets disabled intentionally, or does your client not support them?

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 20, 2020

It is certainly counter productive to leave the cursor in an annoying position.

Agreed.

Out of curiosity, do you have snippets disabled intentionally, or does your client not support them?

Both :) We still need to add this support in govim: govim/govim#698

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Jan 21, 2020

There are two easy options:

  1. Disable "literal" completions if the client doesn't support snippets. The downside to this is other useful literal completions would no longer work. For example, you won't get a completion candidate make(map[some]map[really][]longType) to help you construct a long map type, or get func(i, j int) bool {} completed automatically when calling sort.Slice().

  2. Do nothing. The downside to this is some amount of annoyance due to cursor position.

Personally I find literal candidates very useful and would still want to use them even if my editor didn't support snippets, so I vote for 1).

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 21, 2020

Personally I find literal candidates very useful and would still want to use them even if my editor didn't support snippets, so I vote for 1).

Agreed. We need to get snippet support in soon so we can also take advantage of this. But agree that in the meantime disabling "literal" completions feels like the best solution if the client doesn't support snippets.

@stamblerre stamblerre modified the milestones: gopls/v0.3.0, gopls/v1.0.0 Jan 21, 2020
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 24, 2020

@stamblerre I'd like to suggest that this be moved to the v0.3.0 milestone. I think I'm right in saying that option 1 suggested by @muirdm in #36655 (comment) is relatively straightforward. Not all editor plugins support snippets, but beyond that not everyone wants snippets. So to have these superfluous and awkward completion candidates will be annoying.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 24, 2020

Personally I find literal candidates very useful and would still want to use them even if my editor didn't support snippets, so I vote for 1).

@muirdm: Did you mean 2) in that comment? Because I tend to agree that I would rather have the literal candidates than not.

We have a flag to disable literal candidates, but we don't expose it to the user right now. @myitcv: If we added the ability for the user to disable literal completions, would that be sufficient?

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 24, 2020

If we added the ability for the user to disable literal completions, would that be sufficient?

Yes, that would work. But per @muirdm's comment, I don't think it makes sense to return literal candidates if the client doesn't support snippets, because the cursor ends up in the wrong place.

However I'll defer to you both on this - I don't really have that much experience of the various options in this space.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 24, 2020

While I would prefer to leave them in because I do find them very useful, I think it's probably easiest to disable them for now (if the client doesn't support snippets). It also allows us to delete the Literal configuration :) I'll mail a CL to that effect.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 24, 2020

Change https://golang.org/cl/216299 mentions this issue: internal/lsp: disable literal completion candidates for some clients

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.