Implement name and signature conflict check across extension containers with the same receiver type #77747

AlekseyTs · 2025-03-21T21:50:12Z

Implements the following rule from the spec:
"Within a given enclosing static class, the set of extension member declarations with the same receiver type (modulo identity conversion and type parameter name substitution) are treated as a single declaration space similar to the members within a class or struct declaration, and are subject to the same rules about uniqueness."

Relates to test plan #76130

… with the same receiver type Implements the following rule from the spec: "Within a given enclosing static class, the set of extension member declarations with the same receiver type (modulo identity conversion and type parameter name substitution) are treated as a single declaration space similar to the members within a class or struct declaration, and are subject to the same rules about uniqueness."

AlekseyTs · 2025-03-21T23:05:46Z

@jcouv, @jjonescz, @dotnet/roslyn-compiler Please review

AlekseyTs · 2025-03-24T13:39:31Z

@jcouv, @jjonescz, @dotnet/roslyn-compiler Please review

jjonescz · 2025-03-25T09:49:30Z

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

+                    if (membersByNameToMerge.Count == 0 ||
+                        (membersByNameToMerge.Count == 1 && membersByNameToMerge.Values.Single() is [SynthesizedExtensionMarker]))


nit: could this be simplified to something like the following?

Suggested change

if (membersByNameToMerge.Count == 0 ||

(membersByNameToMerge.Count == 1 && membersByNameToMerge.Values.Single() is [SynthesizedExtensionMarker]))

if (membersByNameToMerge.Values is [] or [[SynthesizedExtensionMarker]])

``` #Resolved

nit: could this be simplified to something like the following?

Even if the suggested expression has equivalent behavior, I do not find it simpler or easier to follow.

BTW, the suggested code doesn't compile:

error CS0021: Cannot apply indexing with [] to an expression of type 'Dictionary<ReadOnlyMemory<char>, ImmutableArray<Symbol>>.ValueCollection' error CS0021: Cannot apply indexing with [] to an expression of type 'Dictionary<ReadOnlyMemory<char>, ImmutableArray<Symbol>>.ValueCollection'

jjonescz · 2025-03-25T10:00:17Z

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

+            public override bool Equals([NotNullWhen(true)] object? obj)
+            {
+                Debug.Assert(false); // Usage of this method is unexpected
+                return Equals((ExtensionGroupingKey)obj!);


Conforming Equals override should handle nulls, I think:

Suggested change

return Equals((ExtensionGroupingKey)obj!);

return obj is ExtensionGroupingKey k && Equals(k);

``` #Resolved

Conforming Equals override should handle nulls,

In general, this is true. However we control usage of this type and don't want it to be boxed for the purpose of comparison. Therefore, we not only do not expect it to be called with null, we do not expected it to be called ever. Hence the assert above.

jjonescz · 2025-03-25T10:06:32Z

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

+
+            public override int GetHashCode()
+            {
+                return ReceiverType.GetHashCode();


Shouldn't TypeCompareKind.AllIgnoreOptions be taken into consideration here as well? Otherwise it might happen that two ExtensionGroupingKeys that are equal by Equals have different hash code (because the receiver types have nullability annotations differences for example). #Resolved

Shouldn't TypeCompareKind.AllIgnoreOptions be taken into consideration here as well?

It should be and it is taken into consideration. Implementations of GetHashCode for symbols are supposed to ignore any differences that could be ignored during symbol comparison. That significantly simplifies code in here and in SymbolEqualityComparer, for example.

jjonescz · 2025-03-25T10:10:08Z

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

+                        }
+                        else
+                        {
+                            membersUnordered = membersUnordered.Concat(extension.GetMembersUnordered());


Consider moving this inside concatMembers as it's otherwise hard to follow why is this done here only when extension.Arity == 0. #Resolved

Consider moving this inside concatMembers as it's otherwise hard to follow why is this done here only when extension.Arity == 0.

Doing so would lead to an incorrect behavior, specifically the resulting array would contain duplicates because concatMembers is called in a loop several times. The set of members concatenated here and the set of members added to the same array in concatMembers are different in a general case.

jjonescz · 2025-03-25T10:29:58Z

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

+
+        comp.VerifyDiagnostics(
+            // (10,21): error CS0111: Type 'Extensions' already defines a member called 'M1' with the same parameter types
+            //         static void M1() {}


Consider adding marker comments like // 1 (or perhaps #line pragmas) otherwise it's not very clear which line this diagnostic is reported at. Applies also to similar cases in new tests. #WontFix

Usually the source on the error lines is unique and/or the test scenarios are pretty small.

jjonescz · 2025-03-25T14:58:28Z

@AlekseyTs did you want to wait for @jcouv's review here as well before merging?

AlekseyTs · 2025-03-25T15:01:38Z

did you want to wait for @jcouv's review here as well before merging?

Yes, my mistake, thank you for pointing out. I'll make sure to get a sign off from @jcouv and follow-up on any feedback.

jcouv · 2025-03-25T15:43:10Z

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

@@ -1964,17 +1963,21 @@ private void CheckRecordMemberNames(BindingDiagnosticBag diagnostics)
            }
        }

