Skip to content

Extensions: xml docs #78365

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.

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 5 commits into from
Apr 30, 2025
Merged

Extensions: xml docs #78365

merged 5 commits into from
Apr 30, 2025

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Apr 29, 2025

Design (confirmed with Cyrus and Bill):

  • callers (IDE or other tools) are responsible for stitching extension-level info (extension parameter and type parameters) with member-level info as needed (ie. depends on whether the extension member is static)
  • implementation methods gets inheritdoc pointing to relevant skeleton member (doc) (note: I verified this works well in VS in prototype project)
  • no warning yet for undocumented extension parameter/type parameter, or for undocumented member parameter/type parameter when there's documentation at the extension level

Note: I had a commit to switch GetDocumentationCommentXml from virtual to abstract (push the implementations into the leaves) but decided to separate such refactoring out.

Relates to test plan #76130

@jcouv jcouv self-assigned this Apr 29, 2025
@ghost ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 29, 2025
@jcouv jcouv force-pushed the extensions-xml branch 3 times, most recently from 166abd7 to 074b68e Compare April 29, 2025 11:31
@jcouv jcouv marked this pull request as ready for review April 29, 2025 13:39
@jcouv jcouv requested a review from a team as a code owner April 29, 2025 13:39
@jcouv jcouv requested review from jjonescz and AlekseyTs April 29, 2025 13:39
{
if (symbol is SourceExtensionImplementationMethodSymbol implementation)
{
var symbolForDocComment = implementation.UnderlyingMethod is SourcePropertyAccessorSymbol accessorSymbol
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 29, 2025

Choose a reason for hiding this comment

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

implementation.UnderlyingMethod is SourcePropertyAccessorSymbol

Consider using IsAccessor helper instead. The type check won't work for events, for example. #Closed

? accessorSymbol.AssociatedSymbol
: implementation.UnderlyingMethod;

if (symbolForDocComment.GetDocumentationCommentXml(preferredCulture: null, _processIncludes, _cancellationToken).IsEmpty())
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 29, 2025

Choose a reason for hiding this comment

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

_processIncludes

It is not obvious to me that this is the value that we should be using. #Closed

? accessorSymbol.AssociatedSymbol
: implementation.UnderlyingMethod;

if (symbolForDocComment.GetDocumentationCommentXml(preferredCulture: null, _processIncludes, _cancellationToken).IsEmpty())
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 29, 2025

Choose a reason for hiding this comment

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

symbolForDocComment.GetDocumentationCommentXml

It would be good to add a comment explaining the rational behind this check. Also, could we simply check syntax for presence of a doc comment on the source declaration, if this is what we are trying to determine here? #Closed

return;
}

DefaultVisit(symbol);
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 29, 2025

Choose a reason for hiding this comment

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

DefaultVisit(symbol);

base.VisitMethod(symbol);? #Closed

@@ -131,6 +135,11 @@ internal override int TryGetOverloadResolutionPriority()
return UnderlyingMethod.TryGetOverloadResolutionPriority();
}

public override string GetDocumentationCommentXml(CultureInfo? preferredCulture = null, bool expandIncludes = false, CancellationToken cancellationToken = default)
{
return SourceDocumentationCommentUtils.GetAndCacheDocumentationComment(this, expandIncludes, ref lazyDocComment);
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 29, 2025

Choose a reason for hiding this comment

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

lazyDocComment

It looks like SourceMemberMethodSymbol uses different cache depending on the value of expandIncludes. #Closed

@@ -131,6 +135,11 @@ internal override int TryGetOverloadResolutionPriority()
return UnderlyingMethod.TryGetOverloadResolutionPriority();
}

public override string GetDocumentationCommentXml(CultureInfo? preferredCulture = null, bool expandIncludes = false, CancellationToken cancellationToken = default)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 29, 2025

Choose a reason for hiding this comment

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

GetDocumentationCommentXml

It would be good to have direct tests for this API. #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.

There's multiple direct tests ( XmlDoc_01 through XmlDoc_03). Do you have specific API tests in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have specific API tests in mind?

Yes, the effect of expandIncludes and effect of caching, including on consecutive calls with different expandIncludes.

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 an assertion

{
if (memberSyntax is BaseMethodDeclarationSyntax { ParameterList: { ParameterCount: > 0 } } baseMethodDeclSyntax)
{
Binder outerBinder = VisitCore(memberSyntax.Parent);
MethodSymbol method = GetMethodSymbol(baseMethodDeclSyntax, outerBinder);

if (isParamRef && method.TryGetInstanceExtensionParameter(out var _))
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 29, 2025

Choose a reason for hiding this comment

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

isParamRef

Is there a real need for this condition? #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.

From an implementation perspective, this condition ensures that <param name="extensionParameter"> may only be placed on the extension block. This is shown in XmlDoc_Param_03.
From a language perspective, this rule ensures that you don't end up with two <param> tags for the extension parameter (one on the extension block and one on a member). I think allowing such duplicates would make consumption of the resulting xml more difficult than it needs to be.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

@jcouv jcouv requested a review from AlekseyTs April 29, 2025 18:48
string result = SourceDocumentationCommentUtils.GetAndCacheDocumentationComment(this, expandIncludes: false, ref lazyDocComment);

#if DEBUG
string withIncludes = DocumentationCommentCompiler.GetDocumentationCommentXml(this, processIncludes: true, default);
Copy link
Contributor

Choose a reason for hiding this comment

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

DocumentationCommentCompiler.GetDocumentationCommentXml

In order to make sure we compare apples to apples, I think SourceDocumentationCommentUtils.GetAndCacheDocumentationComment should be used here as well

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 4)

BillWagner added a commit to BillWagner/csharplang that referenced this pull request Apr 30, 2025
Recent decisions in LDM and dotnet/roslyn#78365 change how XML output is generated and should be consumed.
@jcouv jcouv enabled auto-merge (squash) April 30, 2025 16:28
@jcouv jcouv merged commit 9041fb0 into dotnet:main Apr 30, 2025
23 of 24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 30, 2025
BillWagner added a commit to dotnet/csharplang that referenced this pull request May 1, 2025
* Update docs output

Recent decisions in LDM and dotnet/roslyn#78365 change how XML output is generated and should be consumed.

* Proofread and clarify

* Apply suggestions from code review

Co-authored-by: Julien Couvreur <[email protected]>

* respond to feedback.

* Apply suggestions from code review

Co-authored-by: Genevieve Warren <[email protected]>

* Respond to feedback.

---------

Co-authored-by: Julien Couvreur <[email protected]>
Co-authored-by: Genevieve Warren <[email protected]>
@jcouv jcouv deleted the extensions-xml branch May 8, 2025 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants