Address feedback

Author: Julien Couvreur

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

@@ -35,7 +35,7 @@ private class DocumentationCommentWalker : CSharpSyntaxWalker
             private readonly ArrayBuilder<CSharpSyntaxNode> _includeElementNodes;
 
             private HashSet<ParameterSymbol> _documentedParameters;
-            private HashSet<TypeParameterSymbol> _documentedTypeParameters;
+            private HashSet<string> _documentedTypeParameterNames;
 
             private DocumentationCommentWalker(
                 CSharpCompilation compilation,
@@ -44,7 +44,7 @@ private DocumentationCommentWalker(
                 StringWriter writer,
                 ArrayBuilder<CSharpSyntaxNode> includeElementNodes,
                 HashSet<ParameterSymbol> documentedParameters,
-                HashSet<TypeParameterSymbol> documentedTypeParameters)
+                HashSet<string> documentedTypeParameterNames)
                 : base(SyntaxWalkerDepth.StructuredTrivia)
             {
                 _compilation = compilation;
@@ -54,7 +54,7 @@ private DocumentationCommentWalker(
                 _includeElementNodes = includeElementNodes;
 
                 _documentedParameters = documentedParameters;
-                _documentedTypeParameters = documentedTypeParameters;
+                _documentedTypeParameterNames = documentedTypeParameterNames;
             }
 
             /// <summary>
@@ -71,7 +71,7 @@ public static void GetSubstitutedText(
                 StringBuilder builder)
             {
                 StringWriter writer = new StringWriter(builder, CultureInfo.InvariantCulture);
-                DocumentationCommentWalker walker = new DocumentationCommentWalker(compilation, BindingDiagnosticBag.Discarded, symbol, writer, includeElementNodes, documentedParameters: null, documentedTypeParameters: null);
+                DocumentationCommentWalker walker = new DocumentationCommentWalker(compilation, BindingDiagnosticBag.Discarded, symbol, writer, includeElementNodes, documentedParameters: null, documentedTypeParameterNames: null);
 
                 // Before: <param name="NAME">CONTENT</param>
                 // After: <summary>CONTENT</summary>
@@ -121,17 +121,17 @@ public static string GetSubstitutedText(
                 DocumentationCommentTriviaSyntax trivia,
                 ArrayBuilder<CSharpSyntaxNode> includeElementNodes,
                 ref HashSet<ParameterSymbol> documentedParameters,
-                ref HashSet<TypeParameterSymbol> documentedTypeParameters)
+                ref HashSet<string> documentedTypeParameterNames)
             {
                 PooledStringBuilder pooled = PooledStringBuilder.GetInstance();
                 using (StringWriter writer = new StringWriter(pooled.Builder, CultureInfo.InvariantCulture))
                 {
-                    DocumentationCommentWalker walker = new DocumentationCommentWalker(compilation, diagnostics, symbol, writer, includeElementNodes, documentedParameters, documentedTypeParameters);
+                    DocumentationCommentWalker walker = new DocumentationCommentWalker(compilation, diagnostics, symbol, writer, includeElementNodes, documentedParameters, documentedTypeParameterNames);
                     walker.Visit(trivia);
 
                     // Copy back out in case they have been initialized.
                     documentedParameters = walker._documentedParameters;
-                    documentedTypeParameters = walker._documentedTypeParameters;
+                    documentedTypeParameterNames = walker._documentedTypeParameterNames;
                 }
                 return pooled.ToStringAndFree();
             }
@@ -187,7 +187,7 @@ public override void DefaultVisit(SyntaxNode node)
                     Binder binder = factory.GetBinder(nameAttr, nameAttr.Identifier.SpanStart);
 
                     // Do this for diagnostics, even if we aren't writing.
-                    BindName(nameAttr, binder, _memberSymbol, ref _documentedParameters, ref _documentedTypeParameters, _diagnostics);
+                    BindName(nameAttr, binder, _memberSymbol, ref _documentedParameters, ref _documentedTypeParameterNames, _diagnostics);
 
                     // Do descend - we still need to write out the tokens of the attribute.
                 }

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

@@ -36,15 +36,15 @@ private class IncludeElementExpander
             private int _nextSourceIncludeElementIndex;
             private HashSet<Location> _inProgressIncludeElementNodes;
             private HashSet<ParameterSymbol> _documentedParameters;
