Extensions: check for marker collisions

Author: Julien Couvreur

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -8218,4 +8218,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
   <data name="ERR_InterpolatedStringHandlerArgumentDisallowed" xml:space="preserve">
     <value>Interpolated string handler arguments are not allowed in this context.</value>
   </data>
+  <data name="ERR_ExtensionBlockCollision" xml:space="preserve">
+    <value>This extension block collides with another extension block. They have different signatures, but result in the same grouping or marker type identifier.</value>
+  </data>
 </root>

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -2420,6 +2420,7 @@ internal enum ErrorCode
         ERR_InstanceOperatorExtensionWrongReceiverType = 9323,
         ERR_ExpressionTreeContainsExtensionBasedConditionalLogicalOperator = 9324,
         ERR_InterpolatedStringHandlerArgumentDisallowed = 9325,
+        ERR_ExtensionBlockCollision = 9326,
 
         // Note: you will need to do the following after adding errors:
         //  1) Update ErrorFacts.IsBuildOnlyDiagnostic (src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs)

src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs

@@ -2532,6 +2532,7 @@ or ErrorCode.ERR_InstanceOperatorStructExtensionWrongReceiverRefKind
                 or ErrorCode.ERR_InstanceOperatorExtensionWrongReceiverType
                 or ErrorCode.ERR_ExpressionTreeContainsExtensionBasedConditionalLogicalOperator
                 or ErrorCode.ERR_InterpolatedStringHandlerArgumentDisallowed
+                or ErrorCode.ERR_ExtensionBlockCollision
                     => false,
             };
 #pragma warning restore CS8524 // The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value.

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

@@ -135,10 +135,11 @@ internal sealed class MemberSignatureComparer : IEqualityComparer<Symbol>
             considerName: true,
             considerExplicitlyImplementedInterfaces: true,
             considerReturnType: true,
-            considerTypeConstraints: false,
+            considerTypeConstraints: false, // constraints are checked by caller instead
             considerCallingConvention: false,
             refKindCompareMode: RefKindCompareMode.ConsiderDifferences,
-            typeComparison: TypeCompareKind.ObliviousNullableModifierMatchesAny);
+            typeComparison: TypeCompareKind.ObliviousNullableModifierMatchesAny,
+            considerParameterNames: true);
 
         /// <summary>
         /// Determines if an interceptor has a compatible signature with an interceptable method.
@@ -354,6 +355,21 @@ internal sealed class MemberSignatureComparer : IEqualityComparer<Symbol>
             considerDefaultValues: true,
             typeComparison: TypeCompareKind.AllIgnoreOptions);
 
+        /// <summary>
+        /// This instance is used to determine if two extension blocks match in C# sense and thus are allowed to share a marker type.
+        /// It considers parameter and type parameter names, type constraints, nullability and attributes.
+        /// </summary>
+        public static readonly MemberSignatureComparer ExtensionSignatureComparer = new MemberSignatureComparer(
+            considerName: false,
+            considerExplicitlyImplementedInterfaces: false,
+            considerReturnType: false,
+            considerTypeConstraints: true,
+            considerCallingConvention: false,
+            refKindCompareMode: RefKindCompareMode.ConsiderDifferences,
+            typeComparison: TypeCompareKind.ConsiderEverything,
+            considerParameterNames: true,
+            considerAttributes: true);
+
         // Compare the "unqualified" part of the member name (no explicit part)
         private readonly bool _considerName;
 
@@ -380,6 +396,11 @@ internal sealed class MemberSignatureComparer : IEqualityComparer<Symbol>
         // Equality options for parameter types and return types (if return is considered).
         private readonly TypeCompareKind _typeComparison;
 
