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

Server implementations should have a way to modify ServerCapabilities directly #517

Open
michaelpj opened this issue Aug 26, 2023 · 6 comments

Comments

@michaelpj
Copy link
Collaborator

At the moment we infer the ServerCapabilities from the Handlers and Options. The only way for servers to alter the capabilities that we infer is if we provide a handle for doing that via Options. In some ways that's nice and discoverable, but it might be easier just to let the server directly modify the inferred ServerCapbilities before we send it back.

In many cases I think it would just be fiddly to expose things via Options. For example, most server capabilities for methods allow you to opt-in to client-initiated progress. At the moment we just always disable that and rely on server-initiated progress, but it would be reasonable for a server to want to opt in... but even then you probably only want to do it for certain methods. So we could have a methodsToOptInToClientInitiatedProgress option... or we could just let the server modify the ServerCapabilities.

@SquidDev
Copy link
Contributor

I'm also beginning to bump into this issue with semantic token legends. In this case the server-supplied legend needs to be chosen based on the client capabilities, and then stored somewhere for use when calling makeSemanticTokens.

The simplest change I can think of to enable this would be:

  • The ServerDefinition's doInitialize function chooses the server's legend, and stores it in the state a.
  • Add a new of the form chooseCapabilities :: ServerCapabilities -> m ServerCapabilities to ServerDefinition, that reads from the LSP state and returns the modified capabilities.

The main ugliness here is that that the signature of chooseCapabilities allows you to send messages before initialisation (which you shouldn't!), but I don't think that's really avoidable.

@michaelpj
Copy link
Collaborator Author

So the way I'm currently thinking of approaching this is a bit bigger, namely via #583

But I think there's an intermediary state that would help, namely:

  • Get rid of the staticHandlers field in ServerDefinition
  • Make doInitialize return a ServerCapabilities and a Handlers also
    • So the user has to tell us what they want
    • But they can call inferServerCapabilities themselves to get the ServerCapabilities to start with

I think this pushes us in the right direction and would let people set up the ServerCapabilities as they like.

I'm in the middle of something else, but I'd welcome a PR for that change, I think it should be fairly localised!

@SquidDev
Copy link
Contributor

Ahh, sorry! Had seen that issue, but hadn't quite thought about how it might relate to this one.

With this change, are you proposing removing all the capability-related information from Options then, and removing that as an argument from inferServerCapabilities, or would that stay for now?

@michaelpj
Copy link
Collaborator Author

Hmm yes, I think we could basically get rid of Options entirely, in fact? I'm mildly unsure. Perhaps it's nicer for people to fill in a defined list of possible tweaks and feed it to inferServerCapabilities, rather than poking individual bits of the capability structure themselves. But I would like to at least try the other approach, since I think it might be fine and it's both forwards-compatible and good for offering control the user.

@SquidDev
Copy link
Contributor

Great, thanks!

Hmm yes, I think we could basically get rid of Options entirely, in fact?

There's a couple of options (server info, progress delays), which aren't part of server capabilities. Those could stay as-is, or be merged into ServerDefinition.

@michaelpj
Copy link
Collaborator Author

Yes, those make sense to stay as options indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants