microsoft / language-server-protocol

Defines a common protocol for language servers.
https://microsoft.github.io/language-server-protocol/
Creative Commons Attribution 4.0 International
11.22k stars 799 forks source link

Add a request so that the server can ask the client for user input #1641

Open ayoub-benali opened 1 year ago

ayoub-benali commented 1 year ago

Hello,

is it possible to add to the specification a request sent from the server to the client allowing the user to provide a string value for a given prompt ?

It would be very similar to ShowMessageRequest but without the actions items, the client would have to reply with a string value provided by the user.

Metals has an LSP extension that does this called Inputbox and is used for various commands as a follow up to server commands.

I don't know if other servers have a similar extension but I think it make sense to add it.

dbaeumer commented 1 year ago

I am actually against this. Knowing what I know today I would not add showMessage and showMessageRequest. IMO a client needs to provide a reasonable set of input to a request and a server shouldn't have the need to ask for additional input. This ensure that servers can work without UI.

puremourning commented 1 year ago

Completely agree with @dbaeumer on this. I think server micromanagement of the UI is a bad pattern.

ayoub-benali commented 1 year ago

Maybe my proposal wasn't clear. Unlike showMessageRequest and showMessage if this new request is added it would be configurable via a ClientCapability like most recent LSP requests, this should avoid additional strict UI requirement for the client.

I didn't know that LSP is designed to be UI free but then I don't get why Code Lens Request , Rename Request or Code Action Request for example are part of the spec when they require some form of UI from the client.

server shouldn't have the need to ask for additional input.

Depending on server specific context, I can imagine something like this :

  1. client ends a workspace/executeCommand
  2. server follows up with ShowMessageRequest
  3. client sends a choice
  4. Server needs more info depending of user choice.

It is fine to think this is a bad pattern but IMO it should be also up to the server developer to choose how they want to design their interactions with the user.

tgodzik commented 1 year ago

IMO a client needs to provide a reasonable set of input to a request and a server shouldn't have the need to ask for additional input. This ensure that servers can work without UI.

What about the cases when the server actually knows something more than the editor and needs some additional version?

For example, we haven't specified the version of the formatting tool and let's say it's necessary. There is no way of getting that information through the formatting request and the editor doesn't know there needs to be a version. We can just fail and send a message to the client that they need to add it (without showMessage, we wouldn't be able show a nice error, we would just fail). This overall isn't a great experience for the user, so the server maintainers will come up with an additional method for LSP and this might happen for a bunch of extensions/servers.

So instead of having a single request that allows for some flexibility, we have multiple extensions for each language implementing the same thing.

Besides, don't all the request need some kind of UI? The editor is UI, any existing kind, code actions need dispalying, rename needs a new name, maybe extract value might need some additional UI at some point . Are there any actual servers that are used without a client editor? Wouldn't it be enough to have it behind a capability?

Overall, I totally understand that we can't just put any request into the LSP standard and I fully understand if you choose never to implement this. But I wonder if it's not being strict strictness sake and maybe LSP can be more flexible so that multiple client extensions don't invent the wheel again.

mfussenegger commented 1 year ago

My main complaint in regards to showMessageRequest would be that the server can push it at any time to the client. This can be quite annoying as you get messages when you don't expect it (and are already typing something else). So I think any kind of InputBox should be at least tied to a explicit client request.

Looking at some of the eclipse.jdt.ls extensions, it looks like most are tied to code actions and have to do with choosing one of multiple options.

Examples:

They all follow a pretty similar pattern. Maybe some of that could be generalized into a new capability for code actions. For example one that allows codeAction/resolve to also return a list of options, with an indication if the user must select a single item or if they can select many. You'd have an extra round-trip that allows servers to defer potentially expensive computation to when a user actually selects a code action.

Another alternative could be to extend workspace/executeCommand to have a LSPAny|SelectFromCandidates response or something like that. This would have the advantage that there could be multiple roundtrips, but executeCommand is also used in more places, so that may open the door to more (maybe unwanted) UI interactions. E.g. when selecting a completion item.

puremourning commented 1 year ago

They all follow a pretty similar pattern. Maybe some of that could be generalized into a new capability for code actions.

Something like discussed here covers most of those cases I think (i.e. additional metadata on the codeAction): https://github.com/microsoft/language-server-protocol/issues/1164#issuecomment-1328930377

I would personally avoid adding complexity to executeCommand as it's (IMO) not well designed or specified and difficult to implement in clients.

dbaeumer commented 1 year ago

I see all the use cases but adding UI messages to LSP is IMO the wrong path. The rename refactoring for example added an additional prepareRename request that ask the server to do some validation and let the client know about it. The client can then decide what to do (e.g. can even show some UI) and then execute rename or do nothing. If we need such a multi step flow in other scenarios then we should look into those scenarios and come up with a data driven protocol for them.

I am not saying that a general UI framework could be beneficial, but such a framework shouldn't be part of LSP. It should be speced separately and clients would be able to use it in combination with LSP.

mickaelistria commented 1 year ago

I am not saying that a general UI framework could be beneficial, but such a framework shouldn't be part of LSP. It should be speced separately and clients would be able to use it in combination with LSP.

There is JSON Forms that could fit (and LSP could just have some operation like showForm taking such a json form as paramter to client to display it); but the tricky part, and that will be true for all frameworks, is validation which always becomes something quickly requiring actual code and that cannot be expressed declaratively; so in the end, one needs to bind with 1 particular programming language...

michaelmesser commented 1 year ago

which always becomes something quickly requiring actual code and that cannot be expressed declaratively; so in the end, one needs to bind with 1 particular programming language...

Couldn't the server run the validation?

mheiber commented 1 year ago

Here's an example of a flow that TypeScript has that afaik can't be replicated with LSP without extensions:

  1. When the user selects an expression a yellow lightbulb shows up. Example: z = 3 + /*selection-start*/5000/*selection-end*/ lightbulb

  2. When the user selects "extract into variable" then a new variable called "placeholder" is created in the current scope and the original expression is assigned to it. Example: placeholder = 5000; z = 3 + placeholderrenaming

  3. The first instance of placeholder is highlighted and the text box for renaming pops up. When the user types "the_new_name" and presses Return then the text is: the_new_name = 5000; z = 3 + the_new_name

renamed

Adapted from https://stackoverflow.com/questions/75878281/how-can-i-automatically-trigger-the-rename-flow-after-extracting-into-a-variable