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

Discard LSP requests/notifcation that target outdated versions #6282

Merged
merged 3 commits into from
Mar 16, 2023

Conversation

pascalkuthe
Copy link
Member

@pascalkuthe pascalkuthe commented Mar 12, 2023

This is a slight followup to #6173

I noticed that similar bugs could occur basically anytime the LS sends an edit to the client. The user can always add some new text which makes the received text invalid. However, for these edits the LSP standard actually includes a mechanism to deal with the problem: Whenever we send a change a notification to the server, the client must attach a unique version to the resulting document state.

If the LSP server sends an edit to the client, that edit includes the document version it applies to and if that version doesn't match the current version then that change can not be applied/should be discarded.

For workspace edits specifically there are actually many capabilities which include the exact behavior here, but we didn't set any of them. Even worse the client is supposed to return that applying the edits failed but we always returned a success. I have fixed that by implemented the abort failure mechanism for now (and setting the capabilities appropriately) as it was the easiest to implement (basically just propagating a Result with ?). In the future we likely want to check whether a WorkspaceEdit is valid before we start apply it and switch to transactional failure handling. I think that is how VSCode handles it. This way we won't end-up with half-applied changes (a good LS would resend the missing edits, but I suspect many don't). Implementing this is a bit more work so I left that to future work. The current failure handling strategy is spec compliant and avoids apply incorrect edits/crashes so I wanted a quick fix for that first.

PublishDiagnostic notifications also send a document version and this PR also adds a check to drop outdated PublishDiagnostic notifications. This fixes diagnostics "lagging behind" while typing.

Future Work

I noticed that we don't normalize line endings while applying LSP edits. We currently do this for snippets but not anywhere else. Furthermore, we also need to normalize indentation (replace tabs with spaces and vice versa). I will create an issue for that. Once we do that we should also set the appropriate capability to true (I set it to false for now to reflect what we are doing

@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. E-easy Call for participation: Experience needed to fix: Easy / not much A-language-server Area: Language server client C-bug Category: This is a bug labels Mar 12, 2023
@pascalkuthe pascalkuthe marked this pull request as draft March 12, 2023 14:28
@pascalkuthe

This comment was marked as outdated.

@pascalkuthe pascalkuthe marked this pull request as ready for review March 12, 2023 14:45
Copy link
Member

@dead10ck dead10ck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had one minor question, but this all looks good to me, thanks for fixing these!

@the-mikedavis the-mikedavis self-requested a review March 12, 2023 16:48
@archseer archseer merged commit 7bf168d into helix-editor:master Mar 16, 2023
@pascalkuthe pascalkuthe deleted the lsp_version branch March 16, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants