From 830501754fcfffec24b5b9bf2d28b6bf3210d3c9 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Fri, 30 Apr 2021 11:32:23 +0200 Subject: [PATCH 01/20] Fix Xml docs completion for records --- ...mentationCommentCompletionProviderTests.cs | 22 +++++++++++++++++ .../Core/Extensions/ISymbolExtensions.cs | 24 +++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/XmlDocumentationCommentCompletionProviderTests.cs b/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/XmlDocumentationCommentCompletionProviderTests.cs index 8a0a6aa11c25a..52cbea5299e13 100644 --- a/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/XmlDocumentationCommentCompletionProviderTests.cs +++ b/src/EditorFeatures/CSharpTest/Completion/CompletionProviders/XmlDocumentationCommentCompletionProviderTests.cs @@ -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(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(@" +/// +/// $$ +/// +public record Goo(string MyParameter); +", "paramref name=\"MyParameter\"", "typeparamref name=\"T\""); + } } } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs index 95ab3e5184bc7..4e7db73439afe 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs @@ -364,13 +364,33 @@ public static bool IsParams([NotNullWhen(returnValue: true)] this ISymbol? symbo } public static ImmutableArray 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), _ => ImmutableArray.Empty, }; + ImmutableArray 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.Empty; + } + + return primaryConstructor.Parameters; + } + } + public static ImmutableArray GetTypeParameters(this ISymbol? symbol) => symbol switch { From 6d6f0876993f10f615796fc0ef95c25ae1031683 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Fri, 30 Apr 2021 12:35:32 +0200 Subject: [PATCH 02/20] Add test for record in AddDocCommentNodesCodeFixProviderTests --- .../AddDocCommentNodesCodeFixProviderTests.cs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/EditorFeatures/CSharpTest/DocumentationComments/CodeFixes/AddDocCommentNodesCodeFixProviderTests.cs b/src/EditorFeatures/CSharpTest/DocumentationComments/CodeFixes/AddDocCommentNodesCodeFixProviderTests.cs index eebb1502ff7bc..9f9adf39a2826 100644 --- a/src/EditorFeatures/CSharpTest/DocumentationComments/CodeFixes/AddDocCommentNodesCodeFixProviderTests.cs +++ b/src/EditorFeatures/CSharpTest/DocumentationComments/CodeFixes/AddDocCommentNodesCodeFixProviderTests.cs @@ -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; @@ -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 = @" +/// +/// +/// +/// +record R(int [|First|], int Second, int Third); +"; + + var expected = @" +/// +/// +/// +/// +/// +/// +record R(int First, int Second, int Third); +"; + await TestAsync(initial, expected); + } } } From 41eae469e53d4bc625d9d3c4ce7620fdc44c57cc Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Fri, 30 Apr 2021 14:42:04 +0200 Subject: [PATCH 03/20] Add records support to ChangeSignature (crashing) --- .../ChangeSignature/AddParameterTests.cs | 34 +++++++++++++++++++ .../CSharpChangeSignatureService.cs | 20 ++++++++--- .../AbstractChangeSignatureService.cs | 14 +++++++- 3 files changed, 63 insertions(+), 5 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/ChangeSignature/AddParameterTests.cs b/src/EditorFeatures/CSharpTest/ChangeSignature/AddParameterTests.cs index 136592635bd9a..c04bd4a405e80 100644 --- a/src/EditorFeatures/CSharpTest/ChangeSignature/AddParameterTests.cs +++ b/src/EditorFeatures/CSharpTest/ChangeSignature/AddParameterTests.cs @@ -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 = @" +/// +/// +/// +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 = @" +/// +/// +/// +/// +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); + } } } diff --git a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs index 5ebd9cb87ab0b..65d366a344dfc 100644 --- a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs +++ b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs @@ -42,7 +42,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 _declarationAndInvocableKinds = _declarationKinds.Concat(ImmutableArray.Create( @@ -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)] @@ -265,12 +269,13 @@ public override async Task ChangeSignatureAsync( CancellationToken cancellationToken) { var updatedNode = potentiallyUpdatedNode as CSharpSyntaxNode; - // Update 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 +291,13 @@ public override async Task 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); diff --git a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs index 80ddbceb3bc97..3bc46db8c004c 100644 --- a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs +++ b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs @@ -124,6 +124,19 @@ internal async Task 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. + const int RecordDeclarationRawKind = 9063; + var potentialPrimaryCtor = typeSymbol.InstanceConstructors.FirstOrDefault( + c => c.DeclaringSyntaxReferences.FirstOrDefault().GetSyntax()?.RawKind == RecordDeclarationRawKind); + + if (potentialPrimaryCtor is not null) + { + symbol = potentialPrimaryCtor; + } + } } if (!symbol.MatchesKind(SymbolKind.Method, SymbolKind.Property)) @@ -1001,7 +1014,6 @@ protected ImmutableArray GetPermutedDocCommentTrivia(Document docu node.GetTrailingTrivia(), lastWhiteSpaceTrivia, document.Project.Solution.Options.GetOption(FormattingOptions.NewLine, document.Project.Language)); - var newTrivia = Generator.Trivia(extraDocComments); updatedLeadingTrivia.Add(newTrivia); From ac6de348d3e97bd0c39279a6c6dba65e8f749134 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Fri, 30 Apr 2021 15:02:56 +0200 Subject: [PATCH 04/20] Fix nullable warning --- .../Portable/ChangeSignature/AbstractChangeSignatureService.cs | 2 +- .../Compiler/Core/Extensions/ISymbolExtensions.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs index 3bc46db8c004c..fc4cba1b01dcb 100644 --- a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs +++ b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs @@ -130,7 +130,7 @@ internal async Task GetChangeSignatureContextAsy // TODO: record structs. const int RecordDeclarationRawKind = 9063; var potentialPrimaryCtor = typeSymbol.InstanceConstructors.FirstOrDefault( - c => c.DeclaringSyntaxReferences.FirstOrDefault().GetSyntax()?.RawKind == RecordDeclarationRawKind); + c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax()?.RawKind == RecordDeclarationRawKind); if (potentialPrimaryCtor is not null) { diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs index 4e7db73439afe..f75000cd44c75 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs @@ -380,7 +380,7 @@ ImmutableArray GetRecordParameters(INamedTypeSymbol recordSymb // 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); + c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax()?.RawKind == RecordDeclarationRawKind); if (primaryConstructor is null) { From 6fbf87654752aeb07adf28a476eb0136b6997e46 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Fri, 30 Apr 2021 15:24:12 +0200 Subject: [PATCH 05/20] Fix nullable warning --- .../Portable/ChangeSignature/CSharpChangeSignatureService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs index 65d366a344dfc..9a2832b8fd5f3 100644 --- a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs +++ b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs @@ -292,7 +292,7 @@ public override async Task ChangeSignatureAsync( } // TODO: Record structs - if (updatedNode.IsKind(SyntaxKind.RecordDeclaration, out RecordDeclarationSyntax? record)) + if (updatedNode.IsKind(SyntaxKind.RecordDeclaration, out RecordDeclarationSyntax? record) && record.ParameterList is not null) { var updatedParameters = UpdateDeclaration(record.ParameterList.Parameters, signaturePermutation, CreateNewParameterSyntax); return record.WithParameterList(record.ParameterList.WithParameters(updatedParameters).WithAdditionalAnnotations(changeSignatureFormattingAnnotation)); From 76c88dc35473f14bd3baffe689026d9f649b5187 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sun, 2 May 2021 10:55:00 +0000 Subject: [PATCH 06/20] Fix assertion failure and fix test --- .../CSharpTest/ChangeSignature/AddParameterTests.cs | 4 ++-- .../Portable/CodeGeneration/CSharpSyntaxGenerator.cs | 11 +++++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/ChangeSignature/AddParameterTests.cs b/src/EditorFeatures/CSharpTest/ChangeSignature/AddParameterTests.cs index c04bd4a405e80..11fecb5f73ea8 100644 --- a/src/EditorFeatures/CSharpTest/ChangeSignature/AddParameterTests.cs +++ b/src/EditorFeatures/CSharpTest/ChangeSignature/AddParameterTests.cs @@ -1272,7 +1272,7 @@ public async Task AddParameters_Record() /// record $$R(int First, int Second, int Third) { - static R M(this R r) => new R(1, 2, 3); + static R M() => new R(1, 2, 3); } "; var updatedSignature = new AddedParameterOrExistingIndex[] @@ -1289,7 +1289,7 @@ record $$R(int First, int Second, int Third) /// record R(int First, int Third, int Second, int Forth) { - static R M(this R r) => new R(1, 3, 2, 12345); + static R M() => new R(1, 3, 2, 12345); } "; diff --git a/src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs b/src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs index 3851106207d44..5c6523e64dcc5 100644 --- a/src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs +++ b/src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs @@ -87,10 +87,13 @@ internal override SyntaxNode DocumentationCommentTrivia(IEnumerable SyntaxFactory.List(nodes), SyntaxFactory.Token(SyntaxKind.EndOfDocumentationCommentToken)); - return docTrivia - .WithLeadingTrivia(SyntaxFactory.DocumentationCommentExterior("/// ")) - .WithTrailingTrivia(trailingTrivia) - .WithTrailingTrivia( + docTrivia = docTrivia.WithLeadingTrivia(SyntaxFactory.DocumentationCommentExterior("/// ")) + .WithTrailingTrivia(trailingTrivia); + + if (lastWhitespaceTrivia == default) + return docTrivia.WithTrailingTrivia(SyntaxFactory.EndOfLine(endOfLineString)); + + return docTrivia.WithTrailingTrivia( SyntaxFactory.EndOfLine(endOfLineString), lastWhitespaceTrivia); } From 4c0bcefaa133a68d4c5257dc89ca7f5a7d1f86b3 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sun, 2 May 2021 11:44:02 +0000 Subject: [PATCH 07/20] Don't crash VB when there is no whitespace leading trivia --- .../ChangeSignature/AddParameterTests.vb | 31 +++++++++++++++++++ .../VisualBasicSyntaxGenerator.vb | 11 +++++-- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/src/EditorFeatures/VisualBasicTest/ChangeSignature/AddParameterTests.vb b/src/EditorFeatures/VisualBasicTest/ChangeSignature/AddParameterTests.vb index 358f561f10ea0..0053746e98b52 100644 --- a/src/EditorFeatures/VisualBasicTest/ChangeSignature/AddParameterTests.vb +++ b/src/EditorFeatures/VisualBasicTest/ChangeSignature/AddParameterTests.vb @@ -825,5 +825,36 @@ End Class]]>.NormalizedValue() Await TestChangeSignatureViaCommandAsync(LanguageNames.VisualBasic, markup, updatedSignature:=permutation, expectedUpdatedInvocationDocumentCode:=updatedCode) End Function + + + + Public Async Function TestAddParameter_NoLastWhitespaceTrivia() As Task + + Dim markup = +''' +''' +Sub $$M(a As Integer) +End Sub +End Class]]>.NormalizedValue() + Dim permutation = + { + New AddedParameterOrExistingIndex(0), + New AddedParameterOrExistingIndex(New AddedParameter(Nothing, "Integer", "b", CallSiteKind.Value), "Integer") + } + + Dim updatedCode = + ''' + ''' + ''' + Sub M(a As Integer, b As Integer) + End Sub +End Class]]>.NormalizedValue() + + Await TestChangeSignatureViaCommandAsync(LanguageNames.VisualBasic, markup, updatedSignature:=permutation, expectedUpdatedInvocationDocumentCode:=updatedCode) + End Function End Class End Namespace diff --git a/src/Workspaces/VisualBasic/Portable/CodeGeneration/VisualBasicSyntaxGenerator.vb b/src/Workspaces/VisualBasic/Portable/CodeGeneration/VisualBasicSyntaxGenerator.vb index 7f01fc3b020c4..aca0035917cb5 100644 --- a/src/Workspaces/VisualBasic/Portable/CodeGeneration/VisualBasicSyntaxGenerator.vb +++ b/src/Workspaces/VisualBasic/Portable/CodeGeneration/VisualBasicSyntaxGenerator.vb @@ -67,9 +67,14 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeGeneration Friend Overrides Function DocumentationCommentTrivia(nodes As IEnumerable(Of SyntaxNode), trailingTrivia As SyntaxTriviaList, lastWhitespaceTrivia As SyntaxTrivia, endOfLineString As String) As SyntaxNode Dim node = SyntaxFactory.DocumentationCommentTrivia(SyntaxFactory.List(nodes)) - Return node.WithLeadingTrivia(SyntaxFactory.DocumentationCommentExteriorTrivia("''' ")). - WithTrailingTrivia(node.GetTrailingTrivia()). - WithTrailingTrivia(SyntaxFactory.EndOfLine(endOfLineString), lastWhitespaceTrivia) + node = node.WithLeadingTrivia(SyntaxFactory.DocumentationCommentExteriorTrivia("''' ")). + WithTrailingTrivia(node.GetTrailingTrivia()) + + If lastWhitespaceTrivia = Nothing Then + Return node.WithTrailingTrivia(SyntaxFactory.EndOfLine(endOfLineString)) + End If + + Return node.WithTrailingTrivia(SyntaxFactory.EndOfLine(endOfLineString), lastWhitespaceTrivia) End Function Friend Overrides Function DocumentationCommentTriviaWithUpdatedContent(trivia As SyntaxTrivia, content As IEnumerable(Of SyntaxNode)) As SyntaxNode From df2d61598101338cac8b42aa5068167730460787 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sun, 2 May 2021 12:33:21 +0000 Subject: [PATCH 08/20] More tests --- .../AddParameterTests.Cascading.cs | 22 ++++++++++++++++- .../ChangeSignature/ReorderParametersTests.cs | 24 +++++++++++++++++++ ...arpCompletionCommandHandlerTests_XmlDoc.vb | 19 +++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/src/EditorFeatures/CSharpTest/ChangeSignature/AddParameterTests.Cascading.cs b/src/EditorFeatures/CSharpTest/ChangeSignature/AddParameterTests.Cascading.cs index a996a41288d50..72efbf595644d 100644 --- a/src/EditorFeatures/CSharpTest/ChangeSignature/AddParameterTests.Cascading.cs +++ b/src/EditorFeatures/CSharpTest/ChangeSignature/AddParameterTests.Cascading.cs @@ -9,7 +9,6 @@ using Microsoft.CodeAnalysis.Editor.UnitTests.ChangeSignature; using Microsoft.CodeAnalysis.Test.Utilities; using Microsoft.CodeAnalysis.Test.Utilities.ChangeSignature; -using Roslyn.Test.Utilities; using Xunit; namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.ChangeSignature @@ -412,5 +411,26 @@ public override int M(string x, int newIntegerParameter, int y) }"; await TestChangeSignatureViaCommandAsync(LanguageNames.CSharp, markup, updatedSignature: permutation, expectedUpdatedInvocationDocumentCode: updatedCode); } + + [Fact(Skip = "https://github.com/dotnet/roslyn/issues/53091"), Trait(Traits.Feature, Traits.Features.ChangeSignature)] + public async Task AddParameter_Cascade_Record() + { + var markup = @" +record $$BaseR(int A, int B); + +record DerivedR() : BaseR(0, 1);"; + var permutation = new AddedParameterOrExistingIndex[] + { + new(1), + new(new AddedParameter(null, "int", "C", CallSiteKind.Value, "3"), "int"), + new(0) + }; + var updatedCode = @" +record BaseR(int B, int C, int A); + +record DerivedR() : BaseR(1, 3, 0);"; + + await TestChangeSignatureViaCommandAsync(LanguageNames.CSharp, markup, updatedSignature: permutation, expectedUpdatedInvocationDocumentCode: updatedCode); + } } } diff --git a/src/EditorFeatures/CSharpTest/ChangeSignature/ReorderParametersTests.cs b/src/EditorFeatures/CSharpTest/ChangeSignature/ReorderParametersTests.cs index 90bf56923b963..0fb298efc3e11 100644 --- a/src/EditorFeatures/CSharpTest/ChangeSignature/ReorderParametersTests.cs +++ b/src/EditorFeatures/CSharpTest/ChangeSignature/ReorderParametersTests.cs @@ -951,5 +951,29 @@ class D : C, I await TestChangeSignatureViaCommandAsync(LanguageNames.CSharp, markup, updatedSignature: permutation, expectedUpdatedInvocationDocumentCode: updatedCode); } + + [Fact, Trait(Traits.Feature, Traits.Features.ChangeSignature)] + public async Task ReorderParamTagsInDocComments_Record() + { + var markup = @" +/// +/// +/// +record $$R(int A, int B, int C) +{ + public static R Instance = new(0, 1, 2); +}"; + var permutation = new[] { 2, 1, 0 }; + var updatedCode = @" +/// +/// +/// +record R(int C, int B, int A) +{ + public static R Instance = new(2, 1, 0); +}"; + + await TestChangeSignatureViaCommandAsync(LanguageNames.CSharp, markup, updatedSignature: permutation, expectedUpdatedInvocationDocumentCode: updatedCode); + } } } diff --git a/src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests_XmlDoc.vb b/src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests_XmlDoc.vb index ee1cc0729587b..a4deb3938f818 100644 --- a/src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests_XmlDoc.vb +++ b/src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests_XmlDoc.vb @@ -765,6 +765,25 @@ class c End Using End Function + + Public Async Function CommitParam_Record(showCompletionInArgumentLists As Boolean) As Task + Using state = TestStateFactory.CreateCSharpTestState( + , showCompletionInArgumentLists:=showCompletionInArgumentLists) + + state.SendInvokeCompletionList() + Await state.AssertCompletionSession() + Await state.AssertSelectedCompletionItem(displayText:="param name=""I""") + state.SendReturn() + Await state.AssertNoCompletionSession() + + ' /// Public Async Function CommitParamNoOpenAngle(showCompletionInArgumentLists As Boolean) As Task From 6749276ef4a908c6b1f456ce61170df56f825d0d Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Sun, 2 May 2021 15:46:40 +0200 Subject: [PATCH 09/20] Fix test --- .../IntelliSense/CSharpCompletionCommandHandlerTests_XmlDoc.vb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests_XmlDoc.vb b/src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests_XmlDoc.vb index a4deb3938f818..c329a73fe8b35 100644 --- a/src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests_XmlDoc.vb +++ b/src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests_XmlDoc.vb @@ -780,7 +780,7 @@ record R(int I); Await state.AssertNoCompletionSession() ' /// Date: Tue, 4 May 2021 08:16:34 +0000 Subject: [PATCH 10/20] Address feedback --- .../CSharpChangeSignatureService.cs | 35 +++++++++++++++++-- .../XmlDocCommentCompletionProvider.cs | 23 ++++++++++++ .../AbstractChangeSignatureService.cs | 32 ++++++++--------- .../AbstractDocCommentCompletionProvider.cs | 8 +++-- .../VisualBasicChangeSignatureService.vb | 10 +++++- .../XmlDocCommentCompletionProvider.vb | 4 +++ .../Core/Extensions/ISymbolExtensions.cs | 24 ++----------- 7 files changed, 91 insertions(+), 45 deletions(-) diff --git a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs index 9a2832b8fd5f3..9a8b3dc605f72 100644 --- a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs +++ b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs @@ -757,13 +757,14 @@ private ImmutableArray UpdateParamTagsInLeadingTrivia(Document doc return GetPermutedDocCommentTrivia(document, node, permutedParamNodes); } - private static ImmutableArray VerifyAndPermuteParamNodes(IEnumerable paramNodes, ISymbol declarationSymbol, SignatureChange updatedSignature) + private ImmutableArray VerifyAndPermuteParamNodes(IEnumerable 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.Empty; @@ -887,5 +888,35 @@ protected override bool SupportsOptionalAndParamsArrayParametersSimultaneously() protected override SyntaxToken CommaTokenWithElasticSpace() => Token(SyntaxKind.CommaToken).WithTrailingTrivia(ElasticSpace); + + protected override bool TryGetRecordPrimaryConstructor(INamedTypeSymbol typeSymbol, 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 GetParameters(ISymbol declarationSymbol) + { + var declaredParameters = declarationSymbol.GetParameters(); + 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); + + if (primaryConstructor is not null) + { + declaredParameters = primaryConstructor.Parameters; + } + } + + return declaredParameters; + } } } diff --git a/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs b/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs index 57a8d38828ad3..3dbd89229c621 100644 --- a/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs +++ b/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs @@ -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 GetParameters(ISymbol declarationSymbol) + { + var declaredParameters = declarationSymbol.GetParameters(); + 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); + + if (primaryConstructor is not null) + { + declaredParameters = primaryConstructor.Parameters; + } + } + + return declaredParameters; + } + private static readonly CompletionItemRules s_defaultRules = CompletionItemRules.Create( filterCharacterRules: FilterRules, diff --git a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs index fc4cba1b01dcb..4ce75aaa9b127 100644 --- a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs +++ b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs @@ -126,15 +126,9 @@ internal async Task GetChangeSignatureContextAsy } else if (typeSymbol.IsRecord) { - // A bit hacky to determine the parameters of primary constructor associated with a given record. - // TODO: record structs. - const int RecordDeclarationRawKind = 9063; - var potentialPrimaryCtor = typeSymbol.InstanceConstructors.FirstOrDefault( - c => c.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax()?.RawKind == RecordDeclarationRawKind); - - if (potentialPrimaryCtor is not null) + if (TryGetRecordPrimaryConstructor(typeSymbol, out var primaryConstructor)) { - symbol = potentialPrimaryCtor; + symbol = primaryConstructor; } } } @@ -177,13 +171,15 @@ internal async Task GetChangeSignatureContextAsy } var parameterConfiguration = ParameterConfiguration.Create( - symbol.GetParameters().Select(p => new ExistingParameter(p)).ToImmutableArray(), + GetParameters(symbol).Select(p => new ExistingParameter(p)).ToImmutableArray(), symbol.IsExtensionMethod(), selectedIndex); return new ChangeSignatureAnalysisSucceededContext( declarationDocument, positionForTypeBinding, symbol, parameterConfiguration); } + protected abstract bool TryGetRecordPrimaryConstructor(INamedTypeSymbol typeSymbol, out IMethodSymbol? primaryConstructor); + internal async Task ChangeSignatureWithContextAsync(ChangeSignatureAnalyzedContext context, ChangeSignatureOptionsResult? options, CancellationToken cancellationToken) { return context switch @@ -258,7 +254,7 @@ private static async Task> FindChangeSignatureR var symbols = await FindChangeSignatureReferencesAsync( declaredSymbol, context.Solution, cancellationToken).ConfigureAwait(false); - var declaredSymbolParametersCount = declaredSymbol.GetParameters().Length; + var declaredSymbolParametersCount = GetParameters(declaredSymbol).Length; var telemetryNumberOfDeclarationsToUpdate = 0; var telemetryNumberOfReferencesToUpdate = 0; @@ -454,14 +450,14 @@ private static bool TryGetNodeWithEditableSignatureOrAttributes(Location locatio return nodeToUpdate != null; } - protected static ImmutableArray PermuteArguments( + protected ImmutableArray PermuteArguments( ISymbol declarationSymbol, ImmutableArray arguments, SignatureChange updatedSignature, bool isReducedExtensionMethod = false) { // 1. Determine which parameters are permutable - var declarationParameters = declarationSymbol.GetParameters(); + var declarationParameters = GetParameters(declarationSymbol); var declarationParametersToPermute = GetParametersToPermute(arguments, declarationParameters, isReducedExtensionMethod); var argumentsToPermute = arguments.Take(declarationParametersToPermute.Length).ToList(); @@ -560,19 +556,21 @@ protected static ImmutableArray PermuteArguments( return newArguments.ToImmutableAndFree(); } + protected abstract ImmutableArray GetParameters(ISymbol declarationSymbol); + /// /// Sometimes signature changes can cascade from a declaration with m parameters to one with n > m parameters, such as /// delegate Invoke methods (m) and delegate BeginInvoke methods (n = m + 2). This method adds on those extra parameters /// to the base . /// - private static SignatureChange UpdateSignatureChangeToIncludeExtraParametersFromTheDeclarationSymbol(ISymbol declarationSymbol, SignatureChange updatedSignature) + private SignatureChange UpdateSignatureChangeToIncludeExtraParametersFromTheDeclarationSymbol(ISymbol declarationSymbol, SignatureChange updatedSignature) { - if (declarationSymbol.GetParameters().Length > updatedSignature.OriginalConfiguration.ToListOfParameters().Length) + var realParameters = GetParameters(declarationSymbol); + if (realParameters.Length > updatedSignature.OriginalConfiguration.ToListOfParameters().Length) { var originalConfigurationParameters = updatedSignature.OriginalConfiguration.ToListOfParameters(); var updatedConfigurationParameters = updatedSignature.UpdatedConfiguration.ToListOfParameters(); - var realParameters = declarationSymbol.GetParameters(); var bonusParameters = realParameters.Skip(originalConfigurationParameters.Length); var originalConfigurationParametersWithExtraParameters = originalConfigurationParameters.AddRange(bonusParameters.Select(p => new ExistingParameter(p))); @@ -772,13 +770,14 @@ protected virtual async Task> AddNewArgumentsToL if (updatedParameters[i] != signaturePermutation.UpdatedConfiguration.ThisParameter || !isReducedExtensionMethod) { + var parameters = GetParameters(declarationSymbol); if (updatedParameters[i] is AddedParameter addedParameter) { // Omitting an argument only works in some languages, depending on whether // there is a params array. We sometimes need to reinterpret an requested // omitted parameter as one with a TODO requested. var forcedCallsiteErrorDueToParamsArray = addedParameter.CallSiteKind == CallSiteKind.Omitted && - declarationSymbol.GetParameters().LastOrDefault()?.IsParams == true && + parameters.LastOrDefault()?.IsParams == true && !SupportsOptionalAndParamsArrayParametersSimultaneously(); var isCallsiteActuallyOmitted = addedParameter.CallSiteKind == CallSiteKind.Omitted && !forcedCallsiteErrorDueToParamsArray; @@ -821,7 +820,6 @@ protected virtual async Task> AddNewArgumentsToL } else { - var parameters = declarationSymbol.GetParameters(); if (indexInListOfPreexistingArguments == parameters.Length - 1 && parameters[indexInListOfPreexistingArguments].IsParams) { diff --git a/src/Features/Core/Portable/Completion/Providers/AbstractDocCommentCompletionProvider.cs b/src/Features/Core/Portable/Completion/Providers/AbstractDocCommentCompletionProvider.cs index a33a0baacf034..b2a4945d49de5 100644 --- a/src/Features/Core/Portable/Completion/Providers/AbstractDocCommentCompletionProvider.cs +++ b/src/Features/Core/Portable/Completion/Providers/AbstractDocCommentCompletionProvider.cs @@ -152,7 +152,7 @@ protected IEnumerable GetNestedItems(ISymbol symbol, bool includ private IEnumerable GetParamRefItems(ISymbol symbol) { - var names = symbol.GetParameters().Select(p => p.Name); + var names = GetParameters(symbol).Select(p => p.Name); return names.Select(p => CreateCompletionItem( displayText: FormatParameter(ParameterReferenceElementName, p), @@ -176,7 +176,7 @@ protected IEnumerable GetAttributeValueItems(ISymbol symbol, str { if (tagName == ParameterElementName || tagName == ParameterReferenceElementName) { - return symbol.GetParameters() + return GetParameters(symbol) .Select(parameter => CreateCompletionItem(parameter.Name)); } else if (tagName == TypeParameterElementName) @@ -216,7 +216,7 @@ protected ImmutableArray GetTopLevelItems(ISymbol symbol, TSynta if (symbol != null) { - items.AddRange(GetParameterItems(symbol.GetParameters(), syntax, ParameterElementName)); + items.AddRange(GetParameterItems(GetParameters(symbol), syntax, ParameterElementName)); items.AddRange(GetParameterItems(symbol.GetTypeParameters(), syntax, TypeParameterElementName)); if (symbol is IPropertySymbol && !existingTopLevelTags.Contains(ValueElementName)) @@ -243,6 +243,8 @@ protected ImmutableArray GetTopLevelItems(ISymbol symbol, TSynta return items.ToImmutable(); } + protected abstract ImmutableArray GetParameters(ISymbol symbol); + protected IEnumerable GetItemTagItems() => new[] { TermElementName, DescriptionElementName }.Select(GetItem); diff --git a/src/Features/VisualBasic/Portable/ChangeSignature/VisualBasicChangeSignatureService.vb b/src/Features/VisualBasic/Portable/ChangeSignature/VisualBasicChangeSignatureService.vb index da9d187d8a6a3..142808020a5cf 100644 --- a/src/Features/VisualBasic/Portable/ChangeSignature/VisualBasicChangeSignatureService.vb +++ b/src/Features/VisualBasic/Portable/ChangeSignature/VisualBasicChangeSignatureService.vb @@ -610,7 +610,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ChangeSignature Dim originalParameters = updatedSignature.OriginalConfiguration.ToListOfParameters() Dim reorderedParameters = updatedSignature.UpdatedConfiguration.ToListOfParameters() - Dim declaredParameters = declarationSymbol.GetParameters() + Dim declaredParameters = GetParameters(declarationSymbol) If paramNodes.Length <> declaredParameters.Length Then Return ImmutableArray(Of SyntaxNode).Empty End If @@ -755,5 +755,13 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ChangeSignature Protected Overrides Function CommaTokenWithElasticSpace() As SyntaxToken Return Token(SyntaxKind.CommaToken).WithTrailingTrivia(ElasticSpace) End Function + + Protected Overrides Function TryGetRecordPrimaryConstructor(typeSymbol As INamedTypeSymbol, ByRef primaryConstructor As IMethodSymbol) As Boolean + Return False + End Function + + Protected Overrides Function GetParameters(declarationSymbol As ISymbol) As ImmutableArray(Of IParameterSymbol) + Return declarationSymbol.GetParameters() + End Function End Class End Namespace diff --git a/src/Features/VisualBasic/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.vb b/src/Features/VisualBasic/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.vb index 29c5befe0a147..192e23a5d194a 100644 --- a/src/Features/VisualBasic/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.vb +++ b/src/Features/VisualBasic/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.vb @@ -338,6 +338,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Completion.Providers Return nameSyntax?.LocalName.ValueText End Function + Protected Overrides Function GetParameters(symbol As ISymbol) As ImmutableArray(Of IParameterSymbol) + Return symbol.GetParameters() + End Function + Private Shared ReadOnly s_defaultRules As CompletionItemRules = CompletionItemRules.Create( filterCharacterRules:=FilterRules, diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs index f75000cd44c75..95ab3e5184bc7 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs @@ -364,33 +364,13 @@ public static bool IsParams([NotNullWhen(returnValue: true)] this ISymbol? symbo } public static ImmutableArray GetParameters(this ISymbol? symbol) - { - return symbol switch + => symbol switch { IMethodSymbol m => m.Parameters, - IPropertySymbol p => p.Parameters, - INamedTypeSymbol nt when nt.IsRecord => GetRecordParameters(nt), + IPropertySymbol nt => nt.Parameters, _ => ImmutableArray.Empty, }; - ImmutableArray 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.Empty; - } - - return primaryConstructor.Parameters; - } - } - public static ImmutableArray GetTypeParameters(this ISymbol? symbol) => symbol switch { From e494608cb6c643dcca1c378313b05ab1eb84a307 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Tue, 4 May 2021 08:26:16 +0000 Subject: [PATCH 11/20] Fixes --- .../Portable/ChangeSignature/AbstractChangeSignatureService.cs | 3 ++- .../ChangeSignature/VisualBasicChangeSignatureService.vb | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs index 4ce75aaa9b127..9d44761d6fb75 100644 --- a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs +++ b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -178,7 +179,7 @@ internal async Task GetChangeSignatureContextAsy declarationDocument, positionForTypeBinding, symbol, parameterConfiguration); } - protected abstract bool TryGetRecordPrimaryConstructor(INamedTypeSymbol typeSymbol, out IMethodSymbol? primaryConstructor); + protected abstract bool TryGetRecordPrimaryConstructor(INamedTypeSymbol typeSymbol, [NotNullWhen(true)] out IMethodSymbol? primaryConstructor); internal async Task ChangeSignatureWithContextAsync(ChangeSignatureAnalyzedContext context, ChangeSignatureOptionsResult? options, CancellationToken cancellationToken) { diff --git a/src/Features/VisualBasic/Portable/ChangeSignature/VisualBasicChangeSignatureService.vb b/src/Features/VisualBasic/Portable/ChangeSignature/VisualBasicChangeSignatureService.vb index 142808020a5cf..ce65e2060dbc7 100644 --- a/src/Features/VisualBasic/Portable/ChangeSignature/VisualBasicChangeSignatureService.vb +++ b/src/Features/VisualBasic/Portable/ChangeSignature/VisualBasicChangeSignatureService.vb @@ -604,7 +604,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ChangeSignature Return GetPermutedDocCommentTrivia(document, node, permutedParamNodes) End Function - Private Shared Function VerifyAndPermuteParamNodes(paramNodes As ImmutableArray(Of XmlElementSyntax), declarationSymbol As ISymbol, updatedSignature As SignatureChange) As ImmutableArray(Of SyntaxNode) + Private Function VerifyAndPermuteParamNodes(paramNodes As ImmutableArray(Of XmlElementSyntax), declarationSymbol As ISymbol, updatedSignature As SignatureChange) As ImmutableArray(Of SyntaxNode) ' Only reorder if count and order match originally. Dim originalParameters = updatedSignature.OriginalConfiguration.ToListOfParameters() From e8ac91d0aea7be8f5d2f0bde6e08a62b45db399d Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Tue, 4 May 2021 14:50:37 +0200 Subject: [PATCH 12/20] Add NotNullWhenAttribute --- .../Portable/ChangeSignature/CSharpChangeSignatureService.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs index 9a8b3dc605f72..781beb8bdff02 100644 --- a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs +++ b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs @@ -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; @@ -889,7 +890,7 @@ protected override bool SupportsOptionalAndParamsArrayParametersSimultaneously() protected override SyntaxToken CommaTokenWithElasticSpace() => Token(SyntaxKind.CommaToken).WithTrailingTrivia(ElasticSpace); - protected override bool TryGetRecordPrimaryConstructor(INamedTypeSymbol typeSymbol, out IMethodSymbol? primaryConstructor) + protected override bool TryGetRecordPrimaryConstructor(INamedTypeSymbol typeSymbol, [NotNullWhen(true)] out IMethodSymbol? primaryConstructor) { Debug.Assert(typeSymbol.IsRecord); primaryConstructor = typeSymbol.InstanceConstructors.FirstOrDefault( From a7c5f11ec299895e7171e1a7440d645a969685d2 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Sat, 22 May 2021 08:09:11 +0200 Subject: [PATCH 13/20] Update CSharpChangeSignatureService to address feedback --- .../ChangeSignature/CSharpChangeSignatureService.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs index 781beb8bdff02..c130e738f2794 100644 --- a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs +++ b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs @@ -270,6 +270,7 @@ public override async Task ChangeSignatureAsync( CancellationToken cancellationToken) { var updatedNode = potentiallyUpdatedNode as CSharpSyntaxNode; + // Update tags. // TODO: Record structs if (updatedNode.IsKind(SyntaxKind.MethodDeclaration) || @@ -906,8 +907,9 @@ protected override ImmutableArray GetParameters(ISymbol declar 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. + // 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); From d36653a9b13d65773a03964e45016ae0f1f29693 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Sat, 22 May 2021 08:11:09 +0200 Subject: [PATCH 14/20] Update XmlDocCommentCompletionProvider.cs --- .../XmlDocCommentCompletionProvider.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs b/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs index 3dbd89229c621..64cceb4aa92e4 100644 --- a/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs +++ b/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs @@ -368,14 +368,18 @@ private string GetAttributeValue(XmlAttributeSyntax attribute) protected override ImmutableArray GetParameters(ISymbol declarationSymbol) { + var declaredParameters = declarationSymbol.GetParameters(); + if (declarationSymbol is INamedTypeSymbol { IsRecord: true } recordSymbol) + { var declaredParameters = declarationSymbol.GetParameters(); 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. + // 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); From de681db214ed51f6d4e8c435f7b43bed650c033a Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Sat, 22 May 2021 08:15:59 +0200 Subject: [PATCH 15/20] Move abstract methods to top --- .../Providers/AbstractDocCommentCompletionProvider.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Features/Core/Portable/Completion/Providers/AbstractDocCommentCompletionProvider.cs b/src/Features/Core/Portable/Completion/Providers/AbstractDocCommentCompletionProvider.cs index b2a4945d49de5..d7b0c997fe471 100644 --- a/src/Features/Core/Portable/Completion/Providers/AbstractDocCommentCompletionProvider.cs +++ b/src/Features/Core/Portable/Completion/Providers/AbstractDocCommentCompletionProvider.cs @@ -97,6 +97,10 @@ public override async Task ProvideCompletionsAsync(CompletionContext context) protected abstract IEnumerable GetExistingTopLevelAttributeValues(TSyntax syntax, string tagName, string attributeName); + protected abstract IEnumerable GetKeywordNames(); + + protected abstract ImmutableArray GetParameters(ISymbol symbol); + private CompletionItem GetItem(string name) { if (s_tagMap.TryGetValue(name, out var values)) @@ -202,8 +206,6 @@ protected IEnumerable GetAttributeValueItems(ISymbol symbol, str return SpecializedCollections.EmptyEnumerable(); } - protected abstract IEnumerable GetKeywordNames(); - protected ImmutableArray GetTopLevelItems(ISymbol symbol, TSyntax syntax) { using var _1 = ArrayBuilder.GetInstance(out var items); @@ -243,8 +245,6 @@ protected ImmutableArray GetTopLevelItems(ISymbol symbol, TSynta return items.ToImmutable(); } - protected abstract ImmutableArray GetParameters(ISymbol symbol); - protected IEnumerable GetItemTagItems() => new[] { TermElementName, DescriptionElementName }.Select(GetItem); From 0c8bfef45f63fbe17dc3fa00181f3e27e216b27a Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Sat, 22 May 2021 08:16:28 +0200 Subject: [PATCH 16/20] move abstract methods to top --- .../ChangeSignature/AbstractChangeSignatureService.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs index 9d44761d6fb75..4921724ac04b4 100644 --- a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs +++ b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs @@ -75,6 +75,13 @@ public abstract Task ChangeSignatureAsync( /// protected abstract bool SupportsOptionalAndParamsArrayParametersSimultaneously(); + protected abstract bool TryGetRecordPrimaryConstructor(INamedTypeSymbol typeSymbol, [NotNullWhen(true)] out IMethodSymbol? primaryConstructor); + + /// + /// A temporarily hack that should be removed once/if https://github.com/dotnet/roslyn/issues/53092 is fixed. + /// + protected abstract ImmutableArray GetParameters(ISymbol declarationSymbol); + protected abstract SyntaxGenerator Generator { get; } protected abstract ISyntaxFacts SyntaxFacts { get; } @@ -179,8 +186,6 @@ internal async Task GetChangeSignatureContextAsy declarationDocument, positionForTypeBinding, symbol, parameterConfiguration); } - protected abstract bool TryGetRecordPrimaryConstructor(INamedTypeSymbol typeSymbol, [NotNullWhen(true)] out IMethodSymbol? primaryConstructor); - internal async Task ChangeSignatureWithContextAsync(ChangeSignatureAnalyzedContext context, ChangeSignatureOptionsResult? options, CancellationToken cancellationToken) { return context switch @@ -557,8 +562,6 @@ protected ImmutableArray PermuteArguments( return newArguments.ToImmutableAndFree(); } - protected abstract ImmutableArray GetParameters(ISymbol declarationSymbol); - /// /// Sometimes signature changes can cascade from a declaration with m parameters to one with n > m parameters, such as /// delegate Invoke methods (m) and delegate BeginInvoke methods (n = m + 2). This method adds on those extra parameters From 42bf97a3ad9785a9d31f6059e9fc5ab7623127f4 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Sat, 22 May 2021 08:17:01 +0200 Subject: [PATCH 17/20] Add doc comment --- .../Providers/AbstractDocCommentCompletionProvider.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Features/Core/Portable/Completion/Providers/AbstractDocCommentCompletionProvider.cs b/src/Features/Core/Portable/Completion/Providers/AbstractDocCommentCompletionProvider.cs index d7b0c997fe471..397a98ee862e2 100644 --- a/src/Features/Core/Portable/Completion/Providers/AbstractDocCommentCompletionProvider.cs +++ b/src/Features/Core/Portable/Completion/Providers/AbstractDocCommentCompletionProvider.cs @@ -99,6 +99,9 @@ public override async Task ProvideCompletionsAsync(CompletionContext context) protected abstract IEnumerable GetKeywordNames(); + /// + /// A temporarily hack that should be removed once/if https://github.com/dotnet/roslyn/issues/53092 is fixed. + /// protected abstract ImmutableArray GetParameters(ISymbol symbol); private CompletionItem GetItem(string name) From b7ca33c8eb5b3a7c72f4d424ee393dd53259a1e0 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Sat, 29 May 2021 00:20:08 +0200 Subject: [PATCH 18/20] Fix --- .../CompletionProviders/XmlDocCommentCompletionProvider.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs b/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs index 64cceb4aa92e4..3b70beb961a30 100644 --- a/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs +++ b/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs @@ -368,9 +368,6 @@ private string GetAttributeValue(XmlAttributeSyntax attribute) protected override ImmutableArray GetParameters(ISymbol declarationSymbol) { - var declaredParameters = declarationSymbol.GetParameters(); - if (declarationSymbol is INamedTypeSymbol { IsRecord: true } recordSymbol) - { var declaredParameters = declarationSymbol.GetParameters(); if (declarationSymbol is INamedTypeSymbol { IsRecord: true } recordSymbol) { From 149d57819711415a126c684b3ea8ddbf065eabf3 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sun, 11 Jul 2021 12:19:39 +0200 Subject: [PATCH 19/20] Address feedback --- .../CSharpChangeSignatureService.cs | 35 +++++-------------- .../XmlDocCommentCompletionProvider.cs | 17 ++------- .../AbstractChangeSignatureService.cs | 7 ++-- .../Extensions/ITypeSymbolExtensions.cs | 18 ++++++++++ 4 files changed, 32 insertions(+), 45 deletions(-) diff --git a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs index c130e738f2794..f42b4cd94e0e5 100644 --- a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs +++ b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs @@ -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 _declarationAndInvocableKinds = @@ -89,7 +89,7 @@ internal sealed class CSharpChangeSignatureService : AbstractChangeSignatureServ SyntaxKind.AnonymousMethodExpression, SyntaxKind.ParenthesizedLambdaExpression, SyntaxKind.SimpleLambdaExpression, - // TODO: record structs + SyntaxKind.RecordStructDeclaration, SyntaxKind.RecordDeclaration); [ImportingConstructor] @@ -272,11 +272,11 @@ public override async Task ChangeSignatureAsync( var updatedNode = potentiallyUpdatedNode as CSharpSyntaxNode; // Update 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); @@ -293,8 +293,7 @@ public override async Task 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)); @@ -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 GetParameters(ISymbol declarationSymbol) { var declaredParameters = declarationSymbol.GetParameters(); - 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; diff --git a/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs b/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs index 3b70beb961a30..cc13e18c4e9dc 100644 --- a/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs +++ b/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs @@ -369,21 +369,10 @@ private string GetAttributeValue(XmlAttributeSyntax attribute) protected override ImmutableArray GetParameters(ISymbol declarationSymbol) { var declaredParameters = declarationSymbol.GetParameters(); - if (declarationSymbol is INamedTypeSymbol { IsRecord: true } recordSymbol) + if (declarationSymbol is INamedTypeSymbol { IsRecord: true } recordSymbol && + recordSymbol.TryGetRecordPrimaryConstructor(out var primaryConstructor)) { - 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; diff --git a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs index 4921724ac04b4..1765429d52691 100644 --- a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs +++ b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs @@ -132,12 +132,9 @@ internal async Task 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; } } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs index 9b0cadc0c9d4c..85188e6769291 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs @@ -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; @@ -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; + } } } From 327b8e2167616f920d3b9c33b669cd8e891d2fd4 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sun, 11 Jul 2021 22:30:12 +0200 Subject: [PATCH 20/20] Address feedback --- .../CSharpChangeSignatureService.cs | 4 +-- .../XmlDocCommentCompletionProvider.cs | 4 +-- .../AbstractChangeSignatureService.cs | 2 +- .../Extensions/ITypeSymbolExtensions.cs | 29 +++++++++++-------- 4 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs index f42b4cd94e0e5..e350faa680817 100644 --- a/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs +++ b/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs @@ -896,8 +896,8 @@ protected override bool TryGetRecordPrimaryConstructor(INamedTypeSymbol typeSymb protected override ImmutableArray GetParameters(ISymbol declarationSymbol) { var declaredParameters = declarationSymbol.GetParameters(); - if (declarationSymbol is INamedTypeSymbol { IsRecord: true } recordSymbol && - recordSymbol.TryGetRecordPrimaryConstructor(out var primaryConstructor)) + if (declarationSymbol is INamedTypeSymbol namedTypeSymbol && + namedTypeSymbol.TryGetRecordPrimaryConstructor(out var primaryConstructor)) { declaredParameters = primaryConstructor.Parameters; } diff --git a/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs b/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs index cc13e18c4e9dc..f096d239f8fff 100644 --- a/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs +++ b/src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs @@ -369,8 +369,8 @@ private string GetAttributeValue(XmlAttributeSyntax attribute) protected override ImmutableArray GetParameters(ISymbol declarationSymbol) { var declaredParameters = declarationSymbol.GetParameters(); - if (declarationSymbol is INamedTypeSymbol { IsRecord: true } recordSymbol && - recordSymbol.TryGetRecordPrimaryConstructor(out var primaryConstructor)) + if (declarationSymbol is INamedTypeSymbol namedTypeSymbol && + namedTypeSymbol.TryGetRecordPrimaryConstructor(out var primaryConstructor)) { declaredParameters = primaryConstructor.Parameters; } diff --git a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs index 1765429d52691..dabab4bd296c8 100644 --- a/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs +++ b/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs @@ -132,7 +132,7 @@ internal async Task GetChangeSignatureContextAsy { symbol = typeSymbol.DelegateInvokeMethod; } - else if (typeSymbol.IsRecord && TryGetRecordPrimaryConstructor(typeSymbol, out var primaryConstructor)) + else if (TryGetRecordPrimaryConstructor(typeSymbol, out var primaryConstructor)) { symbol = primaryConstructor; } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs index 85188e6769291..73c1a9ff9649d 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs @@ -182,18 +182,23 @@ public static bool IsIntrinsicType(this ITypeSymbol typeSymbol) 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; + if (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; + } + + primaryConstructor = null; + return false; } } }