Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

LSP-conformant originalRootURI #369

Closed
wants to merge 1 commit into from

Conversation

chrismwendt
Copy link
Contributor

@chrismwendt chrismwendt commented May 9, 2019

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 sourcegraph/sourcegraph-go#28

// `InitializeParams.intitializationOptions` if absent from the
// `InitializeParams`. This makes go-langserver more LSP-compliant so that
// clients don't need to go outside of the LSP specification to communicate
// with it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is still not really compliant from a semantics perspective because initializationOptions is intended to be options you would pass through CLI arguments, i.e. not values that change for every workspace.

@felixfbecker
Copy link

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
Copy link
Contributor Author

Here are a few examples:

langInitParams.RootURI = lsp.DocumentURI("file://" + rootPath)

if !util.IsURI(rootURI) {
return fmt.Errorf("invalid root path %q: must be file:// URI", rootURI)
}
h.RootFSPath = util.UriToPath(rootURI) // retain leading slash

filename := h.FilePath(fileURI)

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
Copy link
Contributor Author

Closing in favor of #373

@keegancsmith keegancsmith deleted the lsp-conformant-original-root-uri branch May 15, 2019 07:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants