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 https://github.com/Microsoft/language-server-protocol/issues/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 https://github.com/Microsoft/language-server-protocol/issues/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 (https://github.com/rust-lang/rfcs/pull/2912#issuecomment-617692559).

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 https://github.com/rust-analyzer/rust-analyzer/pull/4494/commits/3e07c4fb30914f76f6cf054996b530575fc8c953.

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.

[matklad]

Found another problem: insertSnippet wants to re-indent the code. Here's what I am sending to the client:

[Trace - 5:35:30 PM] Received response 'textDocument/codeAction - (19)' in 2ms.
Result: [
    {
        "edit": {
            "documentChanges": [
                {
                    "edits": [
                        {
                            "insertTextFormat": 2,
                            "newText": "{\n        $0Some(_) => {}\n        None => {}\n    }",
                            "range": {
                                "end": {
                                    "character": 5,
                                    "line": 4
                                },
                                "start": {
                                    "character": 12,
                                    "line": 2
                                }
                            }
                        }
                    ],
                    "textDocument": {
                        "uri": "file:///home/matklad/tmp/hello/src/main.rs",
                        "version": null
                    }
                }
            ]
        },
        "kind": "",
        "title": "Fill match arms"
    }
]

And here's what gets inserted:

[Trace - 5:36:01 PM] Sending notification 'textDocument/didChange'.
Params: {
    "textDocument": {
        "uri": "file:///home/matklad/tmp/hello/src/main.rs",
        "version": 5
    },
    "contentChanges": [
        {
            "range": {
                "start": {
                    "line": 2,
                    "character": 12
                },
                "end": {
                    "line": 4,
                    "character": 5
                }
            },
            "rangeLength": 16,
            "text": "{\n            Some(_) => {}\n            None => {}\n        }"
        }
    ]
}

EDIT: I've ended up ditching editor.insertSnippet and just re-implementing minimal subset of snippets manually on the client side.

[DanTup]

@dbaeumer @jrieken is it likely this would be implemented in the near future? Pre-LSP we'd apply edits from code actions via a command that called editor.insertSnippet that allows us to pre-select text. That's noticeably missing when switching to LSP.

If it's unlikely this would be done, it may be worthwhile me implementing a custom command (I guess similar to @matklad has) to handle this, but ofc it would be much better if it was standard (there are multiple editors using the LSP server and the others are more likely to support this if it was standard).

I'm less concerned about full snippets than just selected some text.. if the WorkspaceEdit could contain a selection against a file that would be selected if that file was the active editor or something similar, that would likely cover it.

Thanks!

[matklad]

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: https://github.com/emacs-lsp/lsp-mode/commit/9eb332486eea74daf6cbb4be20ea462a8208fc4c

And yes, this still one of the top missing features for me as well!

[rwols]

How are snippet text edits supposed to work if they are in two different files of the workspace edit?

[DanTup]

@matklad interesting! Did you make a polyfill for it in VS Code? (presumably using middleware you could swap it out for a command?).

[matklad]

@DanTup yup, we have a middleware (it's rathe complicated because it handles code action groups -- another extension of ours). Here's the code that applies snippets. It's ugly and buggy, but works ok-enough for our use-case.

@rwols we just specify that snippets are always in the same file. So far, we didn't need multifile snippets.

[rwols]

we just specify that snippets are always in the same file.

Understandable :) and what if you have a snippet text edit, followed by a regular text edit? Is a snippet edit guaranteed to be the last element in the array of edits?

[matklad]

@rwols this isn’t something we handle specifically, iirc, the semantics of text-edit in this case (in contrast to incremental document sync) is that you can apply them in parallel, because their ranges are guaranteed to not intersect.

[dbaeumer]

From an LSP perspective this is currently not planned for the 3.17 release. And given the discussion so far implementing this properly will require quite some work. But I do understand the need for something like this.

[DanTup]

@dbaeumer given the Rust spec above, is it reasonable to expect that if LSP did get this officially, it would be implemented in a way that doesn't break servers/clients implementing that spec? (for example not reusing the same capability path unless the implementation is compatible)?

[dbaeumer]

It depends on where the capability is defined. If it is defined in the normal namespace then actually no. The capability has an experimental part into which the spec never puts official capabilities. Or at least scoped capability names (e.g. rust.capability) should be used. I am open to add to the spec that capabilities names in an official release use camel casing and have no dots.

[matklad]

All non-standard extensions of rust-analyzer are advertised via experimental field of capabilities object. Those extensions which are not specific to rust are unprefixed, so that other servers feel free to use them.

This particular extension is experimental, but unperfixed.

The only exception is the "utf8 offsets" extension -- it was originally designed in clangd and does not use experimental field.

[dbaeumer]

Great to hear. The offset encoding will be named positionEncoding to make it the same as in LSIF. So we should be fine there.

[jrieken]

We now documented this as a proper extension: https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/dev/lsp-extensions.md#snippet-textedit

How do you handle the case where snippet edits are specified for multiple files? We only support one snippet insertion at a time. Do you just pick one file/editor? Does your backend guarantee that such cases never occur?

[DanTup]

The capability has an experimental part into which the spec never puts official capabilities

All non-standard extensions of rust-analyzer are advertised via experimental field of capabilities object.

That's perfect - thanks!

[matklad]

@jrieken see https://github.com/microsoft/language-server-protocol/issues/724#issuecomment-785076976, we just don't do this.

[DanTup]

@matklad are you checking insertTextFormat on the edits that come back? I couldn't find it in your code, and I can't see an obvious way to ensure when the vscode-languageclient deserialises the JSON it preserves that field.

Edit: Looks like maybe you're just looking in the newText directly? https://github.com/rust-analyzer/rust-analyzer/blob/0537510aef4059d5f79146a8dc2734ffdb27dc74/editors/code/src/snippets.ts#L61-L63

@dbaeumer is there an existing way to intercept this? It looks like I really want to be running code around here?

[dbaeumer]

@DanTup the only way to intercept this is to intercept it in the middleware and use your own implementation of asWorkspaceEdit

[isovector]

We need this in the Haskell language server. Our typing guarantees are strong enough to do a good deal of program synthesis, based on the only solutions that typecheck. For example, given a function that extracts a value out of a Maybe, we can write:

fromMaybe :: a -> Maybe a -> a
fromMaybe = _

where the _ is a language construct that says "I don't know what to put here, please help me." After running the code action, the language server will implement the only possible function:

fromMaybe :: a -> Maybe a -> a
fromMaybe _ (Just a) = a
fromMaybe a Nothing = a

This works well today, but there are cases in which we can almost synthesize an entire solution, save for a few decisions that need to be made by the user. To take the earlier example, but specialized to integers:

fromMaybeInt :: Maybe Int -> Int
fromMaybeInt = _

running the code action will produce:

fromMaybeInt :: Maybe Int -> Int
fromMaybeInt (Just a) = a
fromMaybeInt Nothing = _

and we'd like to move the cursor to the newly produced _.

[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. https://github.com/microsoft/vscode/issues/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 https://github.com/microsoft/vscode-languageserver-node/pull/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 https://github.com/microsoft/language-server-protocol/issues/724#issuecomment-1895943206

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.