support the workspace/configuration request

[euclio]

This is needed to send configuration to rust-analyzer and the TexLab language server.

Fixes #923.

[martskins]

I think this makes sense. Could you fix the linting issues that were raised in the CI job? I'll try your PR out today and tomorrow and merge after the lint errors are fixed. 👍

[euclio]

I realized I was merging this PR into the wrong branch, which is why the lint errors were showing up. Fixed.

[martskins]

This is breaking gopls (the only one I tried so far) for some reason. I still haven't dived in to investigate what's going on, but figured I'd share my finding to let you know why this hasn't been merged yet.

[euclio]

I'm not familiar with gopls, but it sounds like there might be some discrepancy between how it and rust-analyzer handle configuration: golang/go#38819

[euclio]

Yes, this is an instance of microsoft/language-server-protocol#972.

[martskins]

Too bad that discrepancy exists there. I suppose we should wait until that is resolved then, as it could mean breaking the client with some servers.

[ccope]

Could it be merged but only enabled with a config flag? It seems useful for some users now, and could be enabled only for file types with compatible servers.

[martskins]

I guess we could add a config flag for this and have it disabled by default. If you are happy to add that I think we can make this work @euclio .

Also, left a comment with a change I would make.

[martskins]

I was browsing the code for rust-analyzer and it seems like it actually behaves the same as gopls. It expects the initialization options to be under a keyed object with the name of the language server. See https://github.com/rust-analyzer/rust-analyzer/blob/bf84e4958ee31c59e5b78f60059d69a73ef659bb/editors/code/src/client.ts#L40

So I think we're just not picking the correct settings there. We should modify our initialize request to fix this, as it makes workspace/configuration make sense when it sends a section with the name of the server.

I can open a PR to fix that, it will introduce a breaking change, but I suppose it's for a greater good.

EDIT: This is a lot trickier than expected, because by the time we issue the initialiize request we don't know the name of the server, so we can't fetch the settings for it. Maybe we want to fetch the section initializationOptions when the workspace/configuration request requests a section with the same name as the server? Seems a little nasty, but it's an option I suppose, and we don't break compatibility.

[martskins]

Added a new comment. I'm not sure how this would work for you though, as I suppose it would suffer from the same issue that gopls and rust-analyzer suffer with our plugin. Which is that the initializationOptions are not really in the same form as the settings to be read when the server requests configuration via workspace/configuration. It might takes us a little longer to merge this now that we're aware of that issue to be honest.

EDIT: Just saw you addressed my comment already.

[martskins]

@euclio See #1116, where I propose a way to properly handle initializationOptions, which would enable us to merge this without breaking anything.

[martskins]

@euclio I'm planning to merge that PR I linked above (#1116). If you are happy to rebase this one to that one, and adjust this so that it uses State::initialization_options instead of self.workspace_settings, I can merge this as soon as the other one gets merged. I have tested this already and it seems to work just fine.

[euclio]

Rebased.

[martskins]

@euclio Sorry to bother again, just merged that branch to dev, but it seems like this needs rebasing again now. Happy to do it myself, but just sending the notice in case you prefer to do it yourself 😁 .

[euclio]

Happy to do it, but I won't be able to get to it until tonight.

[martskins]

Yeah no worries! Any time is fine 👍

[euclio]

Rebased!

[martskins]

Thank you!