Extensions: address follow-ups for new metadata design

Author: Julien Couvreur

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

@@ -382,7 +382,7 @@ private static void GetDocumentsForMethodsAndNestedTypes(PooledHashSet<Cci.Debug
                                 switch (member.Kind)
                                 {
                                     case SymbolKind.NamedType:
-                                        if (!((NamedTypeSymbol)member).IsExtension) // PROTOTYPE: Figure out what to do about extensions, if anything
+                                        if (!((NamedTypeSymbol)member).IsExtension) // Tracked by https://github.com/dotnet/roslyn/issues/78963 : Figure out what to do about extensions, if anything
                                         {
                                             namespacesAndTypesToProcess.Push((NamespaceOrTypeSymbol)member);
                                         }

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

@@ -345,7 +345,7 @@ private void AddNameAndTypeArgumentsOrParameters(INamedTypeSymbol symbol)
             {
                 if (Format.CompilerInternalOptions.HasFlag(SymbolDisplayCompilerInternalOptions.UseMetadataMemberNames))
                 {
-                    // PROTOTYPE: What should we output as the name here
+                    // Tracked by https://github.com/dotnet/roslyn/issues/78957 : public API, should we output ExtensionMarkerName instead here?
                     var extensionIdentifier = underlyingTypeSymbol!.ExtensionGroupingName; // Tracked by https://github.com/dotnet/roslyn/issues/78957 : public API, use public API once it's available
                     Builder.Add(CreatePart(SymbolDisplayPartKind.ClassName, symbol, extensionIdentifier));
                 }

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

@@ -1012,9 +1012,11 @@ public override ImmutableArray<CSharpAttributeData> GetAttributes()
                 bool checkForRequiredMembers = this.ShouldCheckRequiredMembers() && this.ContainingType.HasAnyRequiredMembers;
                 bool isInstanceIncrementDecrementOrCompoundAssignmentOperator = SourceMethodSymbol.IsInstanceIncrementDecrementOrCompoundAssignmentOperator(this);
 
+                bool isNewExtensionMember = this.GetIsNewExtensionMember();
+
                 bool isExtensionMethod = false;
                 bool isReadOnly = false;
-                if (checkForExtension || checkForIsReadOnly || checkForRequiredMembers || isInstanceIncrementDecrementOrCompoundAssignmentOperator)
+                if (checkForExtension || checkForIsReadOnly || checkForRequiredMembers || isInstanceIncrementDecrementOrCompoundAssignmentOperator || isNewExtensionMember)
                 {
                     attributeData = containingPEModuleSymbol.GetCustomAttributesForToken(_handle,
                         filteredOutAttribute1: out CustomAttributeHandle extensionAttribute,
@@ -1026,7 +1028,7 @@ public override ImmutableArray<CSharpAttributeData> GetAttributes()
                         filteredOutAttribute4: out _,
                         filterOut4: (checkForRequiredMembers && ObsoleteAttributeData is null) ? AttributeDescription.ObsoleteAttribute : default,
                         filteredOutAttribute5: out _,
-                        filterOut5: default,
+                        filterOut5: AttributeDescription.ExtensionMarkerNameAttribute,
                         filteredOutAttribute6: out _,
                         filterOut6: default);
 
@@ -1483,7 +1485,7 @@ internal override UseSiteInfo<AssemblySymbol> GetUseSiteInfo()
 
                 if (_containingType.IsExtension &&
                     (TryGetCorrespondingExtensionImplementationMethod() is null ||
-                    _containingType.GetUseSiteInfo().DiagnosticInfo?.DefaultSeverity == DiagnosticSeverity.Error)) // PROTOTYPE: Cover mismatch in type parameters between grouping and marker method with a test (See usage of MatchesContainingTypeParameters helper).
+                    _containingType.GetUseSiteInfo().DiagnosticInfo?.DefaultSeverity == DiagnosticSeverity.Error))
                 {
                     diagnosticInfo = new CSDiagnosticInfo(ErrorCode.ERR_BindToBogus, this);
                 }

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

@@ -936,7 +936,6 @@ public override ImmutableArray<CSharpAttributeData> GetAttributes()
                 if (IsExtension)
                 {
                     // We do not recognize any attributes on extension blocks
-                    // PROTOTYPE: Add a test demonstrating the fact that we do not load attributes from grouping and marker types.
                     loadedCustomAttributes = [];
                     requiredHandle = default;
                 }
@@ -1863,7 +1862,7 @@ public override string Name
         {
             get
             {
-                return IsExtension ? "" : _name; // PROTOTYPE: add test coverage
+                return IsExtension ? "" : _name;
             }
         }
 
@@ -1874,7 +1873,7 @@ public override string MetadataName
                 if (IsExtension)
                 {
                     Debug.Assert(!MangleName);
-                    return _name; // PROTOTYPE: add test coverage
+                    return _name;
                 }
 
                 return base.MetadataName;
@@ -2165,7 +2164,6 @@ private IEnumerable<PENamedTypeSymbol> CreateNestedTypes()
             if (IsExtension)
             {
                 // We do not support type declarations in extension blocks
-                // PROTOTYPE: Add a test demonstrating the fact that we do not load nested types from grouping and marker types.
                 yield break;
             }
 
@@ -2183,7 +2181,7 @@ private IEnumerable<PENamedTypeSymbol> CreateNestedTypes()
                 yield break;
             }
 
-            // PROTOTYPE: test effect of every condition here
+            // Tracked by https://github.com/dotnet/roslyn/issues/78963 : test effect of every condition here
             bool checkForExtensionGroup = this.IsStatic && this.ContainingType is null && this.TypeKind == TypeKind.Class &&
                                           module.HasExtensionAttribute(_handle, ignoreCase: false) && !this.IsGenericType;
 
@@ -2203,7 +2201,7 @@ private IEnumerable<PENamedTypeSymbol> CreateNestedTypes()
             {
                 var type = PENamedTypeSymbol.Create(moduleSymbol, this, typeRid);
 
-                // PROTOTYPE: test effect of every condition here
+                // Tracked by https://github.com/dotnet/roslyn/issues/78963 : test effect of every condition here
                 if (checkForExtensionGroup &&
                     type.DeclaredAccessibility == Accessibility.Public &&
                     type.HasSpecialName && type.IsSealed && module.HasExtensionAttribute(type.Handle, ignoreCase: false) &&
@@ -2226,7 +2224,7 @@ private IEnumerable<PENamedTypeSymbol> CreateNestedTypes()
                     {
                         var marker = PENamedTypeSymbol.Create(moduleSymbol, type, markerRid);
 
-                        // PROTOTYPE: test effect of every condition here
+                        // Tracked by https://github.com/dotnet/roslyn/issues/78963 : test effect of every condition here
                         if (marker.HasSpecialName && marker.IsStatic && marker.DeclaredAccessibility == Accessibility.Public &&
                             marker.TypeKind == TypeKind.Class && marker.BaseTypeNoUseSiteDiagnostics.IsObjectType() && marker.Arity == 0 &&
                             marker.InterfacesNoUseSiteDiagnostics().IsEmpty &&
@@ -2254,7 +2252,6 @@ private MultiDictionary<string, PEFieldSymbol> CreateFields(ArrayBuilder<PEField
 
             if (IsExtension)
             {
-                // PROTOTYPE: Add a test demonstrating the fact that we do not load fields from grouping and marker types.
                 return privateFieldNameToSymbols;
             }
 
@@ -2352,16 +2349,13 @@ private PooledDictionary<MethodDefinitionHandle, PEMethodSymbol> CreateMethods(A
             var isOrdinaryEmbeddableStruct = (this.TypeKind == TypeKind.Struct) && (this.SpecialType == Microsoft.CodeAnalysis.SpecialType.None) && this.ContainingAssembly.IsLinked;
             bool isExtension = IsExtension;
 
-            // PROTOTYPE: Add a test demonstrating the fact that we do not load methods from marker types into the extension blocks.
-
             try
             {
                 foreach (var methodHandle in module.GetMethodsOfTypeOrThrow(isExtension ? _lazyUncommonProperties.extensionInfo.GroupingTypeSymbol.Handle : _handle))
                 {
                     if (isOrdinaryEmbeddableStruct || module.ShouldImportMethod(_handle, methodHandle, moduleSymbol.ImportOptions))
                     {
-                        // PROTOTYPE: Would it be worth building a map across all methods to optimize this?
-                        // PROTOTYPE: Should we strip the ExtensionMarkerNameAttribute from GetAttributes()?
+                        // Tracked by https://github.com/dotnet/roslyn/issues/78827 : optimization, would it be worth building a map across all methods to optimize this?
                         if (isExtension && (!module.HasExtensionMarkerNameAttribute(methodHandle, out string markerName) || markerName != MetadataName))
                         {
                             // Method doesn't belong to this extension block
@@ -2386,16 +2380,13 @@ private void CreateProperties(Dictionary<MethodDefinitionHandle, PEMethodSymbol>
             var module = moduleSymbol.Module;
             bool isExtension = IsExtension;
 
-            // PROTOTYPE: Add a test demonstrating the fact that we do not load properties from marker types into extension blocks.
-
             try
             {
                 foreach (var propertyDef in module.GetPropertiesOfTypeOrThrow(isExtension ? _lazyUncommonProperties.extensionInfo.GroupingTypeSymbol.Handle : _handle))
                 {
                     try
                     {
-                        // PROTOTYPE: Would it be worth building a map across all properties to optimize this?
-                        // PROTOTYPE: Should we strip the ExtensionMarkerNameAttribute from GetAttributes()?
+                        // Tracked by https://github.com/dotnet/roslyn/issues/78827 : optimization, would it be worth building a map across all properties to optimize this?
                         if (isExtension && (!module.HasExtensionMarkerNameAttribute(propertyDef, out string markerName) || markerName != MetadataName))
                         {
                             // Property doesn't belong to this extension block
@@ -2428,7 +2419,6 @@ private void CreateEvents(
             if (IsExtension)
             {
                 // We do not support event declarations in extension blocks
-                // PROTOTYPE: Add a test demonstrating the fact that we do not load events from grouping and marker types.
                 return;
             }
 
@@ -2704,7 +2694,7 @@ internal override string ExtensionGroupingName
                     throw ExceptionUtilities.Unreachable();
                 }
 
-                return _lazyUncommonProperties.extensionInfo.GroupingTypeSymbol.MetadataName; // PROTOTYPE: is this the right value to return?
+                return _lazyUncommonProperties.extensionInfo.GroupingTypeSymbol.MetadataName;
             }
         }
 
@@ -3149,7 +3139,6 @@ protected override DiagnosticInfo GetUseSiteDiagnosticImpl()
                     }
                     else if (IsExtension && !((PENamedTypeSymbolGeneric)_lazyUncommonProperties.extensionInfo.MarkerTypeSymbol).MatchesContainingTypeParameters())
                     {
-                        // PROTOTYPE: Cover this code path with a test.
                         diagnostic = new CSDiagnosticInfo(ErrorCode.ERR_BogusType, this);
                     }
                 }

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

@@ -738,7 +738,15 @@ public override ImmutableArray<CSharpAttributeData> GetAttributes()
                       out _,
                       this.RefKind == RefKind.RefReadOnly ? AttributeDescription.IsReadOnlyAttribute : default,
                       out CustomAttributeHandle required,
-                      AttributeDescription.RequiredMemberAttribute);
+                      AttributeDescription.RequiredMemberAttribute,
+                      out _,
+                      this.GetIsNewExtensionMember() ? AttributeDescription.ExtensionMarkerNameAttribute : default,
+                      out _,
+                      default,
+                      out _,
+                      default,
+                      out _,
+                      default);
 
                 if (!attributes.IsEmpty)
                 {

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

@@ -137,7 +137,7 @@ public Cci.TypeMemberVisibility GetCorrespondingMarkerMethodVisibility(Synthesiz
                 }
             }
 
-            // PROTOTYPE: Is there a real need to cache this result for reuse?
+            // Tracked by https://github.com/dotnet/roslyn/issues/78827 : optimization, Is there a real need to cache this result for reuse?
             return result;
         }
 
@@ -508,7 +508,6 @@ public override IEnumerable<ICustomAttribute> GetAttributes(EmitContext context)
                 // Preserve only the synthesized IsUnmanagedAttribute.
                 if (MustBeValueType)
                 {
-                    // PROTOTYPE: Make sure we have coverage for the WellKnownMember.System_Runtime_CompilerServices_IsUnmanagedAttribute__ctor case
                     var unmanagedCtor = ((PEModuleBuilder)context.Module).TryGetSynthesizedIsUnmanagedAttribute()?.Constructors[0] ??
                         ((ExtensionGroupingType)DefiningType).ExtensionMarkerTypes[0].UnderlyingExtensions[0].DeclaringCompilation.
                             GetWellKnownTypeMember(WellKnownMember.System_Runtime_CompilerServices_IsUnmanagedAttribute__ctor);

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

@@ -212,7 +212,8 @@ protected override void DecodeWellKnownAttributeImpl(ref DecodeWellKnownAttribut
                 | ReservedAttributes.TupleElementNamesAttribute
                 | ReservedAttributes.NullableAttribute
                 | ReservedAttributes.NativeIntegerAttribute
-                | ReservedAttributes.RequiredMemberAttribute))
+                | ReservedAttributes.RequiredMemberAttribute
+                | ReservedAttributes.ExtensionMarkerNameAttribute))
             {
             }
             else if (attribute.IsTargetAttribute(AttributeDescription.DateTimeConstantAttribute))

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

@@ -811,7 +811,8 @@ protected override void DecodeWellKnownAttributeImpl(ref DecodeWellKnownAttribut
                 ReservedAttributes.TupleElementNamesAttribute |
                 ReservedAttributes.NullableAttribute |
                 ReservedAttributes.NativeIntegerAttribute |
-                ReservedAttributes.ScopedRefAttribute))
+                ReservedAttributes.ScopedRefAttribute |
+                ReservedAttributes.ExtensionMarkerNameAttribute))
             {
             }
             else if (attribute.IsTargetAttribute(AttributeDescription.AllowNullAttribute))

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

@@ -350,7 +350,11 @@ protected sealed override void DecodeWellKnownAttributeImpl(ref DecodeWellKnownA
             {
                 arguments.GetOrCreateData<CommonEventWellKnownAttributeData>().HasSpecialNameAttribute = true;
             }
-            else if (ReportExplicitUseOfReservedAttributes(in arguments, ReservedAttributes.NullableAttribute | ReservedAttributes.NativeIntegerAttribute | ReservedAttributes.TupleElementNamesAttribute))
+            else if (ReportExplicitUseOfReservedAttributes(in arguments,
+                ReservedAttributes.NullableAttribute
+                | ReservedAttributes.NativeIntegerAttribute
+                | ReservedAttributes.TupleElementNamesAttribute
+                | ReservedAttributes.ExtensionMarkerNameAttribute))
             {
             }
             else if (attribute.IsTargetAttribute(AttributeDescription.ExcludeFromCodeCoverageAttribute))

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

@@ -5839,7 +5839,7 @@ private void AddAccessorIfAvailable(ArrayBuilder<Symbol> symbols, MethodSymbol?
         {
             if (IsExtension)
             {
-                // PROTOTYPE: Figure out how to calculate and emit this for extensions. 
+                // Tracked by https://github.com/dotnet/roslyn/issues/78828 : nullability, figure out how to calculate and emit this for extensions. 
                 //            We probably should do that per grouping type. Leaving as is should be fine too, I think.
                 //            Otherwise, marker method should be processed explicitly because it is not among members.
                 return null;