Extensions: update xml docs to new metadata design #79641

jcouv · 2025-07-28T22:32:59Z

Docs on extension blocks are attached to the corresponding extension marker type (docID is <ContainingType>.<GroupingType>.<MarkerType>).
Docs on equivalent extensions blocks (ie. same marker name) are merged silently.

Closes #79043 (fixed by new metadata design change, which uses marker names as grouping keys)
Relates to test plan #76130

jcouv · 2025-07-29T01:50:37Z

@jjonescz @dotnet/roslyn-compiler for review. Thanks

AlekseyTs · 2025-07-30T03:24:31Z

src/Compilers/CSharp/Portable/Symbols/Source/ExtensionGroupingInfo.cs

            throw ExceptionUtilities.Unreachable();
        }

+        internal ImmutableArray<SourceNamedTypeSymbol> GetMatchingExtensions(SourceNamedTypeSymbol extension)


GetMatchingExtensions

Minor, perhaps the name is not very specific. #Closed

AlekseyTs · 2025-07-30T03:30:45Z

src/Compilers/CSharp/Portable/Symbols/Source/ExtensionGroupingInfo.cs

+                    ArrayBuilder<SourceNamedTypeSymbol> extensions = sortSourceNamedTypeSymbols(markerMap[markerName]);
+                    yield return extensions;
+
+                    extensions.Free();


extensions.Free();

