Skip to content
This repository was archived by the owner on Nov 4, 2024. It is now read-only.

Commit 1478379

Browse files
author
Mikhail Arkhipov
authored
Implement proper line split + tests (microsoft#1544)
* Implement proper line split + tests * Out of order test * Rename method * Unused code
1 parent 541264b commit 1478379

File tree

5 files changed

+197
-82
lines changed

5 files changed

+197
-82
lines changed

‎src/Analysis/Ast/Impl/Documents/DocumentBuffer.cs‎

Lines changed: 29 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
// See the Apache Version 2.0 License for specific language governing
1414
// permissions and limitations under the License.
1515

16-
using System;
1716
using System.Collections.Generic;
1817
using System.Linq;
1918
using System.Text;
@@ -47,23 +46,18 @@ public void Update(IEnumerable<DocumentChange> changes) {
4746
lock (_lock) {
4847
_sb = _sb ?? new StringBuilder(_content);
4948

50-
var lastStart = int.MaxValue;
51-
var lineLoc = SplitLines(_sb).ToArray();
52-
5349
foreach (var change in changes) {
50+
// Every change may change where the lines end so in order
51+
// to correctly determine line/offsets we must re-split buffer
52+
// into lines after each change.
53+
var lineLoc = GetNewLineLications().ToArray();
54+
5455
if (change.ReplaceAllText) {
5556
_sb = new StringBuilder(change.InsertedText);
56-
lastStart = int.MaxValue;
57-
lineLoc = SplitLines(_sb).ToArray();
5857
continue;
5958
}
6059

6160
var start = NewLineLocation.LocationToIndex(lineLoc, change.ReplacedSpan.Start, _sb.Length);
62-
if (start > lastStart) {
63-
throw new InvalidOperationException("changes must be in reverse order of start location");
64-
}
65-
lastStart = start;
66-
6761
var end = NewLineLocation.LocationToIndex(lineLoc, change.ReplacedSpan.End, _sb.Length);
6862
if (end > start) {
6963
_sb.Remove(start, end - start);
@@ -78,20 +72,33 @@ public void Update(IEnumerable<DocumentChange> changes) {
7872
}
7973
}
8074

81-
private static IEnumerable<NewLineLocation> SplitLines(StringBuilder text) {
82-
NewLineLocation nextLine;
83-
84-
// TODO: Avoid string allocation by operating directly on StringBuilder
85-
var str = text.ToString();
75+
public IEnumerable<NewLineLocation> GetNewLineLications() {
76+
_sb = _sb ?? new StringBuilder(_content); // for tests
8677

87-
var lastLineEnd = 0;
88-
while ((nextLine = NewLineLocation.FindNewLine(str, lastLineEnd)).EndIndex != lastLineEnd) {
89-
yield return nextLine;
90-
lastLineEnd = nextLine.EndIndex;
78+
if (_sb.Length == 0) {
79+
yield return new NewLineLocation(0, NewLineKind.None);
9180
}
9281

93-
if (lastLineEnd != str.Length) {
94-
yield return nextLine;
82+
for (var i = 0; i < _sb.Length; i++) {
83+
var ch = _sb[i];
84+
var nextCh = i < _sb.Length - 1 ? _sb[i + 1] : '\0';
85+
switch (ch) {
86+
case '\r' when nextCh == '\n':
87+
i++;
88+
yield return new NewLineLocation(i + 1, NewLineKind.CarriageReturnLineFeed);
89+
break;
90+
case '\n':
91+
yield return new NewLineLocation(i + 1, NewLineKind.LineFeed);
92+
break;
93+
case '\r':
94+
yield return new NewLineLocation(i + 1, NewLineKind.CarriageReturn);
95+
break;
96+
default:
97+
if (i == _sb.Length - 1) {
98+
yield return new NewLineLocation(i + 1, NewLineKind.None);
99+
}
100+
break;
101+
}
95102
}
96103
}
97104
}

‎src/Analysis/Ast/Test/DocumentBufferTests.cs‎

Lines changed: 162 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,12 @@
1515

1616
using System;
1717
using System.Collections.Generic;
18+
using System.Linq;
19+
using System.Reflection;
1820
using FluentAssertions;
1921
using Microsoft.Python.Analysis.Documents;
2022
using Microsoft.Python.Core.Text;
23+
using Microsoft.Python.Parsing;
2124
using Microsoft.VisualStudio.TestTools.UnitTesting;
2225

2326
namespace Microsoft.Python.Analysis.Tests {
@@ -77,7 +80,7 @@ public void ResetDocumentBuffer() {
7780
doc.Reset(0, string.Empty);
7881
Assert.AreEqual(string.Empty, doc.Text);
7982

80-
doc.Update(new[] {
83+
doc.Update(new[] {
8184
DocumentChange.Insert("text", SourceLocation.MinValue)
8285
});
8386

@@ -127,5 +130,163 @@ public void ReplaceAllDocumentBuffer() {
127130
Assert.AreEqual(@"text1234", doc.Text);
128131
Assert.AreEqual(4, doc.Version);
129132
}
133+
134+
[TestMethod, Priority(0)]
135+
public void DeleteMultipleDisjoint() {
136+
var doc = new DocumentBuffer();
137+
doc.Reset(0, @"
138+
line1
139+
line2
140+
line3
141+
line4
142+
");
143+
doc.Update(new[] {
144+
DocumentChange.Delete(new SourceSpan(5, 5, 5, 6)),
145+
DocumentChange.Delete(new SourceSpan(4, 5, 4, 6)),
146+
DocumentChange.Delete(new SourceSpan(3, 5, 3, 6)),
147+
DocumentChange.Delete(new SourceSpan(2, 5, 2, 6))
148+
});
149+
Assert.AreEqual(@"
150+
line
151+
line
152+
line
153+
line
154+
", doc.Text);
155+
}
156+
157+
[TestMethod, Priority(0)]
158+
public void InsertMultipleDisjoint() {
159+
var doc = new DocumentBuffer();
160+
doc.Reset(0, @"
161+
line
162+
line
163+
line
164+
line
165+
");
166+
doc.Update(new[] {
167+
DocumentChange.Insert("4", new SourceLocation(5, 5)),
168+
DocumentChange.Insert("3", new SourceLocation(4, 5)),
169+
DocumentChange.Insert("2", new SourceLocation(3, 5)),
170+
DocumentChange.Insert("1", new SourceLocation(2, 5)),
171+
});
172+
Assert.AreEqual(@"
173+
line1
174+
line2
175+
line3
176+
line4
177+
", doc.Text);
178+
}
179+
180+
[TestMethod, Priority(0)]
181+
public void DeleteAcrossLines() {
182+
var doc = new DocumentBuffer();
183+
doc.Reset(0, @"
184+
line1
185+
line2
186+
line3
187+
line4
188+
");
189+
doc.Update(new[] {
190+
DocumentChange.Delete(new SourceSpan(4, 5, 5, 5)),
191+
DocumentChange.Delete(new SourceSpan(2, 5, 3, 5)),
192+
});
193+
Assert.AreEqual(@"
194+
line2
195+
line4
196+
", doc.Text);
197+
}
198+
199+
[TestMethod, Priority(0)]
200+
public void SequentialChanges() {
201+
var doc = new DocumentBuffer();
202+
doc.Reset(0, @"
203+
line1
204+
line2
205+
line3
206+
line4
207+
");
208+
doc.Update(new[] {
209+
DocumentChange.Delete(new SourceSpan(2, 5, 3, 5)),
210+
DocumentChange.Delete(new SourceSpan(3, 5, 4, 5))
211+
});
212+
Assert.AreEqual(@"
213+
line2
214+
line4
215+
", doc.Text);
216+
}
217+
218+
[TestMethod, Priority(0)]
219+
public void InsertTopToBottom() {
220+
var doc = new DocumentBuffer();
221+
doc.Reset(0, @"linelinelineline");
222+
doc.Update(new[] {
223+
DocumentChange.Insert("\n", new SourceLocation(1, 1)),
224+
DocumentChange.Insert("1\n", new SourceLocation(2, 5)),
225+
DocumentChange.Insert("2\r", new SourceLocation(3, 5)),
226+
DocumentChange.Insert("3\r\n", new SourceLocation(4, 5)),
227+
DocumentChange.Insert("4\r\n", new SourceLocation(5, 5)),
228+
});
229+
Assert.AreEqual("\nline1\nline2\rline3\r\nline4\r\n", doc.Text);
230+
}
231+
232+
[NewLineTestData]
233+
[DataTestMethod, Priority(0)]
234+
public void NewLines(string s, NewLineLocation[] expected) {
235+
var doc = new DocumentBuffer();
236+
doc.Reset(0, s);
237+
var nls = doc.GetNewLineLications().ToArray();
238+
for (var i = 0; i < nls.Length; i++) {
239+
Assert.AreEqual(nls[i].Kind, expected[i].Kind);
240+
Assert.AreEqual(nls[i].EndIndex, expected[i].EndIndex);
241+
}
242+
}
243+
244+
private sealed class NewLineTestDataAttribute : Attribute, ITestDataSource {
245+
public IEnumerable<object[]> GetData(MethodInfo methodInfo) =>
246+
new List<object[]> {
247+
new object[] {
248+
string.Empty,
249+
new NewLineLocation[] {
250+
new NewLineLocation(0, NewLineKind.None),
251+
}
252+
},
253+
new object[] {
254+
"\r\r",
255+
new NewLineLocation[] {
256+
new NewLineLocation(1, NewLineKind.CarriageReturn),
257+
new NewLineLocation(2, NewLineKind.CarriageReturn)
258+
}
259+
},
260+
new object[] {
261+
"\n\n",
262+
new NewLineLocation[] {
263+
new NewLineLocation(1, NewLineKind.LineFeed),
264+
new NewLineLocation(2, NewLineKind.LineFeed)
265+
}
266+
},
267+
new object[] {
268+
"\r\n\n\r",
269+
new NewLineLocation[] {
270+
new NewLineLocation(2, NewLineKind.CarriageReturnLineFeed),
271+
new NewLineLocation(3, NewLineKind.LineFeed),
272+
new NewLineLocation(4, NewLineKind.CarriageReturn)
273+
}
274+
},
275+
new object[] {
276+
"a\r\nb\r\n c\r d\ne",
277+
new NewLineLocation[] {
278+
new NewLineLocation(3, NewLineKind.CarriageReturnLineFeed),
279+
new NewLineLocation(6, NewLineKind.CarriageReturnLineFeed),
280+
new NewLineLocation(9, NewLineKind.CarriageReturn),
281+
new NewLineLocation(12, NewLineKind.LineFeed),
282+
new NewLineLocation(13, NewLineKind.None)
283+
}
284+
}
285+
};
286+
public string GetDisplayName(MethodInfo methodInfo, object[] data)
287+
=> data != null ? $"{methodInfo.Name} ({FormatName((string)data[0])})" : null;
288+
289+
private static string FormatName(string s) => s.Replace("\n", "\\n").Replace("\r", "\\r");
290+
}
130291
}
131292
}

‎src/LanguageServer/Impl/Implementation/Server.Documents.cs‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,10 @@
1313
// See the Apache Version 2.0 License for specific language governing
1414
// permissions and limitations under the License.
1515

16+
using System;
1617
using System.Collections.Generic;
1718
using System.Diagnostics;
19+
using System.Linq;
1820
using Microsoft.Python.Analysis.Documents;
1921
using Microsoft.Python.Core;
2022
using Microsoft.Python.LanguageServer.Protocol;
@@ -32,15 +34,10 @@ public void DidOpenTextDocument(DidOpenTextDocumentParams @params) {
3234

3335
public void DidChangeTextDocument(DidChangeTextDocumentParams @params) {
3436
_disposableBag.ThrowIfDisposed();
35-
var uri = @params.textDocument.uri;
36-
var doc = _rdt.GetDocument(uri);
37+
var doc = _rdt.GetDocument(@params.textDocument.uri);
3738
if (doc != null) {
38-
var changes = new List<DocumentChange>();
39-
foreach (var c in @params.contentChanges) {
40-
var change = c.range.HasValue ? DocumentChange.Replace(c.range.Value, c.text) : DocumentChange.ReplaceAll(c.text);
41-
changes.Add(change);
42-
}
43-
doc.Update(changes);
39+
doc.Update(@params.contentChanges
40+
.Select(c => c.range.HasValue ? DocumentChange.Replace(c.range.Value, c.text) : DocumentChange.ReplaceAll(c.text)));
4441
_indexManager.AddPendingDoc(doc);
4542
} else {
4643
_log?.Log(TraceEventType.Warning, $"Unable to find document for {@params.textDocument.uri}");

‎src/LanguageServer/Impl/LanguageServer.cs‎

Lines changed: 1 addition & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -129,57 +129,10 @@ public async Task DidChangeTextDocument(JToken token, CancellationToken cancella
129129
_idleTimeTracker?.NotifyUserActivity();
130130
using (await _prioritizer.DocumentChangePriorityAsync(cancellationToken)) {
131131
var @params = ToObject<DidChangeTextDocumentParams>(token);
132-
var version = @params.textDocument.version;
133-
if (version == null || @params.contentChanges.IsNullOrEmpty()) {
134-
_server.DidChangeTextDocument(@params);
135-
return;
136-
}
137-
138-
// _server.DidChangeTextDocument can handle change buckets with decreasing version and without overlapping
139-
// Split change into buckets that will be properly handled
140-
var changes = SplitDidChangeTextDocumentParams(@params, version.Value);
141-
142-
foreach (var change in changes) {
143-
_server.DidChangeTextDocument(change);
144-
}
145-
}
146-
}
147-
148-
private static IEnumerable<DidChangeTextDocumentParams> SplitDidChangeTextDocumentParams(DidChangeTextDocumentParams @params, int version) {
149-
var changes = new Stack<DidChangeTextDocumentParams>();
150-
var contentChanges = new Stack<TextDocumentContentChangedEvent>();
151-
var previousRange = new Range();
152-
153-
for (var i = @params.contentChanges.Length - 1; i >= 0; i--) {
154-
var contentChange = @params.contentChanges[i];
155-
var range = contentChange.range.GetValueOrDefault();
156-
if (previousRange.end > range.start) {
157-
changes.Push(CreateDidChangeTextDocumentParams(@params, version, contentChanges));
158-
contentChanges = new Stack<TextDocumentContentChangedEvent>();
159-
version--;
160-
}
161-
162-
contentChanges.Push(contentChange);
163-
previousRange = range;
164-
}
165-
166-
if (contentChanges.Count > 0) {
167-
changes.Push(CreateDidChangeTextDocumentParams(@params, version, contentChanges));
132+
_server.DidChangeTextDocument(@params);
168133
}
169-
170-
return changes;
171134
}
172135

173-
private static DidChangeTextDocumentParams CreateDidChangeTextDocumentParams(DidChangeTextDocumentParams @params, int version, Stack<TextDocumentContentChangedEvent> contentChanges)
174-
=> new DidChangeTextDocumentParams {
175-
_enqueueForAnalysis = @params._enqueueForAnalysis,
176-
contentChanges = contentChanges.ToArray(),
177-
textDocument = new VersionedTextDocumentIdentifier {
178-
uri = @params.textDocument.uri,
179-
version = version
180-
}
181-
};
182-
183136
[JsonRpcMethod("textDocument/willSave")]
184137
public void WillSaveTextDocument(JToken token) { }
185138

‎src/LanguageServer/Impl/Protocol/Messages.cs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,9 +126,6 @@ public sealed class DidOpenTextDocumentParams {
126126
public sealed class DidChangeTextDocumentParams {
127127
public VersionedTextDocumentIdentifier textDocument;
128128
public TextDocumentContentChangedEvent[] contentChanges;
129-
130-
// Defaults to true, but can be set to false to suppress analysis
131-
public bool? _enqueueForAnalysis;
132129
}
133130

134131
[Serializable]

0 commit comments

Comments
 (0)