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.