-        private void CheckMemberNameConflicts(BindingDiagnosticBag diagnostics)
+        private static void CheckMemberNameConflicts(
+            SourceMemberContainerTypeSymbol containerForDiagnostics,


nit: I didn't follow why this method was changed to static. The only value passed to containerForDiagnostics is this.
The same applies to CheckIndexerNameConflicts and ReportMethodSignatureCollision and possibly others too

nit: I didn't follow why this method was changed to static. The only value passed to containerForDiagnostics is this. The same applies to CheckIndexerNameConflicts and ReportMethodSignatureCollision

There are additional parameters passed as well. While at the moment the only value passed to containerForDiagnostics is this, it doesn't have to be this, the logic doesn't depend on the fact. The goal was to make sure that the methods do not accidentally use any instance API any instance data, they should operate only on the information passed in explicitly and shouldn't assume that the passed in symbols directly declared in this.

jcouv · 2025-03-25T16:12:13Z

LGTM Thanks

AlekseyTs added Area-Compilers Feature - Extension Everything labels Mar 21, 2025

AlekseyTs requested review from jjonescz and jcouv March 21, 2025 21:50

AlekseyTs requested a review from a team as a code owner March 21, 2025 21:50

dotnet-issue-labeler bot added the untriaged label Mar 21, 2025

AlekseyTs changed the title ~~Implement name an signature conflict check across extension containers with the same receiver type~~ Implement name and signature conflict check across extension containers with the same receiver type Mar 21, 2025

jcouv self-assigned this Mar 22, 2025

jjonescz reviewed Mar 25, 2025

View reviewed changes

jjonescz approved these changes Mar 25, 2025

View reviewed changes

AlekseyTs merged commit 67a009f into dotnet:features/extensions Mar 25, 2025
24 checks passed

jcouv reviewed Mar 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement name and signature conflict check across extension containers with the same receiver type #77747

Implement name and signature conflict check across extension containers with the same receiver type #77747

AlekseyTs commented Mar 21, 2025 •

edited by jcouv

Loading

AlekseyTs commented Mar 21, 2025

AlekseyTs commented Mar 24, 2025

jjonescz Mar 25, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Mar 25, 2025

AlekseyTs Mar 25, 2025

jjonescz Mar 25, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Mar 25, 2025

jjonescz Mar 25, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Mar 25, 2025

jjonescz Mar 25, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Mar 25, 2025

jjonescz Mar 25, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Mar 25, 2025

jjonescz commented Mar 25, 2025

AlekseyTs commented Mar 25, 2025

jcouv Mar 25, 2025 •

edited

Loading

AlekseyTs Mar 25, 2025

jcouv commented Mar 25, 2025

		if (membersByNameToMerge.Count == 0 \|\|
		(membersByNameToMerge.Count == 1 && membersByNameToMerge.Values.Single() is [SynthesizedExtensionMarker]))

	return Equals((ExtensionGroupingKey)obj!);
	return obj is ExtensionGroupingKey k && Equals(k);
	``` #Resolved

Implement name and signature conflict check across extension containers with the same receiver type #77747

Implement name and signature conflict check across extension containers with the same receiver type #77747

Conversation

AlekseyTs commented Mar 21, 2025 • edited by jcouv Loading

AlekseyTs commented Mar 21, 2025

AlekseyTs commented Mar 24, 2025

jjonescz Mar 25, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Mar 25, 2025

Choose a reason for hiding this comment

AlekseyTs Mar 25, 2025

Choose a reason for hiding this comment

jjonescz Mar 25, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Mar 25, 2025

Choose a reason for hiding this comment

jjonescz Mar 25, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Mar 25, 2025

Choose a reason for hiding this comment

jjonescz Mar 25, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Mar 25, 2025

Choose a reason for hiding this comment

jjonescz Mar 25, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Mar 25, 2025

Choose a reason for hiding this comment

jjonescz commented Mar 25, 2025

AlekseyTs commented Mar 25, 2025

jcouv Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 25, 2025

Choose a reason for hiding this comment

jcouv commented Mar 25, 2025

AlekseyTs commented Mar 21, 2025 •

edited by jcouv

Loading

jjonescz Mar 25, 2025 •

edited by AlekseyTs

Loading

jjonescz Mar 25, 2025 •

edited by AlekseyTs

Loading

jjonescz Mar 25, 2025 •

edited by AlekseyTs

Loading

jjonescz Mar 25, 2025 •

edited by AlekseyTs

Loading

jjonescz Mar 25, 2025 •

edited by AlekseyTs

Loading

jcouv Mar 25, 2025 •

edited

Loading