This is very fragile, we don't know whether the consumer is done with the builder. Should find other ways to manage builders. Perhaps this method shouldn't allocate new builder (if it is done for sorting, that could be consumer's responsibility), or it should return a builder of builders and the consumer will be responsible to free them once done. #Closed

Alternatively, consumer can pass a delegate that will be executed for each builder and then the builder will be freed.

AlekseyTs · 2025-07-30T03:33:14Z

src/Compilers/CSharp/Portable/Symbols/Source/ExtensionGroupingInfo.cs

+                get
+                {
+                    Debug.Assert(ExtensionMarkerTypes[0].UnderlyingExtensions[0].ContainingType is not null);
+                    return ExtensionMarkerTypes[0].UnderlyingExtensions[0].ContainingType!.GetCciAdapter();


!

Is the suppression still needed? #Closed

Yes, because of the array access

AlekseyTs · 2025-07-30T03:52:39Z

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

+                        firstExtension = extension;
+                    }
+
+                    if (!TryGetDocumentationCommentNodes(extension, out var maxDocumentationMode, out var foundDocCommentNodes))


var

Consider spelling out the type #Closed

AlekseyTs · 2025-07-30T04:09:30Z

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

+
+                if (documentedTypeParameters != null)
+                {
+                    PooledHashSet<string> documentedTypeParameterNames = PooledHashSet<string>.GetInstance();


documentedTypeParameterNames

Does it make sense to build this set from the start, or there are other consumers that should use the original set? #Resolved

Could you provide details why the switch to hashset of names was reverted?

It caused a regression. I captured the scenario in XmlDoc_16 where we would have spurious WRN_DuplicateTypeParamTag. I'm punting on resolving this for now (added tracking issue to XmlDoc_05)

AlekseyTs · 2025-07-30T04:15:03Z

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

+                    Debug.Assert(extension.IsExtension);
+                    if (firstExtension is null)
+                    {
+                        firstExtension = extension;


firstExtension = extension;

It looks like the input might not be sorted, therefore the "first" might not be deterministic. #Closed

I made this comment because GetMatchingExtensions doesn't explicitly sort the array before returning, but it looks like the array was sorted as part of its creation. It feels confusing though that two APIs return the "same" sequence of types as different collections types.

AlekseyTs · 2025-07-30T16:08:51Z

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

+            if (symbol.IsExtension && (SourceNamedTypeSymbol)symbol.ContainingType is { } containingType)
+            {
+                ImmutableArray<SourceNamedTypeSymbol> extensions = containingType.GetExtensionGroupingInfo().GetMatchingExtensions((SourceNamedTypeSymbol)symbol);
+                appendMergedExtensionBlocks(extensions);


appendMergedExtensionBlocks

It would be good to add a comment about scenario when this code path is reachable. #Closed

AlekseyTs · 2025-07-30T16:15:50Z

src/Compilers/CSharp/Portable/Symbols/Source/ExtensionGroupingInfo.cs

+
+                foreach (var markerName in markerNames)
+                {
+                    ArrayBuilder<SourceNamedTypeSymbol> extensions = sortSourceNamedTypeSymbols(markerMap[markerName]);


sortSourceNamedTypeSymbols(markerMap[markerName]);

Note that GetMatchingExtensions effectively gets the same set as an immutable array, which is already sorted in lexical declaration order. #Closed

Also, note that _lazyGroupingTypes already gives grouping types in deterministic, sorted order, and each of them provides ExtensionMarkerTypes in deterministic, sorted order, and each of them has UnderlyingExtensions in the desired sorted order. BTW, GetMatchingExtensions uses them instead.

Thanks. Simplified

AlekseyTs · 2025-07-30T16:29:28Z

            return _lazyUncommonProperties.extensionInfo.GroupingTypeSymbol.MetadataName; // PROTOTYPE: is this the right value to return?

It looks like the comment can be removed now. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs:2707 in e5261d8. [](commit_id = e5261d8, deletion_comment = False)

AlekseyTs · 2025-07-30T16:41:49Z

src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedNamedTypeSymbol.cs

        }

-        internal override string ExtensionName
+        internal override string ExtensionGroupingName


internal override string ExtensionGroupingName

I think it will be better to not override these APIs in this class. #Closed

AlekseyTs · 2025-07-30T16:48:30Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

+
+        var e = comp.GetMember<NamedTypeSymbol>("E");
+
+        var extensions = e.GetTypeMembers().ToArray();


extensions

Is this local used? #Closed

AlekseyTs · 2025-07-30T16:50:43Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

+        AssertEx.Equal("T:E.<Extension>$C43E2675C7BBF9284AF22FB8A9BF0280.<Extension>$8048A6C8BE30A622530249B904B537EB.<Marker>$D1693D81A12E8DED4ED68FE22D9E856F",
+            nestedExtension.GetDocumentationCommentId());
+
+        // PROTOTYPE should we be merging these nested extension blocks instead?


// PROTOTYPE should we be merging these nested extension blocks instead?

I do not think this error scenario worth the effort #Closed

AlekseyTs · 2025-07-30T16:55:10Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

+    }
+
+    /// <typeparam name="T2">Description for T2</typeparam>
+    extension<T2, U>(C<T2, U> c)


T2

Do we test scenario where this type parameter ins named "T1" (to force merging with the previous extension)? #Closed

XmlDoc_05. Note: it was fixed by tracking type parameters by name (it now reports a warning for documenting type parameter twice)

Do we test scenario where this type parameter ins named "T1" (to force merging with the previous extension)?

XmlDoc_05. Note: it was fixed by tracking type parameters by name (it now reports a warning for documenting type parameter twice)

I assume the answer is "No" then. It appears this test is focusing on a different aspect, ErrorCode.WRN_MissingTypeParamTag.

AlekseyTs · 2025-07-30T16:58:47Z

Done with review pass (commit 1) #Closed

jcouv · 2025-07-30T17:45:52Z

            return _lazyUncommonProperties.extensionInfo.GroupingTypeSymbol.MetadataName; // PROTOTYPE: is this the right value to return?

It's removed in next PR where I also added explicit test for this scenario/path

In reply to: 3137046726

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs:2707 in e5261d8. [](commit_id = e5261d8, deletion_comment = False)

jcouv · 2025-07-30T18:59:13Z

src/Compilers/CSharp/Portable/Symbols/ConstructedNamedTypeSymbol.cs

        }
+
+        internal override string ExtensionGroupingName
+            => _underlyingType.ExtensionGroupingName;


It feels like the concept of grouping and marker name should only be applicable to definitions. Maybe we could avoid implementation here by adjusting callers (symbol display and docID). What do you think? #Resolved

I think the implementation is simple enough to not introduce additional complexity on consumer's side.

jcouv · 2025-07-30T19:03:34Z

@jjonescz for second review. Thanks

AlekseyTs · 2025-07-31T01:28:19Z

@jcouv It looks like there are legitimate test failures

AlekseyTs · 2025-07-31T01:38:35Z

    var extensions = e.GetTypeMembers().ToArray();

Isn't this an array already? #Closed

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:48220 in c556788. [](commit_id = c556788, deletion_comment = False)

AlekseyTs · 2025-07-31T01:43:14Z

src/Compilers/CSharp/Portable/Symbols/Source/ExtensionGroupingInfo.cs

            throw ExceptionUtilities.Unreachable();
        }

+        // Given an extension block, returns all the extensions that are grouped together with it


//

Consider using standard doc comment #Closed

AlekseyTs · 2025-07-31T01:52:36Z

src/Compilers/CSharp/Portable/Symbols/ConstructedNamedTypeSymbol.cs

            get { throw ExceptionUtilities.Unreachable(); }
        }
