-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Handle textDocument/didChange notifications that don't pass across the range #78165
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |||
| // See the LICENSE file in the project root for more information. | ||||
|
|
||||
| using System; | ||||
| using System.Collections.Immutable; | ||||
| using System.Composition; | ||||
| using System.Linq; | ||||
| using System.Threading; | ||||
|
|
@@ -37,7 +38,7 @@ public TextDocumentIdentifier GetTextDocumentIdentifier(DidChangeTextDocumentPar | |||
| return SpecializedTasks.Default<object>(); | ||||
| } | ||||
|
|
||||
| internal static bool AreChangesInReverseOrder(TextDocumentContentChangeEvent[] contentChanges) | ||||
| internal static bool AreChangesInReverseOrder(ImmutableArray<TextDocumentContentChangeEvent> contentChanges) | ||||
| { | ||||
| for (var i = 1; i < contentChanges.Length; i++) | ||||
| { | ||||
|
|
@@ -53,8 +54,14 @@ internal static bool AreChangesInReverseOrder(TextDocumentContentChangeEvent[] c | |||
| return true; | ||||
| } | ||||
|
|
||||
| private static SourceText GetUpdatedSourceText(TextDocumentContentChangeEvent[] contentChanges, SourceText text) | ||||
| private static SourceText GetUpdatedSourceText(SumType<TextDocumentContentChangeEvent, TextDocumentContentChangeFullReplacementEvent>[] contentChanges, SourceText text) | ||||
| { | ||||
| (var remainingContentChanges, text) = GetUpdatedSouorceTextAndChangesAfterFullTextReplacementHandled(contentChanges, text); | ||||
|
|
||||
| // No range-based changes to apply. | ||||
| if (remainingContentChanges.IsEmpty) | ||||
| return text; | ||||
|
|
||||
| // Per the LSP spec, each text change builds upon the previous, so we don't need to translate any text | ||||
| // positions between changes. See | ||||
| // https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#didChangeTextDocumentParams | ||||
|
|
@@ -63,20 +70,41 @@ private static SourceText GetUpdatedSourceText(TextDocumentContentChangeEvent[] | |||
| // If the host sends us changes in a way such that no earlier change can affect the position of a later change, | ||||
| // then we can merge the changes into a single TextChange, allowing creation of only a single new | ||||
| // source text. | ||||
| if (AreChangesInReverseOrder(contentChanges)) | ||||
| if (AreChangesInReverseOrder(remainingContentChanges)) | ||||
| { | ||||
| // The changes were in reverse document order, so we can merge them into a single operation on the source text. | ||||
| // Note that the WithChanges implementation works more efficiently with it's input in forward document order. | ||||
| var newChanges = contentChanges.Reverse().SelectAsArray(change => ProtocolConversions.ContentChangeEventToTextChange(change, text)); | ||||
| var newChanges = remainingContentChanges.Reverse().SelectAsArray(change => ProtocolConversions.ContentChangeEventToTextChange(change, text)); | ||||
| text = text.WithChanges(newChanges); | ||||
| } | ||||
| else | ||||
| { | ||||
| // The host didn't send us the items ordered, so we'll apply each one independently. | ||||
| foreach (var change in contentChanges) | ||||
| foreach (var change in remainingContentChanges) | ||||
| text = text.WithChanges(ProtocolConversions.ContentChangeEventToTextChange(change, text)); | ||||
| } | ||||
|
|
||||
| return text; | ||||
| } | ||||
|
|
||||
| private static (ImmutableArray<TextDocumentContentChangeEvent>, SourceText) GetUpdatedSouorceTextAndChangesAfterFullTextReplacementHandled(SumType<TextDocumentContentChangeEvent, TextDocumentContentChangeFullReplacementEvent>[] contentChanges, SourceText text) | ||||
| { | ||||
| // Per the LSP spec, each content change can be either a TextDocumentContentChangeEvent or TextDocumentContentChangeFullReplacementEvent. | ||||
| // The former is a range-based change while the latter is a full text replacement. If a TextDocumentContentChangeFullReplacementEvent is found, | ||||
| // then make a full text replacement for that and return all subsequent changes as remaining range-based changes. | ||||
| var lastFullTextChangeEventIndex = contentChanges.Length - 1; | ||||
| for (; lastFullTextChangeEventIndex >= 0; lastFullTextChangeEventIndex--) | ||||
| { | ||||
| var change = contentChanges[lastFullTextChangeEventIndex]; | ||||
| if (change.Value is TextDocumentContentChangeFullReplacementEvent onlyTextEvent) | ||||
| { | ||||
| // Found a full text replacement. Create the new text and stop processing. | ||||
| text = text.WithChanges([new TextChange(new TextSpan(0, text.Length), onlyTextEvent.Text)]); | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why doing the WithChanges versus just creating a new text from scratch?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't want to have to duplicate the parameter knowledge involved with doing that, as done here:
|
||||
| break; | ||||
| } | ||||
| } | ||||
|
|
||||
| var remainingContentChanges = contentChanges.Skip(lastFullTextChangeEventIndex + 1).SelectAsArray(c => c.First); | ||||
| return (remainingContentChanges, text); | ||||
| } | ||||
| } | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ internal sealed class TextDocumentContentChangeEvent | |
| /// Gets or sets the range of the text that was changed. | ||
| /// </summary> | ||
| [JsonPropertyName("range")] | ||
| [JsonRequired] | ||
| public Range Range | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than messing with the SumTypes, could we have just made this nullable?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The spec defines it as a union type, so we should use a union type. They do happen to share the same field names right now, but that isn't a guarantee for the futre |
||
| { | ||
| get; | ||
|
|
@@ -39,6 +40,7 @@ public int? RangeLength | |
| /// Gets or sets the new text of the range/document. | ||
| /// </summary> | ||
| [JsonPropertyName("text")] | ||
| [JsonRequired] | ||
| public string Text | ||
| { | ||
| get; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| namespace Roslyn.LanguageServer.Protocol; | ||
|
|
||
| using System.Text.Json.Serialization; | ||
|
|
||
| internal sealed class TextDocumentContentChangeFullReplacementEvent | ||
| { | ||
| /// <summary> | ||
| /// Gets or sets the new text of the document. | ||
| /// </summary> | ||
| [JsonPropertyName("text")] | ||
| [JsonRequired] | ||
| public string Text | ||
| { | ||
| get; | ||
| set; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.