close
Skip to content

Fixed documentation comment ID handling for dynamic and tuples#48359

Merged
333fred merged 6 commits into
dotnet:masterfrom
svick:dynamic-docid
Oct 13, 2020
Merged

Fixed documentation comment ID handling for dynamic and tuples#48359
333fred merged 6 commits into
dotnet:masterfrom
svick:dynamic-docid

Conversation

@svick
Copy link
Copy Markdown
Member

@svick svick commented Oct 6, 2020

Fixes #34839.
Fixes #48495.

Previously, DocumentationCommentId.CreateDeclarationId behaved correctly for named and unnamed tuples, but not for dynamic. And DocumentationCommentId.GetSymbolsForDeclarationId only behaved correctly for unnamed tuples, but not for named tuples or dynamic.

This PR fixes all the cases that didn't behave correctly so that the documentation comment ID represents CLR signature of the member (i.e. System.Object for dynamic and System.ValueTuple for tuples).

@svick svick requested a review from a team as a code owner October 6, 2020 13:13
@jcouv jcouv added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Oct 6, 2020
Comment thread src/Compilers/Core/Portable/DocumentationCommentId.cs Outdated
@333fred
Copy link
Copy Markdown
Member

333fred commented Oct 6, 2020

Done review pass (commit 1)

@svick
Copy link
Copy Markdown
Member Author

svick commented Oct 7, 2020

@333fred I have addressed your comments and also added a test for nint.

Comment thread src/Compilers/Core/Portable/Symbols/SymbolEqualityComparer.cs
Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Done review pass (commit 3)

Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM. @dotnet/roslyn-compiler for a second review.

@333fred
Copy link
Copy Markdown
Member

333fred commented Oct 9, 2020

@dotnet/roslyn-compiler for a second review of this community PR.

@svick
Copy link
Copy Markdown
Member Author

svick commented Oct 10, 2020

I believe this should also fix the recently opened #48495, so I have added a test for that and modified "fixes" accordingly.

Comment thread src/Compilers/Core/CodeAnalysisTest/Symbols/DocumentationCommentIdTests.cs Outdated
@333fred 333fred merged commit 8d6b34d into dotnet:master Oct 13, 2020
@ghost ghost added this to the Next milestone Oct 13, 2020
@333fred
Copy link
Copy Markdown
Member

333fred commented Oct 13, 2020

Thanks @svick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

6 participants