Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is the any form of autocompletion/insertion in the LSP? #715

Open
NikolasKomonen opened this issue Apr 5, 2019 · 35 comments
Open

Is the any form of autocompletion/insertion in the LSP? #715

NikolasKomonen opened this issue Apr 5, 2019 · 35 comments
Labels
completion feature-request Request for new features or functionality
Milestone

Comments

@NikolasKomonen
Copy link

Im working on the XML Language Server and we've implemented a custom service to auto insert the end tag of an element.

Start with:

    <a

Type the '>' character:

    <a>

Without any user interaction '</a>' is automatically inserted:

    <a> </a>

Does something currently exist/is being considered in the protocol that would provide the same functionality?

@kdvolder
Copy link

kdvolder commented Apr 5, 2019

I don't think there's anything that exists specifically for this. But I think that you could probably use workspace/applyEdit to apply automatic changes in response to what a user typed.

I.e. your server will be listening for document change events and you will receive these when a user types something. Then in response to these events you could use workspace/applyEdit to trigger other automatic changes.

https://microsoft.github.io/language-server-protocol/specification#workspace_applyEdit

@mickaelistria
Copy link

mickaelistria commented Apr 5, 2019 via email

@angelozerr
Copy link
Contributor

Thanks guys for your feedback. The main problem with our XML Language Server is that it doesn't support incremental parsing (like the VSCode HTML LSP). So it think it will be hard to implement applyEdit in this case.

And I think it's one reason why VSode HTML lsp provide too a custom command to manage this auto insert tag.

@NikolasKomonen I have started to implement basic incremental parsing that you can try, but it's very very basic and I think there are some bugs. once this incremental parsing will be available, I think we will abele to manage applyEdit.

@mickaelistria
Copy link

I don't think you need incremental parsing for this to be implemented.
I guess many cases are just about comparing the current snapshot with the new document edit and the diff (usually a single character) can be enough to know whether to send an applyEdit or not. The LSP spec itself doesn't require anything more to make it possible.
Dedicated commands are an anti-pattern of LSP and should be avoided as much as possible as they require all clients to create specific implementation for it. Basically, it brings back to a N*M complexity and cost, which we should avoid as much as possible.

@angelozerr
Copy link
Contributor

I don't think you need incremental parsing for this to be implemented.

Are you sure? My understand is to track change of document and trigger an event for applyEdit to manage auto close. To track document change, LSP provide didChange. In you case we need to get the change of the document and not the whole document content. As we are in non incremental support we receive the whole content of the document on the didChange and not only the last change.

@mickaelistria
Copy link

In you case we need to get the change of the document and not the whole document content.

You can store the current content of the document in the LS, and when you receive didChange, diff the new content with the stored one to see what's the delta. From there, you can find which character was inserted and decide whether to send an applyEdit if it's delta is an inserted >.

@angelozerr
Copy link
Contributor

You can store the current content of the document in the LS,

We have already that.

diff the new content with the stored one to see what's the delta.

I would like to avoid doing that. We have today a big problem of performance with large XML file (same problem with HTML LS). As soon as you type some character in large file, the editor freezes because the whole content is pushed from client to server. I would like to support incremental support (already done but not stable for the moment) and after we will able to manage applyEdit with easy mean.

@mickaelistria
Copy link

We have today a big problem of performance with large XML file (same problem with HTML LS). As soon as you type some character in large file, the editor freezes because the whole content is pushed from client to server.

This seems like an issue related to transfer, and not to storage. So I don't think storing the document would have an impact on the duration of the transfer.
About freezing the editor, you may want to report bugs to the IDE where you see the freeze. Freezes are implementation issues on client side.

I would like to support incremental support (already done but not stable for the moment) and after we will able to manage applyEdit with easy mean.

Sure, that would be the best solution.

In any case, it seems like we agree there is no need for extra-feature in LSP and this ticket can be closed ?

@angelozerr
Copy link
Contributor

This seems like an issue related to transfer, and not to storage.

Exactly!

About freezing the editor, you may want to report bugs to the IDE

VSCode has the same problem.

Sure, that would be the best solution.