+
+        internal override string ExtensionGroupingName


internal override string ExtensionGroupingName

I think the implementation in SubstitutedNestedTypeSymbol should be sealed instead. #Closed

AlekseyTs · 2025-07-31T01:55:24Z

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


            private int _nextSourceIncludeElementIndex;
            private HashSet<Location> _inProgressIncludeElementNodes;
            private HashSet<ParameterSymbol> _documentedParameters;


_documentedParameters

Is it possible that we might want to make the same change for this hashset? Consider opening a follow-up issue if so. #Closed

It's possible, but I'd rather not spend time to track this through now and if we open an issue to look at it later I doubt we'll get to it in MQ. So I prefer to let it be.

AlekseyTs · 2025-07-31T02:12:12Z

Done with review pass (commit 2) #Closed

jjonescz · 2025-07-31T08:53:33Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

                }

                return _lazyUncommonProperties.extensionInfo.GroupingTypeSymbol.MetadataName; // PROTOTYPE: is this the right value to return?
            }


Is the prototype comment still valid? #Resolved

It's removed in next PR where I also added explicit test for this scenario/path

jjonescz · 2025-07-31T08:55:15Z

src/Compilers/CSharp/Portable/Symbols/Source/ExtensionGroupingInfo.cs

+            return GetCorrespondingMarkerType(extension).UnderlyingExtensions;
+        }
+
+        // Returns all the extension blocks but grouped/merged by equivalency (ie. same marker name)


Consider turning this into a doc comment. #Resolved

jjonescz · 2025-07-31T08:55:44Z

src/Compilers/CSharp/Portable/Symbols/Source/ExtensionGroupingInfo.cs

