Allow CodeActions to specify cursor position

[matklad]

EDIT: We now documented this as a proper extension: https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/dev/lsp-extensions.md#snippet-textedit. lsp-mode for emacs implements it: emacs-lsp/lsp-mode@9eb3324

WorkspaceEdit intentionally avoids speaking about cursor position, just like the rest of the protocol.

We however slightly bend this rule for completions, where we allow $0 snippets which is a way to place the cursor.

I want to argue that a similar feature is required for code actions.

Here are a couple of examples:

add derive

For add derive in rust-analyzer, I want to turn this (| is the cursor)

struct Foo {
    f: f64,|
}

into this

#[derive(|)]
struct Foo {
    f: f64,
}

It's pretty important that | is in the derive, such that the user can immediately type/complete #[derive(Debug, Clone, Copy)] incantation. Furthermore, the code action is available even if #[derive] already exists on the type: in this case, all the action does is it moves the cursor (which is still very convenient for the user)

create mod

Again, in rust-analyzer, for mod foo; I want to provide a fix which creates an foo.rs file, opens it in the editor and places the cursor at the start of the file (I can imagine an extension where the file is pre-filled with licensing boilerplate, and the cursor should be after it). Note that this is similar to #612

Note that, unlike the completions case, we can't just special-case code actions and add insertTextFormat on code-action itself: the reason is that lazy code-actions are resolved in a round-about way via a command and applyWorkspaceEdit request sent from the server to the client. So looks like we need to add this ability directly to WorkspaceEdit. Perhaps adding an

insertTextFormat?: InsertTextFormat

to the TextEdit (with a corresponding opt-in capability) will do the trick? The insertTextFormat on CompletionItem then becomes redundant though :)

[rcjsuen]

Also see #592.

[dbaeumer]

The problems I see with this is that code actions can be executed from different locations (for example the list of problems) and that moving the cursor in these cases might not be desireable.

