Extensions: xml docs #78365

jcouv · 2025-04-29T11:13:17Z

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

AlekseyTs · 2025-04-29T14:01:44Z

src/Compilers/CSharp/Portable/Compiler/DocumentationCommentCompiler.cs

+        {
+            if (symbol is SourceExtensionImplementationMethodSymbol implementation)
+            {
+                var symbolForDocComment = implementation.UnderlyingMethod is SourcePropertyAccessorSymbol accessorSymbol


implementation.UnderlyingMethod is SourcePropertyAccessorSymbol

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

AlekseyTs · 2025-04-29T14:05:04Z

src/Compilers/CSharp/Portable/Compiler/DocumentationCommentCompiler.cs

+                    ? accessorSymbol.AssociatedSymbol
+                    : implementation.UnderlyingMethod;
+
+                if (symbolForDocComment.GetDocumentationCommentXml(preferredCulture: null, _processIncludes, _cancellationToken).IsEmpty())


_processIncludes

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

AlekseyTs · 2025-04-29T14:09:36Z

src/Compilers/CSharp/Portable/Compiler/DocumentationCommentCompiler.cs

+                    ? accessorSymbol.AssociatedSymbol
+                    : implementation.UnderlyingMethod;
+
+                if (symbolForDocComment.GetDocumentationCommentXml(preferredCulture: null, _processIncludes, _cancellationToken).IsEmpty())


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

AlekseyTs · 2025-04-29T14:11:55Z

src/Compilers/CSharp/Portable/Compiler/DocumentationCommentCompiler.cs

+                return;
+            }
+
+            DefaultVisit(symbol);


DefaultVisit(symbol);

base.VisitMethod(symbol);? #Closed

AlekseyTs · 2025-04-29T14:16:20Z

src/Compilers/CSharp/Portable/Symbols/Extensions/SourceExtensionImplementationMethodSymbol.cs

@@ -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);


lazyDocComment

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

AlekseyTs · 2025-04-29T14:17:03Z

src/Compilers/CSharp/Portable/Symbols/Extensions/SourceExtensionImplementationMethodSymbol.cs

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

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


GetDocumentationCommentXml

It would be good to have direct tests for this API. #Closed

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

Do you have specific API tests in mind?

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

Added an assertion

AlekseyTs · 2025-04-29T14:46:21Z

