-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Changes from 12 commits
8305017
6d6f087
41eae46
ac6de34
6fbf876
76c88dc
4c0bcef
df2d615
6749276
aa0c17d
e494608
e8ac91d
a7c5f11
d36653a
de681db
0c8bfef
42bf97a
b7ca33c
761c811
43eb33e
149d578
327b8e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -7,6 +7,7 @@ | |||||
using System.Collections.Immutable; | ||||||
using System.Composition; | ||||||
using System.Diagnostics; | ||||||
using System.Diagnostics.CodeAnalysis; | ||||||
using System.Linq; | ||||||
using System.Threading; | ||||||
using System.Threading.Tasks; | ||||||
|
@@ -42,7 +43,9 @@ internal sealed class CSharpChangeSignatureService : AbstractChangeSignatureServ | |||||
SyntaxKind.DelegateDeclaration, | ||||||
SyntaxKind.SimpleLambdaExpression, | ||||||
SyntaxKind.ParenthesizedLambdaExpression, | ||||||
SyntaxKind.LocalFunctionStatement); | ||||||
SyntaxKind.LocalFunctionStatement, | ||||||
// TODO: Record structs | ||||||
SyntaxKind.RecordDeclaration); | ||||||
|
||||||
private static readonly ImmutableArray<SyntaxKind> _declarationAndInvocableKinds = | ||||||
_declarationKinds.Concat(ImmutableArray.Create( | ||||||
|
@@ -85,7 +88,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)] | ||||||
|
@@ -265,12 +270,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) | ||||||
|
@@ -286,6 +292,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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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); | ||||||
|
@@ -745,13 +758,14 @@ private ImmutableArray<SyntaxTrivia> UpdateParamTagsInLeadingTrivia(Document doc | |||||
return GetPermutedDocCommentTrivia(document, node, permutedParamNodes); | ||||||
} | ||||||
|
||||||
private static ImmutableArray<SyntaxNode> VerifyAndPermuteParamNodes(IEnumerable<XmlElementSyntax> paramNodes, ISymbol declarationSymbol, SignatureChange updatedSignature) | ||||||
private ImmutableArray<SyntaxNode> VerifyAndPermuteParamNodes(IEnumerable<XmlElementSyntax> paramNodes, ISymbol declarationSymbol, SignatureChange updatedSignature) | ||||||
{ | ||||||
// Only reorder if count and order match originally. | ||||||
var originalParameters = updatedSignature.OriginalConfiguration.ToListOfParameters(); | ||||||
var reorderedParameters = updatedSignature.UpdatedConfiguration.ToListOfParameters(); | ||||||
|
||||||
var declaredParameters = declarationSymbol.GetParameters(); | ||||||
var declaredParameters = GetParameters(declarationSymbol); | ||||||
|
||||||
if (paramNodes.Count() != declaredParameters.Length) | ||||||
{ | ||||||
return ImmutableArray<SyntaxNode>.Empty; | ||||||
|
@@ -875,5 +889,35 @@ protected override bool SupportsOptionalAndParamsArrayParametersSimultaneously() | |||||
|
||||||
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; | ||||||
} | ||||||
|
||||||
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. BUT | ||||||
allisonchou marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
// make sure none of the other callers doesn't need records to be handled. | ||||||
allisonchou marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
var primaryConstructor = recordSymbol.InstanceConstructors.FirstOrDefault( | ||||||
c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is RecordDeclarationSyntax); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider extracting helper for this (you do the same work higher up). |
||||||
|
||||||
if (primaryConstructor is not null) | ||||||
{ | ||||||
declaredParameters = primaryConstructor.Parameters; | ||||||
} | ||||||
} | ||||||
|
||||||
return declaredParameters; | ||||||
} | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
using System.Collections.Generic; | ||
using System.Collections.Immutable; | ||
using System.Composition; | ||
using System.Diagnostics; | ||
using System.Linq; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
@@ -365,6 +366,28 @@ 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) | ||
{ | ||
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. BUT | ||
// make sure none of the other callers doesn't need records to be handled. | ||
var primaryConstructor = recordSymbol.InstanceConstructors.FirstOrDefault( | ||
c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is RecordDeclarationSyntax); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. used again. consider an extension now. |
||
|
||
if (primaryConstructor is not null) | ||
{ | ||
declaredParameters = primaryConstructor.Parameters; | ||
} | ||
} | ||
|
||
return declaredParameters; | ||
} | ||
|
||
private static readonly CompletionItemRules s_defaultRules = | ||
CompletionItemRules.Create( | ||
filterCharacterRules: FilterRules, | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?