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: `workspace/configuration` clarification #38819

Closed
bstaletic opened this issue May 2, 2020 · 11 comments
Closed

x/tools/gopls: `workspace/configuration` clarification #38819

bstaletic opened this issue May 2, 2020 · 11 comments

Comments

@bstaletic
Copy link

@bstaletic bstaletic commented May 2, 2020

This isn't really a bug report, just a question. If you still need me to fill out the issue template, I will.

Today I tried to implement workspace/configuration support in my client. As far as I understand the intent of this request, based on microsoft/language-server-protocol#676

  1. The client notifies the server that there's new config available.
  2. The server queries the config by firing workspace/configuration requests as necessary.

This basically means that the client should respond with a list of setting[ 'some_key' ] values, where 'some_key' comes from section field request[ 'params' ][ 'items' ] list. So far, so good.

Gopls sends a workspace/configuration request that looks like this:

{
  "jsonrpc": "2.0",
  "method": "workspace/configuration",
  "params": {
    "items": [
      {
        "scopeUri": "file:///home/bstaletic/.vim/pack/bundle/start/YouCompleteMe/third_party/ycmd/ycmd/tests/go/go_module",
        "section": "gopls"
      },
      {
        "scopeUri": "file:///home/bstaletic/.vim/pack/bundle/start/YouCompleteMe/third_party/ycmd/ycmd/tests/go/go_module",
        "section": "gopls-go_module"
      }
    ]
  },
  "id": 3
}

The configuration docs only ever mention the gopls key/section. So how is the client supposed to respond to gopls-go_module section?

@gopherbot gopherbot added this to the Unreleased milestone May 2, 2020
@gopherbot gopherbot added the Tools label May 2, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented May 2, 2020

Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here.

@gopherbot gopherbot added the gopls label May 2, 2020
@bstaletic
Copy link
Author

@bstaletic bstaletic commented May 3, 2020

Actually, it's worse than that.

If my gopls settings are these:

    {
      "hoverKind": "Structured",
      "fuzzyMatching": false
    }

Then I do get structured hover.

However, the gopls/doc/settings.md suggests that the settings should looks like this:

    {
      "gopls": {
        "hoverKind": "Structured",
        "fuzzyMatching": false
      }
    }

In which case I get default hover kind and my client runs into a KeyError.

This just further makes it really hard to properly implement workspace/configuration request support.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented May 5, 2020

Sorry about the lack of documentation. Lots of things are still in flux, which is why we've been hesitant to formalize them.

The gopls-<workspace folder name> section is intended to provide more granular configuration for the user, so if you have specific configs for your project, you can set them there. As an example, a case where I turned staticcheck on globally, but off for just my project:

[Trace - 00:05:57.769 AM] Received request 'workspace/configuration - (2)'.
Params: {"items":[{"scopeUri":"file:///path/to/golang.org/x/tools/internal/lsp","section":"gopls"},{"scopeUri":"file:///path/to/golang.org/x/tools/internal/lsp","section":"gopls-lsp"}]}

[Trace - 00:05:57.774 AM] Sending response 'workspace/configuration - (2)' in 4ms.
Result: [{"staticcheck":true},{"staticcheck":false}]

Regarding your second question, the example given in that document is actually an example of how the settings might look in VS Code. Written above that snippet is:

In VSCode, this would be a section in your settings.json file that might look like this

That is definitely not a clear phrase, so I'll make a note to fix it, and thank you for pointing it out. The correct response should look like the one in your first example. However, a user of your LSP client may have a "gopls" section in their config file.

@myitcv
Copy link
Member

@myitcv myitcv commented May 5, 2020

