From 74f4a90d247a1d158e5c3d6580b4010bcb2c990a Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 1 Mar 2023 14:46:28 -0800 Subject: [PATCH] Include parens in crefs when necessary to disambiguate items --- .../CrefCompletionProviderTests.cs | 100 +++++++++++++++--- .../CrefCompletionProvider.cs | 73 ++++++------- 2 files changed, 122 insertions(+), 51 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/CrefCompletionProviderTests.cs b/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/CrefCompletionProviderTests.cs index b0896036fe829..0919eedf85264 100644 --- a/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/CrefCompletionProviderTests.cs +++ b/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/CrefCompletionProviderTests.cs @@ -2,8 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -#nullable disable - using System; using System.Collections.Generic; using System.Collections.Immutable; @@ -18,6 +16,7 @@ using Microsoft.CodeAnalysis.Test.Utilities; using Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion.Data; using Roslyn.Test.Utilities; +using Roslyn.Utilities; using Xunit; using RoslynTrigger = Microsoft.CodeAnalysis.Completion.CompletionTrigger; @@ -29,13 +28,12 @@ public class CrefCompletionProviderTests : AbstractCSharpCompletionProviderTests internal override Type GetCompletionProviderType() => typeof(CrefCompletionProvider); - private protected override async Task VerifyWorkerAsync( - string code, int position, - string expectedItemOrNull, string expectedDescriptionOrNull, - SourceCodeKind sourceCodeKind, bool usePreviousCharAsTrigger, bool checkForAbsence, - int? glyph, int? matchPriority, bool? hasSuggestionItem, string displayTextSuffix, - string displayTextPrefix, string inlineDescription = null, bool? isComplexTextEdit = null, - List matchingFilters = null, CompletionItemFlags? flags = null, CompletionOptions options = null, bool skipSpeculation = false) + private protected override async Task VerifyWorkerAsync(string code, int position, string expectedItemOrNull, + string expectedDescriptionOrNull, SourceCodeKind sourceCodeKind, bool usePreviousCharAsTrigger, + bool checkForAbsence, int? glyph, int? matchPriority, bool? hasSuggestionItem, string displayTextSuffix, + string displayTextPrefix, string? inlineDescription = null, bool? isComplexTextEdit = null, + List? matchingFilters = null, CompletionItemFlags? flags = null, + CompletionOptions? options = null, bool skipSpeculation = false) { await VerifyAtPositionAsync( code, position, usePreviousCharAsTrigger, expectedItemOrNull, expectedDescriptionOrNull, sourceCodeKind, @@ -219,7 +217,7 @@ public C(T x) { } } "; - await VerifyItemExistsAsync(text, "C"); + await VerifyItemExistsAsync(text, "C()"); await VerifyItemExistsAsync(text, "C(T)"); await VerifyItemExistsAsync(text, "C(int)"); } @@ -431,7 +429,7 @@ class C var called = false; var hostDocument = workspace.DocumentWithCursor; - var document = workspace.CurrentSolution.GetDocument(hostDocument.Id); + var document = workspace.CurrentSolution.GetRequiredDocument(hostDocument.Id); var service = GetCompletionService(document.Project); var provider = Assert.IsType(service.GetTestAccessor().GetImportedAndBuiltInProviders(ImmutableHashSet.Empty).Single()); provider.GetTestAccessor().SetSpeculativeNodeCallback(n => @@ -439,13 +437,13 @@ class C // asserts that we aren't be asked speculate on nodes inside documentation trivia. // This verifies that the provider is asking for a speculative SemanticModel // by walking to the node the documentation is attached to. - + Contract.ThrowIfNull(n); called = true; var parent = n.GetAncestor(); Assert.Null(parent); }); - var completionList = await GetCompletionListAsync(service, document, hostDocument.CursorPosition.Value, RoslynTrigger.Invoke); + var completionList = await GetCompletionListAsync(service, document, hostDocument.CursorPosition!.Value, RoslynTrigger.Invoke); Assert.True(called); } @@ -527,5 +525,81 @@ public void M((string s, int i) stringAndInt) { } await VerifyItemExistsAsync(text, "M(ValueTuple{string, int})"); } + + [Fact, WorkItem(43139, "https://github.com/dotnet/roslyn/issues/43139")] + public async Task TestNonOverload1() + { + var text = """ + class C + { + /// + /// + /// + public void M() { } + + void Dispose() { } + } + """; + + await VerifyItemExistsAsync(text, "Dispose"); + } + + [Fact, WorkItem(43139, "https://github.com/dotnet/roslyn/issues/43139")] + public async Task TestNonOverload2() + { + var text = """ + class C + { + /// + /// + /// + public void M() { } + + void Dispose() { } + } + """; + + await VerifyItemExistsAsync(text, "Dispose"); + } + + [Fact, WorkItem(43139, "https://github.com/dotnet/roslyn/issues/43139")] + public async Task TestOverload1() + { + var text = """ + class C + { + /// + /// + /// + public void M() { } + + void Dispose() { } + void Dispose(bool b) { } + } + """; + + await VerifyItemExistsAsync(text, "Dispose()"); + await VerifyItemExistsAsync(text, "Dispose(bool)"); + } + + [Fact, WorkItem(43139, "https://github.com/dotnet/roslyn/issues/43139")] + public async Task TestOverload2() + { + var text = """ + class C + { + /// + /// + /// + public void M() { } + + void Dispose() { } + void Dispose(bool b) { } + } + """; + + await VerifyItemExistsAsync(text, "Dispose()"); + await VerifyItemExistsAsync(text, "Dispose(bool)"); + } } } diff --git a/src/Features/CSharp/Portable/Completion/CompletionProviders/CrefCompletionProvider.cs b/src/Features/CSharp/Portable/Completion/CompletionProviders/CrefCompletionProvider.cs index 465f4a4477a92..648d4e9cd02e6 100644 --- a/src/Features/CSharp/Portable/Completion/CompletionProviders/CrefCompletionProvider.cs +++ b/src/Features/CSharp/Portable/Completion/CompletionProviders/CrefCompletionProvider.cs @@ -7,6 +7,7 @@ using System.Collections.Immutable; using System.Composition; using System.Diagnostics.CodeAnalysis; +using System.Linq; using System.Text; using System.Threading; using System.Threading.Tasks; @@ -242,11 +243,17 @@ private static IEnumerable CreateCompletionItems( var builder = SharedPools.Default().Allocate(); try { - foreach (var symbol in symbols) + foreach (var group in symbols.GroupBy(s => s.Name)) { - yield return CreateItem(semanticModel, symbol, token, position, builder, options); - if (TryCreateSpecialTypeItem(semanticModel, symbol, token, position, builder, options, out var item)) - yield return item; + var groupCount = group.Count(); + foreach (var symbol in group) + { + // For every symbol, we create an item that uses the regular CrefFormat, + // which uses intrinsic type keywords + yield return CreateItem(semanticModel, symbol, groupCount, token, position, builder, options, CrefFormat); + if (TryCreateSpecialTypeItem(semanticModel, symbol, token, position, builder, options, out var item)) + yield return item; + } } } finally @@ -264,7 +271,7 @@ private static bool TryCreateSpecialTypeItem( var typeSymbol = symbol as ITypeSymbol; if (typeSymbol.IsSpecialType()) { - item = CreateItem(semanticModel, symbol, token, position, builder, options, QualifiedCrefFormat); + item = CreateItem(semanticModel, symbol, groupCount: 1, token, position, builder, options, QualifiedCrefFormat); return true; } @@ -273,15 +280,13 @@ private static bool TryCreateSpecialTypeItem( } private static CompletionItem CreateItem( - SemanticModel semanticModel, ISymbol symbol, SyntaxToken token, int position, StringBuilder builder, ImmutableDictionary options) - { - // For every symbol, we create an item that uses the regular CrefFormat, - // which uses intrinsic type keywords - return CreateItem(semanticModel, symbol, token, position, builder, options, CrefFormat); - } - - private static CompletionItem CreateItem( - SemanticModel semanticModel, ISymbol symbol, SyntaxToken token, int position, StringBuilder builder, ImmutableDictionary options, + SemanticModel semanticModel, + ISymbol symbol, + int groupCount, + SyntaxToken token, + int position, + StringBuilder builder, + ImmutableDictionary options, SymbolDisplayFormat unqualifiedCrefFormat) { builder.Clear(); @@ -297,35 +302,27 @@ private static CompletionItem CreateItem( builder.Append(symbol.ToMinimalDisplayString(semanticModel, token.SpanStart, unqualifiedCrefFormat)); var parameters = symbol.GetParameters(); - if (!parameters.IsDefaultOrEmpty) + + // if this has parameters, then add them here. Otherwise, if this is a method without parameters, but + // there are overloads of it, then also add the parameters to disambiguate. + if (parameters.Length > 0 || + (symbol is IMethodSymbol && groupCount >= 2)) { // Note: we intentionally don't add the "params" modifier for any parameters. builder.Append(symbol.IsIndexer() ? '[' : '('); - - for (var i = 0; i < parameters.Length; i++) - { - if (i > 0) - builder.Append(", "); - - var parameter = parameters[i]; - - switch (parameter.RefKind) + builder.AppendJoinedValues(", ", parameters, + (p, builder) => { - case RefKind.Ref: - builder.Append("ref "); - break; - case RefKind.Out: - builder.Append("out "); - break; - case RefKind.In: - builder.Append("in "); - break; - } - - builder.Append(parameter.Type.ToMinimalDisplayString(semanticModel, position, MinimalParameterTypeFormat)); - } - + builder.Append(p.RefKind switch + { + RefKind.Ref => "ref ", + RefKind.Out => "out ", + RefKind.In => "in ", + _ => "", + }); + builder.Append(p.Type.ToMinimalDisplayString(semanticModel, position, MinimalParameterTypeFormat)); + }); builder.Append(symbol.IsIndexer() ? ']' : ')'); } }