Skip to content

Put record parameters in scope of xml docs #49134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jan 5, 2021
Merged

Put record parameters in scope of xml docs #49134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5022bc2
Put record parameters in scope of xml docs
jcouv Oct 30, 2020
de5e638
Suppress missing xml on synthesized record properties
jcouv Nov 5, 2020
ffe4fda
Add tests for various partials
jcouv Nov 5, 2020
1c7f178
Add test for empty parameter list
jcouv Nov 5, 2020
14e6b96
Remove .Parent
jcouv Nov 5, 2020
2af5a82
Add test with nested record
jcouv Nov 5, 2020
38a249c
Split tests and add more error tests
jcouv Nov 6, 2020
32b0bc3
Address feedback
jcouv Dec 3, 2020
7e373fe
Address feedback
jcouv Dec 10, 2020
76d7c1a
nits
jcouv Dec 14, 2020
6ecb064
more tests
jcouv Dec 21, 2020
ecc1f93
test cref
jcouv Dec 21, 2020
61ab83b
tweaks
jcouv Dec 28, 2020
1872663
Address feedback
jcouv Dec 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/BinderFactory.BinderFactoryVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,14 +1132,26 @@ public override Binder VisitXmlNameAttribute(XmlNameAttributeSyntax parent)
/// </summary>
private Binder GetParameterNameAttributeValueBinder(MemberDeclarationSyntax memberSyntax, Binder nextBinder)
{
BaseMethodDeclarationSyntax baseMethodDeclSyntax = memberSyntax as BaseMethodDeclarationSyntax;
if ((object)baseMethodDeclSyntax != null && baseMethodDeclSyntax.ParameterList.ParameterCount > 0)
if (memberSyntax is BaseMethodDeclarationSyntax { ParameterList: { ParameterCount: > 0 } } baseMethodDeclSyntax)
{
Binder outerBinder = VisitCore(memberSyntax.Parent);
MethodSymbol method = GetMethodSymbol(baseMethodDeclSyntax, outerBinder);
return new WithParametersBinder(method.Parameters, nextBinder);
}

if (memberSyntax is RecordDeclarationSyntax { ParameterList: { ParameterCount: > 0 } } recordDeclSyntax)
{
Binder outerBinder = VisitCore(memberSyntax);
SourceNamedTypeSymbol recordType = ((NamespaceOrTypeSymbol)outerBinder.ContainingMemberOrLambda).GetSourceTypeMember(recordDeclSyntax);
var primaryConstructor = recordType.GetMembersUnordered().OfType<SynthesizedRecordConstructor>().SingleOrDefault();
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

primaryConstructor [](start = 24, length = 18)

I think this method might not correspond to the parameters in this partial declaration. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Added tests for partials.
We're basically taking the xml doc from the declaration that has the parameter list. That seems sensible. But we could also try to also find the xml doc on declaration without parameter list. What do you think?

In reply to: 517014473 [](ancestors = 517014473)

Copy link
Contributor

Choose a reason for hiding this comment

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

We're basically taking the xml doc from the declaration that has the parameter list. That seems sensible. But we could also try to also find the xml doc on declaration without parameter list. What do you think?

I think that we should match doc-comment to the primary constructor only if both are on the same partial declaration

In reply to: 518320962 [](ancestors = 518320962,517014473)

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that only one partial declaration declares a primary constructor, parameter lists on other declarations are ignored and not analyzed, can have all kind of differences by comparison to each other.

In reply to: 518379208 [](ancestors = 518379208,518320962,517014473)

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently my comment wasn't clear. Let me rephrase it. I think that here we should find primary constructor symbol only if it corresponds to memberSyntax. We shouldn't be using a symbol that is declared by a different partial declaration.

In reply to: 518380246 [](ancestors = 518380246,518379208,518320962,517014473)

Copy link
Member

@333fred 333fred Dec 29, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

SingleOrDefault [](start = 117, length = 15)

Why will this be necessarily only one constructor? Doesn't the copy constructor count as a SynthesizedRecordConstructor? Consider leaving a comment. #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

Records have exactly one SynthesizedRecordConstructor (it's the primary ctor). The copy constructor is either a source symbol or SynthesizedRecordCopyCtor.

In reply to: 549877846 [](ancestors = 549877846)


if (primaryConstructor.SyntaxRef.SyntaxTree == recordDeclSyntax.SyntaxTree &&
primaryConstructor.GetSyntax() == recordDeclSyntax)
{
return new WithParametersBinder(primaryConstructor.Parameters, nextBinder);
}
}

// As in Dev11, we do not allow <param name="value"> on events.
SyntaxKind memberKind = memberSyntax.Kind();
if (memberKind == SyntaxKind.PropertyDeclaration || memberKind == SyntaxKind.IndexerDeclaration)
Expand Down
6 changes: 5 additions & 1 deletion src/Compilers/CSharp/Portable/Compiler/DocumentationCommentCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,11 @@ public override void DefaultVisit(Symbol symbol)

private static bool ShouldSkip(Symbol symbol)
{
return symbol.IsImplicitlyDeclared || symbol.IsAccessor() || symbol is SynthesizedSimpleProgramEntryPointSymbol || symbol is SimpleProgramNamedTypeSymbol;
return symbol.IsImplicitlyDeclared ||
symbol.IsAccessor() ||
symbol is SynthesizedSimpleProgramEntryPointSymbol ||
symbol is SimpleProgramNamedTypeSymbol ||
Copy link
Member

@333fred 333fred Dec 29, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Could be symbol is SynthesizedSimpleProgramEntryPointSymbol or SimpleProgramNamedTypeSymbol or SynthesizedRecordPropertySymbol. #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

It could (was suggested by Olivier earlier) but I think it reads better as-is.

In reply to: 549878424 [](ancestors = 549878424)

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I don't agree, but it's your call I guess.

In reply to: 549984705 [](ancestors = 549984705,549878424)

symbol is SynthesizedRecordPropertySymbol;
}
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The line is getting very long, consider splitting it. #Closed

Copy link

Choose a reason for hiding this comment

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

Could be reduced to

return symbol.IsImplicitlyDeclared || symbol.IsAccessor() || symbol is SynthesizedSimpleProgramEntryPointSymbol or SimpleProgramNamedTypeSymbol or SynthesizedRecordPropertySymbol;
        }

?


/// <summary>
Expand Down
Loading