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 @@ -44,7 +44,7 @@ internal sealed class CSharpChangeSignatureService : AbstractChangeSignatureServ
SyntaxKind.SimpleLambdaExpression,
SyntaxKind.ParenthesizedLambdaExpression,
SyntaxKind.LocalFunctionStatement,
// TODO: Record structs
SyntaxKind.RecordStructDeclaration,
SyntaxKind.RecordDeclaration);

private static readonly ImmutableArray<SyntaxKind> _declarationAndInvocableKinds =
Expand Down Expand Up @@ -89,7 +89,7 @@ internal sealed class CSharpChangeSignatureService : AbstractChangeSignatureServ
SyntaxKind.AnonymousMethodExpression,
SyntaxKind.ParenthesizedLambdaExpression,
SyntaxKind.SimpleLambdaExpression,
// TODO: record structs
SyntaxKind.RecordStructDeclaration,
SyntaxKind.RecordDeclaration);

[ImportingConstructor]
Expand Down Expand Up @@ -272,11 +272,11 @@ public override async Task<SyntaxNode> ChangeSignatureAsync(
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.RecordStructDeclaration) ||
updatedNode.IsKind(SyntaxKind.RecordDeclaration))
{
var updatedLeadingTrivia = UpdateParamTagsInLeadingTrivia(document, updatedNode, declarationSymbol, signaturePermutation);
Expand All @@ -293,8 +293,7 @@ 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)
if (updatedNode is RecordDeclarationSyntax { ParameterList: not null } record)
{
var updatedParameters = UpdateDeclaration(record.ParameterList.Parameters, signaturePermutation, CreateNewParameterSyntax);
return record.WithParameterList(record.ParameterList.WithParameters(updatedParameters).WithAdditionalAnnotations(changeSignatureFormattingAnnotation));
Expand Down Expand Up @@ -892,31 +891,15 @@ protected override SyntaxToken CommaTokenWithElasticSpace()
=> Token(SyntaxKind.CommaToken).WithTrailingTrivia(ElasticSpace);

protected override bool TryGetRecordPrimaryConstructor(INamedTypeSymbol typeSymbol, [NotNullWhen(true)] out IMethodSymbol? primaryConstructor)
{
Debug.Assert(typeSymbol.IsRecord);
primaryConstructor = typeSymbol.InstanceConstructors.FirstOrDefault(
c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is RecordDeclarationSyntax);
return primaryConstructor is not null;
}
=> typeSymbol.TryGetRecordPrimaryConstructor(out primaryConstructor);

protected override ImmutableArray<IParameterSymbol> GetParameters(ISymbol declarationSymbol)
{
var declaredParameters = declarationSymbol.GetParameters();
allisonchou marked this conversation as resolved.
Show resolved Hide resolved
if (declarationSymbol is INamedTypeSymbol { IsRecord: true } recordSymbol)
{
Debug.Assert(declaredParameters.IsDefaultOrEmpty, "If GetParameters extension handles record, we can remove the handling here.");
// A bit hacky to determine the parameters of primary constructor associated with a given record.
// Simplifying is tracked by: https://github.com/dotnet/roslyn/issues/53092.
// Note: When the issue is handled, we can remove the logic here and handle things in GetParameters extension. BUT
// if GetParameters extension method gets updated to handle records, we need to test EVERY usage
// of the extension method and make sure the change is applicable to all these usages.
var primaryConstructor = recordSymbol.InstanceConstructors.FirstOrDefault(
c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is RecordDeclarationSyntax);

if (primaryConstructor is not null)
{
declaredParameters = primaryConstructor.Parameters;
}
if (declarationSymbol is INamedTypeSymbol { IsRecord: true } recordSymbol &&
recordSymbol.TryGetRecordPrimaryConstructor(out var primaryConstructor))
{
declaredParameters = primaryConstructor.Parameters;
}

return declaredParameters;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -369,21 +369,10 @@ private string GetAttributeValue(XmlAttributeSyntax attribute)
protected override ImmutableArray<IParameterSymbol> GetParameters(ISymbol declarationSymbol)
{
var declaredParameters = declarationSymbol.GetParameters();
allisonchou marked this conversation as resolved.
Show resolved Hide resolved
if (declarationSymbol is INamedTypeSymbol { IsRecord: true } recordSymbol)
if (declarationSymbol is INamedTypeSymbol { IsRecord: true } recordSymbol &&
recordSymbol.TryGetRecordPrimaryConstructor(out var primaryConstructor))
Copy link
Member

Choose a reason for hiding this comment

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

You could move the IsRecord check into TryGetPrimaryConstructor

{
Debug.Assert(declaredParameters.IsDefaultOrEmpty, "If GetParameters extension handles record, we can remove the handling here.");
// A bit hacky to determine the parameters of primary constructor associated with a given record.
// Simplifying is tracked by: https://github.com/dotnet/roslyn/issues/53092.
// Note: When the issue is handled, we can remove the logic here and handle things in GetParameters extension. BUT
// if GetParameters extension method gets updated to handle records, we need to test EVERY usage
// of the extension method and make sure the change is applicable to all these usages.
var primaryConstructor = recordSymbol.InstanceConstructors.FirstOrDefault(
c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is RecordDeclarationSyntax);

if (primaryConstructor is not null)
{
declaredParameters = primaryConstructor.Parameters;
}
declaredParameters = primaryConstructor.Parameters;
}

return declaredParameters;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,9 @@ internal async Task<ChangeSignatureAnalyzedContext> GetChangeSignatureContextAsy
{
symbol = typeSymbol.DelegateInvokeMethod;
}
else if (typeSymbol.IsRecord)
else if (typeSymbol.IsRecord && TryGetRecordPrimaryConstructor(typeSymbol, out var primaryConstructor))
{
if (TryGetRecordPrimaryConstructor(typeSymbol, out var primaryConstructor))
{
symbol = primaryConstructor;
}
symbol = primaryConstructor;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -177,5 +179,21 @@ public static bool IsIntrinsicType(this ITypeSymbol typeSymbol)
return false;
}
}

public static bool TryGetRecordPrimaryConstructor(this INamedTypeSymbol typeSymbol, [NotNullWhen(true)] out IMethodSymbol? primaryConstructor)
{
Debug.Assert(typeSymbol.IsRecord);
Debug.Assert(typeSymbol.GetParameters().IsDefaultOrEmpty, "If GetParameters extension handles record, we can remove the handling here.");

// A bit hacky to determine the parameters of primary constructor associated with a given record.
// Simplifying is tracked by: https://github.com/dotnet/roslyn/issues/53092.
// Note: When the issue is handled, we can remove the logic here and handle things in GetParameters extension. BUT
// if GetParameters extension method gets updated to handle records, we need to test EVERY usage
// of the extension method and make sure the change is applicable to all these usages.

primaryConstructor = typeSymbol.InstanceConstructors.FirstOrDefault(
c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is RecordDeclarationSyntax);
return primaryConstructor is not null;
}
}
}