+        // Compare parameter and type parameter names
+        private readonly bool _considerParameterNames;
+
+        private readonly bool _considerAttributes;
+
         private MemberSignatureComparer(
             bool considerName,
             bool considerExplicitlyImplementedInterfaces,
@@ -389,7 +410,9 @@ private MemberSignatureComparer(
             RefKindCompareMode refKindCompareMode,
             bool considerArity = true,
             bool considerDefaultValues = false,
-            TypeCompareKind typeComparison = TypeCompareKind.IgnoreDynamic | TypeCompareKind.IgnoreNativeIntegers)
+            TypeCompareKind typeComparison = TypeCompareKind.IgnoreDynamic | TypeCompareKind.IgnoreNativeIntegers,
+            bool considerParameterNames = false,
+            bool considerAttributes = false)
         {
             Debug.Assert(!considerExplicitlyImplementedInterfaces || considerName, "Doesn't make sense to consider interfaces separately from name.");
             Debug.Assert(!considerTypeConstraints || considerArity, "If you consider type constraints, you must also consider arity");
@@ -412,6 +435,9 @@ private MemberSignatureComparer(
             {
                 _typeComparison |= TypeCompareKind.FunctionPointerRefMatchesOutInRefReadonly;
             }
+
+            _considerParameterNames = considerParameterNames;
+            _considerAttributes = considerAttributes;
         }
 
         #region IEqualityComparer<Symbol> Members
@@ -447,9 +473,28 @@ public bool Equals(Symbol? member1, Symbol? member2)
 
             // NB: up to, and including, this check, we have not actually forced the (type) parameters
             // to be expanded - we're only using the counts.
-            if (_considerArity && (member1.GetMemberArity() != member2.GetMemberArity()))
+            if (_considerArity)
             {
-                return false;
+                if (member1.GetMemberArity() != member2.GetMemberArity())
+                {
+                    return false;
+                }
+
+                if (member1.GetMemberArity() > 0 && (_considerParameterNames || _considerAttributes))
+                {
+                    ImmutableArray<TypeParameterSymbol> typeParams1 = member1.GetMemberTypeParameters();
+                    ImmutableArray<TypeParameterSymbol> typeParams2 = member2.GetMemberTypeParameters();
+
+                    if (_considerParameterNames && typeParams1.Length > 0 && !haveSameTypeParameterNames(typeParams1, typeParams2))
+                    {
+                        return false;
+                    }
+
+                    if (_considerAttributes && !haveSameTypeParameterAttributes(typeParams1, typeParams2))
+                    {
+                        return false;
+                    }
+                }
             }
 
             if (member1.GetParameterCount() != member2.GetParameterCount())
@@ -465,10 +510,25 @@ public bool Equals(Symbol? member1, Symbol? member2)
                 return false;
             }
 
-            if (member1.GetParameterCount() > 0 && !HaveSameParameterTypes(member1.GetParameters().AsSpan(), typeMap1, member2.GetParameters().AsSpan(), typeMap2,
-                                                                           _refKindCompareMode, considerDefaultValues: _considerDefaultValues, _typeComparison))
+            if (member1.GetParameterCount() > 0)
             {
-                return false;
+                var params1 = member1.GetParameters();
+                var params2 = member2.GetParameters();
+                if (!HaveSameParameterTypes(params1.AsSpan(), typeMap1, params2.AsSpan(), typeMap2,
+                    _refKindCompareMode, considerDefaultValues: _considerDefaultValues, _typeComparison))
+                {
+                    return false;
+                }
+
+                if (_considerParameterNames && !haveSameParameterNames(params1, params2))
+                {
+                    return false;
+                }
+
+                if (_considerAttributes && !haveSameParameterAttributes(params1, params2))
+                {
+                    return false;
+                }
             }
 
             if (_considerCallingConvention)
@@ -526,7 +586,63 @@ public bool Equals(Symbol? member1, Symbol? member2)
                 }
             }
 
