Open DanTup opened 3 years ago
I think it's a slippery slope: the metadata you're asking for are actually going to become form descriptions, then maybe even UI descriptions (a la XUL or HTML). That's not a trivial topic... ie how can one describe that the "OK" button has to be disabled for an empty field, or a field that contains invalid characters? We need to anticipate that in the protocol because such query will happen as soon as we open the door to server-side defined forms. And soon, step by step, the LSP will start defining many parts of an IDE UI instead of providing language-specific operations. It would IMO be more interesting to invest in standardizing some of the code actions, eg "move to file", that client should implement correctly according to the spec, and make codeActions trigger this command. Some refactorings are standard enough (there are books about main refactorings that apply to the vast majority of languages) to get into the spec IMO, "move to file" is one of them. Chaining a code action with a LSP command can already be attempted with the "rename" operation. I'm not sure if there are already decent flows implemented chaining those. Are you aware of anything on that matter?
I think it's a slippery slope: the metadata you're asking for are actually going to become form descriptions, then maybe even UI descriptions (a la XUL or HTML).
I understand the concern, but it doesn't seem like a great reason not to support many types of refactor. If it shouldn't turn into a UI language, that can be enforced without rejecting the whole idea. VS Code has commands for collecting input, but you can't build dialogs/UIs in it (and I presume you never will).
For most of these refactors, a single input will do. I don't think "asking the user for a string" (eg. VS Code's showInputBox
) or "ask the user for a file" are really on the same level as "having a UI language to build dialogs".
Without anything, it will encourage servers to implement things in custom commands just to get input, and that will need duplicating in each editor (and this will probably either be done differently by each language or there will become an "unofficial spec" that editors/servers confirm to to share this). That seems somewhat at-odds with the goals of LSP.
(This isn't just theoretical - I've been considering implementing some of these refactors this way for a while, and it came up again recently by someone wanting to contribute the same.)
Some refactorings are standard enough (there are books about main refactorings that apply to the vast majority of languages) to get into the spec IMO, "move to file" is one of them.
I agree that common refactors would be better as first-class features - it can result in a better user experience - but there are so many refactors that won't fall into this category, it seems like there should be something to support them. For example, is "Extract Method" common enough? "Extract function"? Maybe. What about Flutter's "Extract Widget"? Presumably not.
Chaining a code action with a LSP command can already be attempted with the "rename" operation. I'm not sure if there are already decent flows implemented chaining those. Are you aware of anything on that matter?
Do you mean use a custom command (invoked by the server on the client) that triggers the rename? I've considered this, but LSP doesn't define any well-known commands or a way for the server to know what commands the client has. There's some slightly-related discussion in https://github.com/microsoft/language-server-protocol/issues/1104 that also led me to believe that driving workflows from the server might not be a good idea.
@matklad
Do you mean use a custom command (invoked by the server on the client) that triggers the rename?
Probably something like that. I was more wondering whether CodeActions should allow an alternative to textEdits and commands, something like clientRequest: { "method": "rename", "params": { ... } }
and the spec would say that if present, the client should then send the describe query to server when codeAction is selected.
Probably something like that. I was more wondering whether CodeActions should allow an alternative to textEdits and commands, something like clientRequest: { "method": "rename", "params": { ... } } and the spec would say that if present, the client should then send the describe query to server when codeAction is selected.
That would actually not work for this case, as it doesn't bring the opportunity to capture input. So we're back to this idea that was discussed a few times already (including in your former comment): use commands, and standardize some of them (eg for refactorings).
I was interested in implementing an input-dependent refactor in the Dart language but I was faced between one of two options:
The backend doesn't need to be involved with defining UI, but it could at least expose a schema of the required input. This would be no different from a backend in a traditional client-server app. The backend exposes an API call with several arguments and does not dictate how the client collects this info.
In this scenario, the backend is the LSP server and the clients are the editors (VSCode, Eclipse, Sublime, etc).
I would rank things in the following order of desirability from a refactoring standpoint:
There are many cases where a standard won't be able to capture the needs of all the different languages. In this case, it seems preferable to at least go with (2) and allow the logic to live in the LSP server backend rather than (3) in the client editor. This option isn't as good as (1) the standard, but it at least allows developers to maintain a single refactoring implementation that is shared across editors.
And it won't block implementers from improving the language experience. While standards take a while to get defined, experimentation can happen quickly.
This experimentation could also help define standards. The LSP team could pay attention to the non-standard LSP features that are being implemented in category (2) and standardize the most popular ones. Implementers could replace their custom implementations with standards where possible. Because it's less code to maintain, I imagine they'd have a strong incentive to migrate to LSP standards as they are released.
I have a bunch of thoughts here!
Avoiding GUI
In my experience as IDE user, I find GUI-based refactors awkward to use in practice. I have a pretty optimized text editing workflow, with acejump, multiple cursors, home row navigation, etc (and I even don't use vim), and switching to filling-in a form for a complex refactor breaks the flow. The flow is much better if I can apply the refactor in-place, without leaving the editor.
Quite a few of the refactors can be handled this way. For example, IntelliJ recently introduces in-place "change signature" (refactor which allows you to change types, names, order and quantity of function's parameters, and updates all the call sites): https://blog.jetbrains.com/idea/2020/02/intellij-idea-2020-1-eap4/.
The basic idea is that we just allow the user to change the signature of a method in an IDE, without invoking refactor explicitly, and then supply a code action to fix the call-sites.
The same works for Java's analogue of move between files -- if you select some code, cut it in one file, and paste it into the other, the IDE prompts you with yes/now dialog for updating call-sites.
Text-based UIs
One thing which I think might be interesting, but which wasn't explored are hacky, text-based gui. For move-to-file, let's say the user has this written:
fn foo() { /* cursor here */
}
We can show a code action which does this:
// move-to: some/file.rs /* cursor here */
fn foo() {
}
ie, add a comment which serialized refactoring GUI in a comment. Then the server can show auto-completion for file paths, highlight the element which would be moved using semantic highlighting, etc. To apply the refactor, there's another code action avaialble on the comment, which does the move and removes the comment.
Note that we've got composition for free -- if the user wants to move a bunch of things, they can now select the comment, copy-paste it to all target declarations (maybe using multi-cursor) and invoke a series of moves without selecting a file each time afresh.
Universal Refactors
I am somewhat skeptical about standardizing common refactors. This mostly stems from my values: I seek to build the perfect experience for a specific language in a hypothetical perfect client, rather than building a uniformly good behavior in all clients.
I think languages are sufficiently different that even seemingly identical refactors would have various differences if one looks close enough. For example, "Move to file" in a perfect Rust IDE would be "move to module". There's no 1-1 correpondence between files and modules (there may be several modules within one file). The input would be a path::to::module
rather than path/to/file.rs
, and the auto-completion for that can only be powered by the server.
Prior Art
If we do go with GUIs for refactors, the dart protocol approach feels ok-ish from the "perfect experience" POV:
For each refactor type, Dart documents a specific JSON format with data to be shown to the user as a feedback, and JSON format for the user's reply. It says nothing how the data is displayed in the UI -- client implements specific GUI for each refactor. This obviously means manual work for each (client, refactor)
pair, but allows for the best possible experience.
An interesting facet here is that the communication protocol for a refactor is stateless. This works a bit like HTTP GET/POST for a form. Rather than the server recording that user X now views form Y after X does GET Y, the corresponding POST Y contains all the info required to restore the context.
Today we have showMessageRequest which is a somewhat limited form of VSCode showQuickPick.
If we aim for 80% fidelity in refactorings, ignoring things like reorder parameters, then if we expanded on the idea of showMessageRequest
what if we had the ability to make an inputRequest
that a code action or command could trigger on the server side.
interface InputRequestParams {
questions: InputRequestQuestion[];
}
interface InputRequestResponse {
responses: any[];
}
type InputRequestQuestion = BooleanQuestion | PickQuestion | InputQuestion;
interface Question {
type: string;
label: string;
}
interface BooleanQuestion extends Question {
type: 'boolean';
}
interface InputQuestion extends Question {
type: 'input';
placeholder: string;
}
interface PickQuestion extends Question {
type: 'pick';
values: string[];
}
The client could get a list of questions. If the client wants to render them as a form, it can do so. However it does not need to. The client could also prompt each question in a row (vscode quickpicks, etc) until it gets all the responses. The different types of questions can constrained to ones that easy to implement as both text / gui.
Once all the questions have been answered the client responds with the answer for each item in the order that it was provided similar to how configuration returns it's results.
Additional Specializations that might make sense
filesystem
a file / folder questionworkspaceSymbol
a symbol within the workspacedocumentSymbol
a symbol within the documentI agree with the general points above and suggest we think about the separation like a regular client-server app.
The backend is the LSP server. It does the heavy lifting related to the language and doesn't enforce any UI constraints. We can probably completely disconnect the LSP from any involvement in the UI so we leave the client to decide how to do this.
This would support @makarius's use case of text-based refactoring, some of Dart's use cases of GUI-based refactoring, and allow the client flexibility on how to collect that input as @david-driscoll.
And if the LSP can expose the schema of the API in the same way a GraphQL or other discoverable APIs, many editors can provide sensible defaults based on the schema. In GraphQL APIs, it's relatively straightforward to auto-generate a UI from the inputs the backend expects by reflecting over the schema.
GraphQL is overkill, but it's a good example where you can keep a separation of concerns between client-server without the need for unnecessary boilerplate. If the server exposes the schema of inputs, the client can choose to auto-generate UIs for requesting input or choose to build specialized UIs for some refactors.
For example one LSP API call might look like:
ExtractClassToFile
It could expose this in some form of standard json structure.
The client editor could collect this info in different ways:
To start, we could be conservative about what types we allow. File paths, strings, numbers, and a few primitives could be introduced first before expanding further.
And if the LSP can expose the schema of the API
I don't think a schema for the spec is necessary if we rely on LSP specified operations: To deal with LSP, one would typically use a LSP SDK (vscode-languageserver for node, LSP4J for Java and so on...). Most languages do have introspection mechanism that would allow to read an operation (a method) definition as a schema, and those clients can already generate forms from those operation if they want to. A stronger typing in the necessary spec elements would help to express some additional constraints on the value and build forms with specialized tools & validation for some fields.
But maybe I misunderstand and you suggested something that is more like allowing a new formEntries
field in showMessage
with a description of the forms a-la GraphQL and returning the values to the server? It would indeed work but...
I don't believe automatic generation of UI from a list of fields is a sufficient solution anyway. (this belief is based on my previous experience in the good old time of SOA and BPM and connecting to services dynamically and generating forms for them). Indeed, a generated form is almost always bad! As soon as user deal with a generated form, they complain almost immediately that "form allows incorrect values", "form doesn't have a file/directory/color-picker", "form should provide a more detailed description for field XXX", "entry
So while a way to describe forms/fields that client should render in a dialog can be interesting and useful to "unlock" some cases; and that it might be worth having included in the spec of showMessage
or similar; we shouldn't build too much expectation on the quality of the result.
However, I cannot really advise of a better way to handle that. The 2 extreme sides are
None of them is very satisfying for the users; but concretely, if we want to support advanced refactoring and UIs which are language specific, non-standard and rely on language model elements that are only known by the server; I don't see an alternative to the extreme solution 1 above. And even with solution 1, we may want the client to continuously send the form values on every change to do some extra validation that cannot be done only on client-side (eg does this module exist), so it would also quickly show its limits. IMO, this issue of a server sending forms in a specified form and client showing it according to spec, then sending the "working copy" of the values to server so that the server sends a response about validation and fields to add/change according to the input, and so on, would require its very own technology or protocol, if it's not already existing. It's not something that the LSP should define IMO, LSP should identify one technology for that and provide integration with it.
@mickaelistria You make some good point on the pitfalls of automatic code generation.
I didn't mean that the server would need to dictate the UI. It would only describe a list of arguments (name, description, type) in the same way a typescript function might define a list of args and types. Or an RPC might do the same. I don't have an opinion on how the LSP could query this schema.
The client could choose to generate a UI or it could choose to go a custom route. But I may be getting ahead of myself.
Just allowing LSP refactors to accept a map of arbitrary inputs would be very valuable even without a schema declaration.
I think an additive approach makes sense, having the ability for the server to "ask the client for input". There is no implied schema that the client has to implement. The only thing the client has to know is that "the sever wants some input from the user.".
That could be expanded to include things like validation (regex or a callback) or a completion list, but strictly speaking they're not required. They make the user experience better, if you're executing "Extract Method" you would want a valid method name and the user would prefer want immediate feedback, but it doesn't have to be that way initially.
Yes let's not make an initial approach overly complex. I like the proposed structures from @david-driscoll.
The existing Command
structure could be expanded to have an additional inputs
key with value a list of inputs as described in https://github.com/microsoft/language-server-protocol/issues/1164#issuecomment-742706956. Those inputs would be appended to the arguments
of the Command
. So for instance one of the code actions could be
{
"title": "Rename Method",
"command":
{
"command": "rename-method",
"arguments": ["some", "fixed", "parameters"],
"inputs": [
{
"type": "boolean",
"title": "Frobnicate as well?"
},
{
"type": "input",
"title": "New method name"
},
{
"type": "pick",
"values": ["foo", "bar", "baz"],
"title": ""
}
]
}
}
the client collects the three inputs from the user and then requests workspace/executeCommand
with params
{
"command": "rename-method",
"arguments": ["some", "fixed", "parameters", true, "qux", "foo"]
}
@rwols Thats definitely a nice schema. I feel like there are some more useful ways to extend this. A default field wouldn't go amiss for some the 3 types you mentioned. I'd also suggest a selection type, this would be useful for refactors where there are multiple places to apply and you want the user to select which ones should be done. A common example is renames where there are cases where its not possible to know if a symbol defined refers to the same symbol you are renaming.
{
"type": "selection",
// foo is already selected, bar isnt selected.
// baz is implied not selected
"values": [ {"foo", true}, {"bar", false}, "baz"],
}
I'd imagine the client would reply with either an array of strings that the user selected, or optimise to an array of indexes the user selected.
Another possible type is a reorder, the client could list the items and the user could reorder them, the server would then respond with an array containing the new order(either values or indexes relative to what was sent to the client)
{
"type": "reorder",
"values": ["baz", "foo", "bar"],
}
All this input behaviour would need to be in capabilities, this is a quick example of how it could look.
/**
* Defines the input capabilities of the editor.
*/
export namespace InputMethodType {
export const Text = 1;
export const Bool = 2;
export const Pick = 4;
export const Selection = 8;
export const Reorder = 16;
}
export interface InputCapabilities {
/**
* The supported methods of input, if not provided defaults to no input capabilities.
*/
SupportedInputMethods?: uinteger;
/**
* Clients are free to define their own extra input methods as extensions, but servers aren't required to use them.
*/
ExtendedInputMethods?: string[];
}
Servers would then only request user input types that the client supports.
Come to think of it, We should probably be specifying string|Location
for the values in the pick
, selection
and reorder
types.
I like the latest proposal on the input schema and command execution. We can be conservative with the input types accepted on the first pass.
Like string, int, selection, file path.
The language several can be responsible for validation which may be less than ideal for UX, but it will at least unblock the use cases.
If this standard gets adopted, we can can carefully add new types based on usage. This way clients can provide more optimized UIs for collecting inputs depending on the type.
Relevant bit from JetBrains conf: https://youtu.be/GRmOXuoe648?t=7872
We are hitting the same problem for pylance
. We want to implement rename file code action
, which needs user input for the file name. We want to implement move symbol
, which requires file name
to move the symbol into. And many other code actions.
90% of them require very simple user input, yet we have to have custom code on the client side, and we need to duplicate the efforts for different editors.
Rather than sitting on it for the perfect solution for every possible case one can imagine, it would be nice to provide something that can first satisfy the most straightforward cases.
In my experience, even if such a complex, flexible system is provided, only a few use all that functionality. Most of the others use a small portion of it.
And if one decides to provide refactoring without a message box (inline change), that is fine; But that preference shouldn't block others from providing refactoring differently.
It looks like we already have excellent suggestions from @david-driscoll, @venkatd. @rwols, @njames93
What do we need to make this happen? It is already a 2-year-old feature request.
It is a specification. Once something lands to the spec, it is hard to change it at that point.
I wrote a pretty long message so I split it in sections.
Also keep in mind, while you(the reader) can see this comment as mostly a critique, please take it as my best intentions. I have the most respect to people working at the LSP spec and to people bringing good dev experience to other people.
Wish you all a good weekend!
I am having a similar issue when it comes to implementing "implement interface" code action. What I need is the interface name to be specified by the user before the code action is actually handled. The LSP must know the interface name a priori, to know the name to search for.
Could we simply add a "parameters" field within the codeAction
request? Or some kind of code action resolution query, which asks the client for the user input, or a parameter?
@heejaechang, have you taken any solution, that is out of LSP specification, which solves your problems with pylance
, around features which need user input?
Could we simply add a
"parameters"
field within thecodeAction
request? Or some kind of code action resolution query, which asks the client for the user input, or a parameter?
For Dart we've been exploring something like this in data
and using middleware in the VS Code extension. On the client we wrap the original command with a local extension-provided command so we can provide prompts to the user when invoked and replace the arguments:
This local command then enumerates the parameters
in data
, prompts the user for values (using an appropriate API like showSaveDialog
and passing suitable defaults that were in the parameters
):
And then finally delegates to the original command on the server with the arguments
replaced by those new user-provided values.
Currently the code only supports one kind
of parameter (saveUri
, which maps onto VS Code's window.showSaveDialog
) but this will be extended. The client also tells the server which kind
s of input it can prompt for, so the server can avoid sending any code actions for which the client wouldn't be able to handle (unless there are default values, in which case they will be used).
There's no spec for this yet (because it's still being worked on and behind a flag) but if it's close enough to what others want I would write it up for others to review/contribute to. I'd love to see something standard in LSP, but if that's not going to happen there would still be advantages to aligning languages on an "unofficial" spec for this.
Edit: CodeAction JSON looks approximately like this:
{
"kind": "refactor",
"title": "Move 'main' to file",
"command": {
"arguments": [
// For Dart, we generally pass arguments as an object so we can
// provide names, so the arguments list here is a single item
{
// These arguments are not defined by parameters but are fixed
"filePath": "/Users/danny/Desktop/dart_sample/bin/main.dart",
"selectionOffset": 7,
"selectionLength": 0,
// These additional "arguments" can be overwritten by user-provided values
// using the info in data.parameters
"arguments": [
"file:///Users/danny/Desktop/dart_sample/bin/main.dart"
]
}
],
"command": "move_top_level_to_file",
"title": "Move 'main' to file"
},
"data": {
// These parameters map to "arguments" above
"parameters": [
{
"kind": "saveUri",
"parameterTitle": "Select a file to move to",
"parameterLabel": "Move to:",
"defaultValue": "file:///Users/danny/Desktop/dart_sample/bin/main.dart",
"actionLabel": "Move",
"filters": {
"Dart": [
"dart"
]
}
}
]
}
}
The client uses data.parameters
to drive any UI and replace values into the arguments
list before the command is sent back to the server for execution.
@DanTup That looks nice!
What happens when one of the arguments would have to be passed as-is, as one of the arguments
within the executeCommand
request, from the client to the server?
For example, having a response to codeAction
:
[{
"kind": "refactor",
"command" : {
"arguments": [
"leave-me-be-#1",
"leave-me-be-#2",
"/the/replaceable/path/passed/as/a/parameter"
]
},
"data": {
"parameters" : [
{ "What to put here? " }
]
}
}]
Is the custom user input still supported for such a case?
@KKoovalsky that's why there is a nested arguments
in my example above. The outer arguments (which are actually in a single object so we can use named fields in Dart, rather than index-based arguments) are those that are not replaced by the parameters, and the nested "arguments" are those that correspond to the parameters
in data
.
That said, clients should also pass through the defaultValue
of any parameters they don't know how to prompt for, so:
"data": {
// These parameters map to "arguments" above
"parameters": [
{
"kind": "something_the_client_doesn't_understand",
"defaultValue": "file:///Users/danny/Desktop/dart_sample/bin/main.dart",
}
]
}
Because the client doesn't know what to do with this parameter kind, it should just use defaultValue
when replacing the arguments. It is up to the client to tell the server which kind
s it supports (during initialisation) so that the server can avoid producing any actions with parameters that don't have default values that the client can't handle. That is, the server can send actions with parameter kind
s the client does not advertise support for as long as they have defaultValue
s.
@DanTup Ok, thanks, I see now that in your example filePath
, selectionOffset
and the selectionLength
are the fixed arguments I asked for.
@KKoovalsky correct :-)
One common request of Dart is a refactor for "move to file" (either moving to an existing file, or a new one). Right now I don't think there's a great way to implement this as far as I can see there's no way to ask the user to pick a file (existing or not).
Slightly related, for Extract Method we would like to ask for a name (there's a related issue at https://github.com/microsoft/language-server-protocol/issues/764 suggesting instead allowing us to trigger a rename at the end, although asking for the name up-front would also work).
There are other refactors that Dart supports in other IDEs that have various other inputs/options (for example "inline method" lets you decide whether to keep or remove the method, and when extracting a method there's an option for whether to replace any other instances of the same statements in the file with a call to the new method).
I'm not sure of the best way to do this, but I thought it was worth starting a discussion (I couldn't find an existing one), but some possible ideas:
window/showMessageRequest
)