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 5 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) && record.ParameterList is not null)
Copy link
Member

Choose a reason for hiding this comment

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

just don't do a kind check, and do a syntax-check, and you'll be all good here.

Suggested change
if (updatedNode.IsKind(SyntaxKind.RecordDeclaration, out RecordDeclarationSyntax? record) && record.ParameterList is not null)
if (updatedNode is RecordDeclarationSyntax { ParameterList: not null })

Copy link
Member Author

Choose a reason for hiding this comment

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

image

@CyrusNajmabadi Nullable analysis doesn't seem to handle patterns correctly.

{
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;
Copy link
Member Author

@Youssef1313 Youssef1313 Apr 30, 2021

Choose a reason for hiding this comment

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

Having primary constructor with AssociatedSymbol equals to the record symbol would save this ugly workaround 😄

i.e, Be able to re-write it as recordSymbol.InstanceConstructors.FirstOrDefault(c => recordSymbol.Equals(c.AssociatedSymbol)

@jcouv @jaredpar Is this something you want to do in the compiler side?

Edit: tracked by 53092.

}
}

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