Move to new metadata format for extensions #79553

AlekseyTs · 2025-07-23T20:56:22Z

Addresses part of #78963 ("switch away from ordinals in skeleton type names")
Relates to test plan #76130

AlekseyTs · 2025-07-23T20:57:10Z

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

tmat · 2025-07-23T22:54:07Z

src/Compilers/Core/Portable/Text/SourceText.cs

            }
        }

+        internal static void ReverseEndianness(Span<short> shortSpan)


You can define this in MS.CA.Contracts project as a static extension polyfill API. #WontFix

jcouv · 2025-07-24T02:24:32Z

src/Compilers/Core/Portable/WellKnownMember.cs

        System_Collections_Generic_List_T__AddRange,

        System_Runtime_CompilerServices_ParamCollectionAttribute__ctor,
+        System_Runtime_CompilerServices_ExtensionMarkerNameAttribute__ctor,


FWIW, I added two follow-up items to #78963

add ExtensionMarkerNameAttribute to BCL

disallow ExtensionMarkerName attribute in source #Closed

jcouv · 2025-07-24T02:25:00Z

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

                yield break;
            }

+            bool checkForExtensonGroup = this.IsStatic && this.ContainingType is null && this.TypeKind == TypeKind.Class &&


checkForExtensonGroup

Typo: checkForExtensionGroup #Closed

jcouv · 2025-07-24T02:25:45Z

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

                yield break;
            }

+            bool checkForExtensonGroup = this.IsStatic && this.ContainingType is null && this.TypeKind == TypeKind.Class &&


Should we also check that this.Arity == 0? #Closed

Should we also check that this.Arity == 0?

Good idea

jcouv · 2025-07-24T02:26:55Z

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

