Skip to content

Commit

Permalink
Workaround Roslyn formatting bug
Browse files Browse the repository at this point in the history
Due to dotnet/roslyn#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.
  • Loading branch information
333fred committed Dec 25, 2020
1 parent 46906f2 commit 3d76ac8
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public static async Task<IEnumerable<LinePositionSpanTextChange>> 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 == ';')
Expand All @@ -47,7 +47,7 @@ public static async Task<IEnumerable<LinePositionSpanTextChange>> 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);
}
}

Expand Down Expand Up @@ -91,10 +91,10 @@ public static async Task<IEnumerable<LinePositionSpanTextChange>> GetFormattingC
return null;
}

public static async Task<IEnumerable<LinePositionSpanTextChange>> GetFormattingChanges(Document document, int start, int end, OmniSharpOptions omnisharpOptions, ILoggerFactory loggerFactory)
public static async Task<IEnumerable<LinePositionSpanTextChange>> 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<string> GetFormattedText(Document document, OmniSharpOptions omnisharpOptions, ILoggerFactory loggerFactory)
Expand Down
32 changes: 29 additions & 3 deletions src/OmniSharp.Roslyn/Utilities/TextChangeHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,20 @@ namespace OmniSharp.Roslyn.Utilities
{
internal static class TextChanges
{
public static async Task<IEnumerable<LinePositionSpanTextChange>> GetAsync(Document document, Document oldDocument)
public static async Task<IEnumerable<LinePositionSpanTextChange>> 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<LinePositionSpanTextChange> Convert(SourceText oldText, params TextChange[] changes)
{
return Convert(oldText, (IEnumerable<TextChange>)changes);
}

public static IEnumerable<LinePositionSpanTextChange> Convert(SourceText oldText, IEnumerable<TextChange> changes)
public static IEnumerable<LinePositionSpanTextChange> Convert(SourceText oldText, IEnumerable<TextChange> changes, int? originalEnd = null)
{
return changes
.OrderByDescending(change => change.Span)
Expand All @@ -35,6 +35,32 @@ public static IEnumerable<LinePositionSpanTextChange> 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
Expand Down
76 changes: 76 additions & 0 deletions tests/OmniSharp.Roslyn.CSharp.Tests/FormatAfterKeystrokeTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}");
}
Expand Down

0 comments on commit 3d76ac8

Please sign in to comment.