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.

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

Extensions: xml docs #78365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cf0d4d9
Extensions: xml docs
jcouv Apr 25, 2025
f82ebbf
Address feedback
jcouv Apr 29, 2025
3cc3d48
Add assertion
jcouv Apr 29, 2025
1c1e2a4
Comment
jcouv Apr 30, 2025
0125e74
Tweak DEBUG check
jcouv Apr 30, 2025
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
29 changes: 24 additions & 5 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 @@ -1213,7 +1213,7 @@ public override Binder VisitXmlNameAttribute(XmlNameAttributeSyntax parent)
{
case XmlNameAttributeElementKind.Parameter:
case XmlNameAttributeElementKind.ParameterReference:
result = GetParameterNameAttributeValueBinder(memberSyntax, result);
result = GetParameterNameAttributeValueBinder(memberSyntax, isParamRef: elementKind == XmlNameAttributeElementKind.ParameterReference, nextBinder: result);
break;
case XmlNameAttributeElementKind.TypeParameter:
result = GetTypeParameterNameAttributeValueBinder(memberSyntax, includeContainingSymbols: false, nextBinder: result);
Expand All @@ -1234,16 +1234,28 @@ public override Binder VisitXmlNameAttribute(XmlNameAttributeSyntax parent)
/// We're in a <param> or <paramref> element, so we want a binder that can see
/// the parameters of the associated member and nothing else.
/// </summary>
private Binder GetParameterNameAttributeValueBinder(MemberDeclarationSyntax memberSyntax, Binder nextBinder)
private Binder GetParameterNameAttributeValueBinder(MemberDeclarationSyntax memberSyntax, bool isParamRef, Binder nextBinder)
{
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 •
edited
Loading

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.

{
nextBinder = new WithExtensionParameterBinder(method.ContainingType, nextBinder);
}

return new WithParametersBinder(method.Parameters, nextBinder);
}

if (memberSyntax is TypeDeclarationSyntax { ParameterList: { ParameterCount: > 0 } } typeDeclaration)
else if (memberSyntax is ExtensionDeclarationSyntax extensionDeclaration)
{
Binder outerBinder = VisitCore(memberSyntax);
SourceNamedTypeSymbol type = ((NamespaceOrTypeSymbol)outerBinder.ContainingMemberOrLambda).GetSourceTypeMember((TypeDeclarationSyntax)memberSyntax);
Debug.Assert(type.IsExtension);
return new WithExtensionParameterBinder(type, nextBinder);
}
else if (memberSyntax is TypeDeclarationSyntax { ParameterList: { ParameterCount: > 0 } } typeDeclaration)
{
_ = typeDeclaration.ParameterList;
Binder outerBinder = VisitCore(memberSyntax);
Expand All @@ -1268,6 +1280,11 @@ private Binder GetParameterNameAttributeValueBinder(MemberDeclarationSyntax memb

ImmutableArray<ParameterSymbol> parameters = property.Parameters;

if (isParamRef && property.TryGetInstanceExtensionParameter(out var _))
{
nextBinder = new WithExtensionParameterBinder(property.ContainingType, nextBinder);
}

// BREAK: Dev11 also allows "value" for readonly properties, but that doesn't
// make sense and we don't have a symbol.
if ((object)property.SetMethod != null)
Expand All @@ -1278,8 +1295,10 @@ private Binder GetParameterNameAttributeValueBinder(MemberDeclarationSyntax memb

if (parameters.Any())
{
return new WithParametersBinder(parameters, nextBinder);
nextBinder = new WithParametersBinder(parameters, nextBinder);
}

return nextBinder;
}
else if (memberKind == SyntaxKind.DelegateDeclaration)
{
Expand Down
41 changes: 41 additions & 0 deletions src/Compilers/CSharp/Portable/Compiler/DocumentationCommentCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,47 @@ public override void VisitNamedType(NamedTypeSymbol symbol)
}

#nullable enable
public override void VisitMethod(MethodSymbol symbol)
{
if (symbol is SourceExtensionImplementationMethodSymbol implementation)
{
MethodSymbol underlyingMethod = implementation.UnderlyingMethod;
Symbol symbolForDocComment = underlyingMethod.IsAccessor()
? underlyingMethod.AssociatedSymbol
: underlyingMethod;

if (!hasDocumentationTrivia(symbolForDocComment))
{
return;
}

WriteLine("<member name=\"{0}\">", symbol.GetDocumentationCommentId());
Indent();
WriteLine("<inheritdoc cref=\"{0}\"/>", symbolForDocComment.GetDocumentationCommentId());
Unindent();
WriteLine("</member>");
return;
}

base.VisitMethod(symbol);
return;

static bool hasDocumentationTrivia(Symbol symbol)
{
foreach (SyntaxReference reference in symbol.DeclaringSyntaxReferences)
{
foreach (var trivia in reference.GetSyntax().GetLeadingTrivia())
{
if (trivia.IsDocumentationCommentTrivia)
{
return true;
}
}
}

return false;
}
}

/// <summary>
/// Compile documentation comments on the symbol and write them to the stream if one is provided.
Expand Down
2 changes: 1 addition & 1 deletion ...pilers/CSharp/Portable/DocumentationComments/DocumentationCommentIDVisitor.PartVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ public override object VisitNamedType(NamedTypeSymbol symbol, StringBuilder buil
builder.Append('.');
}

builder.Append(symbol.Name);
builder.Append(symbol.IsExtension ? symbol.ExtensionName : symbol.Name);

if (symbol.Arity != 0)
{
Expand Down
20 changes: 18 additions & 2 deletions ...Compilers/CSharp/Portable/Symbols/Extensions/SourceExtensionImplementationMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,17 @@

using System.Collections.Immutable;
using System.Diagnostics;
using System.Globalization;
using System.Threading;
using Microsoft.CodeAnalysis.CSharp.Emit;
using Microsoft.CodeAnalysis.PooledObjects;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
internal sealed class SourceExtensionImplementationMethodSymbol : RewrittenMethodSymbol // Tracked by https://github.com/dotnet/roslyn/issues/76130 : Do we need to implement ISynthesizedMethodBodyImplementationSymbol?
{
private string? lazyDocComment;

public SourceExtensionImplementationMethodSymbol(MethodSymbol sourceMethod)
: base(sourceMethod, TypeMap.Empty, sourceMethod.ContainingType.TypeParameters.Concat(sourceMethod.TypeParameters))
{
Expand Down Expand Up @@ -57,8 +61,6 @@ internal override int ParameterCount
public sealed override DllImportData? GetDllImportData() => null;
internal sealed override bool IsExternal => false;

// Tracked by https://github.com/dotnet/roslyn/issues/76130 : How doc comments are supposed to work? GetDocumentationCommentXml

internal sealed override bool IsDeclaredReadOnly => false;

public sealed override Symbol ContainingSymbol => _originalMethod.ContainingType.ContainingSymbol;
Expand Down Expand Up @@ -131,6 +133,20 @@ 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 •
edited
Loading

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

{
// Neither the culture nor the expandIncludes affect the XML for extension implementation methods.
string result = SourceDocumentationCommentUtils.GetAndCacheDocumentationComment(this, expandIncludes: false, ref lazyDocComment);

#if DEBUG
string? ignored = null;
string withIncludes = SourceDocumentationCommentUtils.GetAndCacheDocumentationComment(this, expandIncludes: true, lazyXmlText: ref ignored);
Debug.Assert(string.Equals(result, withIncludes, System.StringComparison.Ordinal));
#endif

return result;
}

private sealed class ExtensionMetadataMethodParameterSymbol : RewrittenMethodParameterSymbol
{
public ExtensionMetadataMethodParameterSymbol(SourceExtensionImplementationMethodSymbol containingMethod, ParameterSymbol sourceParameter) :
Expand Down
Loading