From e1f539af9dcb87228916496fb56f538343abf43c Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Thu, 30 Dec 2021 23:13:52 -0800 Subject: [PATCH 1/2] Handle completions with trailing whitespace on previous lines LSP filters expect that the range of the main text edit is going to start on the same line as the cursor. However, when trailing whitespace preceeds the override marker, Roslyn will return a single text edit for the whole range that trims that whitespace. This confuses the vscode intellisense engine, which then unhelpfully displays no completions with no user feedback that this filtering is happening. To avoid this, we now detect when this case occurs, and split up the change into non-overlapping edits from before the current line. Fixes https://github.com/OmniSharp/omnisharp-vscode/issues/4600. --- .../Completion/CompletionListBuilder_Sync.cs | 84 +++++++++++++++---- .../CompletionFacts.cs | 33 ++++++++ 2 files changed, 99 insertions(+), 18 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionListBuilder_Sync.cs b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionListBuilder_Sync.cs index a036e9d11e..07ff3ea35e 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionListBuilder_Sync.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionListBuilder_Sync.cs @@ -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 { @@ -148,6 +149,9 @@ 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))); @@ -155,31 +159,57 @@ private static void GetCompletionInfo( { 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; - } + HandleNonIntersectingEdit(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 + // 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)); + HandleNonIntersectingEdit(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. @@ -236,6 +266,24 @@ private static void GetCompletionInfo( return ($"{beforeText}$0{afterText}", InsertTextFormat.Snippet); } + + static void HandleNonIntersectingEdit(SourceText sourceText, ref List? 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) diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs index 93aa3bc6d4..c51cd6aa33 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs @@ -2220,6 +2220,39 @@ public static void Main() } } + [Theory] + [InlineData("dummy.cs")] + [InlineData("dummy.csx")] + public async Task TestOverrideWithTrailingWhitespacePrior(string filename) + { + const string input = @" +namespace OmniSharp.Cake +{ + internal class CakeContextModel + { +// The trailing tabs on the previous line and the next line are integral to this bug + + override $$ + public CakeContextModel(string filePath) + { + Path = filePath; + } + + public string Path { get; } + } +} +"; + + 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(EndpointName); From eae1e011a7346d52a1aa0423aa2d7c83dfaf0306 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Mon, 3 Jan 2022 21:38:40 -0800 Subject: [PATCH 2/2] Review feedback. --- .../Services/Completion/CompletionListBuilder_Sync.cs | 6 +++--- tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs | 9 +++------ 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionListBuilder_Sync.cs b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionListBuilder_Sync.cs index 07ff3ea35e..b1a0ea746b 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionListBuilder_Sync.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionListBuilder_Sync.cs @@ -159,7 +159,7 @@ private static void GetCompletionInfo( { if (!textChange.Span.IntersectsWith(position)) { - HandleNonIntersectingEdit(sourceText, ref additionalTextEdits, ref adjustedNewPosition, textChange); + handleNonInsertsectingEdit(sourceText, ref additionalTextEdits, ref adjustedNewPosition, textChange); } else { @@ -204,7 +204,7 @@ private static void GetCompletionInfo( // 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)); - HandleNonIntersectingEdit(sourceText, ref additionalTextEdits, ref adjustedNewPosition, prefixChange); + handleNonInsertsectingEdit(sourceText, ref additionalTextEdits, ref adjustedNewPosition, prefixChange); updatedChange = new TextChange(new TextSpan(lineStartPosition, length: textChange.Span.End - lineStartPosition), textChange.NewText.Substring(cutoffPosition)); } @@ -267,7 +267,7 @@ private static void GetCompletionInfo( return ($"{beforeText}$0{afterText}", InsertTextFormat.Snippet); } - static void HandleNonIntersectingEdit(SourceText sourceText, ref List? additionalTextEdits, ref int? adjustedNewPosition, TextChange textChange) + static void handleNonInsertsectingEdit(SourceText sourceText, ref List? additionalTextEdits, ref int? adjustedNewPosition, TextChange textChange) { additionalTextEdits ??= new(); additionalTextEdits.Add(GetChangeForTextAndSpan(textChange.NewText!, textChange.Span, sourceText)); diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs index c51cd6aa33..b54d71d03a 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs @@ -2226,19 +2226,16 @@ public static void Main() public async Task TestOverrideWithTrailingWhitespacePrior(string filename) { const string input = @" -namespace OmniSharp.Cake +namespace N { - internal class CakeContextModel + internal class C { // The trailing tabs on the previous line and the next line are integral to this bug override $$ - public CakeContextModel(string filePath) + public C() { - Path = filePath; } - - public string Path { get; } } } ";