From 220cb4c14e89586c5c59afa05317893d7eba2128 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Fri, 21 Aug 2020 16:12:35 -0700 Subject: [PATCH 1/2] Better handle completion when the display text is not in the final result Override and partial method completion can have scenarios where the DisplayText of the completion item does not appear in the final text, such as when types need to be imported or when nullability differs between original declaration and generation declaration sites. We now handle these by parsing the change if we fail to find the insertion text, and using the last method in the change to find the correct location in the new text. Fixes https://github.com/OmniSharp/omnisharp-roslyn/issues/1907. --- .../Models/v1/Completion/CompletionItem.cs | 8 +- .../v1/Completion/CompletionResponse.cs | 4 +- .../Services/Completion/CompletionService.cs | 70 ++++++-- .../CompletionFacts.cs | 151 +++++++++++++++--- 4 files changed, 196 insertions(+), 37 deletions(-) diff --git a/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs b/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs index 141c3a2575..aceb370b5a 100644 --- a/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs +++ b/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionItem.cs @@ -1,6 +1,6 @@ #nullable enable -using System.Collections.Immutable; +using System.Collections.Generic; namespace OmniSharp.Models.v1.Completion { @@ -23,7 +23,7 @@ public class CompletionItem /// /// Tags for this completion item /// - public ImmutableArray? Tags { get; set; } + public IReadOnlyList? Tags { get; set; } /// /// A human-readable string with additional information @@ -69,7 +69,7 @@ public class CompletionItem /// An optional set of characters that when pressed while this completion is active will accept it first and /// then type that character. /// - public ImmutableArray? CommitCharacters { get; set; } + public IReadOnlyList? CommitCharacters { get; set; } /// /// An optional array of additional text edits that are applied when @@ -80,7 +80,7 @@ public class CompletionItem /// (for example adding an import statement at the top of the file if the completion item will /// insert an unqualified type). /// - public ImmutableArray? AdditionalTextEdits { get; set; } + public IReadOnlyList? AdditionalTextEdits { get; set; } /// /// Index in the completions list that this completion occurred. diff --git a/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionResponse.cs b/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionResponse.cs index e624f14158..057bf44f50 100644 --- a/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionResponse.cs +++ b/src/OmniSharp.Abstractions/Models/v1/Completion/CompletionResponse.cs @@ -1,6 +1,6 @@ #nullable enable -using System.Collections.Immutable; +using System.Collections.Generic; namespace OmniSharp.Models.v1.Completion { @@ -14,6 +14,6 @@ public class CompletionResponse /// /// The completion items. /// - public ImmutableArray Items { get; set; } + public IReadOnlyList Items { get; set; } = null!; } } diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs index 52554abfc7..e10f871fee 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs @@ -7,9 +7,10 @@ using System.Linq; using System.Text; using System.Threading.Tasks; -using System.Xml.Resolvers; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Completion; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Tags; using Microsoft.CodeAnalysis.Text; using Microsoft.Extensions.Logging; @@ -159,17 +160,19 @@ public async Task Handle(CompletionRequest request) // the completion as done. bool seenUnimportedCompletions = false; bool expectingImportedItems = expandedItemsAvailable && _workspace.Options.GetOption(CompletionItemExtensions.ShowItemsFromUnimportedNamespaces, LanguageNames.CSharp) == true; + var syntax = await document.GetSyntaxTreeAsync(); for (int i = 0; i < completions.Items.Length; i++) { var completion = completions.Items[i]; var insertTextFormat = InsertTextFormat.PlainText; - ImmutableArray? additionalTextEdits = null; + IReadOnlyList? additionalTextEdits = null; char sortTextPrepend = '0'; if (!completion.TryGetInsertionText(out string insertText)) { - switch (completion.GetProviderName()) + string providerName = completion.GetProviderName(); + switch (providerName) { case CompletionItemExtensions.InternalsVisibleToCompletionProvider: // The IVT completer doesn't add extra things before the completion @@ -240,7 +243,7 @@ public async Task Handle(CompletionRequest request) } int additionalEditEndOffset; - (additionalTextEdits, additionalEditEndOffset) = GetAdditionalTextEdits(change, sourceText, typedSpan, completion.DisplayText, isImportCompletion: false); + (additionalTextEdits, additionalEditEndOffset) = await GetAdditionalTextEdits(change, sourceText, (CSharpParseOptions)syntax!.Options, typedSpan, completion.DisplayText, providerName); // Now that we have the additional edit, adjust the rest of the new text (insertText, insertTextFormat) = getAdjustedInsertTextWithPosition(change, position, additionalEditEndOffset); @@ -435,14 +438,16 @@ public async Task Handle(CompletionResolveRequest req request.Item.Documentation = textBuilder.ToString(); - switch (lastCompletionItem.GetProviderName()) + string providerName = lastCompletionItem.GetProviderName(); + switch (providerName) { case CompletionItemExtensions.ExtensionMethodImportCompletionProvider: case CompletionItemExtensions.TypeImportCompletionProvider: + var syntax = await document.GetSyntaxTreeAsync(); var sourceText = await document.GetTextAsync(); var typedSpan = completionService.GetDefaultCompletionListSpan(sourceText, position); var change = await completionService.GetChangeAsync(document, lastCompletionItem, typedSpan); - (request.Item.AdditionalTextEdits, _) = GetAdditionalTextEdits(change, sourceText, typedSpan, lastCompletionItem.DisplayText, isImportCompletion: true); + (request.Item.AdditionalTextEdits, _) = await GetAdditionalTextEdits(change, sourceText, (CSharpParseOptions)syntax!.Options, typedSpan, lastCompletionItem.DisplayText, providerName); break; } @@ -452,19 +457,20 @@ public async Task Handle(CompletionResolveRequest req }; } - private (ImmutableArray edits, int endOffset) GetAdditionalTextEdits(CompletionChange change, SourceText sourceText, TextSpan typedSpan, string completionDisplayText, bool isImportCompletion) + private async ValueTask<(IReadOnlyList edits, int endOffset)> GetAdditionalTextEdits( + CompletionChange change, + SourceText sourceText, + CSharpParseOptions parseOptions, + TextSpan typedSpan, + string completionDisplayText, + string providerName) { // We know the span starts before the text we're keying off of. So, break that // out into a separate edit. We need to cut out the space before the current word, // 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 = isImportCompletion - // 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); - + int additionalEditEndOffset = await getAdditionalTextEditEndOffset(change, sourceText, parseOptions, typedSpan, completionDisplayText, providerName); if (additionalEditEndOffset < 1) { // The first index of this was either 0 and the edit span was wrong, @@ -484,6 +490,44 @@ public async Task Handle(CompletionResolveRequest req EndLine = additionalEditEndPosition.Line, EndColumn = additionalEditEndPosition.Character, }), additionalEditEndOffset); + + static async ValueTask getAdditionalTextEditEndOffset(CompletionChange change, SourceText sourceText, CSharpParseOptions parseOptions, TextSpan typedSpan, 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) + { + return endOffset; + } + + // The DisplayText wasn't in the final string. This can happen in a few cases: + // * The override or partial method completion is involving types that need + // to have a using added in the final version, and won't be fully qualified + // as they were in the DisplayText + // * Nullable context differences, such as if the thing you're overriding is + // annotated but the final context being generated into does not have + // annotations enabled. + // For these cases, we currently should only be seeing override or partial + // completions, as import completions don't have nullable annotations or + // fully-qualified types in their DisplayTexts. If that ever changes, we'll have + // 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. + Debug.Assert(providerName == CompletionItemExtensions.OverrideCompletionProvider || + providerName == CompletionItemExtensions.PartialMethodCompletionProvider); + + var parsedTree = CSharpSyntaxTree.ParseText(change.TextChange.NewText, parseOptions); + var lastMethodDecl = (await parsedTree.GetRootAsync()).DescendantNodes().OfType().Last(); + return lastMethodDecl.Identifier.SpanStart; + } } } } diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs index c9c050e2b9..8c4cd04908 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs @@ -249,8 +249,8 @@ public static void Test(this object o) var completions = await FindCompletionsWithImportedAsync(filename, input, host); var resolved = await ResolveCompletionAsync(completions.Items.First(c => c.InsertText == "Test"), host); - Assert.Single(resolved.Item.AdditionalTextEdits.Value); - var additionalEdit = resolved.Item.AdditionalTextEdits.Value[0]; + Assert.Single(resolved.Item.AdditionalTextEdits); + var additionalEdit = resolved.Item.AdditionalTextEdits[0]; Assert.Equal(NormalizeNewlines("using N2;\n\nnamespace N1\r\n{\r\n public class C1\r\n {\r\n public void M(object o)\r\n {\r\n o"), additionalEdit.NewText); Assert.Equal(0, additionalEdit.StartLine); @@ -290,8 +290,8 @@ public static void Test(this object o) var completions = await FindCompletionsWithImportedAsync(filename, input, host); var resolved = await ResolveCompletionAsync(completions.Items.First(c => c.InsertText == "Guid"), host); - Assert.Single(resolved.Item.AdditionalTextEdits.Value); - var additionalEdit = resolved.Item.AdditionalTextEdits.Value[0]; + Assert.Single(resolved.Item.AdditionalTextEdits); + var additionalEdit = resolved.Item.AdditionalTextEdits[0]; Assert.Equal(NormalizeNewlines("using System;\n\nnamespace N1\r\n{\r\n public class C1\r\n {\r\n public void M(object o)\r\n {\r\n /*Guid*"), additionalEdit.NewText); Assert.Equal(0, additionalEdit.StartLine); @@ -337,8 +337,8 @@ public class C3 var completions = await FindCompletionsWithImportedAsync(filename, input, host); var resolved = await ResolveCompletionAsync(completions.Items.First(c => c.InsertText == "C2"), host); - Assert.Single(resolved.Item.AdditionalTextEdits.Value); - var additionalEdit = resolved.Item.AdditionalTextEdits.Value[0]; + Assert.Single(resolved.Item.AdditionalTextEdits); + var additionalEdit = resolved.Item.AdditionalTextEdits[0]; Assert.Equal(NormalizeNewlines("N2;\nusing N3;\r\nnamespace N1\r\n{\r\n public class C1\r\n {\r\n public void M(object o)\r\n {\r\n "), additionalEdit.NewText); Assert.Equal(1, additionalEdit.StartLine); @@ -631,9 +631,9 @@ class FooChild : Foo "public override void", "public override void", "public override string"}, - completions.Items.Select(c => c.AdditionalTextEdits.Value.Single().NewText)); + completions.Items.Select(c => c.AdditionalTextEdits.Single().NewText)); - Assert.All(completions.Items.Select(c => c.AdditionalTextEdits.Value.Single()), + Assert.All(completions.Items.Select(c => c.AdditionalTextEdits.Single()), r => { Assert.Equal(9, r.StartLine); @@ -684,9 +684,9 @@ class CN3 : IN2 "public override int", "protected override N1.CN1", "public override string"}, - completions.Items.Select(c => c.AdditionalTextEdits.Value.Single().NewText)); + completions.Items.Select(c => c.AdditionalTextEdits.Single().NewText)); - Assert.All(completions.Items.Select(c => c.AdditionalTextEdits.Value.Single()), + Assert.All(completions.Items.Select(c => c.AdditionalTextEdits.Single()), r => { Assert.Equal(15, r.StartLine); @@ -776,9 +776,9 @@ class Derived : Base "public override int", "protected override Test", "public override string"}, - completions.Items.Select(c => c.AdditionalTextEdits.Value.Single().NewText)); + completions.Items.Select(c => c.AdditionalTextEdits.Single().NewText)); - Assert.All(completions.Items.Select(c => c.AdditionalTextEdits.Value.Single()), + Assert.All(completions.Items.Select(c => c.AdditionalTextEdits.Single()), r => { Assert.Equal(8, r.StartLine); @@ -790,6 +790,64 @@ class Derived : Base Assert.All(completions.Items, c => Assert.Equal(InsertTextFormat.Snippet, c.InsertTextFormat)); } + [Fact] + public async Task OverrideCompletion_TypesNeedImport() + { + const string baseText = @" +using System; +public class Base +{ + public virtual Action GetAction(Action a) => null; +} +"; + + const string derivedText = @" +public class Derived : Base +{ + override $$ +}"; + + var completions = await FindCompletionsAsync("derived.cs", derivedText, SharedOmniSharpTestHost, additionalFiles: new[] { new TestFile("base.cs", baseText) }); + var item = completions.Items.Single(c => c.Label.StartsWith("GetAction")); + Assert.Equal("GetAction(System.Action a)", item.Label); + + Assert.Single(item.AdditionalTextEdits); + Assert.Equal(NormalizeNewlines("using System;\n\npublic class Derived : Base\r\n{\r\n public override Action"), item.AdditionalTextEdits[0].NewText); + Assert.Equal(1, item.AdditionalTextEdits[0].StartLine); + Assert.Equal(0, item.AdditionalTextEdits[0].StartColumn); + Assert.Equal(3, item.AdditionalTextEdits[0].EndLine); + Assert.Equal(12, item.AdditionalTextEdits[0].EndColumn); + Assert.Equal("", item.InsertText); + } + + [Fact] + public async Task OverrideCompletion_FromNullableToNonNullableContext() + { + const string text = @" +#nullable enable +public class Base +{ + public virtual object? M1(object? param) => throw null; +} +#nullable disable +public class Derived : Base +{ + override $$ +}"; + + var completions = await FindCompletionsAsync("derived.cs", text, SharedOmniSharpTestHost); + var item = completions.Items.Single(c => c.Label.StartsWith("M1")); + Assert.Equal("M1(object? param)", item.Label); + + Assert.Single(item.AdditionalTextEdits); + Assert.Equal("public override object", item.AdditionalTextEdits[0].NewText); + Assert.Equal(9, item.AdditionalTextEdits[0].StartLine); + 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;$0\n \\}", item.InsertText); + } + [Theory] [InlineData("dummy.cs")] [InlineData("dummy.csx")] @@ -817,6 +875,57 @@ partial class C Assert.All(completions.Items, c => Assert.Equal(InsertTextFormat.Snippet, c.InsertTextFormat)); } + [Fact] + public async Task PartialCompletion_TypesNeedImport() + { + const string file1 = @" +using System; +public partial class C +{ + partial void M(Action a); +} +"; + + const string file2 = @" +public partial class C +{ + partial $$ +}"; + + var completions = await FindCompletionsAsync("derived.cs", file2, SharedOmniSharpTestHost, additionalFiles: new[] { new TestFile("base.cs", file1) }); + var item = completions.Items.Single(c => c.Label.StartsWith("M")); + + Assert.Single(item.AdditionalTextEdits); + Assert.Equal(NormalizeNewlines("using System;\n\npublic partial class C\r\n{\r\n partial void"), item.AdditionalTextEdits[0].NewText); + Assert.Equal(1, item.AdditionalTextEdits[0].StartLine); + Assert.Equal(0, item.AdditionalTextEdits[0].StartColumn); + Assert.Equal(3, item.AdditionalTextEdits[0].EndLine); + Assert.Equal(11, item.AdditionalTextEdits[0].EndColumn); + Assert.Equal("void M1(string param)\n {\n throw new System.NotImplementedException();$0\n \\}", item.InsertText); + } + + [Fact] + public async Task PartialCompletion_FromNullableToNonNullableContext() + { + const string text = @" +#nullable enable +public partial class C +{ + partial void M1(object? param); +} +#nullable disable +public partial class C +{ + partial $$ +}"; + + var completions = await FindCompletionsAsync("derived.cs", text, SharedOmniSharpTestHost); + 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); + } + [Theory] [InlineData("dummy.cs")] [InlineData("dummy.csx")] @@ -841,9 +950,9 @@ override Ge$$ Assert.Equal(new[] { "public override bool", "public override int", "public override string"}, - completions.Items.Select(c => c.AdditionalTextEdits.Value.Single().NewText)); + completions.Items.Select(c => c.AdditionalTextEdits.Single().NewText)); - Assert.All(completions.Items.Select(c => c.AdditionalTextEdits.Value.Single()), + Assert.All(completions.Items.Select(c => c.AdditionalTextEdits.Single()), r => { Assert.Equal(3, r.StartLine); @@ -1230,11 +1339,17 @@ public async Task InternalsVisibleToCompletionSkipsMiscProject() private CompletionService GetCompletionService(OmniSharpTestHost host) => host.GetRequestHandler(EndpointName); - protected async Task FindCompletionsAsync(string filename, string source, OmniSharpTestHost testHost, char? triggerChar = null) + protected async Task FindCompletionsAsync(string filename, string source, OmniSharpTestHost testHost, char? triggerChar = null, TestFile[] additionalFiles = null) { var testFile = new TestFile(filename, source); - testHost.AddFilesToWorkspace(testFile); + var files = new[] { testFile }; + if (additionalFiles is object) + { + files = files.Concat(additionalFiles).ToArray(); + } + + testHost.AddFilesToWorkspace(files); var point = testFile.Content.GetPointFromPosition(); var request = new CompletionRequest @@ -1289,7 +1404,7 @@ private OmniSharpTestHost GetImportCompletionHost() private static string NormalizeNewlines(string str) => str.Replace("\r\n", Environment.NewLine); - private static void VerifySortOrders(ImmutableArray items) + private static void VerifySortOrders(IReadOnlyList items) { Assert.All(items, c => { @@ -1300,6 +1415,6 @@ private static void VerifySortOrders(ImmutableArray items) internal static class CompletionResponseExtensions { - public static bool IsSuggestionMode(this CompletionItem item) => (item.CommitCharacters?.IsDefaultOrEmpty ?? true) || !item.CommitCharacters.Contains(' '); + public static bool IsSuggestionMode(this CompletionItem item) => !item.CommitCharacters?.Contains(' ') ?? true; } } From 125cd740ed687c48a6be6b2ee49482b6cb9b06e0 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Fri, 21 Aug 2020 16:33:58 -0700 Subject: [PATCH 2/2] Fix baselines. --- tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs index 8c4cd04908..917fb0dec2 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/CompletionFacts.cs @@ -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("", item.InsertText); + Assert.Equal("GetAction(Action a)\n {\n return base.GetAction(a);$0\n \\}", item.InsertText); } [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;$0\n \\}", item.InsertText); + Assert.Equal("M1(object param)\n {\n return base.M1(param);$0\n \\}", item.InsertText); } [Theory] @@ -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("void M1(string param)\n {\n throw new System.NotImplementedException();$0\n \\}", item.InsertText); + Assert.Equal("M(Action a)\n {\n throw new NotImplementedException();$0\n \\}", item.InsertText); } [Fact]