Add records support to XmlDocCommentCompletion and ChangeSignature #53052

Youssef1313 · 2021-04-30T12:48:30Z

The change signature part is crashing. The assertion failure is far down to the compiler layer. I wasn't able to figure out how to fix that. Tagging @allisonchou for help here since the issue was assigned to you. Figured out the reason. It's actually an existing bug (see #53089).

Fixes #44558
Fixes #52738

TODOs

src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs

Youssef1313 · 2021-04-30T15:08:45Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs

+                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;


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.

Youssef1313 · 2021-04-30T15:17:15Z

CI is stuck:

Closing and reopening to retrigger run.

Youssef1313 · 2021-04-30T16:30:58Z

Closing and reopening:

C:\Users\VssAdministrator\.nuget\packages\microsoft.dotnet.helix.sdk\6.0.0-beta.21167.3\tools\azure-pipelines\AzurePipelines.MultiQueue.targets(30,5): error : System.Net.Http.HttpRequestException: Response status code does not indicate success: 500 (Internal Server Error). [D:\a\1\s\helix-tmp.csproj]
C:\Users\VssAdministrator\.nuget\packages\microsoft.dotnet.helix.sdk\6.0.0-beta.21167.3\tools\azure-pipelines\AzurePipelines.MultiQueue.targets(30,5): error :    at System.Net.Http.HttpResponseMessage.EnsureSuccessStatusCode() [D:\a\1\s\helix-tmp.csproj]
C:\Users\VssAdministrator\.nuget\packages\microsoft.dotnet.helix.sdk\6.0.0-beta.21167.3\tools\azure-pipelines\AzurePipelines.MultiQueue.targets(30,5): error :    at Microsoft.DotNet.Helix.Sdk.CreateTestsForWorkItems.<>c__DisplayClass5_0.<<AttachResultFileToTestResultAsync>b__0>d.MoveNext() in /_/src/Microsoft.DotNet.Helix/Sdk/CreateFailedTestsForFailedWorkItems.cs:line 73 [D:\a\1\s\helix-tmp.csproj]
C:\Users\VssAdministrator\.nuget\packages\microsoft.dotnet.helix.sdk\6.0.0-beta.21167.3\tools\azure-pipelines\AzurePipelines.MultiQueue.targets(30,5): error : --- End of stack trace from previous location --- [D:\a\1\s\helix-tmp.csproj]
C:\Users\VssAdministrator\.nuget\packages\microsoft.dotnet.helix.sdk\6.0.0-beta.21167.3\tools\azure-pipelines\AzurePipelines.MultiQueue.targets(30,5): error :    at Microsoft.DotNet.Helix.AzureDevOps.AzureDevOpsTask.<>c__DisplayClass13_0.<<RetryAsync>b__0>d.MoveNext() in /_/src/Microsoft.DotNet.Helix/Sdk/AzureDevOpsTask.cs:line 91 [D:\a\1\s\helix-tmp.csproj]
C:\Users\VssAdministrator\.nuget\packages\microsoft.dotnet.helix.sdk\6.0.0-beta.21167.3\tools\azure-pipelines\AzurePipelines.MultiQueue.targets(30,5): error : --- End of stack trace from previous location --- [D:\a\1\s\helix-tmp.csproj]
C:\Users\VssAdministrator\.nuget\packages\microsoft.dotnet.helix.sdk\6.0.0-beta.21167.3\tools\azure-pipelines\AzurePipelines.MultiQueue.targets(30,5): error :    at Microsoft.DotNet.Helix.AzureDevOps.AzureDevOpsTask.RetryAsync[T](Func`1 function, Action`1 logRetry, Func`2 isRetryable, CancellationToken cancellationToken) in /_/src/Microsoft.DotNet.Helix/Sdk/AzureDevOpsTask.cs:line 206 [D:\a\1\s\helix-tmp.csproj]
C:\Users\VssAdministrator\.nuget\packages\microsoft.dotnet.helix.sdk\6.0.0-beta.21167.3\tools\azure-pipelines\AzurePipelines.MultiQueue.targets(30,5): error :    at Microsoft.DotNet.Helix.AzureDevOps.AzureDevOpsTask.RetryAsync(Func`1 function) in /_/src/Microsoft.DotNet.Helix/Sdk/AzureDevOpsTask.cs:line 86 [D:\a\1\s\helix-tmp.csproj]

Youssef1313 · 2021-04-30T19:08:37Z

C:\Users\VssAdministrator\.nuget\packages\microsoft.dotnet.helix.sdk\6.0.0-beta.21167.3\tools\azure-pipelines\AzurePipelines.MultiQueue.targets(30,5): error : System.Net.Http.HttpRequestException: Response status code does not indicate success: 500 (Internal Server Error). [D:\a\1\s\helix-tmp.csproj]

Is this a known issue? Re-run CI for the third time with the same error.

Youssef1313 · 2021-05-21T16:09:16Z

Pinging @CyrusNajmabadi and @allisonchou for review.

allisonchou

Looking really good! Just have a few small comments/questions.

src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs

src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs

src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs

allisonchou

Thanks! This LGTM. 😄

src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs

allisonchou · 2021-05-28T22:12:38Z

@Youssef1313 It looks like there's some check failures, could you take a look?

Youssef1313 · 2021-07-11T05:28:06Z

@CyrusNajmabadi Can you review please? Thanks!

CyrusNajmabadi · 2021-07-11T08:14:59Z

src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs

+                // 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);


consider extracting helper for this (you do the same work higher up).

CyrusNajmabadi · 2021-07-11T08:15:21Z

src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs

+                // 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);


used again. consider an extension now.

CyrusNajmabadi · 2021-07-11T08:15:52Z

src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs

+                else if (typeSymbol.IsRecord)
+                {
+                    if (TryGetRecordPrimaryConstructor(typeSymbol, out var primaryConstructor))
+                    {
+                        symbol = primaryConstructor;
+                    }
+                }


Suggested change

else if (typeSymbol.IsRecord)

{

if (TryGetRecordPrimaryConstructor(typeSymbol, out var primaryConstructor))

{

symbol = primaryConstructor;

}

}

else if (typeSymbol.IsRecord && TryGetRecordPrimaryConstructor(typeSymbol, out var primaryConstructor))

{

symbol = primaryConstructor;

}

CyrusNajmabadi

lgtm. let me know if you want to add the support for record structs.

…Roslyn into records-xml-doc-ide

CyrusNajmabadi · 2021-07-11T18:28:02Z

src/Features/CSharp/Portable/Completion/CompletionProviders/XmlDocCommentCompletionProvider.cs

@@ -369,21 +369,10 @@ private string GetAttributeValue(XmlAttributeSyntax attribute)
        protected override ImmutableArray<IParameterSymbol> 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))


