From 3d76ac84bbf5e52ffe11ffa06956704fac1232cf Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Thu, 24 Dec 2020 16:09:37 -0800 Subject: [PATCH 1/2] Workaround Roslyn formatting bug Due to https://github.com/dotnet/roslyn/issues/50129, `FormatAsync` will give us changes past the location that we wanted if there were changes to be made on the current line. This can lead to a frustrating typing experience now that we listen for `\n` in vscode to trigger formatting. I've implemented a simple workaround for this while the bug is fixed upstream. --- .../Workers/Formatting/FormattingWorker.cs | 8 +- .../Utilities/TextChangeHelper.cs | 32 +++++++- .../FormatAfterKeystrokeTests.cs | 76 +++++++++++++++++++ 3 files changed, 109 insertions(+), 7 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Formatting/FormattingWorker.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Formatting/FormattingWorker.cs index 17cd5405a2..df7863c77b 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Formatting/FormattingWorker.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Formatting/FormattingWorker.cs @@ -36,7 +36,7 @@ public static async Task> GetFormattingC Debug.Assert(targetLine.Text != null); if (!string.IsNullOrWhiteSpace(targetLine.Text!.ToString(targetLine.Span))) { - return await GetFormattingChanges(document, targetLine.Start, targetLine.End, omnisharpOptions, loggerFactory); + return await GetFormattingChanges(document, targetLine.Start, targetLine.End, omnisharpOptions, loggerFactory, trimExtendingChanges: true); } } else if (character == '}' || character == ';') @@ -47,7 +47,7 @@ public static async Task> GetFormattingC var node = FindFormatTarget(root!, position); if (node != null) { - return await GetFormattingChanges(document, node.FullSpan.Start, node.FullSpan.End, omnisharpOptions, loggerFactory); + return await GetFormattingChanges(document, node.FullSpan.Start, node.FullSpan.End, omnisharpOptions, loggerFactory, trimExtendingChanges: false); } } @@ -91,10 +91,10 @@ public static async Task> GetFormattingC return null; } - public static async Task> GetFormattingChanges(Document document, int start, int end, OmniSharpOptions omnisharpOptions, ILoggerFactory loggerFactory) + public static async Task> GetFormattingChanges(Document document, int start, int end, OmniSharpOptions omnisharpOptions, ILoggerFactory loggerFactory, bool trimExtendingChanges = false) { var newDocument = await FormatDocument(document, omnisharpOptions, loggerFactory, TextSpan.FromBounds(start, end)); - return await TextChanges.GetAsync(newDocument, document); + return await TextChanges.GetAsync(newDocument, document, trimExtendingChanges ? end : (int?)null); } public static async Task GetFormattedText(Document document, OmniSharpOptions omnisharpOptions, ILoggerFactory loggerFactory) diff --git a/src/OmniSharp.Roslyn/Utilities/TextChangeHelper.cs b/src/OmniSharp.Roslyn/Utilities/TextChangeHelper.cs index dbac3714d5..ae292b5ee5 100644 --- a/src/OmniSharp.Roslyn/Utilities/TextChangeHelper.cs +++ b/src/OmniSharp.Roslyn/Utilities/TextChangeHelper.cs @@ -9,12 +9,12 @@ namespace OmniSharp.Roslyn.Utilities { internal static class TextChanges { - public static async Task> GetAsync(Document document, Document oldDocument) + public static async Task> GetAsync(Document document, Document oldDocument, int? originalEnd = null) { var changes = await document.GetTextChangesAsync(oldDocument); var oldText = await oldDocument.GetTextAsync(); - return Convert(oldText, changes); + return Convert(oldText, changes, originalEnd); } public static IEnumerable Convert(SourceText oldText, params TextChange[] changes) @@ -22,7 +22,7 @@ public static IEnumerable Convert(SourceText oldText return Convert(oldText, (IEnumerable)changes); } - public static IEnumerable Convert(SourceText oldText, IEnumerable changes) + public static IEnumerable Convert(SourceText oldText, IEnumerable changes, int? originalEnd = null) { return changes .OrderByDescending(change => change.Span) @@ -35,6 +35,32 @@ public static IEnumerable Convert(SourceText oldText if (newText.Length > 0) { + // Due to https://github.com/dotnet/roslyn/issues/50129, we might get a text change + // that extends further than the requested end bound. This doesn't matter for every + // scenario, but is annoying when hitting enter and having a trailing space on the previous + // line, as that will end up removing the newly-added indentation on the current line. + // For the cases that matters, originalEnd will be not null, and we reduce the end of the span + // to be that location. + if (originalEnd is int oEnd && span.Start < oEnd && span.End > oEnd) + { + // Since the new change is beyond the requested line, it's also going to end in either a + // \r\n or \n, which replaces the newline on the current line. To avoid that newline + // becoming an extra blank line, we extend the span an addition 2 or 1 characters to replace + // that point. + int newLength = span.Length - (span.End - oEnd); + if (newText.EndsWith("\r\n")) + { + newLength += 2; + } + else if (newText.EndsWith("\n")) + { + newLength += 1; + } + + span = new TextSpan(span.Start, newLength); + } + + // Roslyn computes text changes on character arrays. So it might happen that a // change starts inbetween \r\n which is OK when you are offset-based but a problem // when you are line,column-based. This code extends text edits which just overlap diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs index e87ca36993..446bf71a7f 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs @@ -360,6 +360,82 @@ public void M() { /// $$ + } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task LeadingSpacesNotRemovedOnNewline_ExtraWhitespaceOnPreviousLine(string fileName) + { + await VerifyChange(fileName, "\n", +@"class C +{ + public void M() + { + // There is a space after the ; here + var i = 1; + $$ + } +}", +@"class C +{ + public void M() + { + // There is a space after the ; here + var i = 1; + + } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task LeadingSpacesNotRemovedOnNewline_FormattingChangesOnPreviousLine(string fileName) + { + await VerifyChange(fileName, "\n", +@"class C +{ + public void M() + { + var i =1; + $$ + } +}", +@"class C +{ + public void M() + { + var i = 1; + + } +}"); + } + + [Theory] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task LeadingSpacesNotRemovedOnNewline_NewlinesWithSpaceAfter(string fileName) + { + await VerifyChange(fileName, "\n", +@"class C +{ + public void M() + { + var i =1; + $$ + + } +}", +@"class C +{ + public void M() + { + var i = 1; + + } }"); } From 3c8dca6a8def79780475ebfab0b82e5aee28124b Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Thu, 24 Dec 2020 16:41:27 -0800 Subject: [PATCH 2/2] Base changes on the correct document, which corrects behavior when the newlines added are of a different flavor than the ones in the new text, add additional skipped test for incomplete declarations. --- .../Utilities/TextChangeHelper.cs | 17 +++++++------ .../FormatAfterKeystrokeTests.cs | 24 +++++++++++++++++++ 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/OmniSharp.Roslyn/Utilities/TextChangeHelper.cs b/src/OmniSharp.Roslyn/Utilities/TextChangeHelper.cs index ae292b5ee5..75a72a4c4e 100644 --- a/src/OmniSharp.Roslyn/Utilities/TextChangeHelper.cs +++ b/src/OmniSharp.Roslyn/Utilities/TextChangeHelper.cs @@ -48,13 +48,16 @@ public static IEnumerable Convert(SourceText oldText // becoming an extra blank line, we extend the span an addition 2 or 1 characters to replace // that point. int newLength = span.Length - (span.End - oEnd); - if (newText.EndsWith("\r\n")) + if (newText[newText.Length - 1] == '\n') { - newLength += 2; - } - else if (newText.EndsWith("\n")) - { - newLength += 1; + // The new text ends with a newline. Check the original text to see what type of newline + // it used at the end location and increase by that amount + newLength += oldText[oEnd] switch + { + '\r' => 2, + '\n' => 1, + _ => 0 + }; } span = new TextSpan(span.Start, newLength); @@ -93,7 +96,7 @@ public static IEnumerable Convert(SourceText oldText EndLine = linePositionSpan.End.Line, EndColumn = linePositionSpan.End.Character }; - }); + }).ToList(); } } } diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs index 446bf71a7f..fea77807f8 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs @@ -436,6 +436,30 @@ public void M() var i = 1; + } +}"); + } + + [Theory(Skip = "https://github.com/dotnet/roslyn/issues/50130")] + [InlineData("file.cs")] + [InlineData("file.csx")] + public async Task LeadingSpacesNotRemovedOnNewline_IncompleteDeclaration(string fileName) + { + await VerifyChange(fileName, "\n", +@"class C +{ + public void M() + { + var i + $$ + } +}", +@"class C +{ + public void M() + { + var i + } }"); }