Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Handle completions with trailing whitespace on previous lines #2319

Merged
merged 2 commits into from
Jan 6, 2022
Merged

Handle completions with trailing whitespace on previous lines #2319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1f539a
Handle completions with trailing whitespace on previous lines
333fred Dec 31, 2021
eae1e01
Review feedback.
333fred Jan 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 66 additions & 18 deletions src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionListBuilder_Sync.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using CSharpCompletionItem = Microsoft.CodeAnalysis.Completion.CompletionItem;
using CSharpCompletionList = Microsoft.CodeAnalysis.Completion.CompletionList;
using CSharpCompletionService = Microsoft.CodeAnalysis.Completion.CompletionService;
using OmniSharp.Extensions;

namespace OmniSharp.Roslyn.CSharp.Services.Completion
{
Expand Down Expand Up @@ -148,38 +149,67 @@ private static void GetCompletionInfo(
// span.
var adjustedNewPosition = change!.NewPosition;

var cursorPoint = sourceText.GetPointFromPosition(position);
var lineStartPosition = sourceText.GetPositionFromLineAndOffset(cursorPoint.Line, offset: 0);

// There must be at least one change that affects the current location, or something is seriously wrong
Debug.Assert(change.TextChanges.Any(change => change.Span.IntersectsWith(position)));

foreach (var textChange in change.TextChanges)
{
if (!textChange.Span.IntersectsWith(position))
{
additionalTextEdits ??= new();
additionalTextEdits.Add(GetChangeForTextAndSpan(textChange.NewText!, textChange.Span, sourceText));

if (adjustedNewPosition is int newPosition)
{
// Find the diff between the original text length and the new text length.
var diff = (textChange.NewText?.Length ?? 0) - textChange.Span.Length;

// If the new text is longer than the replaced text, we want to subtract that
// length from the current new position to find the adjusted position in the old
// document. If the new text was shorter, diff will be negative, and subtracting
// will result in increasing the adjusted position as expected
adjustedNewPosition = newPosition - diff;
}
handleNonInsertsectingEdit(sourceText, ref additionalTextEdits, ref adjustedNewPosition, textChange);
}
else
{
// Either there should be no new position, or it should be within the text that is being added
// by this change.
int changeSpanStart = textChange.Span.Start;
Debug.Assert(adjustedNewPosition is null ||
(adjustedNewPosition.Value <= textChange.Span.Start + textChange.NewText!.Length) &&
(adjustedNewPosition.Value >= textChange.Span.Start));
(adjustedNewPosition.Value <= changeSpanStart + textChange.NewText!.Length) &&
(adjustedNewPosition.Value >= changeSpanStart));

// Filtering needs a range that is a _single_ line. Consider a case like this (whitespace documented with escapes):
//
// 1: class C
// 2: {\t\r\n
// 3: override $$
//
// Roslyn will see the trailing \t on line 2 and remove it when creating the _main_ text change. However, that will
333fred marked this conversation as resolved.
Show resolved Hide resolved
// break filtering because filtering expects a single line as part of the range. So what we want to do is break the
// the text change up into two: one to cover the previous line, as an additional edit, and then one to cover the
// rest of the change.

var updatedChange = textChange;

if (changeSpanStart < lineStartPosition)
{
// We know we're in the special case. In order to correctly determine the amount of leading newlines to trim, we want
// to calculate the number of lines before the cursor we're editing
var editStartPoint = sourceText.GetPointFromPosition(changeSpanStart);
var numLinesEdited = cursorPoint.Line - editStartPoint.Line;

changeSpan = textChange.Span;
(insertText, insertTextFormat) = getPossiblySnippetizedInsertText(textChange, adjustedNewPosition);
Debug.Assert(textChange.NewText != null);

// Now count that many newlines forward in the edited text
int cutoffPosition = 0;
for (int numNewlinesFound = 0; numNewlinesFound < numLinesEdited; cutoffPosition++)
{
if (textChange.NewText![cutoffPosition] == '\n')
{
numNewlinesFound++;
}
}

// Now that we've found the cuttoff, we can build our two subchanges
var prefixChange = new TextChange(new TextSpan(changeSpanStart, length: lineStartPosition - changeSpanStart), textChange.NewText!.Substring(0, cutoffPosition));
handleNonInsertsectingEdit(sourceText, ref additionalTextEdits, ref adjustedNewPosition, prefixChange);
updatedChange = new TextChange(new TextSpan(lineStartPosition, length: textChange.Span.End - lineStartPosition), textChange.NewText.Substring(cutoffPosition));
}

changeSpan = updatedChange.Span;
(insertText, insertTextFormat) = getPossiblySnippetizedInsertText(updatedChange, adjustedNewPosition);

// If we're expecting there to be unimported types, put in an explicit sort text to put things already in scope first.
// Otherwise, omit the sort text if it's the same as the label to save on space.
Expand Down Expand Up @@ -236,6 +266,24 @@ private static void GetCompletionInfo(

return ($"{beforeText}$0{afterText}", InsertTextFormat.Snippet);
}

static void handleNonInsertsectingEdit(SourceText sourceText, ref List<LinePositionSpanTextChange>? additionalTextEdits, ref int? adjustedNewPosition, TextChange textChange)
{
additionalTextEdits ??= new();
additionalTextEdits.Add(GetChangeForTextAndSpan(textChange.NewText!, textChange.Span, sourceText));

if (adjustedNewPosition is int newPosition)
{
// Find the diff between the original text length and the new text length.
var diff = (textChange.NewText?.Length ?? 0) - textChange.Span.Length;

// If the new text is longer than the replaced text, we want to subtract that
// length from the current new position to find the adjusted position in the old
// document. If the new text was shorter, diff will be negative, and subtracting
// will result in increasing the adjusted position as expected
adjustedNewPosition = newPosition - diff;
}
}
}

private static string? GetSortText(CSharpCompletionItem completion, string labelText, bool expectingImportedItems)
Expand Down
30 changes: 30 additions & 0 deletions tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2220,6 +2220,36 @@ public static void Main()
}
}

[Theory]
[InlineData("dummy.cs")]
[InlineData("dummy.csx")]
public async Task TestOverrideWithTrailingWhitespacePrior(string filename)
{
const string input = @"
namespace N
{
internal class C
{
// The trailing tabs on the previous line and the next line are integral to this bug

override $$
public C()
{
}
}
}
";

var completions = await FindCompletionsAsync(filename, input, SharedOmniSharpTestHost);

foreach (var item in completions.Items)
{
Assert.Single(item.AdditionalTextEdits);
Assert.Equal("\n // The trailing tabs on the previous line and the next line are integral to this bug\n\n", NormalizeNewlines(item.AdditionalTextEdits[0].NewText));
Assert.StartsWith(" public override ", item.TextEdit.NewText);
}
}

private CompletionService GetCompletionService(OmniSharpTestHost host)
=> host.GetRequestHandler<CompletionService>(EndpointName);

Expand Down