You could move the IsRecord check into TryGetPrimaryConstructor

CyrusNajmabadi · 2021-07-12T01:17:38Z

Thanks!

Youssef1313 added 3 commits April 30, 2021 11:44

Fix Xml docs completion for records

8305017

Add test for record in AddDocCommentNodesCodeFixProviderTests

6d6f087

Add records support to ChangeSignature (crashing)

41eae46

Youssef1313 requested a review from a team as a code owner April 30, 2021 12:48

dotnet-issue-labeler bot added the Area-IDE label Apr 30, 2021

Youssef1313 commented Apr 30, 2021

View reviewed changes

src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs Outdated Show resolved Hide resolved

Youssef1313 added 2 commits April 30, 2021 15:02

Fix nullable warning

ac6de34

Fix nullable warning

6fbf876

Youssef1313 commented Apr 30, 2021

View reviewed changes

Youssef1313 closed this Apr 30, 2021

Youssef1313 reopened this Apr 30, 2021

Youssef1313 closed this Apr 30, 2021

Youssef1313 reopened this Apr 30, 2021

Youssef1313 closed this Apr 30, 2021

Youssef1313 reopened this Apr 30, 2021

jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 30, 2021

jinujoseph assigned allisonchou Apr 30, 2021

Youssef1313 closed this May 1, 2021