+        private class ExtensionInfo(TypeDefinitionHandle groupingType, MethodDefinitionHandle markerMethod)
        {
-            public readonly MethodDefinitionHandle MarkerMethod = markerMethod;
+            public readonly TypeDefinitionHandle GroupingType = groupingType;


GroupingType

nit: consider GroupingTypeHandle #Closed

jcouv · 2025-07-24T02:28:52Z

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


-        internal override string ExtensionName
-            => Name; // Tracked by https://github.com/dotnet/roslyn/issues/76130 : Confirm implementation
+        internal override string ExtensionName // Tracked by https://github.com/dotnet/roslyn/issues/76130 : Confirm implementation


This was changed in main branch to a specific link:

internal override string ExtensionName => Name; // Tracked by [https://github.com/dotnet/roslyn/issues/78963](https://github.com/dotnet/roslyn/issues/78963) : Revisit when adopting new metadata design with content-based type names

Consider using a PROTOTYPE comment or removing the link entirely since there's a PROTOTYPE in implementation. #Closed

Consider using a PROTOTYPE comment or removing the link entirely since there's a PROTOTYPE in implementation.

Removing

jcouv · 2025-07-24T02:30:31Z

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

+
+                if (IsExtension)
+                {
+                    // We do not recognize any attributes on extension blocks


nit: consider leaving PROTOTYPE comments to cover the changes in GetAttributes, CreateEvents, CreateFields and CreateNestedTypes showing that attributes/events/fields/types in metadata are ignored. #Closed

jcouv · 2025-07-24T02:30:48Z

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

+            public override IEnumerable<TypeReferenceWithAttributes> GetConstraints(EmitContext context)
+            {
+                // Drop all attributes from constraints, they are about C# specific semantics.
+                foreach (var constrint in base.GetConstraints(context))


constrint

Typo: constraint #Closed

jcouv · 2025-07-24T02:31:52Z

        if (AdaptedNamedTypeSymbol.IsScriptClass || AdaptedNamedTypeSymbol.IsExtension) // Tracked by https://github.com/dotnet/roslyn/issues/76130 : we should have checked the presence of System.Object

Is the extension case still reachable in GetBaseClass? #Closed

Refers to: src/Compilers/CSharp/Portable/Emitter/Model/NamedTypeSymbolAdapter.cs:293 in 221fc17. [](commit_id = 221fc17, deletion_comment = False)

jcouv · 2025-07-24T02:33:47Z

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.


If there are any PROTOTYPE comments in this PR that you don't consider blocking for merging the feature branch back to main, it would be helpful to use linked issues instead or tagging them somehow. #Closed

If there are any PROTOTYPE comments in this PR that you don't consider blocking for merging the feature branch back to main, it would be helpful to use linked issues instead or tagging them somehow.

The things I would consider truly blocking:

dock IDs and doc comments on merged extension blocks

Detecting and reporting collisions during merge into grouping and marker types.

Finalizing anything that affects metadata names. Prefixes, etc.

Test that the right things are dropped from grouping types (we can follow up offline)

Test that the right things are preserved on marker types (we can follow up offline)

However, I'd rather not make this decision on my own, hence leaving the comments. The comments are also more preferable for me from the context and time consumption perspective.

jcouv · 2025-07-24T02:35:50Z

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

+            {
+                return ((SourceMemberContainerTypeSymbol)containingType.ContainingType).GetExtensionGroupingInfo().GetCorrespondingMarkerType((SourceNamedTypeSymbol)containingType);
+            }
+            else if (containingType.IsExtension)


containingType.IsExtension

nit: consider AdaptedMethodSymbol.GetIsNewExtensionMember() to be consistent with other adapter checks (I know that we already have containingType at hand above, but the consistency improves readability somewhat) #Closed

jcouv · 2025-07-24T02:43:11Z

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

+{
+    internal sealed class ExtensionGroupingInfo
+    {
+        private readonly Dictionary<string, MultiDictionary<string, SourceNamedTypeSymbol>> _groupingMap;


nit: Consider documenting. If I understood correctly, the outer string key is a grouping name, the inner string key is a marker name, and the named types are the extension declarations
nit: it may be good to extract the computation logic that creates the dictionary and ExtensionGroupingInfo from GetExtensionGroupingInfo and move it into ExtensionGroupingInfo so the logic constructing this dictionary is closer to the dictionary's definition. #Closed

jcouv · 2025-07-24T03:13:47Z

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

+            }
+        }
+
+        private class ExtensionGroupingTypeParameter : InheritedTypeParameter


ExtensionGroupingTypeParameter

nit: ExtensionGroupingTypeTypeParameter? #Closed

jcouv · 2025-07-24T03:19:03Z

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

+            int IComparable<ExtensionGroupingType>.CompareTo(ExtensionGroupingType? other)
+            {
+                Debug.Assert(other is { });
+                return ExtensionMarkerTypes[0].CompareTo(other.ExtensionMarkerTypes[0]);


Could we simply compare names? Question also applies to ExtensionMarkerType comparison #Closed

Could we simply compare names?

I think we should stick to the lexical declaration order for emit purposes.

jcouv · 2025-07-24T03:22:10Z

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

+                builder.Sort();
+                ExtensionMarkerTypes = builder.ToImmutableAndFree();
+
+                _typeParameters = ExtensionMarkerTypes[0].UnderlyingExtensions[0].Arity != 0 ?


Would it be useful to check/assert that the merged signatures are identical (ignoring C#-isms)? That could help catch issues as new features are added in the future and the content-hashes were not updated.
The question also applies to ExtensionMarkerType (that one would not ignore C#-isms)
#Closed

Would it be useful to check/assert that the merged signatures are identical (ignoring C#-isms)?

This check should be performed not as an assert. It is not implemented in this PR and a PROTOTYPE comment is left in SourceMemberContainerTypeSymbol.

jcouv · 2025-07-24T03:23:10Z

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

+
+            protected override bool IsSealed => true;
+
+            protected override ITypeDefinition ContainingTypeDefinition => ExtensionMarkerTypes[0].UnderlyingExtensions[0].ContainingType!.GetCciAdapter();


!

This should be an assertion (here or possibly in the constructor) #WontFix

This should be an assertion (here or possibly in the constructor)

I do not think its worth the ceremony. The fact is obvious by construction, i.e. these are nested types symbols.

jcouv · 2025-07-24T03:24:09Z

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

+                        {
+                            Debug.Assert((object)method != null);
+
+                            // PROTOTYPE: SynthesizedExtensionMarker should not be added as a member of the extension type.


I'm not clear what you intend for SynthesizedExtensionMarker. Consider leaving more details if this is something I'll need to handle #Closed

I'm not clear what you intend for SynthesizedExtensionMarker. Consider leaving more details if this is something I'll need to handle

I went ahead and made the change in this PR

jcouv · 2025-07-24T03:26:43Z

src/Dependencies/Collections/Extensions/IEnumerableExtensions.cs

+
+            var builder = ArrayBuilder<TResult>.GetInstance();
+
+            int index = 0;


The index seems unused #Closed

jcouv · 2025-07-24T03:59:34Z

""";

The tests that show IL show the ExtensionMarkerName attribute, but the value is garbled. Would it be useful to have some direct tests showing the value (ie. the marker name)?
Do we have a test with two extension blocks that result in a single extension group with two marker types?
Do we have a test with two identical extension blocks that get merged? (we probably need a PROTOTYPE for xml docs) #Closed

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

jcouv

Done with review pass (commit 1)

jjonescz · 2025-07-24T06:27:37Z

src/Compilers/CSharp/Portable/Binder/Semantics/Operators/UnaryOperatorOverloadResolution.cs

+                           y.OriginalDefinition.AsMember(normalize(yExtension)));
+            }
+
+            private static NamedTypeSymbol normalize(NamedTypeSymbol extension)


Suggested change

private static NamedTypeSymbol normalize(NamedTypeSymbol extension)

private static NamedTypeSymbol Normalize(NamedTypeSymbol extension)

``` #Resolved

jjonescz · 2025-07-24T06:46:57Z

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

+            }
+        }
+
+        private class ExtensionGroupingTypeParameter : InheritedTypeParameter


Suggested change

private class ExtensionGroupingTypeParameter : InheritedTypeParameter

private sealed class ExtensionGroupingTypeParameter : InheritedTypeParameter

``` #Resolved

jjonescz · 2025-07-24T06:48:36Z

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

+                                yield return synthesized;
+                                break;


Looks like we don't need an iterator method but can simply return:

Suggested change

yield return synthesized;

break;

return [synthesized];

``` #Resolved

jjonescz · 2025-07-24T07:33:31Z

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

+                            if (marker.TryGetExtensionMarkerMethod() is { IsNil: false } markerHandle)
+                            {
+                                marker = PENamedTypeSymbol.Create(moduleSymbol, this, markerRid, type.Handle, markerHandle);
+                                yield return marker;


Aren't marker types nested in the grouping type? But we seem to be returning them as nested for the extension enclosing static class. #Resolved

Aren't marker types nested in the grouping type? But we seem to be returning them as nested for the extension enclosing static class.

That is correct, we are transforming what we have in metadata to how the language sees things, i.e. extension blocks directly in the top level static class.

AlekseyTs · 2025-07-24T12:27:11Z

""";

For multiple markers in a group and attribute values adding some extra checks to Implementation_StaticMethod_02_WithLocalFunction and Nullability_Attribute_22.

For merged blocks under one marker adding some extra checks to StaticMethodInvocation_PartialStaticClass. Added a work item for XML docs to #78963.

In reply to: 3111874120

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

AlekseyTs · 2025-07-24T13:52:34Z

        if (AdaptedNamedTypeSymbol.IsScriptClass || AdaptedNamedTypeSymbol.IsExtension) // Tracked by https://github.com/dotnet/roslyn/issues/76130 : we should have checked the presence of System.Object

Is the extension case still reachable in GetBaseClass?

I do not think so, but I will leave this clean-up for a follow-up.

In reply to: 3111750451

Refers to: src/Compilers/CSharp/Portable/Emitter/Model/NamedTypeSymbolAdapter.cs:293 in 221fc17. [](commit_id = 221fc17, deletion_comment = False)

…al consumers

jcouv

LGTM Thanks (commit 3)

jcouv · 2025-07-24T17:07:42Z

""";

Btw, I think this PR closes issue #79043 (adjust conflict rules). I'll add a test and close in a follow-up PR

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

jcouv · 2025-07-24T17:48:41Z

            return new SynthesizedExtensionMarker(this, parameterList);

The marker method symbol still has the extension block as containing type (and when we produce metadata, we re-parent it). Should we attach it directly to the right marker type instead?

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol_Extension.cs:1106 in faf186a. [](commit_id = faf186a, deletion_comment = False)

Move to new metadata format for extensions

221fc17

AlekseyTs requested review from jcouv and jjonescz July 23, 2025 20:56

AlekseyTs requested a review from a team as a code owner July 23, 2025 20:56

AlekseyTs added Area-Compilers Feature - Extension Everything The extension everything feature labels Jul 23, 2025

tmat reviewed Jul 23, 2025

View reviewed changes

jcouv self-assigned this Jul 23, 2025

jcouv reviewed Jul 24, 2025

View reviewed changes

jjonescz reviewed Jul 24, 2025

View reviewed changes

PR Feedback

3c51045

jjonescz approved these changes Jul 24, 2025

View reviewed changes

AlekseyTs requested a review from jcouv July 24, 2025 14:57

Remove extension marker from the list of members accessible by extern…

faf186a

…al consumers

jcouv approved these changes Jul 24, 2025

View reviewed changes

jcouv mentioned this pull request Jul 24, 2025

Extensions: follow-ups for metadata #78963

Open

28 tasks

AlekseyTs merged commit f388cb7 into dotnet:features/extensions Jul 24, 2025
28 checks passed

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


		protected override bool IsSealed => true;

		protected override ITypeDefinition ContainingTypeDefinition => ExtensionMarkerTypes[0].UnderlyingExtensions[0].ContainingType!.GetCciAdapter();


		var builder = ArrayBuilder<TResult>.GetInstance();

		int index = 0;

	private static NamedTypeSymbol normalize(NamedTypeSymbol extension)
	private static NamedTypeSymbol Normalize(NamedTypeSymbol extension)
	``` #Resolved

	private class ExtensionGroupingTypeParameter : InheritedTypeParameter
	private sealed class ExtensionGroupingTypeParameter : InheritedTypeParameter
	``` #Resolved

-                                yield return synthesized;
-                                break;
+                                return [synthesized];
+``` #Resolved

Move to new metadata format for extensions #79553

Move to new metadata format for extensions #79553

Uh oh!

Conversation

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

Uh oh!

Uh oh!

AlekseyTs commented Jul 23, 2025

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 24, 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!

jcouv Jul 24, 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!

jcouv Jul 24, 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!

jcouv Jul 24, 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!

jcouv Jul 24, 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!

jcouv Jul 24, 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!

jcouv Jul 24, 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!

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

Uh oh!

Uh oh!

jcouv Jul 24, 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!

jcouv Jul 24, 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!

jcouv Jul 24, 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!

jcouv Jul 24, 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!

jcouv Jul 24, 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!

jcouv Jul 24, 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!

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

Uh oh!

AlekseyTs commented Jul 23, 2025 •

edited by jcouv

Loading

tmat Jul 23, 2025 •

edited by AlekseyTs

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv commented Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited by AlekseyTs

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv Jul 24, 2025 •

edited

Loading

jcouv commented Jul 24, 2025 •

edited

Loading

jjonescz Jul 24, 2025 •

edited by AlekseyTs

Loading

jjonescz Jul 24, 2025 •

edited by AlekseyTs

Loading

jjonescz Jul 24, 2025 •

edited by AlekseyTs

Loading

jjonescz Jul 24, 2025 •

edited by AlekseyTs

Loading

jcouv commented Jul 24, 2025 •

edited

Loading

jcouv commented Jul 24, 2025 •

edited

Loading