-            return !_considerTypeConstraints || HaveSameConstraints(member1, typeMap1, member2, typeMap2);
+            return !_considerTypeConstraints || HaveSameConstraints(member1, typeMap1, member2, typeMap2, ignoreNullability: (_typeComparison & TypeCompareKind.IgnoreNullableModifiersForReferenceTypes) != 0);
+
+            static bool haveSameParameterNames(ImmutableArray<ParameterSymbol> params1, ImmutableArray<ParameterSymbol> params2)
+            {
+                return params1.SequenceEqual(params2, (p1, p2) => p1.Name == p2.Name);
+            }
+
+            static bool haveSameTypeParameterNames(ImmutableArray<TypeParameterSymbol> typeParams1, ImmutableArray<TypeParameterSymbol> typeParams2)
+            {
+                return typeParams1.SequenceEqual(typeParams2, (p1, p2) => p1.Name == p2.Name);
+            }
+
+            static bool haveSameParameterAttributes(ImmutableArray<ParameterSymbol> params1, ImmutableArray<ParameterSymbol> params2)
+            {
+                return params1.SequenceEqual(params2, (p1, p2) => hasSameAttribute(p1.GetAttributes(), p2.GetAttributes()));
+            }
+
+            static bool haveSameTypeParameterAttributes(ImmutableArray<TypeParameterSymbol> typeParams1, ImmutableArray<TypeParameterSymbol> typeParams2)
+            {
+                return typeParams1.SequenceEqual(typeParams2, (p1, p2) => hasSameAttribute(p1.GetAttributes(), p2.GetAttributes()));
+            }
+
+            static bool hasSameAttribute(ImmutableArray<CSharpAttributeData> attributes1, ImmutableArray<CSharpAttributeData> attributes2)
+            {
+                if (attributes1.Length != attributes2.Length)
+                {
+                    return false;
+                }
+
+                var comparer = CommonAttributeDataComparer.InstanceIgnoringNamedArgumentOrder;
+
+                if (attributes1 is [var single1] && attributes2 is [var single2])
+                {
+                    // Fast path for single attribute case
+                    return comparer.Equals(single1, single2);
+                }
+
+                // Tracked by https://github.com/dotnet/roslyn/issues/78827 : optimization, consider using a pool
+                var counts = new Dictionary<CSharpAttributeData, int>(comparer);
+
+                foreach (var attribute in attributes1)
+                {
+                    counts[attribute] = counts.TryGetValue(attribute, out var foundCount) ? foundCount + 1 : 1;
+                }
+
+                foreach (var attribute in attributes2)
+                {
+                    if (!counts.TryGetValue(attribute, out var foundCount) || foundCount == 0)
+                    {
+                        return false;
+                    }
+
+                    counts[attribute] = foundCount - 1;
+                }
+
+                return counts.Values.All(c => c == 0);
+            }
         }
 
         public int GetHashCode(Symbol? member)
@@ -623,7 +739,7 @@ public static bool HaveSameReturnTypes(Symbol member1, TypeMap? typeMap1, Symbol
                     true);
         }
 