Youssef1313 reopened this May 1, 2021

Fix assertion failure and fix test

76c88dc

Youssef1313 mentioned this pull request May 2, 2021

Add test: AddParameters_NoLastWhitespaceTrivia #53089

Closed

Youssef1313 added 2 commits May 2, 2021 11:44

Don't crash VB when there is no whitespace leading trivia

4c0bcef

More tests

df2d615

Youssef1313 mentioned this pull request May 2, 2021

Change signature should cascade record's primary constructor parameter changes #53091

Open

allisonchou reviewed May 21, 2021

View reviewed changes

Youssef1313 added 5 commits May 22, 2021 08:09

Update CSharpChangeSignatureService to address feedback

a7c5f11

Update XmlDocCommentCompletionProvider.cs

d36653a

Move abstract methods to top

de681db

move abstract methods to top

0c8bfef

Add doc comment

42bf97a

allisonchou approved these changes May 22, 2021

View reviewed changes

src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs Show resolved Hide resolved

Fix

b7ca33c

CyrusNajmabadi reviewed Jul 11, 2021

View reviewed changes

CyrusNajmabadi approved these changes Jul 11, 2021

View reviewed changes

Youssef1313 added 3 commits July 11, 2021 11:27

Merge remote-tracking branch 'upstream/main' into records-xml-doc-ide

761c811

Merge branch 'records-xml-doc-ide' of https://github.com/Youssef1313/…

43eb33e

…Roslyn into records-xml-doc-ide

Address feedback

149d578

CyrusNajmabadi reviewed Jul 11, 2021

View reviewed changes

Address feedback

327b8e2

CyrusNajmabadi approved these changes Jul 12, 2021

View reviewed changes

CyrusNajmabadi merged commit 0049504 into dotnet:main Jul 12, 2021

ghost added this to the Next milestone Jul 12, 2021

Youssef1313 deleted the records-xml-doc-ide branch July 12, 2021 02:27

allisonchou modified the milestones: Next, 17.0.P3 Jul 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add records support to XmlDocCommentCompletion and ChangeSignature #53052

Add records support to XmlDocCommentCompletion and ChangeSignature #53052

Youssef1313 commented Apr 30, 2021 •

edited

Loading

Youssef1313 Apr 30, 2021 •

edited

Loading

Youssef1313 commented Apr 30, 2021

Youssef1313 commented Apr 30, 2021

Youssef1313 commented Apr 30, 2021

Youssef1313 commented May 21, 2021

allisonchou left a comment

allisonchou left a comment

allisonchou commented May 28, 2021

Youssef1313 commented Jul 11, 2021

CyrusNajmabadi Jul 11, 2021

CyrusNajmabadi Jul 11, 2021

CyrusNajmabadi Jul 11, 2021

CyrusNajmabadi left a comment

CyrusNajmabadi Jul 11, 2021

CyrusNajmabadi commented Jul 12, 2021

Add records support to XmlDocCommentCompletion and ChangeSignature #53052

Add records support to XmlDocCommentCompletion and ChangeSignature #53052

Conversation

Youssef1313 commented Apr 30, 2021 • edited Loading

TODOs

Youssef1313 Apr 30, 2021 • edited Loading

Choose a reason for hiding this comment

Youssef1313 commented Apr 30, 2021

Youssef1313 commented Apr 30, 2021

Youssef1313 commented Apr 30, 2021

Youssef1313 commented May 21, 2021

allisonchou left a comment

Choose a reason for hiding this comment

allisonchou left a comment

Choose a reason for hiding this comment

allisonchou commented May 28, 2021

Youssef1313 commented Jul 11, 2021

CyrusNajmabadi Jul 11, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Jul 11, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Jul 11, 2021

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

CyrusNajmabadi Jul 11, 2021

Choose a reason for hiding this comment

CyrusNajmabadi commented Jul 12, 2021

Youssef1313 commented Apr 30, 2021 •

edited

Loading

Youssef1313 Apr 30, 2021 •

edited

Loading