We are agree, that's cool:)

In any case, it seems like we agree there is no need for extra-feature in LSP and this ticket can be closed ?

Please wait for @NikolasKomonen answer to be sure that we can close it.

@NikolasKomonen
Copy link
Author

My main issue was with possible performance issues with using a diff. I'll implement this method and report back with my findings.

@angelozerr
Copy link
Contributor

@NikolasKomonen IMHO instead of implementing a diff, I suggest you that you activate incremental support and fix bugs of this feature.

@LaurentTreguier
Copy link
Contributor

LaurentTreguier commented Apr 8, 2019

I second @angelozerr's proposition; in my own language server I implemented a basic diff algorithm somewhere, and my code for incremental support is shorter and simpler than that

@NikolasKomonen
Copy link
Author

@angelozerr Are the bugs documented anywhere? If not, could you list some known issues?

@angelozerr
Copy link
Contributor

Are the bugs documented anywhere? If not, could you list some known issues?

Unfortunately, no -( I'm really sorry, I had implemented that quickly. See eclipse/lemminx#133 for incremental support issue.

@matklad
Copy link
Contributor

matklad commented Apr 11, 2019

In rust-anlyzer, I [ab]use OnTypeFormatting to do this: https://github.com/rust-analyzer/rust-analyzer/blob/e6e2571bdf780d304c792d4317bbaf1d6f5d7a0a/crates/ra_lsp_server/src/main_loop/handlers.rs#L94-L122. It works pretty well, except for the fact that I can't intercept \n with it.

@dbaeumer
Copy link
Member

Let me loop in @aeschli. He implements the HTML language server. If something is missing to make the LSP nice here I am open to do so.

@NikolasKomonen
Copy link
Author

I would like to note that we've implemented the same work-around used in the HTML language server.

@mickaelistria
Copy link

What makes you say it's a workaround?

@NikolasKomonen
Copy link
Author

NikolasKomonen commented Apr 11, 2019

@mickaelistria

What makes you say it's a workaround?

IMO if it requires a custom command/ isn't using a dedicated/intended LSP command.

@mickaelistria
Copy link

What would such a command do better or more easily than an "applyEdit"? What would be the input/output to better serve the purpose? What would be the sequence of message?

@NikolasKomonen
Copy link
Author

NikolasKomonen commented Apr 11, 2019

I think applyEdit is fine as a response. My search was more for a dedicated command/client-request that would be sent to the server with information such as the last character change(s), and defined server capabilities for auto-completion trigger characters.

@kdvolder
Copy link

with information such as the last character change(s)

All you really have to do is enable 'incremental' document change events. Then the client will not send you whole documents but only the small changes that happen as a user types. Disabling this and then trying to compute your own diffs seems like much more difficult than just telling the client you want to be receiving the diffs. Then it is up to you to applying these diffs to your locally stored copy of the document. But that really isn't terribly hard. To boot it may avoid a lot of the performance issues you are talking about with large documents. (It is going to be a lot more efficient for the client to send you a small one character diff instead of the whole document contents every time user types a character in the editor).

@angelozerr
Copy link
Contributor

Disabling this and then trying to compute your own diffs seems like much more difficult than just telling the client you want to be receiving the diffs.

Totally agree with you.

Then it is up to you to applying these diffs to your locally stored copy of the document.

It's exactly that I have started experimenting in lsp4xml https://github.com/angelozerr/lsp4xml/blob/master/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/commons/TextDocument.java#L139

@NikolasKomonen I'm sorry to insist, but IMHO we should try to enable this incremental support (we must write tests for that)

@NikolasKomonen
Copy link
Author

@angelozerr Don't worry, I am still going to to work on incremental support.

My main concern is with the protocol in general. Without a dedicated auto-completion command, servers are forced to implement incremental parsing if they want the ability to autocomplete.

@angelozerr
Copy link
Contributor

@angelozerr Don't worry, I am still going to to work on incremental support.

It's a really cool news. Thanks!

My main concern is with the protocol in general. Without a dedicated auto-completion command, servers are forced to implement incremental parsing if they want the ability to autocomplete.

Ok

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Apr 25, 2019
@dbaeumer
Copy link
Member

Pinging @aeschli again. Is there anything you would recommend from your HTML work that would make the LSP better in this case.

@aeschli
Copy link
Contributor

aeschli commented Apr 25, 2019

I would also favour a dedicated LSP-request.. APIwise it would look a lot like OnTypeFormatting. Actually I wasn't aware that OnTypeFormatting is so similar. But I always thought that 'formatting' should only add/remove whitespaces...

@dbaeumer
Copy link
Member

dbaeumer commented Apr 25, 2019

When would a client trigger such a request? On typing like with formatting

@aeschli
Copy link
Contributor

aeschli commented Apr 25, 2019

There's a set of trigger characters are maybe character sequences that the server will tell the client on initialization.
The client will only make a 'autocomplete' request when such a character is entered by typing or pasting and the cursor get to lie after that character.
No triggering on redo / undo. Unclear to me whether programmatic document changes should trigger this. Not really important for the LSP discussion here, as it will be up to the clients to figure out when to trigger and when not. But it's a tricky issue that we haven't solved in HTML as our extensions don't have an API to know from where a document change originated. See our debt item .

@NikolasKomonen
Copy link
Author

What about adding a flag to CompletionItem's to indicate if it should be automatically inserted?


Separately, another possible requirement for the protocol and auto completion could be access to the previous AST before the trigger character was inserted. I'm assuming textDocument/didChange has priority over all capabilities, and the old AST would have been over written before autoComplete had time to analyse it.

An example in our XML language server is the automatic removal of end tags after the starting one is
self closed:

<a></a>
<a/></a>   <-- insert '/'
<a/>

It would be a lot easier to check the old AST, than the new invalid one:

  • if an end tag already existed
  • or that there was no content within the element.

This might be a small use case, but could be something to consider.

@dbaeumer dbaeumer added feature-request Request for new features or functionality and removed info-needed Issue requires more information from poster labels Oct 31, 2019
@dbaeumer dbaeumer added this to the Backlog milestone Oct 31, 2019
@a-h
Copy link

a-h commented May 31, 2021

I was looking to implement auto-close features for my Go templating language templ.

I initially implemented it using workspaceEdit but found that it didn't work well because I wanted to insert text after the caret. The issue was I'd sometimes end up overwriting text that was after the caret, or if I did an insert, the caret moved to the end of the newly inserted text rather than inside the HTML element, which wasn't a great editing experience.

I realised it was simpler to use the autocompletion snippets instead, because you can use snippet syntax to set the final caret position with the $0 placeholder. Maybe it's not the same interaction you're looking for, but I think it works well for my use case.

So far, I've implemented a couple of HTML tags and the templating language constructs, the migration between the two approaches (document change vs snippets) is in this commit: a-h/templ@15e40f0

The result looks like this in VS Code:

snippet

@dbaeumer
Copy link
Member

dbaeumer commented Jun 3, 2021

@aeschli might help you here since he implemented something custom for HTML as well.

@mickaelistria
Copy link

I now see many language servers have custom operations for that. The use-case seems common enough and there is now enough maturity to probably extract a standard solution from the current workarounds.
LemMinX has xml/closeTag and HTML Language Server has (autoQuote|autoClose)[https://github.com/microsoft/vscode/blob/main/extensions/html-language-features/client/src/autoInsertion.ts#L67]. Both are actually mostly sending the same as applyEdit but allowing a snippet. @a-h 's comment above would also work with it I believe.
Some benefit would be that clients wouldn't have to throw a particular request, it would be pure-LS side to notify when it has an edit to suggest.

So maybe the simplest thing to do wold be to add to applyEdit the capability to send snippets?

@rchl
Copy link
Contributor

rchl commented Sep 13, 2022

@mickaelistria
Copy link

Isn't this already supported through onTypeFormatting

This doesn't seem to use snippets, which are necessary for the use-cases mentioned above.
but thinking much further, we can wonder whether TextEdit could/should be the data object capable of supporting snippets. That would allow snippets to work everywhere, and solve this story and many others...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completion feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

10 participants