src/Compilers/CSharp/Portable/Binder/BinderFactory.BinderFactoryVisitor.cs

            {
                if (memberSyntax is BaseMethodDeclarationSyntax { ParameterList: { ParameterCount: > 0 } } baseMethodDeclSyntax)
                {
                    Binder outerBinder = VisitCore(memberSyntax.Parent);
                    MethodSymbol method = GetMethodSymbol(baseMethodDeclSyntax, outerBinder);
+
+                    if (isParamRef && method.TryGetInstanceExtensionParameter(out var _))


isParamRef

Is there a real need for this condition? #Closed

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 · 2025-04-29T14:57:21Z

Done with review pass (commit 1)

AlekseyTs · 2025-04-30T13:57:22Z

src/Compilers/CSharp/Portable/Symbols/Extensions/SourceExtensionImplementationMethodSymbol.cs

+            string result = SourceDocumentationCommentUtils.GetAndCacheDocumentationComment(this, expandIncludes: false, ref lazyDocComment);
+
+#if DEBUG
+            string withIncludes = DocumentationCommentCompiler.GetDocumentationCommentXml(this, processIncludes: true, default);


DocumentationCommentCompiler.GetDocumentationCommentXml

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

AlekseyTs

LGTM (commit 4)

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

* 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 self-assigned this Apr 29, 2025

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 29, 2025

jcouv force-pushed the extensions-xml branch 3 times, most recently from 166abd7 to 074b68e Compare April 29, 2025 11:31

Extensions: xml docs

cf0d4d9

jcouv force-pushed the extensions-xml branch from 074b68e to cf0d4d9 Compare April 29, 2025 11:42

jcouv marked this pull request as ready for review April 29, 2025 13:39

jcouv requested a review from a team as a code owner April 29, 2025 13:39

jcouv requested review from jjonescz and AlekseyTs April 29, 2025 13:39

AlekseyTs reviewed Apr 29, 2025

View reviewed changes

jcouv added 2 commits April 29, 2025 17:27

Address feedback

f82ebbf

Add assertion

3cc3d48

jcouv requested a review from AlekseyTs April 29, 2025 18:48

Comment

1c1e2a4

jjonescz approved these changes Apr 30, 2025

View reviewed changes

AlekseyTs reviewed Apr 30, 2025

View reviewed changes

AlekseyTs approved these changes Apr 30, 2025

View reviewed changes

BillWagner added a commit to BillWagner/csharplang that referenced this pull request Apr 30, 2025

Update docs output

55d1a1f

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

BillWagner mentioned this pull request Apr 30, 2025

Update docs output for extensions dotnet/csharplang#9346

Merged

Tweak DEBUG check

0125e74

jcouv enabled auto-merge (squash) April 30, 2025 16:28

jcouv merged commit 9041fb0 into dotnet:main Apr 30, 2025
23 of 24 checks passed

dotnet-policy-service bot added this to the Next milestone Apr 30, 2025

This was referenced May 2, 2025

[Automated] PRs inserted in VS build main-10601.69 #78406

Closed

[Automated] PRs inserted in VS build feature.debugger.main-10601.163 #78410

Closed

jcouv deleted the extensions-xml branch May 8, 2025 23:52

jcouv mentioned this pull request May 30, 2025

Compiler crash generating XML docs for extension #78768

Closed

dotnet-bot mentioned this pull request May 31, 2025

[Automated] PRs inserted in VS build feature.telemetry.d18OpenTelemetry-10630.96 #78782

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Extensions: xml docs #78365

Extensions: xml docs #78365

Uh oh!

jcouv commented Apr 29, 2025 •

edited

Loading

Uh oh!

AlekseyTs Apr 29, 2025 •

edited

Loading

Uh oh!

AlekseyTs Apr 29, 2025 •

edited

Loading

Uh oh!

AlekseyTs Apr 29, 2025 •

edited

Loading

Uh oh!

AlekseyTs Apr 29, 2025 •

edited

Loading

Uh oh!

AlekseyTs Apr 29, 2025 •

edited

Loading

Uh oh!

AlekseyTs Apr 29, 2025 •

edited

Loading

Uh oh!

jcouv Apr 29, 2025

Uh oh!

AlekseyTs Apr 29, 2025

Uh oh!

jcouv Apr 29, 2025

Uh oh!

AlekseyTs Apr 29, 2025 •

edited

Loading

Uh oh!

jcouv Apr 29, 2025

Uh oh!

AlekseyTs commented Apr 29, 2025

Uh oh!

AlekseyTs Apr 30, 2025

Uh oh!

AlekseyTs left a comment

Uh oh!

Uh oh!

Uh oh!

Extensions: xml docs #78365

Extensions: xml docs #78365

Uh oh!

Conversation

jcouv commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Apr 29, 2025 • 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!

AlekseyTs Apr 29, 2025 • 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!

AlekseyTs Apr 29, 2025 • 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!

AlekseyTs Apr 29, 2025 • 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!

AlekseyTs Apr 29, 2025 • 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!

AlekseyTs Apr 29, 2025 • 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!

jcouv Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 29, 2025 • 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!

jcouv Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Apr 29, 2025

Uh oh!

AlekseyTs Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jcouv commented Apr 29, 2025 •

edited

Loading

AlekseyTs Apr 29, 2025 •

edited

Loading

AlekseyTs Apr 29, 2025 •

edited

Loading

AlekseyTs Apr 29, 2025 •

edited

Loading

AlekseyTs Apr 29, 2025 •

edited

Loading

AlekseyTs Apr 29, 2025 •

edited

Loading

AlekseyTs Apr 29, 2025 •

edited

Loading

AlekseyTs Apr 29, 2025 •

edited

Loading