Code action in its latest version do have edit supprt (see CodeAction#edit). So if at all I would only consider such a support for code actions.

@jrieken if someone wnt to implement something like this directly using the VS Code API what would be your suggestion? Have a command associated with the code action which then uses TextEditor#selection API to set the selection?

[jrieken]

Yeah, the command is invoked after applying the edit and would be possible to invoke a command that positions the cursor accordingly. Might not be trivial but a fair workaround.

As @dbaeumer already said, the difference between completions and code actions is that the former always targets the current editor (in which you type) whereas the latter is a workspace edit and it's not clear what should happen when you enter snippet mode without having an editor. Options are ignore, explode, defer...

[matklad]

The problems I see with this is that code actions can be executed from different locations

I think what we really have here is two different UI concepts:

1. Fixes for diagnostics, which could be invoked outside of the editor, in batch
2. Code assists, which only make sense in the context of particular cursor position/selection

Cursor position is nonsensical for 1., but is central for 2.

Code action in its latest version do have edit supprt (see CodeAction#edit).

Yeah, but, if you want to compute the edit only when it is actually required, you have to use command. OTOH, I think it's fair to assume that assists which control the cursor are cheap local transformations, and that computing them directly is the way to go.

With this in mind, what about adding

interface EditorCodeAction extends CodeAction {
    edit: WorkspaceEdit; // you have to use the new edit interface
    insertTextFormat?: InsertTextFormat; // but you can use snippets

    diagnostics: null; // not sure how to express this, but diagnostics should be forbidden if you use snippet
}

EditorCodeAction would be used only for cases where we know that the editor is in focus, so it can make use of, and control editor state. Currently the state is just the selection, but I imagine we could (but probably don't want) extend this to control things like scrolling or folding.

Or perhaps (if we ignore backwards compat which we obviously can't)

// A generic change to the workspace state
interface CodeAction {
    ....
}

// A code action executed in the context of a particular editor
interface EditorAction: CodeAction {
    insertTextFormat: InsertTextFormat
}

// A code action that fixes particular diagnostics and can be called in batch mode. 
interface FixAction: CodeAction {
     diagnostics: DIagnostic[]
}

[jrieken]

I think what we really have here is two different UI concepts:

What's missing is 2' which goes like this. You author file A, file A has an issue for which code assist applies. The fix is to make a change in file B, e.g change visibility from 'private' to 'public' or 'package-private'. Now, because you are inside file A what should happen when that fix suggests a snippets, e.g showing a choice of 'public' or 'package-private'? Should B be opened/revealed? Should only the text be inserted and snippet mode be ignored? Should the code action know in advance of the UX state so that it computes the 'right' snippet?

[matklad]

@jrieken interesting example! Yeah, I guess even fixes might want to insert snippets in other files.

Another example here would be "create declaration from usage" fix: in file A, you type b.foo(92, "hello"), where b is an object from file B which doesn't have foo. Here, we should suggest a fix which declares method foo on B with integer and string parameters, and the snippet should be used to open the B file, cycle through parameter names and end in the function body.

So looks like it does make sense to use snippets for other files, but maybe it's a good idea to have a restriction that only a single resource can have snippet markers (seems easier to implement initially, and easy to relax later).

[jrieken]

So looks like it does make sense to use snippets for other files, but maybe it's a good idea to have a restriction that only a single resource can have snippet markers

Not yet sure. In theory every snippet can be inserted without an editor. In that case it will just be the text (like a normal text edit) without selection tricks and without any linked editing. However, the snippet must be complete, e.g. the code action must work without additional user input (also like a normal text edit).

[matklad]

Makes sense. It's interesting that even in "active editor" case snippets ideally should be complete: user can exist a snippet early anyway.

Would it be right to support reveal and snippets from problems view? I guess there's no fundamental reason why "interactive" fixes can't be invoked from this view? Perhaps we need some way to marks a fix as "fully auto-applicable" or "may require user input".

[dbaeumer]

To make this a nice user expierence we need to treat all code actions that would insert a snippet as require user input unless marked otherwise. I would then argue that fixes that are not auto applicable can only be executed in the active editor from within the editor.

[dbaeumer]

If we support this for code actions we might want to make it consistent and allow it for workspace edits as well (see #731)

[matklad]

So, what about this minimal proposal:

1. Change WorkspaceEdit to

export interface WorkspaceEdit {
	changes?: { [uri: string]: TextEdit[]; };

	documentChanges?: (TextDocumentEdit[] | (TextDocumentEdit | CreateFile | RenameFile | DeleteFile)[]);
	insertTextFormat?: InsertTextFormat;
}

2. Add workspace.workspaceEdit.snippet capability

3. Specify that clients may omit snippet edits in non-interactive contexts

4. Optionally: add autoApplicable?: bool to WorkspaceEdit to indicate that edit could be applied in a non-interactive context even if it contains snippets, with a requirement that all snippet variables should have a default value.

[dbaeumer]

Regarding 1.

I think this is to coarsely granular. I think we need (like with the completion item) to be able to specify this on an edit basis. I see two possibilities. We have a wrapper around a text edit with the insertTextFormat or we extend the structure. I lean more towards extending it (e.g. having something like SnippetTextEdit extends TextEdit).

Regarding 4:

I am not sure if this is a good path since it is not only about variables. The snippet can't define a cursor position either. Specing this feels strange to me. So may be we should say that snippets are only allowed for one TextDocumentEdit and that that one become the active editor when an edit is applied.

[kdvolder]

I think this is to coarsely granular.

+1

The snippet can't define a cursor position either.

I don't see why not. Even if the editor is not the active one, you can still move it's cursor can't you?

That being said I am also not so sure we really need support for snippets here. If all we need is a way to control where the cursor ends up, then snippets may be somewhat more than is needed. If we look back at the original issue title... it talks about controlling cursor positions only... not snippets. So it may be worthwhile to take a step back and think of simpler mechanism to achieve the goal to "control the cursor position" more directly.

[matklad]

This increased in priority for me, as our custom contraption for hacking this in blocks making rust-analyzer the official LSP server for Rust (rust-lang/rfcs#2912 (comment)).

On rust-analyzer end, I think I'll just try a SnippetTextEdit proposal:

• add snippet_text_edit to experimental capabilities
• define interface SnippetTextEdit { insertTextFormat?: InsertTextFormat; }
• use these in code actions if the client sets the experiment feature
• on VS Code client side, intercept the code action requests middleware. I think I can poly fill support for snippets by inserting them via custom command after code action.

This way I'll both solve the problem of rust-analyzer not working in many clients, and also will test-drive the realistic protocol extensions (as opposed to a hack which we currently use).

[jrieken]

I think I can poly fill support for snippets by inserting them via custom command after code action.

Won't be easy. Today, snippets require an editor, on the API you can use insertSnippet and there might be commands that achieve similar things:

• intercept request
• remove the snippet edit from the workspace edit
• apply workspace edit
• invoke insert snippet

The limitation is that there can only one snippet at a single position, you cannot spread a snippet onto multiple positions nor across files (which is multiple positions). Also, the undo behaviour is unhappy because you have at least 2 undo-stops.

[matklad]

Did a first cut at rust-lang/rust-analyzer@3e07c4f.

It wasn't too horrible, but, but splitting edits to the document into plain and single snippet and adjusting snippet range is annoying. If we move forward with this, it would be helpful if vscode API supported "multi-segment" snippets. I imagine adding SnippetString overloads to builder methods of editor.edit((builder) => ...) would work as an API. This API can also be simulated by collapsing TextEdits into a single giant TextEdit, but that's obviously awkward and can be slow.

[matklad]

That being said I am also not so sure we really need support for snippets here. If all we need is a way to control where the cursor ends up, then snippets may be somewhat more than is needed.

After some experimentation, it seems like snippets are super useuful. For example, when genrating dummy impls for rust you want to use ${0:todo!()} and not just set the cursor. In general, I am pretty happy with snippet API on the server side, seems very natural and useful.

[mickaelistria]

@dbaeumer Seems like the same story as in #592 and all related issues about generalizing snippets to CodeActions (or even TextEdits in general)

[dbaeumer]

VS Code has no support for snippets in workspace edits and we should definitely look into bringing them to LSP 3.18.

[MangelMaxime]

To me it seems like VSCode now support snippets for workspace edits. microsoft/vscode#145374

Does that mean this is now possible to add supports for SnippetEdit in the LSP protocol ?

[dbaeumer]

Yes. PR is highly welcome if someone wants to work on it.

[dbaeumer]

work is under way to add snippers to workspace edits.

[rchl]

Based on the spec updates in microsoft/vscode-languageserver-node#1343 I still feel quite unclear on some details:

• how should editor handle multiple snippet text edits for the same file? If the intention is that the snippet sets the cursor position then if first snippet sets one cursor position then does the second one replaces it or adds another cursor position?
• is editor supposed to trigger completions after inserting the snippet (and how would multiple snippets be handled if so)?

[mickaelistria]

Shouldn't we reopen until the specification gets derived from standard implementation formally here?

[dbaeumer]

Agree, we should reopen until we have the spec change.

[dbaeumer]

Regarding #724 (comment)

For the cursor position I would simply add to the spec that only one snippet text can specify a cursor position. If there are more than one the position of the cursor can be either of them, depending on what the client decides.

I think the editor should not trigger completion after inserting a snippet via a workspace edit.