Skip to content

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Nov 2, 2020

Fixes #44571

@jcouv jcouv marked this pull request as ready for review November 2, 2020 21:39
@jcouv jcouv requested a review from a team as a code owner November 2, 2020 21:39
var i1 = model.GetDeclaredSymbol(parameter);

comp.VerifyDiagnostics(
// (4,21): warning CS1591: Missing XML comment for publicly visible type or member 'C.I1'
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2020

Choose a reason for hiding this comment

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

warning CS1591: Missing XML comment for publicly visible type or member 'C.I1' [](start = 27, length = 78)

Is this warning for generated property? I think we need to suppress it one way or another. #Closed

Copy link
Member Author

@jcouv jcouv Nov 5, 2020

Choose a reason for hiding this comment

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

Yes, the warning is for the generated property. I'm okay to suppress it #Resolved

<summary>Property summary</summary>
</member>
", property.GetDocumentationCommentXml());
}
}
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2020

Choose a reason for hiding this comment

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

} [](start = 4, length = 1)

I think we need to have tests with multiple partial declarations:

  • when the primary constructor declaration is in a different part
  • when the type part has parameters in syntax, but they are not parameters of the primary constructor for the record,
  • other error scenarios
  • etc. #Closed

{
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.Parent);
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2020

Choose a reason for hiding this comment

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

memberSyntax.Parent [](start = 51, length = 19)

Is there a reason to go up to the parent here? Could we get a binder for the type instead? #Closed

Copy link
Member Author

@jcouv jcouv Nov 5, 2020

Choose a reason for hiding this comment

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

Indeed, we get the same binder without .Parent. Will remove. Thanks #Resolved

{
Binder outerBinder = VisitCore(memberSyntax.Parent);
SourceNamedTypeSymbol recordType = ((NamespaceOrTypeSymbol)outerBinder.ContainingMemberOrLambda).GetSourceTypeMember(recordDeclSyntax);
var primaryConstructor = recordType.GetMembersUnordered().OfType<SynthesizedRecordConstructor>().SingleOrDefault();
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2020

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)

SourceNamedTypeSymbol recordType = ((NamespaceOrTypeSymbol)outerBinder.ContainingMemberOrLambda).GetSourceTypeMember(recordDeclSyntax);
var primaryConstructor = recordType.GetMembersUnordered().OfType<SynthesizedRecordConstructor>().SingleOrDefault();
Debug.Assert(primaryConstructor is not null);
return new WithParametersBinder(primaryConstructor.Parameters, nextBinder);
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2020

Choose a reason for hiding this comment

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

nextBinder [](start = 83, length = 10)

What is this binder? Is it the same kind as the one used when we return from the previous if? #Closed

Copy link
Member Author

@jcouv jcouv Nov 5, 2020

Choose a reason for hiding this comment

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

It's the binder used to bring parameters in scope into xmldocs. Yes, it's the same that is used in the previous if (which handles the xmldoc binding on methods).

    /// Binder used to place the parameters of a method, property, indexer, or delegate
    /// in scope when binding &lt;param&gt; tags inside of XML documentation comments.
