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: cannot send false responses in capabilities #46052

Closed
glennsarti opened this issue May 8, 2021 · 6 comments
Closed

x/tools/gopls: cannot send false responses in capabilities #46052

glennsarti opened this issue May 8, 2021 · 6 comments

Comments

@glennsarti
Copy link

@glennsarti glennsarti commented May 8, 2021

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

$ go version
go version go1.15.8 windows/amd64

Does this issue reproduce with the latest release?

Probably

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

go env Output
$ go env

set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\glenn\AppData\Local\go-build
set GOENV=C:\Users\glenn\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\glenn\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\glenn\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Source\oss\puppet-editor-services-go\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\glenn\AppData\Local\Temp\go-build835385066=/tmp/go-build -gno-record-gcc-switches

What did you do?

I'm working on implementing a Language Server in Go (but not for golang itself). I was trying send a response from Lang. Server to the Language client to indicate I did not support many of the providers, e.g. CodeLens however the JSON response was not showing the false entries. e.g.

When I use the following response in Go.

		Capabilities: protocol.ServerCapabilities {
			FoldingRangeProvider: true,
			TextDocumentSync: protocol.ParseTextDocumentSyncKind("Full"),
			DocumentLinkProvider: protocol.DocumentLinkOptions{
				ResolveProvider: false,
			},
		},
	}

When it is converted to JSON, the output just ignores it

{{"capabilities":{"textDocumentSync":1,"completionProvider":{},"signatureHelpProvider":{},"codeLensProvider":{},"documentLinkProvider":{},"documentOnTypeFormattingProvider":{"firstTriggerCharacter":""},"foldingRangeProvider":true,"executeCommandProvider":{"commands":null},"workspace":{"workspaceFolders":{}}},"serverInfo":{"name":"Puppet Language Server in Go","version":"0.0.1"}}

Note the documentLinkProvider":{} . It should have { resolveProvider: false}

What did you expect to see?

It should out the response fully in JSON

What did you see instead?

It ignores bools with false

This is probably due to:
https://github.com/golang/tools/blob/master/internal/lsp/protocol/tsprotocol.go#L1739

ResolveProvider bool `json:"resolveProvider,omitempty"`

This could (probably) be solved using *bool but as this file is automatically generated, this seems like a fix that needs to be made in the tools.

Now, while this code is internal and my usecase is not normal, I did notice that gopls will set a false value and this will not be sent to the Language client. And would be considered a bug.

https://github.com/golang/tools/blob/3e17c62e37b686d70c24a370a51cd33f8aa52df2/internal/lsp/general.go#L150

@gopherbot gopherbot added the gopls label May 8, 2021
@stamblerre stamblerre added this to the Unreleased milestone May 10, 2021
@findleyr findleyr changed the title gopls cannot send false responses in capabilities x/tools/gopls: cannot send false responses in capabilities May 10, 2021
@findleyr
Copy link
Contributor

@findleyr findleyr commented May 10, 2021

Thanks for the report.

And would be considered a bug.

I'm not sure this is a bug. Per the spec, "A missing property should be interpreted as an absence of the capability.". Is there a case where the client would actually interpret incorrect capabilities as a result of the json:"omitempty" tag?

@glennsarti
Copy link
Author

@glennsarti glennsarti commented May 12, 2021

Hi @findleyr

Sure a couple of ideas come to mind....

"A missing property should be interpreted as an absence of the capability."

That referred the to the Client Capabilities from the client to the server. I couldn't find the same for the Server Capabilities.

On searching through the spec I couldn't see any booleans that defaulted to true if omitted. But I could've missed one...

That said, I can see that what I was actually seeing was the server capabilities for things like documentOnTypeFormattingProvider. There didn't appear to be a way to not have that in the JSON response. Same with executeCommandProvider and bits that used a struct.

https://github.com/golang/tools/blob/master/internal/lsp/protocol/tsprotocol.go#L3603

Unless I'm doing something completely wrong 🤷‍♂️

@findleyr
Copy link
Contributor

@findleyr findleyr commented May 12, 2021

That referred the to the Client Capabilities from the client to the server

Yes that part of the spec referenced client capabilities, but absent any commentary on server capabilities (which is directly below), I assume the same applies to server capabilities. The spec tends not to have a lot of details, unfortunately.

On searching through the spec I couldn't see any booleans that defaulted to true if omitted. But I could've missed one...

I don't think there can be any basic capabilities that can default to true, as per the wording missing configuration=>no capability.

I agree that it would be cleaner if these were all pointers, but I don't think it's a bug per-se. I think the zero-value configuration has the correct semantics when marshaled to JSON.

CC @pjweinb who knows more of the history here, and might have a different opinion.

@pjweinb
Copy link

@pjweinb pjweinb commented May 12, 2021

@stamblerre stamblerre modified the milestones: Backlog, gopls/unplanned May 12, 2021
@glennsarti
Copy link
Author

@glennsarti glennsarti commented May 15, 2021

Thanks both of you!

I'm happy to close this issue and raise one about the non-nil-able resolvers (like completionProvider) which can't seem to be turned off.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented May 17, 2021

Sure sounds good--please raise a new issue if needed.

@stamblerre stamblerre closed this May 17, 2021
@stamblerre stamblerre removed this from the gopls/unplanned milestone May 17, 2021
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
5 participants