Add records support to XmlDocCommentCompletion and ChangeSignature by Youssef1313 · Pull Request #53052 · dotnet/roslyn

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.

allisonchou · 2021-05-03T18:46:46Z

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

            SyntaxKind.ParenthesizedLambdaExpression,
-            SyntaxKind.LocalFunctionStatement);
+            SyntaxKind.LocalFunctionStatement,
+            // TODO: Record structs


Would it be possible to create + link a work item here so we can track adding record struct support in the future?

@allisonchou It should fall into the existing test plan issue for record structs (#51199). Tagging @jcouv to edit the issue

we have record structs now right? so can we just add that in this PR?

allisonchou · 2021-05-03T21:21:11Z

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

+                    // 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(


Hmm, I'm a little worried about hardcoding the raw kind here. I almost think we should wait on merging until we have a proper workaround that doesn't involve hardcoding. @CyrusNajmabadi do you have any thoughts?

I think #53092 may not be very soon 😕

we're in an abstract type, just provide a helper that C# implements, going back to source to answer this.

CyrusNajmabadi · 2021-05-03T23:09:16Z

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

            }

+            // TODO: Record structs
+            if (updatedNode.IsKind(SyntaxKind.RecordDeclaration, out RecordDeclarationSyntax? record) && record.ParameterList is not null)


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

Suggested change

if (updatedNode.IsKind(SyntaxKind.RecordDeclaration, out RecordDeclarationSyntax? record) && record.ParameterList is not null)

if (updatedNode is RecordDeclarationSyntax { ParameterList: not null })

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

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

            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

ghost 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

Closed

Fix test

6749276

allisonchou reviewed May 3, 2021

View reviewed changes

CyrusNajmabadi reviewed May 3, 2021

View reviewed changes

Add NotNullWhenAttribute

e8ac91d

Youssef1313 requested review from CyrusNajmabadi and allisonchou May 4, 2021 18:45

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

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

Conversation

Youssef1313 commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODOs

Uh oh!

Uh oh!

Youssef1313 Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented Apr 30, 2021

Uh oh!

Youssef1313 commented Apr 30, 2021

Uh oh!

Youssef1313 commented Apr 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented May 21, 2021

Uh oh!

allisonchou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

allisonchou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

allisonchou commented May 28, 2021

Uh oh!

Youssef1313 commented Jul 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jul 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Youssef1313 commented Apr 30, 2021 •

edited

Loading

Youssef1313 Apr 30, 2021 •

edited

Loading