+            {
+                get
+                {
+                    Debug.Assert(ExtensionMarkerTypes[0].UnderlyingExtensions[0].ContainingType is not null);


Consider extracting ExtensionMarkerTypes[0].UnderlyingExtensions[0].ContainingType into a variable and removing the suppression below. #Resolved

jjonescz · 2025-07-31T08:57:54Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol_Extension.cs

-            }
+                if (_lazyExtensionInfo.LazyExtensionMarkerName is null)
+                {
+                    _lazyExtensionInfo.LazyExtensionMarkerName = "<Marker>$" + RawNameToHashString(ComputeExtensionMarkerRawName()); // PROTOTYPE: Add a constant for this prefix.


I wonder whether we should choose shorter metadata prefixes like <E>$ and <M>$ (I'm not sure if even all the special characters are necessary, perhaps keep only the $) to avoid bloating up metadata sizes. #ByDesign

Seems reasonable. Please reply to the email thread I started on the naming convention topic if you want to suggest a change

AlekseyTs · 2025-07-31T16:57:18Z

        //// (13,20): warning CS1710: XML comment has a duplicate typeparam tag for 'T'

Is there a similar warning for parameters? If so, perhaps one should be expected in this scenario too. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:48213 in 7f20a26. [](commit_id = 7f20a26, deletion_comment = False)

AlekseyTs

LGTM (commit 6)

jcouv self-assigned this Jul 28, 2025

jcouv added Area-Compilers Feature - Extension Everything The extension everything feature labels Jul 28, 2025

jcouv force-pushed the extensions-xmldocs branch from f78dea1 to dc61bd5 Compare July 28, 2025 22:54

Extensions: update xml docs to new metadata design

e5261d8

jcouv force-pushed the extensions-xmldocs branch from dc61bd5 to e5261d8 Compare July 28, 2025 23:15

jcouv mentioned this pull request Jul 24, 2025

Extensions: follow-ups for metadata #78963

Open

28 tasks

jcouv marked this pull request as ready for review July 29, 2025 01:50

jcouv requested a review from a team as a code owner July 29, 2025 01:50

jcouv requested a review from jjonescz July 29, 2025 01:50

AlekseyTs reviewed Jul 30, 2025

View reviewed changes

Address feedback

c556788

jcouv commented Jul 30, 2025

View reviewed changes

jcouv requested a review from AlekseyTs July 30, 2025 19:02

AlekseyTs reviewed Jul 31, 2025

View reviewed changes

Julien Couvreur added 3 commits July 30, 2025 21:33

Address feedback

92978f2

Revert HashSet change

0cfcaec

Comment

2d37470

jjonescz reviewed Jul 31, 2025

View reviewed changes

Address feedback

7f20a26

jcouv requested review from AlekseyTs and jjonescz July 31, 2025 16:33

AlekseyTs approved these changes Jul 31, 2025

View reviewed changes

jjonescz approved these changes Jul 31, 2025

View reviewed changes

adjust comment

7badad1

jcouv merged commit 21dede8 into dotnet:features/extensions Jul 31, 2025
24 of 28 checks passed

jcouv deleted the extensions-xmldocs branch July 31, 2025 18:39

This was referenced Aug 19, 2025

[Automated] PRs inserted in VS build main-10918.105 #79952

Closed

[Automated] PRs inserted in VS build feature.debugger.main-10918.21 #79956

Closed

[Automated] PRs inserted in VS build feature.Wix5-10921.95 #80011

Closed


		var e = comp.GetMember<NamedTypeSymbol>("E");

		var extensions = e.GetTypeMembers().ToArray();

Extensions: update xml docs to new metadata design #79641

Extensions: update xml docs to new metadata design #79641

Uh oh!

Conversation

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

Uh oh!

Uh oh!

jcouv commented Jul 29, 2025

Uh oh!

AlekseyTs Jul 30, 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 Jul 30, 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 30, 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 30, 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 Jul 30, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 30, 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 30, 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 Jul 30, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jul 30, 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 Jul 30, 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 Jul 30, 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 Jul 30, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jcouv commented Jul 28, 2025 •

edited

Loading

AlekseyTs Jul 30, 2025 •

edited

Loading

AlekseyTs Jul 30, 2025 •

edited

Loading

AlekseyTs Jul 30, 2025 •

edited

Loading

AlekseyTs Jul 30, 2025 •

edited

Loading

AlekseyTs Jul 30, 2025 •

edited by jcouv

Loading

AlekseyTs Jul 30, 2025 •

edited

Loading

AlekseyTs Jul 30, 2025 •

edited

Loading

AlekseyTs Jul 30, 2025 •

edited

Loading

AlekseyTs commented Jul 30, 2025 •

edited

Loading

AlekseyTs Jul 30, 2025 •

edited

Loading

AlekseyTs Jul 30, 2025 •

edited

Loading

AlekseyTs Jul 30, 2025 •

edited

Loading

AlekseyTs Jul 30, 2025 •

edited

Loading

AlekseyTs commented Jul 30, 2025 •

edited

Loading

jcouv Jul 30, 2025 •

edited

Loading

AlekseyTs commented Jul 31, 2025 •

edited

Loading

AlekseyTs Jul 31, 2025 •

edited

Loading

AlekseyTs Jul 31, 2025 •

edited

Loading

AlekseyTs Jul 31, 2025 •

edited

Loading

AlekseyTs commented Jul 31, 2025 •

edited

Loading

jjonescz Jul 31, 2025 •

edited by jcouv

Loading

jjonescz Jul 31, 2025 •

edited by jcouv

Loading

jjonescz Jul 31, 2025 •

edited by jcouv

Loading

jjonescz Jul 31, 2025 •

edited by jcouv

Loading