Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add records support to XmlDocCommentCompletion and ChangeSignature #53052

Merged
merged 22 commits into from
Jul 12, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1261,5 +1261,39 @@ public void M()
}";
await TestChangeSignatureViaCommandAsync(LanguageNames.CSharp, markup, updatedSignature: permutation, expectedUpdatedInvocationDocumentCode: updatedCode);
}

[Fact, Trait(Traits.Feature, Traits.Features.ChangeSignature)]
[WorkItem(44558, "https://github.com/dotnet/roslyn/issues/44558")]
public async Task AddParameters_Record()
{
var markup = @"
/// <param name=""First""></param>
/// <param name=""Second""></param>
/// <param name=""Third""></param>
record $$R(int First, int Second, int Third)
{
static R M(this R r) => new R(1, 2, 3);
}
";
var updatedSignature = new AddedParameterOrExistingIndex[]
{
new(0),
new(2),
new(1),
new(new AddedParameter(null, "int", "Forth", CallSiteKind.Value, "12345"), "System.Int32")
};
var updatedCode = @"
/// <param name=""First""></param>
/// <param name=""Third""></param>
/// <param name=""Second""></param>
/// <param name=""Forth""></param>
record R(int First, int Third, int Second, int Forth)
{
static R M(this R r) => new R(1, 3, 2, 12345);
}
";

await TestChangeSignatureViaCommandAsync(LanguageNames.CSharp, markup, updatedSignature: updatedSignature, expectedUpdatedInvocationDocumentCode: updatedCode);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1012,5 +1012,27 @@ static void Goo()
await VerifyItemExistsAsync(text, "term");
await VerifyItemExistsAsync(text, "description");
}

[WorkItem(52738, "https://github.com/dotnet/roslyn/issues/52738")]
[Fact, Trait(Traits.Feature, Traits.Features.Completion)]
public async Task RecordParam()
{
await VerifyItemsExistAsync(@"
/// $$
public record Goo<T>(string MyParameter);
", "param name=\"MyParameter\"", "typeparam name=\"T\"");
}

[WorkItem(52738, "https://github.com/dotnet/roslyn/issues/52738")]
[Fact, Trait(Traits.Feature, Traits.Features.Completion)]
public async Task RecordParamRef()
{
await VerifyItemsExistAsync(@"
/// <summary>
/// $$
/// <summary>
public record Goo<T>(string MyParameter);
", "paramref name=\"MyParameter\"", "typeparamref name=\"T\"");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Diagnostics;
using Microsoft.CodeAnalysis.Test.Utilities;
using Roslyn.Test.Utilities;
using Xunit;
using Xunit.Abstractions;

Expand Down Expand Up @@ -785,5 +786,29 @@ public void Fizz(int i, int j, int k) {}

await TestAsync(initial, expected);
}

[WorkItem(52738, "https://github.com/dotnet/roslyn/issues/52738")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddDocCommentNodes)]
public async Task AddsParamTag_Record()
{
var initial = @"
/// <summary>
///
/// </summary>
/// <param name=""Second""></param>
record R(int [|First|], int Second, int Third);
";

var expected = @"
/// <summary>
///
/// </summary>
/// <param name=""First""></param>
/// <param name=""Second""></param>
/// <param name=""Third""></param>
record R(int First, int Second, int Third);
";
await TestAsync(initial, expected);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ internal sealed class CSharpChangeSignatureService : AbstractChangeSignatureServ
SyntaxKind.DelegateDeclaration,
SyntaxKind.SimpleLambdaExpression,
SyntaxKind.ParenthesizedLambdaExpression,
SyntaxKind.LocalFunctionStatement);
SyntaxKind.LocalFunctionStatement,
// TODO: Record structs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to create + link a work item here so we can track adding record struct support in the future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allisonchou It should fall into the existing test plan issue for record structs (#51199). Tagging @jcouv to edit the issue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have record structs now right? so can we just add that in this PR?

SyntaxKind.RecordDeclaration);

private static readonly ImmutableArray<SyntaxKind> _declarationAndInvocableKinds =
_declarationKinds.Concat(ImmutableArray.Create(
Expand Down Expand Up @@ -85,7 +87,9 @@ internal sealed class CSharpChangeSignatureService : AbstractChangeSignatureServ
SyntaxKind.NameMemberCref,
SyntaxKind.AnonymousMethodExpression,
SyntaxKind.ParenthesizedLambdaExpression,
SyntaxKind.SimpleLambdaExpression);
SyntaxKind.SimpleLambdaExpression,
// TODO: record structs
SyntaxKind.RecordDeclaration);

