Properly populate ExportedType metadata table in presence of extension block. #80311

AlekseyTs · 2025-09-16T21:00:32Z

Related to #79894.

Also temporarily blocks the logic that causes a crash tracked by #80294. This is needed to unblock testing.

…n block. Related to dotnet#79894. Also temporarily blocks the logic that causes a crash tracked by dotnet#80294. This is needed to unblock testing.

333fred

Done review pass (commit 1)

333fred · 2025-09-16T23:02:57Z

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

+        private void SetExtensionGroupingTypeNestedTypes(ArrayBuilder<PENamedTypeSymbol> groupingNestedTypes)
+        {
+            var exchangeResult = Interlocked.CompareExchange(ref _lazyNestedTypes, GroupByName(groupingNestedTypes), null);
+            Debug.Assert(exchangeResult == null);


Why is this safe to assume? Is it not possible that multiple threads may get here at the same time? Certainly, given that EnsureNestedTypesAreLoaded does not assume that it is only called once, I do not expect it is safe to assume here either. #Resolved

Why is this safe to assume?

It is safe to assume by design and the assert verifies that the design is not violated, This method is called on a symbol that is not part of a regular container/member hierarchy, no one can get its instance by accident, only through ExtensionGroupingType API (added below) on an extension symbol. No one can get an extension symbol from which this symbol is accessible before this function is called, because it is called before the extensions are added to their contained type.

jcouv · 2025-09-17T09:59:41Z

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

+                    }
+
+                    var groupingType = ((PENamedTypeSymbol)type).ExtensionGroupingType;
+                    if (seenGroupingTypes.Add(groupingType))


I didn't follow why we need seeGroupingTypes logic. When could we get duplicates? Do we have a test demonstrating this?
Same question for Sort operation below (line 634) #Resolved

When could we get duplicates?

A grouping type might contain several marker types, each representing an extension block. Here we are enumerating extension blocks. Therefore, can see the same grouping type multiple times.

Do we have a test demonstrating this?

Many tests demonstrating duplication, but not specifically for this code path. I'll add a scenario.

Same question for Sort operation below

Sorting guarantees deterministic emit

Sorting guarantees deterministic emit
Aren't the APIs above already deterministic?

Aren't the APIs above already deterministic?

As far as I know a lookup by name doesn't guarantee a particular order. I.e. it is not part of the contract.

jcouv · 2025-09-17T10:53:39Z

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

                var namespaceOrType = member as NamespaceOrTypeSymbol;
-                if ((object)namespaceOrType != null &&
-                    member is not NamedTypeSymbol { IsExtension: true }) // https://github.com/dotnet/roslyn/issues/78963 - This is a temporary handling, we should get grouping and marker types processed instead.
+                if ((object)namespaceOrType != null)


When could this be null? #ByDesign

When could this be null?

I am not adding this condition.

jcouv · 2025-09-17T11:22:02Z

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

                        }
                    }

+                    type.SetExtensionGroupingTypeNestedTypes(groupingNestedTypes);


Is there a way to observe this, maybe by checking the nested types of a grouping type via the internal ExtensionGroupingType API? Removing this line doesn't affect extension tests #Resolved

Is there a way to observe this

In order to observe the effect, you would need to revert:

this line

changes in the PEModuleSymbol

If this line is removed on its own, when PEModuleBuilder tries to enumerate nested types of a grouping type we would end up in EnsureNestedTypesAreLoaded with null _lazyNestedTypes. It will load the marker types that we just loaded again, so we will end up with duplicate symbols for the same types in metadata. Also, it will register those nested types in various side tables and we don't want that to happen because these instances are not part of a regular container/nested type hierarchy.

jcouv

Done with review pass (commit 1)

jcouv · 2025-09-17T11:55:33Z

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

                }
+
+                groupingTypes.Free();
            }


nit: consider adding an assertion in GetForwardedTypes logic that the types being forwarded can't be extensions #Resolved

consider adding an assertion in GetForwardedTypes logic that the types being forwarded can't be extensions

This is not related to the purpose of this PR and extensions can be forwarded. Forwarding will be dealt with in a separate PR

AlekseyTs · 2025-09-18T00:19:56Z

@jcouv, @333fred, @jjonescz, @dotnet/roslyn-compiler Please review, I have another change in the pipeline that builds on top of changes in this PR.

jcouv

LGTM Thanks (commit 3)

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

…ol.cs Fix typo Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>