-            private HashSet<TypeParameterSymbol> _documentedTypeParameters;
+            private HashSet<string> _documentedTypeParameterNames;
             private DocumentationCommentIncludeCache _includedFileCache;
 
             private IncludeElementExpander(
                 Symbol memberSymbol,
                 ImmutableArray<CSharpSyntaxNode> sourceIncludeElementNodes,
                 CSharpCompilation compilation,
                 HashSet<ParameterSymbol> documentedParameters,
-                HashSet<TypeParameterSymbol> documentedTypeParameters,
+                HashSet<string> documentedTypeParameterNames,
                 DocumentationCommentIncludeCache includedFileCache,
                 BindingDiagnosticBag diagnostics,
                 CancellationToken cancellationToken)
@@ -56,7 +56,7 @@ private IncludeElementExpander(
                 _cancellationToken = cancellationToken;
 
                 _documentedParameters = documentedParameters;
-                _documentedTypeParameters = documentedTypeParameters;
+                _documentedTypeParameterNames = documentedTypeParameterNames;
                 _includedFileCache = includedFileCache;
 
                 _nextSourceIncludeElementIndex = 0;
@@ -68,7 +68,7 @@ public static void ProcessIncludes(
                 ImmutableArray<CSharpSyntaxNode> sourceIncludeElementNodes,
                 CSharpCompilation compilation,
                 ref HashSet<ParameterSymbol> documentedParameters,
-                ref HashSet<TypeParameterSymbol> documentedTypeParameters,
+                ref HashSet<string> documentedTypeParameterNames,
                 ref DocumentationCommentIncludeCache includedFileCache,
                 TextWriter writer,
                 BindingDiagnosticBag diagnostics,
@@ -117,7 +117,7 @@ public static void ProcessIncludes(
                     sourceIncludeElementNodes,
                     compilation,
                     documentedParameters,
-                    documentedTypeParameters,
+                    documentedTypeParameterNames,
                     includedFileCache,
                     diagnostics,
                     cancellationToken);
@@ -135,7 +135,7 @@ public static void ProcessIncludes(
                 Debug.Assert(expander._nextSourceIncludeElementIndex == expander._sourceIncludeElementNodes.Length);
 
                 documentedParameters = expander._documentedParameters;
-                documentedTypeParameters = expander._documentedTypeParameters;
+                documentedTypeParameterNames = expander._documentedTypeParameterNames;
                 includedFileCache = expander._includedFileCache;
             }
 
@@ -537,7 +537,7 @@ private void BindName(XAttribute attribute, CSharpSyntaxNode originatingSyntax,
 
                 var nameDiagnostics = BindingDiagnosticBag.GetInstance(_diagnostics);
                 Binder binder = MakeNameBinder(isParameter, isTypeParameterRef, _memberSymbol, _compilation, originatingSyntax.SyntaxTree);
-                DocumentationCommentCompiler.BindName(attrSyntax, binder, _memberSymbol, ref _documentedParameters, ref _documentedTypeParameters, nameDiagnostics);
+                DocumentationCommentCompiler.BindName(attrSyntax, binder, _memberSymbol, ref _documentedParameters, ref _documentedTypeParameterNames, nameDiagnostics);
                 RecordBindingDiagnostics(nameDiagnostics, sourceLocation); // Respects DocumentationMode.
                 nameDiagnostics.Free();
             }

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

@@ -226,7 +226,8 @@ public override void VisitNamedType(NamedTypeSymbol symbol)
             // 1. visit the type
             if (symbol.IsExtension && (SourceNamedTypeSymbol)symbol.ContainingType is { } containingType)
             {
-                ImmutableArray<SourceNamedTypeSymbol> extensions = containingType.GetExtensionGroupingInfo().GetMatchingExtensions((SourceNamedTypeSymbol)symbol);
+                // We've been asked to generate the docs for a given extension block. We'll produce the merged docs for the merged blocks.
+                ImmutableArray<SourceNamedTypeSymbol> extensions = containingType.GetExtensionGroupingInfo().GetMergedExtensions((SourceNamedTypeSymbol)symbol);
                 appendMergedExtensionBlocks(extensions);
             }
             else
@@ -264,7 +265,7 @@ void appendContainedExtensions(NamedTypeSymbol containingType)
                 Debug.Assert(!_isForSingleSymbol);
                 ExtensionGroupingInfo extensionGroupingInfo = ((SourceMemberContainerTypeSymbol)containingType).GetExtensionGroupingInfo();
 
-                foreach (ArrayBuilder<SourceNamedTypeSymbol> extensions in extensionGroupingInfo.EnumerateMergedExtensionBlocks())
+                foreach (ImmutableArray<SourceNamedTypeSymbol> extensions in extensionGroupingInfo.EnumerateMergedExtensionBlocks())
                 {
                     appendMergedExtensionBlocks(extensions);
 
@@ -309,8 +310,12 @@ bool collectDocCommentNodes(IEnumerable<SourceNamedTypeSymbol> extensions, Array
                     {
                         firstExtension = extension;
                     }
+                    else
+                    {
+                        Debug.Assert(firstExtension.GetEscapedDocumentationCommentId() == extension.GetEscapedDocumentationCommentId());
+                    }
 
-                    if (!TryGetDocumentationCommentNodes(extension, out var maxDocumentationMode, out var foundDocCommentNodes))
+                    if (!TryGetDocumentationCommentNodes(extension, out DocumentationMode maxDocumentationMode, out ImmutableArray<DocumentationCommentTriviaSyntax> foundDocCommentNodes))
                     {
                         // If the XML in any of the doc comments is invalid, skip all further processing (for this symbol) and 
                         // just write a comment saying that info was lost for this symbol.
@@ -530,10 +535,10 @@ private void ProcessDocumentationCommentTriviaNodes(
         {
             string? withUnprocessedIncludes;
             bool haveParseError;
-            HashSet<TypeParameterSymbol>? documentedTypeParameters;
+            HashSet<string>? documentedTypeParameterNames;
             HashSet<ParameterSymbol>? documentedParameters;
             ImmutableArray<CSharpSyntaxNode> includeElementNodes;
-            if (!tryProcessDocumentationCommentTriviaNodes(symbol, shouldSkipPartialDefinitionComments, docCommentNodes, out withUnprocessedIncludes, out haveParseError, out documentedTypeParameters, out documentedParameters, out includeElementNodes))
+            if (!tryProcessDocumentationCommentTriviaNodes(symbol, shouldSkipPartialDefinitionComments, docCommentNodes, out withUnprocessedIncludes, out haveParseError, out documentedTypeParameterNames, out documentedParameters, out includeElementNodes))
             {
                 return;
             }
@@ -557,7 +562,7 @@ private void ProcessDocumentationCommentTriviaNodes(
                 // the formatting engine would have trouble determining what prefix to remove from each line.
                 TextWriter? expanderWriter = shouldSkipPartialDefinitionComments ? null : _writer; // Don't actually write partial method definition parts.
                 IncludeElementExpander.ProcessIncludes(withUnprocessedIncludes, symbol, includeElementNodes,
-                    _compilation, ref documentedParameters, ref documentedTypeParameters, ref _includedFileCache, expanderWriter, _diagnostics, _cancellationToken);
+                    _compilation, ref documentedParameters, ref documentedTypeParameterNames, ref _includedFileCache, expanderWriter, _diagnostics, _cancellationToken);
             }
             else if (_writer != null && !shouldSkipPartialDefinitionComments)
             {
@@ -588,14 +593,8 @@ private void ProcessDocumentationCommentTriviaNodes(
                     }
                 }
 
-                if (documentedTypeParameters != null)
+                if (documentedTypeParameterNames != null)
                 {
-                    PooledHashSet<string> documentedTypeParameterNames = PooledHashSet<string>.GetInstance();
-                    foreach (var documentedTypeParameter in documentedTypeParameters)
-                    {
-                        documentedTypeParameterNames.Add(documentedTypeParameter.Name);
-                    }
-
                     foreach (TypeParameterSymbol typeParameter in GetTypeParameters(symbol))
                     {
                         if (!documentedTypeParameterNames.Contains(typeParameter.Name))
@@ -606,8 +605,6 @@ private void ProcessDocumentationCommentTriviaNodes(
                             _diagnostics.Add(ErrorCode.WRN_MissingTypeParamTag, location, typeParameter, symbol);
                         }
                     }
-
-                    documentedTypeParameterNames.Free();
                 }
             }
             return;
@@ -625,7 +622,7 @@ bool tryProcessDocumentationCommentTriviaNodes(
                 ImmutableArray<DocumentationCommentTriviaSyntax> docCommentNodes,
                 [NotNullWhen(true)] out string? withUnprocessedIncludes,
                 out bool haveParseError,
-                out HashSet<TypeParameterSymbol>? documentedTypeParameters,
+                out HashSet<string>? documentedTypeParameterNames,
                 out HashSet<ParameterSymbol>? documentedParameters,
                 out ImmutableArray<CSharpSyntaxNode> includeElementNodes)
             {
@@ -636,7 +633,7 @@ bool tryProcessDocumentationCommentTriviaNodes(
                 ArrayBuilder<CSharpSyntaxNode>? includeElementNodesBuilder = null;
 
                 documentedParameters = null;
-                documentedTypeParameters = null;
+                documentedTypeParameterNames = null;
 
                 // Saw an XmlException while parsing one of the DocumentationCommentTriviaSyntax nodes.
                 haveParseError = false;
@@ -678,7 +675,7 @@ bool tryProcessDocumentationCommentTriviaNodes(
 
                     // Will respect the DocumentationMode.
                     string substitutedText = DocumentationCommentWalker.GetSubstitutedText(_compilation, _diagnostics, symbol, trivia,
-                        includeElementNodesBuilder, ref documentedParameters, ref documentedTypeParameters);
+                        includeElementNodesBuilder, ref documentedParameters, ref documentedTypeParameterNames);
 
                     string formattedXml = FormatComment(substitutedText);
 
@@ -1260,7 +1257,7 @@ private static void BindName(
             Binder binder,
             Symbol memberSymbol,
             ref HashSet<ParameterSymbol> documentedParameters,
-            ref HashSet<TypeParameterSymbol> documentedTypeParameters,
+            ref HashSet<string> documentedTypeParameterNames,
             BindingDiagnosticBag diagnostics)
         {
             XmlNameAttributeElementKind elementKind = syntax.GetElementKind();
@@ -1277,9 +1274,9 @@ private static void BindName(
             }
             else if (elementKind == XmlNameAttributeElementKind.TypeParameter)
             {
-                if (documentedTypeParameters == null)
+                if (documentedTypeParameterNames == null)
                 {
-                    documentedTypeParameters = new HashSet<TypeParameterSymbol>();
+                    documentedTypeParameterNames = new HashSet<string>();
                 }
             }
 
@@ -1335,9 +1332,9 @@ private static void BindName(
                     else if (elementKind == XmlNameAttributeElementKind.TypeParameter)
                     {
                         Debug.Assert(referencedSymbol.Kind == SymbolKind.TypeParameter);
-                        Debug.Assert(documentedTypeParameters != null);
+                        Debug.Assert(documentedTypeParameterNames != null);
 
-                        if (!documentedTypeParameters.Add((TypeParameterSymbol)referencedSymbol))
+                        if (!documentedTypeParameterNames.Add(((TypeParameterSymbol)referencedSymbol).Name))
                         {
                             diagnostics.Add(ErrorCode.WRN_DuplicateTypeParamTag, syntax.Location, identifier);
                         }

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

@@ -49,6 +49,12 @@ protected override NamedTypeSymbol WithTupleDataCore(TupleExtraData newData)
         {
             throw ExceptionUtilities.Unreachable();
         }
+
+        internal override string ExtensionGroupingName
+            => _underlyingType.ExtensionGroupingName;
+
+        internal override string ExtensionMarkerName
+            => _underlyingType.ExtensionMarkerName;
     }
 
     /// <summary>
@@ -134,5 +140,11 @@ public sealed override bool AreLocalsZeroed
         {
             get { throw ExceptionUtilities.Unreachable(); }
         }
+
+        internal override string ExtensionGroupingName
+            => _underlyingType.ExtensionGroupingName;
+
+        internal override string ExtensionMarkerName
+            => _underlyingType.ExtensionMarkerName;
     }
 }

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

@@ -300,6 +300,12 @@ internal sealed override bool HasAsyncMethodBuilderAttribute(out TypeSymbol? bui
             return false;
         }
 
+        internal override string ExtensionGroupingName
+            => throw ExceptionUtilities.Unreachable();
+
+        internal override string ExtensionMarkerName
+            => throw ExceptionUtilities.Unreachable();
+
         private sealed class NativeIntegerTypeMap : AbstractTypeMap
         {
             private readonly NativeIntegerTypeSymbol _type;

src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingNamedTypeSymbol.cs

@@ -471,5 +471,11 @@ internal sealed override bool HasAsyncMethodBuilderAttribute(out TypeSymbol? bui
         }
 
         internal override bool HasCompilerLoweringPreserveAttribute => _underlyingType.HasCompilerLoweringPreserveAttribute;
+
+        internal override string ExtensionGroupingName
+            => _underlyingType.ExtensionGroupingName;
+
+        internal override string ExtensionMarkerName
+            => _underlyingType.ExtensionMarkerName;
     }
 }