-        private static bool HaveSameConstraints(Symbol member1, TypeMap? typeMap1, Symbol member2, TypeMap? typeMap2)
+        private static bool HaveSameConstraints(Symbol member1, TypeMap? typeMap1, Symbol member2, TypeMap? typeMap2, bool ignoreNullability = true)
         {
             Debug.Assert(member1.GetMemberArity() == member2.GetMemberArity());
 
@@ -635,17 +751,17 @@ private static bool HaveSameConstraints(Symbol member1, TypeMap? typeMap1, Symbo
 
             var typeParameters1 = member1.GetMemberTypeParameters();
             var typeParameters2 = member2.GetMemberTypeParameters();
-            return HaveSameConstraints(typeParameters1, typeMap1, typeParameters2, typeMap2);
+            return HaveSameConstraints(typeParameters1, typeMap1, typeParameters2, typeMap2, ignoreNullability);
         }
 
-        public static bool HaveSameConstraints(ImmutableArray<TypeParameterSymbol> typeParameters1, TypeMap? typeMap1, ImmutableArray<TypeParameterSymbol> typeParameters2, TypeMap? typeMap2)
+        public static bool HaveSameConstraints(ImmutableArray<TypeParameterSymbol> typeParameters1, TypeMap? typeMap1, ImmutableArray<TypeParameterSymbol> typeParameters2, TypeMap? typeMap2, bool ignoreNullability = true)
         {
             Debug.Assert(typeParameters1.Length == typeParameters2.Length);
 
             int arity = typeParameters1.Length;
             for (int i = 0; i < arity; i++)
             {
-                if (!HaveSameConstraints(typeParameters1[i], typeMap1, typeParameters2[i], typeMap2))
+                if (!HaveSameConstraints(typeParameters1[i], typeMap1, typeParameters2[i], typeMap2, ignoreNullability))
                 {
                     return false;
                 }
@@ -654,10 +770,16 @@ public static bool HaveSameConstraints(ImmutableArray<TypeParameterSymbol> typeP
             return true;
         }
 
-        public static bool HaveSameConstraints(TypeParameterSymbol typeParameter1, TypeMap? typeMap1, TypeParameterSymbol typeParameter2, TypeMap? typeMap2)
+        public static bool HaveSameConstraints(TypeParameterSymbol typeParameter1, TypeMap? typeMap1, TypeParameterSymbol typeParameter2, TypeMap? typeMap2, bool ignoreNullability = true)
         {
             // Spec 13.4.3: Implementation of generic methods.
 
+            if (!ignoreNullability &&
+                typeParameter1.HasNotNullConstraint != typeParameter2.HasNotNullConstraint)
+            {
+                return false;
+            }
+
             if ((typeParameter1.HasConstructorConstraint != typeParameter2.HasConstructorConstraint) ||
                 (typeParameter1.HasReferenceTypeConstraint != typeParameter2.HasReferenceTypeConstraint) ||
                 (typeParameter1.HasValueTypeConstraint != typeParameter2.HasValueTypeConstraint) ||
@@ -903,7 +1025,7 @@ internal static bool ConsideringTupleNamesCreatesDifference(Symbol member1, Symb
         internal enum RefKindCompareMode
         {
             /// <summary>
-            /// Ref parameter modifiers are ignored.
+            /// All ref modifiers are considered equivalent.
             /// </summary>
             DoNotConsiderDifferences = 0,
 

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

@@ -42,6 +42,15 @@ internal static ImmutableArray<ParameterSymbol> GetParameters(this Symbol member
                     return ((PropertySymbol)member).Parameters;
                 case SymbolKind.Event:
                     return ImmutableArray<ParameterSymbol>.Empty;
+                case SymbolKind.NamedType:
+                    var namedType = (NamedTypeSymbol)member;
+                    if (namedType.IsExtension)
+                    {
+                        return namedType.ExtensionParameter is not null
+                            ? [namedType.ExtensionParameter]
+                            : [];
+                    }
+                    goto default;
                 default:
                     throw ExceptionUtilities.UnexpectedValue(member.Kind);
             }
@@ -341,6 +350,13 @@ internal static int GetParameterCount(this Symbol member)
                 case SymbolKind.Event:
                 case SymbolKind.Field:
                     return 0;
+                case SymbolKind.NamedType:
+                    var namedType = (NamedTypeSymbol)member;
+                    if (namedType.IsExtension)
+                    {
+                        return namedType.ExtensionParameter is not null ? 1 : 0;
+                    }
+                    goto default;
                 default:
                     throw ExceptionUtilities.UnexpectedValue(member.Kind);
             }

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

@@ -184,6 +184,33 @@ internal IEnumerable<ImmutableArray<SourceNamedTypeSymbol>> EnumerateMergedExten
             }
         }
 
+        /// <summary>
+        /// Reports a diagnostic when two extension blocks grouped into a single marker type have different C#-level signatures.
+        /// Note: It's fine for two extension blocks with different IL-level signatures to be grouped into the same grouping type, so we don't check for collisions there.
+        /// </summary>
+        internal void CheckSignatureCollisions(BindingDiagnosticBag diagnostics)
+        {
+            foreach (ImmutableArray<SourceNamedTypeSymbol> mergedBlocks in EnumerateMergedExtensionBlocks())
+            {
+                checkCollisions(mergedBlocks, diagnostics);
+            }
+
+            return;
+
+            static void checkCollisions(ImmutableArray<SourceNamedTypeSymbol> mergedBlocks, BindingDiagnosticBag diagnostics)
+            {
+                SourceNamedTypeSymbol first = mergedBlocks[0];
+
+                for (int i = 1; i < mergedBlocks.Length; i++)
+                {
+                    if (!MemberSignatureComparer.ExtensionSignatureComparer.Equals(first, (SourceNamedTypeSymbol?)mergedBlocks[i]))
+                    {
+                        diagnostics.Add(ErrorCode.ERR_ExtensionBlockCollision, mergedBlocks[i].Locations[0]);
+                    }
+                }
+            }
+        }
+
         private abstract class ExtensionGroupingOrMarkerType : Cci.INestedTypeDefinition
         {
             ushort ITypeDefinition.Alignment => 0;

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

@@ -287,8 +287,7 @@ private void PartialConstructorChecks(SourceConstructorSymbol implementation, Bi
             {
                 diagnostics.Add(ErrorCode.ERR_PartialMemberInconsistentTupleNames, implementation.GetFirstLocation(), this, implementation);
             }
-            else if (!MemberSignatureComparer.PartialMethodsStrictComparer.Equals(this, implementation)
-                || !Parameters.SequenceEqual(implementation.Parameters, static (a, b) => a.Name == b.Name))
+            else if (!MemberSignatureComparer.PartialMethodsStrictComparer.Equals(this, implementation))
             {
                 diagnostics.Add(ErrorCode.WRN_PartialMemberSignatureDifference, implementation.GetFirstLocation(),
                     new FormattedSymbol(this, SymbolDisplayFormat.MinimallyQualifiedFormat),

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

@@ -2362,6 +2362,7 @@ private void CheckMemberNameConflictsAndUnmatchedOperators(BindingDiagnosticBag
             if (this.declaration.ContainsExtensionDeclarations)
             {
                 checkMemberNameConflictsInExtensions(diagnostics);
+                this.GetExtensionGroupingInfo().CheckSignatureCollisions(diagnostics);
             }
 
             checkMemberNameConflicts(GetMembersByName(), GetTypeMembersDictionary(), GetMembersUnordered(), diagnostics);
@@ -6000,7 +6001,6 @@ internal ExtensionGroupingInfo GetExtensionGroupingInfo()
 
             if (_lazyExtensionGroupingInfo is null)
             {
-                // PROTOTYPE: Find the right place and perform checks for conflicting declarations getting into the same group or marker, and reporting appropriate errors.
                 Interlocked.CompareExchange(ref _lazyExtensionGroupingInfo, new ExtensionGroupingInfo(this), null);
             }
 

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

@@ -795,9 +795,12 @@ static void appendAttributes(ImmutableArray<CSharpAttributeData> attributes, Str
 
                 foreach (CSharpAttributeData attribute in attributes)
                 {
-                    var stringBuilder = PooledStringBuilder.GetInstance();
-                    appendAttribute(attribute, stringBuilder.Builder);
-                    attributesBuilder.Add(stringBuilder.ToStringAndFree());
+                    if (!attribute.IsConditionallyOmitted)
+                    {
+                        var stringBuilder = PooledStringBuilder.GetInstance();
+                        appendAttribute(attribute, stringBuilder.Builder);
+                        attributesBuilder.Add(stringBuilder.ToStringAndFree());
+                    }
                 }
 
                 attributesBuilder.Sort(StringComparer.Ordinal); // Actual order doesn't matter - just want to be deterministic