* upstream/main: (206 commits) Remove bogus xlf tag (#80357) Fix missing type argument checks Add tests Use dotnet run file for generating compiler code (#80248) Only restore based on assets file changes if the actual content changed (#80341) make expressionbody analyzer use semanticspananalysis (#80339) [EnC] Use ignoreAssemblyKey: false to resolve symbol keys (#80342) Properly populate ExportedType metadata table in presence of extension block. (#80311) Propagate `params` to lambdas and local functions (#79880) Change 17.15 to VS 2026 preview. (#80325) Improve virtualproject support for older .NET SDKs (#80324) Update dependencies from https://github.com/dotnet/dotnet build 283666 (#80344) Update dependencies from https://github.com/dotnet/arcade build 20250917.6 (#80343) Simplifying Fix tests Fix tests Fix introduce variable placement in top level statements move to immutable types in signature help move to immutable types in signature help Fix check ...

* upstream/main: (31 commits) Remove bogus xlf tag (dotnet#80357) Fix missing type argument checks Add tests Use dotnet run file for generating compiler code (dotnet#80248) Only restore based on assets file changes if the actual content changed (dotnet#80341) make expressionbody analyzer use semanticspananalysis (dotnet#80339) [EnC] Use ignoreAssemblyKey: false to resolve symbol keys (dotnet#80342) Properly populate ExportedType metadata table in presence of extension block. (dotnet#80311) Propagate `params` to lambdas and local functions (dotnet#79880) Change 17.15 to VS 2026 preview. (dotnet#80325) Improve virtualproject support for older .NET SDKs (dotnet#80324) Update dependencies from https://github.com/dotnet/dotnet build 283666 (dotnet#80344) Update dependencies from https://github.com/dotnet/arcade build 20250917.6 (dotnet#80343) Simplifying Fix tests Fix tests Fix introduce variable placement in top level statements move to immutable types in signature help move to immutable types in signature help Fix check ...

Properly populate ExportedType metadata table in presence of extensio…

90e3097

…n block. Related to dotnet#79894. Also temporarily blocks the logic that causes a crash tracked by dotnet#80294. This is needed to unblock testing.

AlekseyTs requested review from jcouv and jjonescz September 16, 2025 21:00

AlekseyTs requested a review from a team as a code owner September 16, 2025 21:00

AlekseyTs added Area-Compilers Feature - Extension Everything The extension everything feature labels Sep 16, 2025

333fred reviewed Sep 16, 2025

View reviewed changes

AlekseyTs requested a review from 333fred September 17, 2025 01:59

jcouv reviewed Sep 17, 2025

View reviewed changes

jcouv self-assigned this Sep 17, 2025

jcouv reviewed Sep 17, 2025

View reviewed changes

Strengthen a test

51e6bed

AlekseyTs requested a review from jcouv September 17, 2025 15:45

Merge remote-tracking branch 'upstream/main' into Issue79894_02

bf574da

AlekseyTs requested a review from a team September 18, 2025 00:17

jcouv approved these changes Sep 18, 2025

View reviewed changes

jjonescz approved these changes Sep 18, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEModuleSymbol.cs Outdated Show resolved Hide resolved

Update src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEModuleSymb…

285f65a

…ol.cs Fix typo Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>

AlekseyTs enabled auto-merge (squash) September 18, 2025 13:00

AlekseyTs merged commit 9bb6ddc into dotnet:main Sep 18, 2025
23 of 24 checks passed

dotnet-policy-service bot added this to the Next milestone Sep 18, 2025

dotnet-bot mentioned this pull request Sep 20, 2025

[Automated] PRs inserted in VS build main-11019.114 #80386

Closed

akhera99 modified the milestones: Next, 18.0 P1 Sep 22, 2025

akhera99 modified the milestones: 18.0 P1, 18.0 P2 Sep 23, 2025

This was referenced Sep 25, 2025

[Automated] PRs inserted in VS build feature.debugger.main-11023.335 #80458

Closed

[Automated] PRs inserted in VS build feature.Wix5-11030.241 #80529

Closed

Properly populate ExportedType metadata table in presence of extension block. #80311

Properly populate ExportedType metadata table in presence of extension block. #80311

Uh oh!

Conversation

AlekseyTs commented Sep 16, 2025

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Sep 16, 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!

AlekseyTs Sep 17, 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 Sep 17, 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!

AlekseyTs Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 17, 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!

AlekseyTs Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 17, 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!

AlekseyTs Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 17, 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!

AlekseyTs Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Sep 18, 2025

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

333fred Sep 16, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Sep 17, 2025 •

edited

Loading

jcouv Sep 17, 2025 •

edited by AlekseyTs

Loading

jcouv Sep 17, 2025 •

edited by AlekseyTs

Loading

jcouv Sep 17, 2025 •

edited by AlekseyTs

Loading

jcouv Sep 17, 2025 •

edited by AlekseyTs

Loading