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: implement didChangeConfiguration #32258

Closed
zchee opened this issue May 26, 2019 · 9 comments

Comments

@zchee
Copy link
Contributor

@zchee zchee commented May 26, 2019

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

$ go version
go version devel +385b2e0cac Fri May 24 21:34:53 2019 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Might be Yes. It's related LSP protocol spec.

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/zchee/.cache/go-build"
GOENV="/Users/zchee/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/zchee/go"
GOPROXY="https://proxy.golang.org"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang"
CXX="/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++"
CGO_ENABLED="1"
GOMOD="/Users/zchee/go/src/github.com/zchee/nvim-lsp/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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/6_/10hjzx0d027_8rfblcw1lbm40000gp/T/go-build972805647=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

gopls send protocol.RegistrationParams when initialized, the ref is here:

https://github.com/golang/tools/blob/7be61e1b0e514e106d83fc439d56a79143738603/internal/lsp/general.go#L117-L125

But AFAIK, the vscode-languageserver-node/client just ignore if []Registration. RegisterOptions is empty.

I also now develop Language Server Protocol client side for IDE written in Go, but I can't handle that gopls send RegisterCapability call.

What did you expect to see?

empty Registration.RegisterOptions fields RegisterCapability call.

What did you see instead?

non-nil Registration.RegisterOptions.

@gopherbot gopherbot added this to the Unreleased milestone May 26, 2019
@gopherbot gopherbot added the gopls label May 26, 2019
@zchee

This comment has been minimized.

Copy link
Contributor Author

@zchee zchee commented May 26, 2019

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented May 28, 2019

Thanks for catching this! This means that the RegistrationOptions field of the Registration struct should be an *interface{}, not an interface{}. @pjweinb, can you make this change in the TypeScript generator?

@stamblerre stamblerre changed the title x/tools/internal/lsp: misuse `protocol.RegistrationParams` x/tools/internal/lsp: protocol RegistrationOptions should be a pointer May 28, 2019
@stamblerre stamblerre changed the title x/tools/internal/lsp: protocol RegistrationOptions should be a pointer x/tools/internal/lsp: protocol.Registration RegistrationOptions field should be a pointer May 28, 2019
@zchee

This comment has been minimized.

Copy link
Contributor Author

@zchee zchee commented May 30, 2019

@stamblerre
Also, the Registration.ID will use to client/unregisterCapability call.
It would be better to use unique ID such as UUID like LSP spec.

{
	"method": "client/registerCapability",
	"params": {
		"registrations": [
			{
				"id": "79eee87c-c409-4664-8102-e03263673f6f",
				"method": "textDocument/willSaveWaitUntil",
				"registerOptions": {
					"documentSelector": [
						{ "language": "javascript" }
					]
				}
			}
		]
	}
}
@ianthehat

This comment has been minimized.

Copy link

@ianthehat ianthehat commented May 30, 2019

The id only has to be unique within the server (not the client) and is only used if you make an unregister call.
Given that we only register in this function, which only happens once, and we never unregister, the current values meet the requirements for uniqueness.

@pjweinb

This comment has been minimized.

Copy link
Contributor

@pjweinb pjweinb commented May 31, 2019

@zchee

This comment has been minimized.

Copy link
Contributor Author

@zchee zchee commented May 31, 2019

@stamblerre @ianthehat @pjweinb
I'll details reply later, but I understand and totally agreed @ianthehat @pjweinb said.

Anyway, I posted issue first means not "should be a pointer", "misuse RegisterCapability call". I think no needs to change to pointer too. I suggests just fix gopls initialized implements.
Because that's specificication like RFC.

@stamblerre stamblerre changed the title x/tools/internal/lsp: protocol.Registration RegistrationOptions field should be a pointer x/tools/internal/lsp: implement didChangeConfiguration Jun 5, 2019
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jun 5, 2019

I'm sorry, I misunderstood - I now see that we don't correctly implement didChangeConfiguration.

@zchee

This comment has been minimized.

Copy link
Contributor Author

@zchee zchee commented Jun 5, 2019

@stamblerre

I'll details reply later

At first, Sorry for not reply to details comment.

But yeah, RegistrationOptions is optional field on LSP spec, but actually it required param for handle RegisterCapability on client side.
So, we should adding any interface{} value to RegistrationOptions instead of change to pointer.

Thanks for reading my comment!

@stamblerre stamblerre changed the title x/tools/internal/lsp: implement didChangeConfiguration x/tools/gopls: implement didChangeConfiguration Jul 2, 2019
@stamblerre stamblerre added help wanted and removed Suggested labels Aug 8, 2019
@stamblerre stamblerre modified the milestones: Unreleased, gopls v1.0 Aug 26, 2019
myitcv added a commit to govim/govim that referenced this issue Sep 7, 2019
Provide options to disable the now-default fuzzy and/or deep completion
modes.

Whilst we await golang/go#32258, users will
need to set these config options in their .vimrc and then restart Vim.
Even then there is a race condition for Vim8 package users that might
mean even this doesn't work.
myitcv added a commit to govim/govim that referenced this issue Sep 7, 2019
Provide options to disable the now-default fuzzy and/or deep completion
modes.

Whilst we await golang/go#32258, users will
need to set these config options in their .vimrc and then restart Vim.
Even then there is a race condition for Vim8 package users that might
mean even this doesn't work.

Fixes #321
myitcv added a commit to govim/govim that referenced this issue Sep 10, 2019
Provide options to disable the now-default fuzzy and/or deep completion
modes. Also provide a test to ensure that deep fuzzy is the default.

Whilst we await golang/go#32258, users will
need to set these config options in their .vimrc and then restart Vim.
Even then there is a race condition for Vim8 package users that might
mean even this doesn't work.

Fixes #321
myitcv added a commit to govim/govim that referenced this issue Sep 10, 2019
Provide options to disable the now-default fuzzy and/or deep completion
modes. Also provide a test to ensure that deep fuzzy is the default.

Whilst we await golang/go#32258, users will
need to set these config options in their .vimrc and then restart Vim.
Even then there is a race condition for Vim8 package users that might
mean even this doesn't work.

Fixes #321
myitcv added a commit to govim/govim that referenced this issue Sep 11, 2019
Provide options to disable the now-default fuzzy and/or deep completion
modes. Also provide a test to ensure that deep fuzzy is the default.

Whilst we await golang/go#32258, users will
need to set these config options in their .vimrc and then restart Vim.
Even then there is a race condition for Vim8 package users that might
mean even this doesn't work.

Fixes #321
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 8, 2019

Change https://golang.org/cl/206148 mentions this issue: internal/lsp: handle the didChangeConfiguration message

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