Implement proper line split + tests

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

@@ -13,7 +13,6 @@
 // See the Apache Version 2.0 License for specific language governing
 // permissions and limitations under the License.
 
-using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Text;
@@ -47,23 +46,18 @@ public void Update(IEnumerable<DocumentChange> changes) {
             lock (_lock) {
                 _sb = _sb ?? new StringBuilder(_content);
 
-                var lastStart = int.MaxValue;
-                var lineLoc = SplitLines(_sb).ToArray();
-
                 foreach (var change in changes) {
+                    // Every change may change where the lines end so in order 
+                    // to correctly determine line/offsets we must re-split buffer
+                    // into lines after each change.
+                    var lineLoc = GetNewLineLications().ToArray();
+
                     if (change.ReplaceAllText) {
                         _sb = new StringBuilder(change.InsertedText);
-                        lastStart = int.MaxValue;
-                        lineLoc = SplitLines(_sb).ToArray();
                         continue;
                     }
 
                     var start = NewLineLocation.LocationToIndex(lineLoc, change.ReplacedSpan.Start, _sb.Length);
-                    if (start > lastStart) {
-                        throw new InvalidOperationException("changes must be in reverse order of start location");
-                    }
-                    lastStart = start;
-
                     var end = NewLineLocation.LocationToIndex(lineLoc, change.ReplacedSpan.End, _sb.Length);
                     if (end > start) {
                         _sb.Remove(start, end - start);
@@ -78,20 +72,33 @@ public void Update(IEnumerable<DocumentChange> changes) {
             }
         }
 
-        private static IEnumerable<NewLineLocation> SplitLines(StringBuilder text) {
-            NewLineLocation nextLine;
-
-            // TODO: Avoid string allocation by operating directly on StringBuilder
-            var str = text.ToString();
+        public IEnumerable<NewLineLocation> GetNewLineLications() {
+            _sb = _sb ?? new StringBuilder(_content); // for tests
 
-            var lastLineEnd = 0;
-            while ((nextLine = NewLineLocation.FindNewLine(str, lastLineEnd)).EndIndex != lastLineEnd) {
-                yield return nextLine;
-                lastLineEnd = nextLine.EndIndex;
+            if (_sb.Length == 0) {
+                yield return new NewLineLocation(0, NewLineKind.None);
             }
 
-            if (lastLineEnd != str.Length) {
-                yield return nextLine;
+            for (var i = 0; i < _sb.Length; i++) {
+                var ch = _sb[i];
+                var nextCh = i < _sb.Length - 1 ? _sb[i + 1] : '\0';
+                switch (ch) {
+                    case '\r' when nextCh == '\n':
+                        i++;
+                        yield return new NewLineLocation(i + 1, NewLineKind.CarriageReturnLineFeed);
+                        break;
+                    case '\n':
+                        yield return new NewLineLocation(i + 1, NewLineKind.LineFeed);
+                        break;
+                    case '\r':
+                        yield return new NewLineLocation(i + 1, NewLineKind.CarriageReturn);
+                        break;
+                    default:
+                        if (i == _sb.Length - 1) {
+                            yield return new NewLineLocation(i + 1, NewLineKind.None);
+                        }
+                        break;
+                }
             }
         }
     }

src/Analysis/Ast/Test/DocumentBufferTests.cs

@@ -15,9 +15,12 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq;
+using System.Reflection;
 using FluentAssertions;
 using Microsoft.Python.Analysis.Documents;
 using Microsoft.Python.Core.Text;
+using Microsoft.Python.Parsing;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 
 namespace Microsoft.Python.Analysis.Tests {
@@ -77,7 +80,7 @@ public void ResetDocumentBuffer() {
             doc.Reset(0, string.Empty);
             Assert.AreEqual(string.Empty, doc.Text);
 
-            doc.Update(new[] { 
+            doc.Update(new[] {
                 DocumentChange.Insert("text", SourceLocation.MinValue)
             });
 
@@ -127,5 +130,163 @@ public void ReplaceAllDocumentBuffer() {
             Assert.AreEqual(@"text1234", doc.Text);
             Assert.AreEqual(4, doc.Version);
         }
+
+        [TestMethod, Priority(0)]
+        public void DeleteMultipleDisjoint() {
+            var doc = new DocumentBuffer();
+            doc.Reset(0, @"
+line1
+line2
+line3
+line4
+");
+            doc.Update(new[] {
+                DocumentChange.Delete(new SourceSpan(5, 5, 5, 6)),
+                DocumentChange.Delete(new SourceSpan(4, 5, 4, 6)),
+                DocumentChange.Delete(new SourceSpan(3, 5, 3, 6)),
+                DocumentChange.Delete(new SourceSpan(2, 5, 2, 6))
+            });
+            Assert.AreEqual(@"
+line
+line
+line
+line
+", doc.Text);
+        }
+
+        [TestMethod, Priority(0)]
+        public void InsertMultipleDisjoint() {
+            var doc = new DocumentBuffer();
+            doc.Reset(0, @"
+line
+line
+line
+line
+");
+            doc.Update(new[] {
+                DocumentChange.Insert("4", new SourceLocation(5, 5)),
+                DocumentChange.Insert("3", new SourceLocation(4, 5)),
+                DocumentChange.Insert("2", new SourceLocation(3, 5)),
+                DocumentChange.Insert("1", new SourceLocation(2, 5)),
+            });
+            Assert.AreEqual(@"
+line1
+line2
+line3
+line4
+", doc.Text);
+        }
+
+        [TestMethod, Priority(0)]
+        public void DeleteAcrossLines() {
+            var doc = new DocumentBuffer();
+            doc.Reset(0, @"
+line1
+line2
+line3
+line4
+");
+            doc.Update(new[] {
+                DocumentChange.Delete(new SourceSpan(4, 5, 5, 5)),
+                DocumentChange.Delete(new SourceSpan(2, 5, 3, 5)),
+            });
+            Assert.AreEqual(@"
+line2
+line4
+", doc.Text);
+        }
+
+        [TestMethod, Priority(0)]
+        public void SequentialChanges() {
+            var doc = new DocumentBuffer();
+            doc.Reset(0, @"
+line1
+line2
+line3
+line4
+");
+            doc.Update(new[] {
+                DocumentChange.Delete(new SourceSpan(2, 5, 3, 5)),
+                DocumentChange.Delete(new SourceSpan(3, 5, 4, 5))
+            });
+            Assert.AreEqual(@"
+line2
+line4
+", doc.Text);
+        }
+
+        [TestMethod, Priority(0)]
+        public void InsertTopToBottom() {
+            var doc = new DocumentBuffer();
+            doc.Reset(0, @"linelinelineline");
+            doc.Update(new[] {
+                DocumentChange.Insert("\n", new SourceLocation(1, 1)),
+                DocumentChange.Insert("1\n", new SourceLocation(2, 5)),
+                DocumentChange.Insert("2\r", new SourceLocation(3, 5)),
+                DocumentChange.Insert("3\r\n", new SourceLocation(4, 5)),
+                DocumentChange.Insert("4\r\n", new SourceLocation(5, 5)),
+            });
+            Assert.AreEqual("\nline1\nline2\rline3\r\nline4\r\n", doc.Text);
+        }
+
+        [NewLineTestData]
+        [DataTestMethod, Priority(0)]
+        public void NewLines(string s, NewLineLocation[] expected) {
+            var doc = new DocumentBuffer();
+            doc.Reset(0, s);
+            var nls = doc.GetNewLineLications().ToArray();
+            for (var i = 0; i < nls.Length; i++) {
+                Assert.AreEqual(nls[i].Kind, expected[i].Kind);
+                Assert.AreEqual(nls[i].EndIndex, expected[i].EndIndex);
+            }
+        }
+
+        private sealed class NewLineTestDataAttribute : Attribute, ITestDataSource {
+            public IEnumerable<object[]> GetData(MethodInfo methodInfo) =>
+                new List<object[]> {
+                    new object[] {
+                        string.Empty,
+                        new NewLineLocation[] {
+                            new NewLineLocation(0, NewLineKind.None),
+                        }
+                    },
+                    new object[] {
+                        "\r\r",
+                        new NewLineLocation[] {
+                            new NewLineLocation(1, NewLineKind.CarriageReturn),
+                            new NewLineLocation(2, NewLineKind.CarriageReturn)
+                        }
+                    },
+                    new object[] {
+                        "\n\n",
+                        new NewLineLocation[] {
+                            new NewLineLocation(1, NewLineKind.LineFeed),
+                            new NewLineLocation(2, NewLineKind.LineFeed)
+                        }
+                    },
+                    new object[] {
+                        "\r\n\n\r",
+                        new NewLineLocation[] {
+                            new NewLineLocation(2, NewLineKind.CarriageReturnLineFeed),
+                            new NewLineLocation(3, NewLineKind.LineFeed),
+                            new NewLineLocation(4, NewLineKind.CarriageReturn)
+                        }
+                    },
+                    new object[] {
+                        "a\r\nb\r\n c\r d\ne",
+                        new NewLineLocation[] {
+                            new NewLineLocation(3, NewLineKind.CarriageReturnLineFeed),
+                            new NewLineLocation(6, NewLineKind.CarriageReturnLineFeed),
+                            new NewLineLocation(9, NewLineKind.CarriageReturn),
+                            new NewLineLocation(12, NewLineKind.LineFeed),
+                            new NewLineLocation(13, NewLineKind.None)
+                        }
+                    }
+                };
+            public string GetDisplayName(MethodInfo methodInfo, object[] data)
+                => data != null ? $"{methodInfo.Name} ({FormatName((string)data[0])})" : null;
+
+            private static string FormatName(string s) => s.Replace("\n", "\\n").Replace("\r", "\\r");
+        }
     }
 }

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

@@ -13,8 +13,10 @@
 // See the Apache Version 2.0 License for specific language governing
 // permissions and limitations under the License.
 
+using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Linq;
 using Microsoft.Python.Analysis.Documents;
 using Microsoft.Python.Core;
 using Microsoft.Python.LanguageServer.Protocol;
@@ -32,15 +34,10 @@ public void DidOpenTextDocument(DidOpenTextDocumentParams @params) {
 
         public void DidChangeTextDocument(DidChangeTextDocumentParams @params) {
             _disposableBag.ThrowIfDisposed();
-            var uri = @params.textDocument.uri;
-            var doc = _rdt.GetDocument(uri);
+            var doc = _rdt.GetDocument(@params.textDocument.uri);
             if (doc != null) {
-                var changes = new List<DocumentChange>();
-                foreach (var c in @params.contentChanges) {
-                    var change = c.range.HasValue ? DocumentChange.Replace(c.range.Value, c.text) : DocumentChange.ReplaceAll(c.text);
-                    changes.Add(change);
-                }
-                doc.Update(changes);
+                doc.Update(@params.contentChanges
+                    .Select(c => c.range.HasValue ? DocumentChange.Replace(c.range.Value, c.text) : DocumentChange.ReplaceAll(c.text)));
                 _indexManager.AddPendingDoc(doc);
             } else {
                 _log?.Log(TraceEventType.Warning, $"Unable to find document for {@params.textDocument.uri}");

src/LanguageServer/Impl/LanguageServer.cs

@@ -129,57 +129,10 @@ public async Task DidChangeTextDocument(JToken token, CancellationToken cancella
             _idleTimeTracker?.NotifyUserActivity();
             using (await _prioritizer.DocumentChangePriorityAsync(cancellationToken)) {
                 var @params = ToObject<DidChangeTextDocumentParams>(token);
-                var version = @params.textDocument.version;
-                if (version == null || @params.contentChanges.IsNullOrEmpty()) {
-                    _server.DidChangeTextDocument(@params);
-                    return;
-                }
-
-                // _server.DidChangeTextDocument can handle change buckets with decreasing version and without overlapping 
-                // Split change into buckets that will be properly handled
-                var changes = SplitDidChangeTextDocumentParams(@params, version.Value);
-
-                foreach (var change in changes) {
-                    _server.DidChangeTextDocument(change);
-                }
-            }
-        }
-
-        private static IEnumerable<DidChangeTextDocumentParams> SplitDidChangeTextDocumentParams(DidChangeTextDocumentParams @params, int version) {
-            var changes = new Stack<DidChangeTextDocumentParams>();
-            var contentChanges = new Stack<TextDocumentContentChangedEvent>();
-            var previousRange = new Range();
-
-            for (var i = @params.contentChanges.Length - 1; i >= 0; i--) {
-                var contentChange = @params.contentChanges[i];
-                var range = contentChange.range.GetValueOrDefault();
-                if (previousRange.end > range.start) {
-                    changes.Push(CreateDidChangeTextDocumentParams(@params, version, contentChanges));
-                    contentChanges = new Stack<TextDocumentContentChangedEvent>();
-                    version--;
-                }
-
-                contentChanges.Push(contentChange);
-                previousRange = range;
-            }
-
-            if (contentChanges.Count > 0) {
-                changes.Push(CreateDidChangeTextDocumentParams(@params, version, contentChanges));
+                _server.DidChangeTextDocument(@params);
             }
-
-            return changes;
         }
 
-        private static DidChangeTextDocumentParams CreateDidChangeTextDocumentParams(DidChangeTextDocumentParams @params, int version, Stack<TextDocumentContentChangedEvent> contentChanges)
-            => new DidChangeTextDocumentParams {
-                _enqueueForAnalysis = @params._enqueueForAnalysis,
-                contentChanges = contentChanges.ToArray(),
-                textDocument = new VersionedTextDocumentIdentifier {
-                    uri = @params.textDocument.uri,
-                    version = version
-                }
-            };
-
         [JsonRpcMethod("textDocument/willSave")]
         public void WillSaveTextDocument(JToken token) { }
 

src/LanguageServer/Impl/Protocol/Messages.cs

@@ -126,9 +126,6 @@ public sealed class DidOpenTextDocumentParams {
     public sealed class DidChangeTextDocumentParams {
         public VersionedTextDocumentIdentifier textDocument;
         public TextDocumentContentChangedEvent[] contentChanges;
-
-        // Defaults to true, but can be set to false to suppress analysis
-        public bool? _enqueueForAnalysis;
     }
 
     [Serializable]