``` #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the binder used to bring parameters in scope into xmldocs. Yes, it's the same that is used in the previous if (which handles the xmldoc binding on methods).

This doesn't really answer my question. Could you stop on both code paths under debugger and examine what is nextBinder on each of them? What is the type of the binder, containing symbols, etc.


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

Copy link
Member Author

@jcouv jcouv Nov 5, 2020

Choose a reason for hiding this comment

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

Sorry, I'd misunderstood the question (didn't notice you were pointing at "nextBinder").
nextBinder is a BinderWithContainingMemberOrLambda for the global namespace.

BuckStopsHereBinder
└─BinderWithContainingMemberOrLambda
  └─containing symbol: <global namespace>

But in the previous if, we'd have nextBinder being the containing type C:

BuckStopsHereBinder
└─BinderWithContainingMemberOrLambda
  └─containing symbol: C

You raise a good question (implicitly): should we chain a binder for the type C before we chain a binder for parameter list? #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

You raise a good question (implicitly): should we chain a binder for the type C before we chain a binder for parameter list?

If the binder that we return is used to resolve something other than parameter references, then, I think, we should add some targeted tests to verify/observe what the nextBinder brings/doesn't bring in scope and whether the behavior matches our expectations.


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

Copy link
Contributor

@AlekseyTs AlekseyTs Dec 11, 2020

Choose a reason for hiding this comment

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

I'd like to follow up offline on:

If the binder that we return is used to resolve something other than parameter references, then, I think, we should add some targeted tests to verify/observe what the nextBinder brings/doesn't bring in scope and whether the behavior matches our expectations.


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

Copy link
Member Author

Choose a reason for hiding this comment

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

Did some further review of impact.
The change to BinderFactory.GetParameterNameAttributeValueBinder only affects the name property as it is only used in BinderFactory.VisitXmlNameAttribute. Only <param> and <paramref> tags use it.
The resulting binder is only used in DocumentationCommentCompiler.BindName. I added some tests for some of the cases in that method.


In reply to: 541433186 [](ancestors = 541433186,517015109)

Assert.Equal(
@"<member name=""T:C"">
<summary>Summary</summary>
<param name=""I1"">Description for I1</param>
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2020

Choose a reason for hiding this comment

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

Description for I1 [](start = 4, length = 45)

Is it Ok to have a param tag on the type? #Closed

Copy link
Member Author

@jcouv jcouv Nov 4, 2020

Choose a reason for hiding this comment

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

Yes, this is what we had discussed. See notes: #44571 (comment) #Closed

<summary>Summary</summary>
<param name=""I1"">Description for I1</param>
</member>
", constructor.GetDocumentationCommentXml());
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2020

Choose a reason for hiding this comment

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

GetDocumentationCommentXml [](start = 15, length = 26)

I didn't see any special code change to add this doc-comment to the constructor. How does this work? #Closed

Copy link
Member Author

@jcouv jcouv Nov 5, 2020

Choose a reason for hiding this comment

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

The declaring syntax for the constructor is the record declaration, so we find the proper trivia containing xmldocs. #ByDesign

Copy link
Contributor

Choose a reason for hiding this comment

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

The declaring syntax for the constructor is the record declaration, so we find the proper trivia containing xmldocs.

So, we attach types doc-comment to the constructor? I do not think we made an explicit decision to do that. It works this way by accident.


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

Copy link
Member Author

@jcouv jcouv Nov 5, 2020

Choose a reason for hiding this comment

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

I think we want the xmldoc on the record to apply to the contructor too, that's consistent with wanting the <param> to apply to the primary constructor parameters.
Can you clarify what is your expectation or proposal? #ByDesign

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want the xmldoc on the record to apply to the contructor too, that's consistent with wanting the to apply to the primary constructor parameters.
Can you clarify what is your expectation or proposal?

I believe, we decided to transfer content of <param> to the constructor, not the entire doc comment. I am proposing to either explicitly make this decision (outside of this PR), or to follow previous decision by the letter. I.e. transfer the tags rather than the entire doc-comment.


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

Copy link
Member Author

Choose a reason for hiding this comment

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

Confirmed expectations by email with LDM. Notes from 11/9/2020:

  1. The entire xml doc from the record applies to the primary constructor (not just )
  2. Missing xml doc warning is suppressed on synthesized properties for now

In reply to: 518293736 [](ancestors = 518293736,518290263)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Nov 3, 2020

Done with review pass (iteration 1) #Closed

", e.GetDocumentationCommentXml());

var eConstructor = e.GetMembers(".ctor").First();
Assert.Equal(1, eConstructor.DeclaringSyntaxReferences.Count());
Copy link
Member Author

@jcouv jcouv Nov 5, 2020

Choose a reason for hiding this comment

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

📝 If we kept track of two declaring syntax references for this error scenario, we could collect both xml docs. I'm not sure that's worthwhile. #Closed


/// <summary>Summary</summary>
/// <param name=""I1"">Description for I1</param>
public partial record E;
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2020

Choose a reason for hiding this comment

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

Do we have a test with partial declarations in different order? #Closed

var src = @"
public partial record C;

/// <summary>Summary</summary>
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2020

Choose a reason for hiding this comment

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

Summary [](start = 13, length = 7)

Please make sure each doc comment has distinct content so that we could be certain where things are coming from. #Closed


[Fact]
[WorkItem(44571, "https://github.com/dotnet/roslyn/issues/44571")]
public void XmlDoc_Partial_DuplicateParameterList()
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2020

Choose a reason for hiding this comment

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

XmlDoc_Partial_DuplicateParameterList [](start = 20, length = 37)

Are we testing with different parameter lists, perhaps with different parameter names, etc.? #Closed

public partial record C(int I1);

/// <summary>Summary</summary>
/// <param name=""I1"">Description for I1</param>
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2020

Choose a reason for hiding this comment

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

I would expect a diagnostics on this tag. This partial declaration doesn't declare any primary constructor and cannot refer to a parameters. #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.

We do have a diagnostic for this situation ( on the partial that doesn't declare primary constructor):

                // (5,18): warning CS1572: XML comment has a param tag for 'I1', but there is no parameter by that name
                // /// <param name="I1">Description for I1</param>
                Diagnostic(ErrorCode.WRN_UnmatchedParamTag, "I1").WithArguments("I1").WithLocation(5, 18)

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


/// <summary>Summary</summary>
/// <param name=""I1"">Description for I1</param>
public partial record C(int I1);
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2020

Choose a reason for hiding this comment

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

(int I1) [](start = 23, length = 8)

We should also have a test with a different parameter list. There is no guarantee that all partials will have the same parameter list. #Closed

// (12,24): error CS8863: Only a single record partial declaration may have a parameter list
// public partial record D(int I1);
Diagnostic(ErrorCode.ERR_MultipleRecordParameterLists, "(int I1)").WithLocation(12, 24),
// (19,12): warning CS1571: XML comment has a duplicate param tag for 'I1'
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2020

Choose a reason for hiding this comment

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

// (19,12): warning CS1571: XML comment has a duplicate param tag for 'I1' [](start = 16, length = 74)

This looks unexpected, I do not see duplicate tags in the doc-comment. #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.

commented at new location in source


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

var src = @"
public partial record C(int I1);

/// <summary>Summary</summary>
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2020

Choose a reason for hiding this comment

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

Summary [](start = 13, length = 7)

I cannot tell doc comments apart, therefore cannot verify expected behavior. #Closed


[Fact]
[WorkItem(44571, "https://github.com/dotnet/roslyn/issues/44571")]
public void XmlDoc_Partial()
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2020

Choose a reason for hiding this comment

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

XmlDoc_Partial [](start = 20, length = 14)

Consider putting each distinct scenario in a separate unit-test #Closed

</member>
", e.GetDocumentationCommentXml());

var eConstructor = e.GetMembers(".ctor").First();
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 11, 2020

Choose a reason for hiding this comment

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

.First() [](start = 52, length = 8)

This feels fragile, there are several constructors there and we are relying on their order in the list, which can change in the future because the order is not specified by the language. Reviewers also cannot tell which constructor we look at. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 11, 2020

Done with review pass (commit 9) #Closed

@eluchsinger
Copy link

eluchsinger commented Dec 21, 2020

When can we count on a release containing this fix? #Resolved

@jcouv
Copy link
Member Author

jcouv commented Dec 21, 2020

@eluchsinger Aiming for 16.9p4 at this point #Resolved

@jcouv jcouv requested a review from AlekseyTs December 21, 2020 23:58
return new WithParametersBinder(primaryConstructor.Parameters, nextBinder);
}

return nextBinder;
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 23, 2020

Choose a reason for hiding this comment

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

return nextBinder; [](start = 20, length = 18)

Consider falling through. We handled the case we were looking for and letting the rest of the function to see if something else is applicable. #Resolved

<param name=""I1"">Description for I1</param>
</member>
", cMember.GetDocumentationCommentXml());
var constructor = cMember.GetMembers(".ctor").OfType<MethodSymbol>().Where(m => m.Parameters.AsSingleton()!.Name == "I1").Single();
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 23, 2020

Choose a reason for hiding this comment

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

MethodSymbol [](start = 65, length = 12)

Would it be simpler and more robust to use SynthesizedRecordConstructor type check instead? There would be no need to check parameter names, etc. #Resolved

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 12)

@jcouv
Copy link
Member Author

jcouv commented Dec 28, 2020

@dotnet/roslyn-compiler for a second review. Thanks #Resolved

{
Binder outerBinder = VisitCore(memberSyntax);
SourceNamedTypeSymbol recordType = ((NamespaceOrTypeSymbol)outerBinder.ContainingMemberOrLambda).GetSourceTypeMember(recordDeclSyntax);
var primaryConstructor = recordType.GetMembersUnordered().OfType<SynthesizedRecordConstructor>().SingleOrDefault();
Copy link
Member

@333fred 333fred Dec 29, 2020

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)

return symbol.IsImplicitlyDeclared ||
symbol.IsAccessor() ||
symbol is SynthesizedSimpleProgramEntryPointSymbol ||
symbol is SimpleProgramNamedTypeSymbol ||
Copy link
Member

@333fred 333fred Dec 29, 2020

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)


[Fact]
[WorkItem(44571, "https://github.com/dotnet/roslyn/issues/44571")]
public void XmlDoc_Cref()
Copy link
Member

@333fred 333fred Dec 29, 2020

Choose a reason for hiding this comment

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

I get why this test case works the way it does, but I bet someone will be confused by it anyway. #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 threw me off at first too...


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

comp.VerifyDiagnostics(
// (3,18): warning CS1572: XML comment has a param tag for 'Error', but there is no parameter by that name
// /// <param name="Error"></param>
Diagnostic(ErrorCode.WRN_UnmatchedParamTag, "Error").WithArguments("Error").WithLocation(3, 18),
Copy link
Member

@333fred 333fred Dec 29, 2020

Choose a reason for hiding this comment

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

Can we dedup these? #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically the warning on the property could be fixed separately, so the warnings are separate.
But mostly I don't think it's worth the effort to dedupe :-/


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


/// <summary>Summary</summary>
/// <param name=""I1"">Description for I1</param>
/// <param name=""O1"">Error</param>
Copy link
Member

@333fred 333fred Dec 29, 2020

Choose a reason for hiding this comment

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

Consider putting Error O1 and similar for these cases, so they're differentiated in the test output. #Closed

@333fred
Copy link
Member

333fred commented Dec 29, 2020

Done review pass (commit 13). #Resolved

@jcouv jcouv requested a review from 333fred December 30, 2020 18:58
Copy link
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 (commit 14)

@jcouv jcouv merged commit 9f87b44 into dotnet:master Jan 5, 2021
@ghost ghost added this to the Next milestone Jan 5, 2021
@jcouv jcouv deleted the xml-docs branch January 5, 2021 20:05
@Cosifne Cosifne modified the milestones: Next, 16.9 Jan 27, 2021
@JoeRobich JoeRobich modified the milestones: 16.9, 16.9.P4 Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Figure out XML docs for positional records
8 participants