I happened to exchange some messages with @findleyr on this yesterday. I think there are two issues we should look to solve with the current server-to-client Configuration call:

  • there appear to be N calls for the N workspaces that exist. The *protocol.ParamConfiguration.Items always has "gopls" as the first section, then the workspace-specific section (which gives the cascade @stamblerre described above). But this doesn't really make sense because it seems to suggest the root "gopls" section can/could be different per workspace. Instead I think we could replace this with a single call, with the "gopls" root section followed by all workspace-specific values
  • there does not appear to be a way to modify the session config initially set in the client-to-server Initialize call. This config is a different to the "gopls" root workspace config because it applies across workspaces, e.g. for the upcoming workspace symbol changes in https://go-review.googlesource.com/c/tools/+/227677 and friends. One thought here would be to pass a "session" section
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v0.5.0 May 5, 2020
@bstaletic
Copy link
Author

@bstaletic bstaletic commented May 5, 2020

@stamblerre

That is definitely not a clear phrase, so I'll make a note to fix it, and thank you for pointing it out. The correct response should look like the one in your first example. However, a user of your LSP client may have a "gopls" section in their config file.

Considering sections that gopls sends, this is problematic, if a client wishes to implement workspace/configuration support in a generic way. Allow me to elaborate.

So far, the only two servers that I tested workspace/configuration with are gopls and rust-analyzer (further called "RA"). Where these two servers differ, when we talk about workspace/configuration, is how they treat section.

RA wants clients to do something like this:

result = []
for section in sections:
    result.append(server_settings[ section ])

This approach clearly wouldn't work with gopls, since even gopls doesn't actually exist in the server settings. As you said, the correct settings don't include the enclosing 'gopls' key. That's if we forget about gopls-<workspace> sections.

Considering microsoft/language-server-protocol#676, I believe RA is correct here.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented May 5, 2020

I'm sorry, @bstaletic, but I'm not entirely sure I understand your comment. Are you saying that you expect your client to send settings that looks like your second example?

{
    "gopls": {
        "hoverKind": "Structured",
        "fuzzyMatching": false
    }
}

Effectively, all gopls expects back from the workspace/configuration request is a map[string]interface{}, and "gopls" is just not a setting that we have. You don't have to add support for a gopls-<workspace> section in your client, and as @myitcv suggested above, it should probably be removed.

There is also no requirement that the user's settings look a certain way; the client can have different names for these settings or a different format, as long as it "translates" them into something that gopls recognizes. gopls will also return errors for unrecognized settings.

@bstaletic
Copy link
Author

@bstaletic bstaletic commented May 6, 2020

Are you saying that you expect your client to send settings that looks like your second example?

My client doesn't yet support workspace/configuration. I am looking for a way to implement it in a generic way, so it works with all servers. With rust-analyzer and gopls having different client-side expectations, right now that seems impossible. At least without "translating" settings, as you have put it, which requires server specific knowledge.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented May 6, 2020

I'm still not sure that I understand what your request is - what do the configurations look like for rust-analyzer?

@bstaletic
Copy link
Author

@bstaletic bstaletic commented May 7, 2020

For rust-analyzer they look like this:

{
  "rust-analyzer": {
    # settings
  }
}

And the server sends the request with section set to rust-analyzer. With that, the clients can do server_settings.get(section), but that's not what tools expects from the clients.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented May 7, 2020

This behavior is not defined in the LSP spec, so I'm not inclined to change it here. The section in the workspace request is gopls, so it seems unintuitive to me to expect that the section also be present in the response from the client. Furthermore, the spec states:

The order of the returned configuration settings correspond to the order of the passed ConfigurationItems (e.g. the first item in the response is the result for the first configuration item in the params).

The intent is to make the mapping between sections and results clear, meaning that servers need not require the name of the section in the result. I'm sorry, but unless something is in the LSP spec, we will likely not modify our behavior.

@bstaletic
Copy link
Author

@bstaletic bstaletic commented May 7, 2020

Rust-analyzer isn't expecting the name of the sections in the response either. It's expecting the value of the key named the same as the section - i.e. the value for the rust-analyzer key.

Anyway, thanks for your time. I realize that this part of the protocol is impossible to implement correctly.

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
4 participants
You can’t perform that action at this time.