search

sourcegraph / go-langserver

Go language server to add Go support to editors and other tools that use the Language Server Protocol (LSP)
https://sourcegraph.com
MIT License
1.17k stars 90 forks source link

LSP-conformant originalRootURI #369

Closed chrismwendt closed 5 years ago

chrismwendt commented 5 years ago

Background

Because originalRootURI is not part of the LSP specification for InitializeParams, clients cannot use LSP-conformant libraries such as https://github.com/sourcegraph/lsp-client without adding a workaround specifically for communicating with go-langserver.

There is an untyped field, InitializeParams.initializationOptions, which can easily be used to send the originalRootURI.

Details

After this change, go-langserver will read the originalRootURI from InitializeParams.initializationOptions if it's absent from the top level InitializeParams. It will still be compatible with existing clients that currently put originalRootURI in the top level InitializeParams.

One other alternative considered

Alternatively, the InitializeParams.rootURI might be able to carry the current value of originalRootURI because it's currently set to the informationless value of file:/// in sourcegraph-go. However, this change seems difficult to make in go-langserver:

After attempting to move originalRootURI to rootUri for ~2 hours, I'm finding that the assumption that rootUri is file:/// pervades the LSP request handlers, utility functions, and glue code. Changing it will take longer than I had expected and I'm concerned it might introduce bugs despite the pretty good test coverage of go-langserver.

This helps integrate lsp-client into the Go extension https://github.com/sourcegraph/sourcegraph-go/issues/28

felixfbecker commented 5 years ago

After attempting to move originalRootURI to rootUri for ~2 hours, I'm finding that the assumption that rootUri is file:/// pervades the LSP request handlers, utility functions, and glue code. Changing it will take longer than I had expected and I'm concerned it might introduce bugs despite the pretty good test coverage of go-langserver.

Could you give some examples of such assumptions?

chrismwendt commented 5 years ago

Here are a few examples:

https://github.com/sourcegraph/go-langserver/blob/33d2968a49e1131825a0accdd64bdf9901cf144c/buildserver/environment.go#L104

https://github.com/sourcegraph/go-langserver/blob/33d2968a49e1131825a0accdd64bdf9901cf144c/langserver/handler_common.go#L32-L35

https://github.com/sourcegraph/go-langserver/blob/33d2968a49e1131825a0accdd64bdf9901cf144c/langserver/loader.go#L40

Unless this PR looks ok, I'll try moving forward with moving originalRootURI to rootUri tomorrow. Currently, same-repo j2d returns file: URIs whereas cross-repo j2d returns git: URIs, and I think that moving originalRootURI to rootUri will make that uniform so they're all git: URIs, which would simplify the Go Sourcegraph extension a bit and avoid the need for another parameter to the serverToClientURI function in lsp-client.

chrismwendt commented 5 years ago

Closing in favor of https://github.com/sourcegraph/go-langserver/pull/373