From a329ce5f0899d3855e7c62671fe50d71eb0695a7 Mon Sep 17 00:00:00 2001 From: nohwnd Date: Mon, 14 Sep 2020 15:16:11 +0200 Subject: [PATCH 1/6] Fix completion on part of existing string --- .../Services/Completion/CompletionService.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs index 187383eb8d..b674f2cb23 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs @@ -242,6 +242,19 @@ public async Task Handle(CompletionRequest request) break; } + // If the span we are using is re-using part of the typed text we just need to grab the completion an prefix it + // with the existing text. Such as Onenabled -> OnEnabled, this will re-use On of the typed text + if (typedSpan.Start < change.TextChange.Span.Start && typedSpan.Start < change.TextChange.Span.End && typedSpan.End == change.TextChange.Span.End) + { + var prefix = typedText.Substring(0, change.TextChange.Span.Start - typedSpan.Start); + + (insertText, insertTextFormat) = getAdjustedInsertTextWithPosition(change, position, newOffset: 0); + + insertText = prefix + insertText; + + break; + } + int additionalEditEndOffset; (additionalTextEdits, additionalEditEndOffset) = await GetAdditionalTextEdits(change, sourceText, (CSharpParseOptions)syntax!.Options, typedSpan, completion.DisplayText, providerName); From e21a232572a8b6833c484d9ddd98afa9a1614cd4 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Thu, 17 Sep 2020 22:45:02 -0700 Subject: [PATCH 2/6] Use text edit for main insert text Because I misread the LSP spec when imlementing completion, I didn't realize that InsertTextFormat applied to both InsertText, and to the NewText property of the main TextEdit of a completion item. This latter version is much more robust to casing issues than using InsertText directly, and ensures that matching of completion items in the completion list works better as well. Fixes https://github.com/OmniSharp/omnisharp-vscode/issues/4063. --- .../Models/v1/Completion/CompletionItem.cs | 9 ++- .../Services/Completion/CompletionService.cs | 12 ++- .../CompletionFacts.cs | 81 ++++++++++++------- 3 files changed, 70 insertions(+), 32 deletions(-) diff --git a/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs b/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs index aceb370b5a..1a659e4617 100644 --- a/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs +++ b/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs @@ -61,10 +61,17 @@ public class CompletionItem public string? InsertText { get; set; } /// - /// The format of . + /// The format of . This applies to both and + /// .. /// public InsertTextFormat? InsertTextFormat { get; set; } + /// + /// An edit which is applied to a document when selecting this completion. When an edit is provided the value of + /// is ignored. + /// + public LinePositionSpanTextChange? TextEdit { get; set; } + /// /// An optional set of characters that when pressed while this completion is active will accept it first and /// then type that character. diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs index b674f2cb23..88e9818e1d 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs @@ -162,6 +162,9 @@ public async Task Handle(CompletionRequest request) bool expectingImportedItems = expandedItemsAvailable && _workspace.Options.GetOption(CompletionItemExtensions.ShowItemsFromUnimportedNamespaces, LanguageNames.CSharp) == true; var syntax = await document.GetSyntaxTreeAsync(); + var typedSpanStartPosition = sourceText.Lines.GetLinePosition(typedSpan.Start); + var typedSpanEndPosition = sourceText.Lines.GetLinePosition(typedSpan.End); + for (int i = 0; i < completions.Items.Length; i++) { var completion = completions.Items[i]; @@ -284,7 +287,14 @@ public async Task Handle(CompletionRequest request) completionsBuilder.Add(new CompletionItem { Label = completion.DisplayTextPrefix + completion.DisplayText + completion.DisplayTextSuffix, - InsertText = insertText, + TextEdit = new LinePositionSpanTextChange + { + NewText = insertText, + StartLine = typedSpanStartPosition.Line, + StartColumn = typedSpanStartPosition.Character, + EndLine = typedSpanEndPosition.Line, + EndColumn = typedSpanEndPosition.Character + }, InsertTextFormat = insertTextFormat, AdditionalTextEdits = additionalTextEdits, // Ensure that unimported items are sorted after things already imported. diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs index 917fb0dec2..df02928cc2 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs @@ -44,7 +44,7 @@ public Class1() var completions = await FindCompletionsAsync(filename, input, SharedOmniSharpTestHost); Assert.Contains("Foo", completions.Items.Select(c => c.Label)); - Assert.Contains("Foo", completions.Items.Select(c => c.InsertText)); + Assert.Contains("Foo", completions.Items.Select(c => c.TextEdit.NewText)); } [Theory] @@ -63,7 +63,7 @@ public Class1() var completions = await FindCompletionsAsync(filename, input, SharedOmniSharpTestHost); Assert.Contains("foo", completions.Items.Select(c => c.Label)); - Assert.Contains("foo", completions.Items.Select(c => c.InsertText)); + Assert.Contains("foo", completions.Items.Select(c => c.TextEdit.NewText)); } [Theory] @@ -105,7 +105,7 @@ public Class1() }"; var completions = await FindCompletionsAsync(filename, input, SharedOmniSharpTestHost); - Assert.Contains("TryParse", completions.Items.Select(c => c.InsertText)); + Assert.Contains("TryParse", completions.Items.Select(c => c.TextEdit.NewText)); } [Theory] @@ -123,7 +123,7 @@ public Class1() var completions = await FindCompletionsAsync(filename, input, SharedOmniSharpTestHost); Assert.False(completions.IsIncomplete); - Assert.DoesNotContain("Guid", completions.Items.Select(c => c.InsertText)); + Assert.DoesNotContain("Guid", completions.Items.Select(c => c.TextEdit.NewText)); } [Theory] @@ -144,7 +144,7 @@ public Class1() // First completion request should kick off the task to update the completion cache. var completions = await FindCompletionsAsync(filename, input, host); Assert.True(completions.IsIncomplete); - Assert.DoesNotContain("Guid", completions.Items.Select(c => c.InsertText)); + Assert.DoesNotContain("Guid", completions.Items.Select(c => c.TextEdit.NewText)); // Populating the completion cache should take no more than a few ms, don't let it take too // long @@ -159,7 +159,7 @@ await Task.Run(async () => }, cts.Token); Assert.False(completions.IsIncomplete); - Assert.Contains("Guid", completions.Items.Select(c => c.InsertText)); + Assert.Contains("Guid", completions.Items.Select(c => c.TextEdit.NewText)); } [Theory] @@ -179,8 +179,8 @@ public Class1() using var host = GetImportCompletionHost(); var completions = await FindCompletionsWithImportedAsync(filename, input, host); - CompletionItem localCompletion = completions.Items.First(c => c.InsertText == "guid"); - CompletionItem typeCompletion = completions.Items.First(c => c.InsertText == "Guid"); + CompletionItem localCompletion = completions.Items.First(c => c.TextEdit.NewText == "guid"); + CompletionItem typeCompletion = completions.Items.First(c => c.TextEdit.NewText == "Guid"); Assert.True(localCompletion.Data < typeCompletion.Data); Assert.StartsWith("0", localCompletion.SortText); Assert.StartsWith("1", typeCompletion.SortText); @@ -215,7 +215,7 @@ public static void Test(this object o) using var host = GetImportCompletionHost(); var completions = await FindCompletionsWithImportedAsync(filename, input, host); - Assert.Contains("Test", completions.Items.Select(c => c.InsertText)); + Assert.Contains("Test", completions.Items.Select(c => c.TextEdit.NewText)); VerifySortOrders(completions.Items); } @@ -247,7 +247,7 @@ public static void Test(this object o) using var host = GetImportCompletionHost(); var completions = await FindCompletionsWithImportedAsync(filename, input, host); - var resolved = await ResolveCompletionAsync(completions.Items.First(c => c.InsertText == "Test"), host); + var resolved = await ResolveCompletionAsync(completions.Items.First(c => c.TextEdit.NewText == "Test"), host); Assert.Single(resolved.Item.AdditionalTextEdits); var additionalEdit = resolved.Item.AdditionalTextEdits[0]; @@ -288,7 +288,7 @@ public static void Test(this object o) using var host = GetImportCompletionHost(); var completions = await FindCompletionsWithImportedAsync(filename, input, host); - var resolved = await ResolveCompletionAsync(completions.Items.First(c => c.InsertText == "Guid"), host); + var resolved = await ResolveCompletionAsync(completions.Items.First(c => c.TextEdit.NewText == "Guid"), host); Assert.Single(resolved.Item.AdditionalTextEdits); var additionalEdit = resolved.Item.AdditionalTextEdits[0]; @@ -335,7 +335,7 @@ public class C3 using var host = GetImportCompletionHost(); var completions = await FindCompletionsWithImportedAsync(filename, input, host); - var resolved = await ResolveCompletionAsync(completions.Items.First(c => c.InsertText == "C2"), host); + var resolved = await ResolveCompletionAsync(completions.Items.First(c => c.TextEdit.NewText == "C2"), host); Assert.Single(resolved.Item.AdditionalTextEdits); var additionalEdit = resolved.Item.AdditionalTextEdits[0]; @@ -500,7 +500,7 @@ public class Foo {}"; var completions = await FindCompletionsAsync(filename, source, SharedOmniSharpTestHost); Assert.Contains(completions.Items, c => c.Label == "Bar"); - Assert.Contains(completions.Items, c => c.InsertText == "Bar"); + Assert.Contains(completions.Items, c => c.TextEdit.NewText == "Bar"); Assert.All(completions.Items, c => { switch (c.Label) @@ -538,7 +538,7 @@ public MyClass2() var completions = await FindCompletionsAsync(filename, source, SharedOmniSharpTestHost); Assert.Single(completions.Items); Assert.Equal("Foo", completions.Items[0].Label); - Assert.Equal("Foo", completions.Items[0].InsertText); + Assert.Equal("Foo", completions.Items[0].TextEdit.NewText); } [Theory] @@ -564,7 +564,7 @@ public MyClass2() var completions = await FindCompletionsAsync(filename, source, SharedOmniSharpTestHost); var item = completions.Items.First(c => c.Label == "text:"); Assert.NotNull(item); - Assert.Equal("text", item.InsertText); + Assert.Equal("text", item.TextEdit.NewText); Assert.All(completions.Items, c => { switch (c.Label) @@ -624,7 +624,7 @@ class FooChild : Foo "Test(string text, string moreText)\n {\n base.Test(text, moreText);$0\n \\}", "ToString()\n {\n return base.ToString();$0\n \\}" }, - completions.Items.Select(c => c.InsertText)); + completions.Items.Select(c => c.TextEdit.NewText)); Assert.Equal(new[] { "public override bool", "public override int", @@ -678,7 +678,7 @@ class CN3 : IN2 "GetN1()\n {\n throw new System.NotImplementedException();$0\n \\}", "ToString()\n {\n return base.ToString();$0\n \\}" }, - completions.Items.Select(c => c.InsertText)); + completions.Items.Select(c => c.TextEdit.NewText)); Assert.Equal(new[] { "public override bool", "public override int", @@ -717,7 +717,7 @@ public override $$ "int GetHashCode()\n {\n return base.GetHashCode();$0\n \\}", "string ToString()\n {\n return base.ToString();$0\n \\}" }, - completions.Items.Select(c => c.InsertText)); + completions.Items.Select(c => c.TextEdit.NewText)); Assert.All(completions.Items.Select(c => c.AdditionalTextEdits), a => Assert.Null(a)); Assert.All(completions.Items, c => Assert.Equal(InsertTextFormat.Snippet, c.InsertTextFormat)); @@ -739,7 +739,7 @@ public override bool $$ completions.Items.Select(c => c.Label)); Assert.Equal(new[] { "Equals(object obj)\n {\n return base.Equals(obj);$0\n \\}" }, - completions.Items.Select(c => c.InsertText)); + completions.Items.Select(c => c.TextEdit.NewText)); Assert.All(completions.Items.Select(c => c.AdditionalTextEdits), a => Assert.Null(a)); Assert.All(completions.Items, c => Assert.Equal(InsertTextFormat.Snippet, c.InsertTextFormat)); @@ -770,7 +770,7 @@ class Derived : Base "Test()\n {\n throw new System.NotImplementedException();$0\n \\}", "ToString()\n {\n return base.ToString();$0\n \\}" }, - completions.Items.Select(c => c.InsertText)); + completions.Items.Select(c => c.TextEdit.NewText)); Assert.Equal(new[] { "public override bool", "public override int", @@ -817,7 +817,7 @@ public class Derived : Base Assert.Equal(0, item.AdditionalTextEdits[0].StartColumn); Assert.Equal(3, item.AdditionalTextEdits[0].EndLine); Assert.Equal(12, item.AdditionalTextEdits[0].EndColumn); - Assert.Equal("GetAction(Action a)\n {\n return base.GetAction(a);$0\n \\}", item.InsertText); + Assert.Equal("GetAction(Action a)\n {\n return base.GetAction(a);$0\n \\}", item.TextEdit.NewText); } [Fact] @@ -845,7 +845,7 @@ public class Derived : Base Assert.Equal(4, item.AdditionalTextEdits[0].StartColumn); Assert.Equal(9, item.AdditionalTextEdits[0].EndLine); Assert.Equal(12, item.AdditionalTextEdits[0].EndColumn); - Assert.Equal("M1(object param)\n {\n return base.M1(param);$0\n \\}", item.InsertText); + Assert.Equal("M1(object param)\n {\n return base.M1(param);$0\n \\}", item.TextEdit.NewText); } [Theory] @@ -869,7 +869,7 @@ partial class C completions.Items.Select(c => c.Label)); Assert.Equal(new[] { "void M1(string param)\n {\n throw new System.NotImplementedException();$0\n \\}" }, - completions.Items.Select(c => c.InsertText)); + completions.Items.Select(c => c.TextEdit.NewText)); Assert.All(completions.Items.Select(c => c.AdditionalTextEdits), a => Assert.Null(a)); Assert.All(completions.Items, c => Assert.Equal(InsertTextFormat.Snippet, c.InsertTextFormat)); @@ -901,7 +901,7 @@ public partial class C Assert.Equal(0, item.AdditionalTextEdits[0].StartColumn); Assert.Equal(3, item.AdditionalTextEdits[0].EndLine); Assert.Equal(11, item.AdditionalTextEdits[0].EndColumn); - Assert.Equal("M(Action a)\n {\n throw new NotImplementedException();$0\n \\}", item.InsertText); + Assert.Equal("M(Action a)\n {\n throw new NotImplementedException();$0\n \\}", item.TextEdit.NewText); } [Fact] @@ -923,7 +923,7 @@ public partial class C var item = completions.Items.Single(c => c.Label.StartsWith("M1")); Assert.Equal("M1(object param)", item.Label); Assert.Null(item.AdditionalTextEdits); - Assert.Equal("void M1(object param)\n {\n throw new System.NotImplementedException();$0\n \\}", item.InsertText); + Assert.Equal("void M1(object param)\n {\n throw new System.NotImplementedException();$0\n \\}", item.TextEdit.NewText); } [Theory] @@ -945,7 +945,7 @@ override Ge$$ "GetHashCode()\n {\n return base.GetHashCode();$0\n \\}", "ToString()\n {\n return base.ToString();$0\n \\}" }, - completions.Items.Select(c => c.InsertText)); + completions.Items.Select(c => c.TextEdit.NewText)); Assert.Equal(new[] { "public override bool", "public override int", @@ -1030,8 +1030,8 @@ public class MyClass1 { "see cref=\"$0\"/>", "seealso cref=\"$0\"/>" }, - completions.Items.Select(c => c.InsertText)); - Assert.All(completions.Items, c => Assert.Equal(c.InsertText.Contains("$0"), c.InsertTextFormat == InsertTextFormat.Snippet)); + completions.Items.Select(c => c.TextEdit.NewText)); + Assert.All(completions.Items, c => Assert.Equal(c.TextEdit.NewText.Contains("$0"), c.InsertTextFormat == InsertTextFormat.Snippet)); } [Fact] @@ -1307,7 +1307,7 @@ public async Task InternalsVisibleToCompletion() var completions = await FindCompletionsAsync("dummy.cs", input, SharedOmniSharpTestHost); Assert.Single(completions.Items); Assert.Equal("AssemblyNameVal", completions.Items[0].Label); - Assert.Equal("AssemblyNameVal", completions.Items[0].InsertText); + Assert.Equal("AssemblyNameVal", completions.Items[0].TextEdit.NewText); } [Fact] @@ -1333,7 +1333,28 @@ public async Task InternalsVisibleToCompletionSkipsMiscProject() var completions = await FindCompletionsAsync("dummy.cs", input, SharedOmniSharpTestHost); Assert.Single(completions.Items); Assert.Equal("AssemblyNameVal", completions.Items[0].Label); - Assert.Equal("AssemblyNameVal", completions.Items[0].InsertText); + Assert.Equal("AssemblyNameVal", completions.Items[0].TextEdit.NewText); + } + + [Theory] + [InlineData("dummy.cs")] + [InlineData("dummy.csx")] + public async Task ChangeSpanIsExpected(string filename) + { + const string input = +@"public class Base +{ + protected virtual void OnEnable() {} +} +public class Derived : Base +{ + protected override void On$$ +}"; + + var completions = await FindCompletionsAsync(filename, input, SharedOmniSharpTestHost); + var onEnable = completions.Items.Single(c => c.TextEdit.NewText.Contains("OnEnable")); + Assert.Equal(onEnable.TextEdit.StartLine, onEnable.TextEdit.EndLine); + Assert.Equal(2, onEnable.TextEdit.EndColumn - onEnable.TextEdit.StartColumn); } private CompletionService GetCompletionService(OmniSharpTestHost host) From 255a88314f2596052e760ede68a3f9fc332580b9 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Fri, 18 Sep 2020 00:04:10 -0700 Subject: [PATCH 3/6] Add more tests for override spans within the original typed text, comment with examples on vscode behavior for replacing ranges. --- .../Services/Completion/CompletionService.cs | 44 +++++++++-------- .../CompletionFacts.cs | 47 ++++++++++++++++++- 2 files changed, 70 insertions(+), 21 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs index 88e9818e1d..4f8f6eaf2d 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs @@ -162,8 +162,8 @@ public async Task Handle(CompletionRequest request) bool expectingImportedItems = expandedItemsAvailable && _workspace.Options.GetOption(CompletionItemExtensions.ShowItemsFromUnimportedNamespaces, LanguageNames.CSharp) == true; var syntax = await document.GetSyntaxTreeAsync(); - var typedSpanStartPosition = sourceText.Lines.GetLinePosition(typedSpan.Start); - var typedSpanEndPosition = sourceText.Lines.GetLinePosition(typedSpan.End); + var replacingSpanStartPosition = sourceText.Lines.GetLinePosition(typedSpan.Start); + var replacingSpanEndPosition = sourceText.Lines.GetLinePosition(typedSpan.End); for (int i = 0; i < completions.Items.Length; i++) { @@ -236,25 +236,28 @@ public async Task Handle(CompletionRequest request) var change = await completionService.GetChangeAsync(document, completion); - // If the span we're using to key the completion off is the same as the replacement - // span, then we don't need to do anything special, just snippitize the text and - // exit if (typedSpan == change.TextChange.Span) { + // If the span we're using to key the completion off is the same as the replacement + // span, then we don't need to do anything special, just snippitize the text and + // exit (insertText, insertTextFormat) = getAdjustedInsertTextWithPosition(change, position, newOffset: 0); break; } - - // If the span we are using is re-using part of the typed text we just need to grab the completion an prefix it - // with the existing text. Such as Onenabled -> OnEnabled, this will re-use On of the typed text - if (typedSpan.Start < change.TextChange.Span.Start && typedSpan.Start < change.TextChange.Span.End && typedSpan.End == change.TextChange.Span.End) + if (change.TextChange.Span.Start > typedSpan.Start) { + // If the span we're using to key the replacement span within the original typed span + // span, we want to prepend the missing text from the original typed text to here. The + // reason is that some lsp clients, such as vscode, use the range from the text edit as + // the selector for what filter text to use. This can lead to odd scenarios where invoking + // completion and typing `EQ` will bring up the Equals override, but then dismissing and + // reinvoking completion will have a range that just replaces the Q. Vscode will then consider + // that capital Q to be the start of the filter text, and filter out the Equals overload + // leaving the user with no completion and no explanation + Debug.Assert(change.TextChange.Span.End == typedSpan.End); var prefix = typedText.Substring(0, change.TextChange.Span.Start - typedSpan.Start); - - (insertText, insertTextFormat) = getAdjustedInsertTextWithPosition(change, position, newOffset: 0); - - insertText = prefix + insertText; + (insertText, insertTextFormat) = getAdjustedInsertTextWithPosition(change, position, newOffset: 0, prefix); break; } @@ -290,10 +293,10 @@ public async Task Handle(CompletionRequest request) TextEdit = new LinePositionSpanTextChange { NewText = insertText, - StartLine = typedSpanStartPosition.Line, - StartColumn = typedSpanStartPosition.Character, - EndLine = typedSpanEndPosition.Line, - EndColumn = typedSpanEndPosition.Character + StartLine = replacingSpanStartPosition.Line, + StartColumn = replacingSpanStartPosition.Character, + EndLine = replacingSpanEndPosition.Line, + EndColumn = replacingSpanEndPosition.Character }, InsertTextFormat = insertTextFormat, AdditionalTextEdits = additionalTextEdits, @@ -382,7 +385,8 @@ static ImmutableArray buildCommitCharacters(CSharpCompletionList completio static (string, InsertTextFormat) getAdjustedInsertTextWithPosition( CompletionChange change, int originalPosition, - int newOffset) + int newOffset, + string? prependText = null) { // We often have to trim part of the given change off the front, but we // still want to turn the resulting change into a snippet and control @@ -398,7 +402,7 @@ static ImmutableArray buildCommitCharacters(CSharpCompletionList completio if (!(change.NewPosition is int newPosition) || newPosition >= (change.TextChange.Span.Start + newText.Length)) { - return (newText.Substring(newOffset), InsertTextFormat.PlainText); + return (prependText + newText.Substring(newOffset), InsertTextFormat.PlainText); } if (newPosition < (originalPosition + newOffset)) @@ -412,7 +416,7 @@ static ImmutableArray buildCommitCharacters(CSharpCompletionList completio // requested start to the new position, and from the new position to the end of the // string. int midpoint = newPosition - change.TextChange.Span.Start; - var beforeText = LspSnippetHelpers.Escape(newText.Substring(newOffset, midpoint - newOffset)); + var beforeText = LspSnippetHelpers.Escape(prependText + newText.Substring(newOffset, midpoint - newOffset)); var afterText = LspSnippetHelpers.Escape(newText.Substring(midpoint)); return (beforeText + "$0" + afterText, InsertTextFormat.Snippet); diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs index df02928cc2..10ce20edcf 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs @@ -1339,7 +1339,7 @@ public async Task InternalsVisibleToCompletionSkipsMiscProject() [Theory] [InlineData("dummy.cs")] [InlineData("dummy.csx")] - public async Task ChangeSpanIsExpected(string filename) + public async Task PrefixHeaderIsFullyCorrect(string filename) { const string input = @"public class Base @@ -1355,6 +1355,51 @@ protected override void On$$ var onEnable = completions.Items.Single(c => c.TextEdit.NewText.Contains("OnEnable")); Assert.Equal(onEnable.TextEdit.StartLine, onEnable.TextEdit.EndLine); Assert.Equal(2, onEnable.TextEdit.EndColumn - onEnable.TextEdit.StartColumn); + Assert.Equal("OnEnable()\n {\n base.OnEnable();$0\n \\}", onEnable.TextEdit.NewText); + } + + [Theory] + [InlineData("dummy.cs")] + [InlineData("dummy.csx")] + public async Task PrefixHeaderIsPartiallyCorrect_1(string filename) + { + const string input = +@"public class Base +{ + protected virtual void OnEnable() {} +} +public class Derived : Base +{ + protected override void ON$$ +}"; + + var completions = await FindCompletionsAsync(filename, input, SharedOmniSharpTestHost); + var onEnable = completions.Items.Single(c => c.TextEdit.NewText.Contains("OnEnable")); + Assert.Equal(onEnable.TextEdit.StartLine, onEnable.TextEdit.EndLine); + Assert.Equal(2, onEnable.TextEdit.EndColumn - onEnable.TextEdit.StartColumn); + Assert.Equal("OnEnable()\n {\n base.OnEnable();$0\n \\}", onEnable.TextEdit.NewText); + } + + [Theory] + [InlineData("dummy.cs")] + [InlineData("dummy.csx")] + public async Task PrefixHeaderIsPartiallyCorrect_2(string filename) + { + const string input = +@"public class Base +{ + protected virtual void OnEnable() {} +} +public class Derived : Base +{ + protected override void on$$ +}"; + + var completions = await FindCompletionsAsync(filename, input, SharedOmniSharpTestHost); + var onEnable = completions.Items.Single(c => c.TextEdit.NewText.Contains("OnEnable")); + Assert.Equal(onEnable.TextEdit.StartLine, onEnable.TextEdit.EndLine); + Assert.Equal(2, onEnable.TextEdit.EndColumn - onEnable.TextEdit.StartColumn); + Assert.Equal("OnEnable()\n {\n base.OnEnable();$0\n \\}", onEnable.TextEdit.NewText); } private CompletionService GetCompletionService(OmniSharpTestHost host) From 3805152212c0acca29cac379d5dd2d79073bc305 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Sat, 19 Sep 2020 17:25:11 -0700 Subject: [PATCH 4/6] Update after merge * Remove incorrect assert * Update the tests for the TextEdit changes * Mark InsertText as obsolete to prevent new uses. --- .../Models/v1/Completion/CompletionItem.cs | 2 ++ .../Services/Completion/CompletionService.cs | 7 ------- tests/OmniSharp.Cake.Tests/CompletionFacts.cs | 14 +++++++------- .../CompletionFacts.cs | 4 ++-- 4 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs b/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs index 1a659e4617..53532d63a6 100644 --- a/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs +++ b/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs @@ -1,5 +1,6 @@ #nullable enable +using System; using System.Collections.Generic; namespace OmniSharp.Models.v1.Completion @@ -58,6 +59,7 @@ public class CompletionItem /// A string that should be inserted into a document when selecting /// this completion.When null or empty the label is used. /// + [Obsolete("Use TextEdit instead", error: true)] public string? InsertText { get; set; } /// diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs index 6669b2290b..38564ffde1 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs @@ -415,13 +415,6 @@ static ImmutableArray buildCommitCharacters(CSharpCompletionList completio return (prependText + newText.Substring(newOffset), InsertTextFormat.PlainText); } - if (newPosition < (originalPosition + newOffset)) - { - Debug.Fail($"Unknown case of attempting to move cursor before the text that needs to be cut off. Requested cutoff: {newOffset}. New Position: {newPosition}"); - // Gracefully handle as best we can in release - return (newText.Substring(newOffset), InsertTextFormat.PlainText); - } - // Roslyn wants to move the cursor somewhere inside the result. Substring from the // requested start to the new position, and from the new position to the end of the // string. diff --git a/tests/OmniSharp.Cake.Tests/CompletionFacts.cs b/tests/OmniSharp.Cake.Tests/CompletionFacts.cs index 83812f57b3..c44ebd4b3c 100644 --- a/tests/OmniSharp.Cake.Tests/CompletionFacts.cs +++ b/tests/OmniSharp.Cake.Tests/CompletionFacts.cs @@ -37,7 +37,7 @@ public async Task ShouldGetCompletionFromHostObject() var completions = await FindCompletionsAsync(fileName, input, host); Assert.Contains("TaskSetup", completions.Items.Select(c => c.Label)); - Assert.Contains("TaskSetup", completions.Items.Select(c => c.InsertText)); + Assert.Contains("TaskSetup", completions.Items.Select(c => c.TextEdit.NewText)); } } @@ -57,7 +57,7 @@ public async Task ShouldGetCompletionFromDSL() var completions = await FindCompletionsAsync(fileName, input, host); Assert.Contains("Information", completions.Items.Select(c => c.Label)); - Assert.Contains("Information", completions.Items.Select(c => c.InsertText)); + Assert.Contains("Information", completions.Items.Select(c => c.TextEdit.NewText)); } } @@ -75,7 +75,7 @@ public async Task ShouldResolveFromDSL() { var fileName = Path.Combine(testProject.Directory, "build.cake"); var completion = (await FindCompletionsAsync(fileName, input, host)) - .Items.First(x => x.Preselect && x.InsertText == "Information"); + .Items.First(x => x.Preselect && x.TextEdit.NewText == "Information"); var resolved = await ResolveCompletionAsync(completion, host); @@ -99,7 +99,7 @@ public async Task ShouldRemoveAdditionalTextEditsFromResolvedCompletions() // First completion request should kick off the task to update the completion cache. var completions = await FindCompletionsAsync(fileName, input, host); Assert.True(completions.IsIncomplete); - Assert.DoesNotContain("Regex", completions.Items.Select(c => c.InsertText)); + Assert.DoesNotContain("Regex", completions.Items.Select(c => c.TextEdit.NewText)); // Populating the completion cache should take no more than a few ms, don't let it take too // long @@ -114,9 +114,9 @@ await Task.Run(async () => }, cts.Token); Assert.False(completions.IsIncomplete); - Assert.Contains("Regex", completions.Items.Select(c => c.InsertText)); + Assert.Contains("Regex", completions.Items.Select(c => c.TextEdit.NewText)); - var completion = completions.Items.First(c => c.InsertText == "Regex"); + var completion = completions.Items.First(c => c.TextEdit.NewText == "Regex"); var resolved = await ResolveCompletionAsync(completion, host); // Due to the fact that AdditionalTextEdits return the complete buffer, we can't currently use that in Cake. @@ -161,7 +161,7 @@ class FooChild : Foo "Test(string text, string moreText)\n {\n base.Test(text, moreText);$0\n \\}", "ToString()\n {\n return base.ToString();$0\n \\}" }, - completions.Items.Select(c => c.InsertText)); + completions.Items.Select(c => c.TextEdit.NewText)); Assert.Equal(new[] { diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs index 1dc060f67b..c2c9bac7bb 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs @@ -1420,7 +1420,7 @@ public void M() var completions = await FindCompletionsAsync(filename, input, SharedOmniSharpTestHost); var aCompletion = completions.Items.First(c => c.Label == @"\A"); Assert.NotNull(aCompletion); - Assert.Equal(@"\\A", aCompletion.InsertText); + Assert.Equal(@"\\A", aCompletion.TextEdit.NewText); } [ConditionalTheory(typeof(WindowsOnly))] @@ -1441,7 +1441,7 @@ public void M() var completions = await FindCompletionsAsync(filename, input, SharedOmniSharpTestHost); var aCompletion = completions.Items.First(c => c.Label == @"\A"); Assert.NotNull(aCompletion); - Assert.Equal(@"\A", aCompletion.InsertText); + Assert.Equal(@"\A", aCompletion.TextEdit.NewText); } private CompletionService GetCompletionService(OmniSharpTestHost host) From 568c9ccff33e701dd0c0b8ff01d5e11436e7851b Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Sat, 26 Sep 2020 15:20:19 -0700 Subject: [PATCH 5/6] Support property override completion --- .../Services/Completion/CompletionService.cs | 73 +++++++++++++------ .../AbstractAutoCompleteTestFixture.cs | 2 + .../CompletionFacts.cs | 61 ++++++++++++++++ 3 files changed, 112 insertions(+), 24 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs index 38564ffde1..5fc9749de4 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs @@ -152,7 +152,7 @@ public async Task Handle(CompletionRequest request) } var triggerCharactersBuilder = ImmutableArray.CreateBuilder(completions.Rules.DefaultCommitCharacters.Length); - var completionsBuilder = ImmutableArray.CreateBuilder(); + var completionsBuilder = ImmutableArray.CreateBuilder(completions.Items.Length); // If we don't encounter any unimported types, and the completion context thinks that some would be available, then // that completion provider is still creating the cache. We'll mark this completion list as not completed, and the @@ -254,9 +254,10 @@ public async Task Handle(CompletionRequest request) (insertText, insertTextFormat) = getAdjustedInsertTextWithPosition(change, position, newOffset: 0); break; } + if (change.TextChange.Span.Start > typedSpan.Start) { - // If the span we're using to key the replacement span within the original typed span + // If the span we're using to key the replacement span is within the original typed span // span, we want to prepend the missing text from the original typed text to here. The // reason is that some lsp clients, such as vscode, use the range from the text edit as // the selector for what filter text to use. This can lead to odd scenarios where invoking @@ -272,7 +273,17 @@ public async Task Handle(CompletionRequest request) } int additionalEditEndOffset; - (additionalTextEdits, additionalEditEndOffset) = await GetAdditionalTextEdits(change, sourceText, (CSharpParseOptions)syntax!.Options, typedSpan, completion.DisplayText, providerName); + (additionalTextEdits, additionalEditEndOffset) = GetAdditionalTextEdits(change, sourceText, (CSharpParseOptions)syntax!.Options, typedSpan, completion.DisplayText, providerName); + + if (additionalEditEndOffset < 0) + { + // We couldn't find the position of the change in the new text. This shouldn't happen in normal cases, + // but handle as best we can in release + Debug.Fail("Couldn't find the new cursor position in the replacement text!"); + additionalTextEdits = null; + insertText = completion.DisplayText; + break; + } // Now that we have the additional edit, adjust the rest of the new text (insertText, insertTextFormat) = getAdjustedInsertTextWithPosition(change, position, additionalEditEndOffset); @@ -324,7 +335,7 @@ public async Task Handle(CompletionRequest request) return new CompletionResponse { IsIncomplete = !seenUnimportedCompletions && expectingImportedItems, - Items = completionsBuilder.ToImmutableArray() + Items = completionsBuilder.Capacity == completionsBuilder.Count ? completionsBuilder.MoveToImmutable() : completionsBuilder.ToImmutable() }; CompletionTrigger getCompletionTrigger(bool includeTriggerCharacter) @@ -477,7 +488,12 @@ public async Task Handle(CompletionResolveRequest req var sourceText = await document.GetTextAsync(); var typedSpan = completionService.GetDefaultCompletionListSpan(sourceText, position); var change = await completionService.GetChangeAsync(document, lastCompletionItem, typedSpan); - (request.Item.AdditionalTextEdits, _) = await GetAdditionalTextEdits(change, sourceText, (CSharpParseOptions)syntax!.Options, typedSpan, lastCompletionItem.DisplayText, providerName); + var (additionalTextEdits, offset) = GetAdditionalTextEdits(change, sourceText, (CSharpParseOptions)syntax!.Options, typedSpan, lastCompletionItem.DisplayText, providerName); + if (offset > 0) + { + Debug.Assert(additionalTextEdits is object); + request.Item.AdditionalTextEdits = additionalTextEdits; + } break; } @@ -487,7 +503,7 @@ public async Task Handle(CompletionResolveRequest req }; } - private async ValueTask<(IReadOnlyList edits, int endOffset)> GetAdditionalTextEdits( + private (IReadOnlyList? edits, int endOffset) GetAdditionalTextEdits( CompletionChange change, SourceText sourceText, CSharpParseOptions parseOptions, @@ -500,7 +516,7 @@ public async Task Handle(CompletionResolveRequest req // as the additional edit is not allowed to overlap with the insertion point. var additionalEditStartPosition = sourceText.Lines.GetLinePosition(change.TextChange.Span.Start); var additionalEditEndPosition = sourceText.Lines.GetLinePosition(typedSpan.Start - 1); - int additionalEditEndOffset = await getAdditionalTextEditEndOffset(change, sourceText, parseOptions, typedSpan, completionDisplayText, providerName); + int additionalEditEndOffset = getAdditionalTextEditEndOffset(change, sourceText, parseOptions, completionDisplayText, providerName); if (additionalEditEndOffset < 1) { // The first index of this was either 0 and the edit span was wrong, @@ -508,7 +524,7 @@ public async Task Handle(CompletionResolveRequest req // send the whole string wtih no additional edits and log a warning. _logger.LogWarning("Could not find the first index of the display text.\nDisplay text: {0}.\nCompletion Text: {1}", completionDisplayText, change.TextChange.NewText); - return default; + return (null, -1); } return (ImmutableArray.Create(new LinePositionSpanTextChange @@ -521,20 +537,12 @@ public async Task Handle(CompletionResolveRequest req EndColumn = additionalEditEndPosition.Character, }), additionalEditEndOffset); - static async ValueTask getAdditionalTextEditEndOffset(CompletionChange change, SourceText sourceText, CSharpParseOptions parseOptions, TextSpan typedSpan, string completionDisplayText, string providerName) + static int getAdditionalTextEditEndOffset(CompletionChange change, SourceText sourceText, CSharpParseOptions parseOptions, string completionDisplayText, string providerName) { - // For many simple cases, we can just find the first or last index of the completionDisplayText and that's good - // enough - int endOffset = (providerName == CompletionItemExtensions.ExtensionMethodImportCompletionProvider || - providerName == CompletionItemExtensions.TypeImportCompletionProvider) - // Import completion will put the displaytext at the end of the line, override completion will - // put it at the front. - ? change.TextChange.NewText!.LastIndexOf(completionDisplayText) - : change.TextChange.NewText!.IndexOf(completionDisplayText); - - if (endOffset > -1) + if (providerName == CompletionItemExtensions.ExtensionMethodImportCompletionProvider || + providerName == CompletionItemExtensions.TypeImportCompletionProvider) { - return endOffset; + return change.TextChange.NewText!.LastIndexOf(completionDisplayText); } // The DisplayText wasn't in the final string. This can happen in a few cases: @@ -550,13 +558,30 @@ static async ValueTask getAdditionalTextEditEndOffset(CompletionChange chan // to adjust the API here. // // In order to find the correct location here, we parse the change. The location - // of the name of the last method is the correct location in the string. + // of the method or property that contains the new cursor position is the location + // of the new changes Debug.Assert(providerName == CompletionItemExtensions.OverrideCompletionProvider || providerName == CompletionItemExtensions.PartialMethodCompletionProvider); + Debug.Assert(change.NewPosition.HasValue); + + var parsedTree = CSharpSyntaxTree.ParseText(sourceText.WithChanges(change.TextChange).ToString(), parseOptions); + + var tokenOfNewPosition = parsedTree.GetRoot().FindToken(change.NewPosition!.Value); + var finalNode = tokenOfNewPosition.Parent; + while (finalNode != null) + { + switch (finalNode) + { + case MethodDeclarationSyntax decl: + return decl.Identifier.SpanStart - change.TextChange.Span.Start; + case PropertyDeclarationSyntax prop: + return prop.Identifier.SpanStart - change.TextChange.Span.Start; + } + + finalNode = finalNode.Parent; + } - var parsedTree = CSharpSyntaxTree.ParseText(change.TextChange.NewText, parseOptions); - var lastMethodDecl = (await parsedTree.GetRootAsync()).DescendantNodes().OfType().Last(); - return lastMethodDecl.Identifier.SpanStart; + return -1; } } } diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/AbstractAutoCompleteTestFixture.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/AbstractAutoCompleteTestFixture.cs index 9910a5435f..6ba40e8ddf 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/AbstractAutoCompleteTestFixture.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/AbstractAutoCompleteTestFixture.cs @@ -7,7 +7,9 @@ namespace OmniSharp.Roslyn.CSharp.Tests { +#pragma warning disable CS0618 // Type or member is obsolete: Keeping the tests until we remove entirely public class AbstractAutoCompleteTestFixture : AbstractSingleRequestHandlerTestFixture +#pragma warning restore CS0618 // Type or member is obsolete { protected AbstractAutoCompleteTestFixture(ITestOutputHelper output, SharedOmniSharpHostFixture sharedOmniSharpHostFixture) : base(output, sharedOmniSharpHostFixture) diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs index c2c9bac7bb..f6b339d179 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs @@ -848,6 +848,67 @@ public class Derived : Base Assert.Equal("M1(object param)\n {\n return base.M1(param);$0\n \\}", item.TextEdit.NewText); } + [Theory] + [InlineData("dummy.cs")] + [InlineData("dummy.csx")] + public async Task OverrideCompletion_PropertyGetSet(string filename) + { + const string source = @" +using System; +public class Base +{ + public abstract string Prop { get; set; } +} +public class Derived : Base +{ + override $$ +} +"; + + var completions = await FindCompletionsAsync(filename, source, SharedOmniSharpTestHost); + var item = completions.Items.Single(c => c.Label.StartsWith("Prop")); + Assert.Equal("Prop", item.Label); + + Assert.Single(item.AdditionalTextEdits); + Assert.Equal("public override string", item.AdditionalTextEdits[0].NewText); + Assert.Equal(8, item.AdditionalTextEdits[0].StartLine); + Assert.Equal(4, item.AdditionalTextEdits[0].StartColumn); + Assert.Equal(8, item.AdditionalTextEdits[0].EndLine); + Assert.Equal(12, item.AdditionalTextEdits[0].EndColumn); + Assert.Equal("Prop { get => throw new NotImplementedException()$0; set => throw new NotImplementedException(); \\}", item.TextEdit.NewText); + } + + [Theory] + [InlineData("dummy.cs")] + [InlineData("dummy.csx")] + public async Task OverrideCompletion_PropertyGet(string filename) + { + const string source = @" +using System; +public class Base +{ + public abstract string Prop { get; } +} +public class Derived : Base +{ + override $$ +} +"; + + var completions = await FindCompletionsAsync(filename, source, SharedOmniSharpTestHost); + var item = completions.Items.Single(c => c.Label.StartsWith("Prop")); + Assert.Equal("Prop", item.Label); + + Assert.Single(item.AdditionalTextEdits); + Assert.Equal("public override string", item.AdditionalTextEdits[0].NewText); + Assert.Equal(8, item.AdditionalTextEdits[0].StartLine); + Assert.Equal(4, item.AdditionalTextEdits[0].StartColumn); + Assert.Equal(8, item.AdditionalTextEdits[0].EndLine); + Assert.Equal(12, item.AdditionalTextEdits[0].EndColumn); + Assert.Equal("Prop => throw new NotImplementedException();", item.TextEdit.NewText); + Assert.Equal(InsertTextFormat.PlainText, item.InsertTextFormat); + } + [Theory] [InlineData("dummy.cs")] [InlineData("dummy.csx")] From 74a8cc36a87ece195b78380bd15d900ec73db4fe Mon Sep 17 00:00:00 2001 From: Fred Silberberg Date: Sun, 27 Sep 2020 16:35:46 -0700 Subject: [PATCH 6/6] Remove InsertText entirely. --- .../Models/v1/Completion/CompletionItem.cs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs b/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs index 53532d63a6..6d93d48196 100644 --- a/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs +++ b/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs @@ -55,13 +55,6 @@ public class CompletionItem /// public string? FilterText { get; set; } - /// - /// A string that should be inserted into a document when selecting - /// this completion.When null or empty the label is used. - /// - [Obsolete("Use TextEdit instead", error: true)] - public string? InsertText { get; set; } - /// /// The format of . This applies to both and /// ..