[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
Expand Down Expand Up @@ -265,12 +269,13 @@ public override async Task<SyntaxNode> ChangeSignatureAsync(
CancellationToken cancellationToken)
{
var updatedNode = potentiallyUpdatedNode as CSharpSyntaxNode;

allisonchou marked this conversation as resolved.
Show resolved Hide resolved
// Update <param> tags.
// TODO: Record structs
if (updatedNode.IsKind(SyntaxKind.MethodDeclaration) ||
updatedNode.IsKind(SyntaxKind.ConstructorDeclaration) ||
updatedNode.IsKind(SyntaxKind.IndexerDeclaration) ||
updatedNode.IsKind(SyntaxKind.DelegateDeclaration))
updatedNode.IsKind(SyntaxKind.DelegateDeclaration) ||
updatedNode.IsKind(SyntaxKind.RecordDeclaration))
{
var updatedLeadingTrivia = UpdateParamTagsInLeadingTrivia(document, updatedNode, declarationSymbol, signaturePermutation);
if (updatedLeadingTrivia != default && !updatedLeadingTrivia.IsEmpty)
Expand All @@ -286,6 +291,13 @@ public override async Task<SyntaxNode> ChangeSignatureAsync(
return method.WithParameterList(method.ParameterList.WithParameters(updatedParameters).WithAdditionalAnnotations(changeSignatureFormattingAnnotation));
}

// TODO: Record structs
if (updatedNode.IsKind(SyntaxKind.RecordDeclaration, out RecordDeclarationSyntax? record))
{
var updatedParameters = UpdateDeclaration(record.ParameterList.Parameters, signaturePermutation, CreateNewParameterSyntax);
return record.WithParameterList(record.ParameterList.WithParameters(updatedParameters).WithAdditionalAnnotations(changeSignatureFormattingAnnotation));
}

if (updatedNode.IsKind(SyntaxKind.LocalFunctionStatement, out LocalFunctionStatementSyntax? localFunction))
{
var updatedParameters = UpdateDeclaration(localFunction.ParameterList.Parameters, signaturePermutation, CreateNewParameterSyntax);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,19 @@ internal async Task<ChangeSignatureAnalyzedContext> GetChangeSignatureContextAsy
{
symbol = typeSymbol.DelegateInvokeMethod;
}
else if (typeSymbol.IsRecord)
{
// A bit hacky to determine the parameters of primary constructor associated with a given record.
// TODO: record structs.
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
const int RecordDeclarationRawKind = 9063;
var potentialPrimaryCtor = typeSymbol.InstanceConstructors.FirstOrDefault(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm a little worried about hardcoding the raw kind here. I almost think we should wait on merging until we have a proper workaround that doesn't involve hardcoding. @CyrusNajmabadi do you have any thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #53092 may not be very soon 😕

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're in an abstract type, just provide a helper that C# implements, going back to source to answer this.

c => c.DeclaringSyntaxReferences.FirstOrDefault().GetSyntax()?.RawKind == RecordDeclarationRawKind);

if (potentialPrimaryCtor is not null)
{
symbol = potentialPrimaryCtor;
}
}
}

if (!symbol.MatchesKind(SymbolKind.Method, SymbolKind.Property))
Expand Down Expand Up @@ -1001,7 +1014,6 @@ protected ImmutableArray<SyntaxTrivia> GetPermutedDocCommentTrivia(Document docu
node.GetTrailingTrivia(),
lastWhiteSpaceTrivia,
document.Project.Solution.Options.GetOption(FormattingOptions.NewLine, document.Project.Language));

var newTrivia = Generator.Trivia(extraDocComments);

updatedLeadingTrivia.Add(newTrivia);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -364,13 +364,33 @@ public static bool IsParams([NotNullWhen(returnValue: true)] this ISymbol? symbo
}

public static ImmutableArray<IParameterSymbol> GetParameters(this ISymbol? symbol)
=> symbol switch
{
return symbol switch
{
IMethodSymbol m => m.Parameters,
IPropertySymbol nt => nt.Parameters,
IPropertySymbol p => p.Parameters,
INamedTypeSymbol nt when nt.IsRecord => GetRecordParameters(nt),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i do not like this at all. i'd say the caller needs to potentially deal with records.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CyrusNajmabadi Done. However, it became a little bit more complicated.

_ => ImmutableArray<IParameterSymbol>.Empty,
};

ImmutableArray<IParameterSymbol> GetRecordParameters(INamedTypeSymbol recordSymbol)
{
const int RecordDeclarationRawKind = 9063;

// A bit hacky to determine the parameters of primary constructor associated with a given record.
// TODO: record structs.
var primaryConstructor = recordSymbol.InstanceConstructors.FirstOrDefault(
c => c.DeclaringSyntaxReferences.FirstOrDefault().GetSyntax()?.RawKind == RecordDeclarationRawKind);

if (primaryConstructor is null)
{
return ImmutableArray<IParameterSymbol>.Empty;
}

return primaryConstructor.Parameters;
}
}

public static ImmutableArray<ITypeParameterSymbol> GetTypeParameters(this ISymbol? symbol)
=> symbol switch
{
Expand Down