Skip to content

Commit

Permalink
Merge pull request #67140 from CyrusNajmabadi/ambiguousCref
Browse files Browse the repository at this point in the history 
Include parens in crefs when necessary to disambiguate items
  • Loading branch information
@CyrusNajmabadi
CyrusNajmabadi committed Mar 6, 2023
2 parents f64507a + 74f4a90 commit c10fcd3
Show file tree
Hide file tree
Showing 2 changed files with 122 additions and 51 deletions.
100 changes: 87 additions & 13 deletions src/EditorFeatures/CSharpTest/Completion/CompletionProviders/CrefCompletionProviderTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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<CompletionFilter> 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<CompletionFilter>? matchingFilters = null, CompletionItemFlags? flags = null,
CompletionOptions? options = null, bool skipSpeculation = false)
{
await VerifyAtPositionAsync(
code, position, usePreviousCharAsTrigger, expectedItemOrNull, expectedDescriptionOrNull, sourceCodeKind,
Expand Down Expand Up @@ -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)");
}
Expand Down Expand Up @@ -431,21 +429,21 @@ 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<CrefCompletionProvider>(service.GetTestAccessor().GetImportedAndBuiltInProviders(ImmutableHashSet<string>.Empty).Single());
provider.GetTestAccessor().SetSpeculativeNodeCallback(n =>
{
// 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<DocumentationCommentTriviaSyntax>();
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);
}
Expand Down Expand Up @@ -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
{
/// <summary>
/// <seealso cref="C.$$"/>
/// </summary>
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
{
/// <summary>
/// <seealso cref="$$"/>
/// </summary>
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
{
/// <summary>
/// <seealso cref="C.$$"/>
/// </summary>
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
{
/// <summary>
/// <seealso cref="$$"/>
/// </summary>
public void M() { }

void Dispose() { }
void Dispose(bool b) { }
}
""";

await VerifyItemExistsAsync(text, "Dispose()");
await VerifyItemExistsAsync(text, "Dispose(bool)");
}
}
}
73 changes: 35 additions & 38 deletions src/Features/CSharp/Portable/Completion/CompletionProviders/CrefCompletionProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -242,11 +243,17 @@ private static IEnumerable<CompletionItem> CreateCompletionItems(
var builder = SharedPools.Default<StringBuilder>().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
Expand All @@ -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;
}

Expand All @@ -273,15 +280,13 @@ private static bool TryCreateSpecialTypeItem(
}

private static CompletionItem CreateItem(
SemanticModel semanticModel, ISymbol symbol, SyntaxToken token, int position, StringBuilder builder, ImmutableDictionary<string, string> 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<string, string> options,
SemanticModel semanticModel,
ISymbol symbol,
int groupCount,
SyntaxToken token,
int position,
StringBuilder builder,
ImmutableDictionary<string, string> options,
SymbolDisplayFormat unqualifiedCrefFormat)
{
builder.Clear();
Expand All @@ -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() ? ']' : ')');
}
}
Expand Down

0 comments on commit c10fcd3

Please sign in to comment.