microsoft / language-server-protocol

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

Add snippet text edit specification #1892

Closed MariaSolOs closed 4 months ago

MariaSolOs commented 5 months ago

Follow-up of https://github.com/microsoft/vscode-languageserver-node/pull/1343. This time it actually closes https://github.com/microsoft/language-server-protocol/issues/724.

rchl commented 5 months ago

Can you also check out my questions in https://github.com/microsoft/language-server-protocol/issues/724#issuecomment-1895943206 and potentially address in the spec?

dbaeumer commented 5 months ago

@rchl I commented here: https://github.com/microsoft/language-server-protocol/issues/724#issuecomment-1921011006

jwortmann commented 5 months ago

To make snippets with multiple tab stops (https://github.com/microsoft/language-server-protocol/issues/724#issuecomment-483206386) work properly, clients likely need to use built-in editor functionality to insert the text as a snippet. This might imply that the editor automatically re-indents inserted text which contains multiple lines; for example it seems to be the case in VSCode (https://github.com/microsoft/language-server-protocol/issues/724#issuecomment-631558796), and it also happens in Sublime Text. A similar situation for when snippets are inserted as part of autocompletions is solved by the client announcing the insertTextMode and insertTextModeSupport properties in its completion capability. Will this be handled for SnippetTextEdit in a similar manner, so that servers know how to format the whitespace in multi-line snippets?

An alternative would be to declare in the specs that SnippetTextEdits are only allowed to contain a single tab stop. Then clients could do a raw insert of the text and after that manually modify the cursor position in a second step.

MariaSolOs commented 5 months ago

@dbaeumer Should I make any changes to the metamodel, or is it done automatically?

MariaSolOs commented 4 months ago

@dbaeumer with https://github.com/microsoft/vscode-languageserver-node/issues/1414 fixed, I think this PR is ready :)

dbaeumer commented 4 months ago

I think we should add some additional clarification to the spec about some expectations. Things I have in mind:

Since snippets it workspace edits are most likely used with code actions there will be an open active editor to which snippets can be applied.

@MariaSolOs can you add this to the PR?

MariaSolOs commented 4 months ago
  • if a snippet is specified for the non active editor the client should downgrade the snippet to a normal text edit and apply it to the file. This avoids that a workspace edit when applied opens up random files.

By "apply it to the file", do you mean the currently open editor, or the closed one specified by the snippet edit?

If it's the currently opened editor, that sounds counterintuitive since the server (and user) expect the change to happen in the specified editor.

If it's the non-active editor, how does that prevent the workspace from opening up random files?

dbaeumer commented 4 months ago

By "apply it to the file", do you mean the currently open editor, or the closed one specified by the snippet edit?

No to the file that is specific to the snippet edit without opening the file in the editor. This is what is done for normal text edits as well.

jwortmann commented 4 months ago

Just to confirm, because there wasn't any response to my question from https://github.com/microsoft/language-server-protocol/pull/1892#issuecomment-1921520641, and it's unclear from the text in the specs:

Is it correct to assume that language servers have to figure out by themselves (for examply using the clientInfo property from the initialize request) what kind of indentation (InsertTextMode) the client expects for SnippetTextEdits with multiple tab stops?

dbaeumer commented 4 months ago

@jwortmann sorry for that. The InsertTextMode is only valid for completion items. Workspace edits in general don't have support for automatic indentation. So they are always asIs.

jwortmann commented 4 months ago

@jwortmann sorry for that. The InsertTextMode is only valid for completion items. Workspace edits in general don't have support for automatic indentation. So they are always asIs.

The problem with asIs is, that it is simply not possible to imlement at the client side. At least not for clients which are implemented in form of a third-party plugin, relying on the API and built-in functionalities from the editor, and in case the snippet functionality automatically adds the whitespace from the current indentation level to the whitespace provided in the snippet content. For example this happens in Sublime Text, where snippets can be inserted via a built-in command like

view.run_command('insert_snippet', {'contents': edit['snippet']['value']})

and without any means to change indentation behavior. I wonder how does VS Code solve this problem, because the comment I linked above (https://github.com/microsoft/language-server-protocol/issues/724#issuecomment-631558796) indicates that the same happens in VS Code. Also compare https://github.com/microsoft/language-server-protocol/issues/83#issuecomment-269763139 explaining the "magic indentation" that happens in VS Code. As far as I understand, insertTextMode was introduced to make multi-line completions work properly in clients which have no direct control about this. Now the same problem will happen with SnippetTextEdits if they contain multiple lines.

I described a possible workaround in my comment above, which should be sufficient if setting the final cursor position is the only goal of the SnippetTextEdit:

An alternative would be to declare in the specs that SnippetTextEdits are only allowed to contain a single tab stop. Then clients could do a raw insert of the text and after that manually modify the cursor position in a second step.

But in that case this should be explained in the specs. Because currently the snippet content only has a link to the StringValue, and its description explicitly mentions multiple tab stops.