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

[LSP] textDocument/didChange handler behavior doesn't follow lsp spec #70392

Closed
jmederosalvarado opened this issue Oct 15, 2023 · 5 comments · Fixed by #70407
Closed

[LSP] textDocument/didChange handler behavior doesn't follow lsp spec #70392

jmederosalvarado opened this issue Oct 15, 2023 · 5 comments · Fixed by #70407
Assignees
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead

Comments

@jmederosalvarado
Copy link

When handling textDocument/didChange with a list of changes, these are being applied to the document "at once" based on the previous document version, moreover, the implementation currently assumes that these changes are sorted. The current behavior matches this description. However the lsp spec for this clearly states that changes should be applied sequentially and each change N acts on the resulting state after applying change 0...N-1.

The relevant code for the current behavior can be found here and here.

For example the following code should print ahbello where it should print abhello:

var text = SourceText.From("hello");

var changes = new[]
{
    new TextChange(TextSpan.FromBounds(0, 0), "a"),
    new TextChange(TextSpan.FromBounds(1, 1), "b"),
};

var changed = text.WithChanges(changes);

Console.WriteLine(changed); // should print "abhello", but prints "ahbello"
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 15, 2023
@CyrusNajmabadi
Copy link
Member

That does seem like a problem. We should check with @kayle to make sure that VS LSP client is following the spec here as well before changing anything

@jmederosalvarado
Copy link
Author

Thanks @CyrusNajmabadi, just to give you a little bit more context, this is causing issues when trying to use an lsp client other than vscode's, for example neovim.

@kayle
Copy link

kayle commented Oct 16, 2023

I took a quick look at the VS LSP client, and from what I can tell it follows the spec. Note in particular that if you make several edits to a document in a single edit, the didChange notification may represent those edits differently as long as the logical resulting text is the same. Roslyn certainly should not reorder the changes in a didChange notification since that could change the resulting text.

@jmederosalvarado
Copy link
Author

jmederosalvarado commented Oct 17, 2023

Note in particular that if you make several edits to a document in a single edit, the didChange notification may represent those edits differently as long as the logical resulting text is the same

@kayle what exactly do you mean here? The LSP spec states that if a didChange message contains multiple edits, these should be applied sequentially by the server, and each edit should be applied on the result of the previous ones.

Roslyn computes the text positions where these edits should be applied using the same text state for every edit in the didChange message. See the code here.

For example, consider a didChange message contains two edits c1 that acts on range [0:0,0:0] and c2 that acts on range [0:1,0:1]. Roslyn currently computes the positions in the text model where c1 and c2 should be applied upon receiving the notification, which means that c2 will be applied on 0:1 (column 1) of the original text state and not on column 1 of the text resulting from applying c1, the latter is the correct behavior.

Roslyn certainly should not reorder the changes in a didChange notification since that could change the resulting text.

Roslyn orders the changes in a didChange notification. Moreover, it doesn't allow for overlapping changes, because it assumes that each change doesn't act upon the previous one, but rather all changes in a didChange message act upon the same original text.

For example, consider a didChange message contains two edits c1 that insets on range [0:0,0:0] and c2 that also inserts on range [0:0,0:0]. The current implementation throws an exception because it considers these edits as overlapping (they are both acting on the same segment of the original text). However the expected behavior is that c2 inserts at column 0 of whatever text results from c1 inserting at column 0.

@CyrusNajmabadi
Copy link
Member

@jmederosalvarado he means that the lsp changes you hear about may not correspond precisely to the ITextChanges in the actual snapshot.

The vs text model is much more precise and fine-grained. To match the lsp spec it may have to do things like consolidate discontinuous changes into single changes.

That's totally fine here, and we'll update our impl to match the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
4 participants