Set IsClosedTypeAttribute.DerivedTypes by RikkiGibson · Pull Request #84350 · dotnet/roslyn

RikkiGibson · 2026-07-01T00:22:23Z

Implement metadata format change from dotnet/runtime#129009

Notes:

Compiler doesn't use DerivedTypes property as a source of information at all and (in this PR at least) doesn't verify correctness of the attribute when importing types from metadata.
The DerivedTypes property does not need to exist, but, compiler will report an error if it doesn't have the expected shape. Perhaps we might eventually want to require the property to exist, but, for now we need to be compatible with frameworks where the property is missing.

Microsoft Reviewers: Open in CodeFlow

RikkiGibson · 2026-07-01T04:46:35Z

@eiriktsarpalis for visibility

Copilot

Pull request overview

This PR updates Roslyn’s compiler-emitted metadata for System.Runtime.CompilerServices.IsClosedTypeAttribute to include the new DerivedTypes named argument when available, and adjusts related well-known-member plumbing and tests to recognize the additional member.

Changes:

Emit IsClosedTypeAttribute(DerivedTypes = …) from the C# compiler when the DerivedTypes property and System.Type are available.
Add a new WellKnownMember entry for IsClosedTypeAttribute.DerivedTypes and update “missing platform member” test allowlists (C# + VB).
Expand closed-classes tests to validate the emitted metadata encoding for DerivedTypes across multiple scenarios (including malformed/missing members).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs	Emits `IsClosedTypeAttribute` with a `DerivedTypes` named argument when possible.
src/Compilers/Core/Portable/WellKnownMember.cs	Adds a new well-known member enum value for `IsClosedTypeAttribute__DerivedTypes`.
src/Compilers/Core/Portable/WellKnownMembers.cs	Adds descriptor + name entry for the `DerivedTypes` well-known property.
src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs	Updates the test definition of `IsClosedTypeAttribute` to include `DerivedTypes`.
src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs	Updates the test allowlist to treat `DerivedTypes` as not-yet-in-platform.
src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/WellKnownTypeValidationTests.vb	Updates VB allowlists to include `IsClosedTypeAttribute__DerivedTypes`.
src/Compilers/CSharp/Test/CSharp15/ClosedClassesTests.cs	Updates IL baselines and adds/extends coverage validating `DerivedTypes` metadata emission.

RikkiGibson · 2026-07-01T05:13:17Z

                AddSynthesizedAttribute(
                    ref attributes,
-                    compilation.TrySynthesizeAttribute(WellKnownMember.System_Runtime_CompilerServices_IsClosedTypeAttribute__ctor));
+                    compilation.TrySynthesizeAttribute(
+                        WellKnownMember.System_Runtime_CompilerServices_IsClosedTypeAttribute__ctor,
+                        namedArguments: namedArguments));


It's not obvious to me how to generate a use site error which is specific to DerivedTypes property, in order to exercise the scenario, except perhaps via the property type, which seems not straightforward to exercise.

Looking at the impl of TrySynthesizeAttribute, I think my null-check on the derivedTypesProperty was already enough to expect the property to not cause us to fail synthesis.

It doesn't seem wrong to check for use site error though? I'll think about it a bit more in the morning.

@RikkiGibson If that helps in your investigation, any member can be made "bad" with unsupported CompilerFeatureRequiredFeatures attribute applied to it.

jjonescz · 2026-07-01T12:33:16Z

+                    var derivedTypesConstant = new TypedConstant(
+                        arrayOfSystemType,
+                        CandidateClosedSubtypeDefinitions.SelectAsArray(
+                            subtype => new TypedConstant(systemType, TypedConstantKind.Type, subtype.GetUnboundGenericTypeOrSelf())));


nit: consider using overload of SelectAsArray taking an extra argument, and passing static lambda

jjonescz · 2026-07-01T12:44:47Z

+            var peModule = (PEModuleSymbol)module;
+            var peType = (PENamedTypeSymbol)module.GlobalNamespace.GetMember<NamedTypeSymbol>("C");
+            AssertEx.SetEqual([
+                    "System.Runtime.CompilerServices.IsClosedTypeAttribute(DerivedTypes = {typeof(D1), typeof(D2)})"


Is this going to throw when loaded by reflection? Wouldn't it be better to have a well-known member for the setter instead of the property to match how reflection works?

If IL verification passes, I think we should be good.

I tried to match the pattern established by e.g. WellKnownMember.System_Diagnostics_DebuggerDisplayAttribute__Type. In this case the property is the only thing mentioned in the attribute blob, the setter is essentially ignored.

But, I went ahead and added an expected shape validation of the DerivedTypes property, which will complain if the setter is missing.

AlekseyTs · 2026-07-01T19:39:51Z

+
+                ImmutableArray<KeyValuePair<WellKnownMember, TypedConstant>> namedArguments;
+                // Note: some definitions of IsClosedTypeAttribute may be missing DerivedTypes property.
+                if (systemType is not null and not ErrorTypeSymbol && derivedTypesProperty != null && !derivedTypesProperty.HasUseSiteError)


and not ErrorTypeSymbol

This feels somewhat wrong. If property is there, but we are missing a reference for System.Type, we silently won't emit the property. Perhaps, when we are checking for a presence of the System_Runtime_CompilerServices_IsClosedTypeAttribute__ctor, we should also check that System.Type is "good" (and report an error otherwise) when the property is found.
And, BTW, in general, a type check for ErrorTypeSymbol doesn't detect all "bad" scenarios.

I'm using Binder.GetWellKnownType() now to get the System.Type symbol and expecting it to report an error if the System.Type is bad

AlekseyTs · 2026-07-01T19:40:45Z

There is no requirement for DerivedTypes property to exist or to be well-formed.

Consider going with the rule: "If the property is present, it should be good, or an error is reported."

AlekseyTs · 2026-07-01T19:46:33Z

+                    var arrayOfSystemType = ArrayTypeSymbol.CreateSZArray(systemType.ContainingAssembly, TypeWithAnnotations.Create(systemType, NullableAnnotation.NotAnnotated));
+                    var derivedTypesConstant = new TypedConstant(
+                        arrayOfSystemType,
+                        CandidateClosedSubtypeDefinitions.SelectAsArray(


CandidateClosedSubtypeDefinitions

Will this sequence order-wise always match what CandidateClosedSubtypeDefinitions would return for a type imported from metadata?

I think it's possible that the order will diverge when it comes to certain nested types scenarios.

Basically, the order of types in DerivedTypes property will be deterministic, but, the order might be different than CandidateClosedSubtypeDefinitions returns for the same closed type.

This would come down to difference in the order that the TypeDef table is populated versus the order the source module is traversed in order to discover the subtypes when producing the DerivedTypes argument.

AlekseyTs · 2026-07-01T19:48:23Z

+                }
+                else
+                {
+                    namedArguments = [];


[]

default?

AlekseyTs · 2026-07-01T19:54:02Z

+        comp.MakeTypeMissing(WellKnownType.System_Type);
+
+        var verifier = CompileAndVerify(comp, symbolValidator: verifyMetadata, verify: Verification.Skipped);
+        verifier.VerifyDiagnostics();


verifier.VerifyDiagnostics();

I would expect an error for this scenario

AlekseyTs · 2026-07-01T19:57:05Z

+            }
+            """;
+
+        var comp = CreateCompilation([source, isClosedTypeAttribute], targetFramework: TargetFramework.Net100);


TargetFramework.Net100

I guess we are skipping IL verification for many of the tests because we use this target framework. Why is it necessary?

I think we can still VerifyTypeIL when passing this. I didn't feel that such baselines were very "illustrative" though due to the attribute arguments being displayed as a hex blob.

So I mostly went with using metadata symbol helpers to dig the attribute arguments back out, and, I'm adding some runtime tests to dig through the attribute via reflection as well.

In general, we can run tests without this TargetFramework, it just requires us passing more source declarations, such as CompilerFeatureRequiredAttribute. See test Subtypes_Retargeting_01 for example.

Sorry for the confusion. The IL verification that I was referring to is the PEVerify style verification. The verify: parameter of CompileAndVerify

I changed the DerivedTypesMetadata tests which CompileAndVerify to use the default for verify: (i.e. enforce that verify passes).

AlekseyTs · 2026-07-01T20:00:22Z

Done with review pass (commit 4)

Copilot

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.

RikkiGibson · 2026-07-01T22:30:52Z

+                    Binder.ReportUseSite(wellKnownDerivedTypesProperty, diagnostics, location);
+                    _ = Binder.GetWellKnownType(compilation, WellKnownType.System_Type, diagnostics, location);
+
+                    var isValid = wellKnownDerivedTypesProperty is
+                    {
+                        IsStatic: false,
+                        GetMethod.DeclaredAccessibility: Accessibility.Public,
+                        SetMethod.DeclaredAccessibility: Accessibility.Public,
+                        RefKind: RefKind.None,
+                        ParameterCount: 0,
+                        Type: ArrayTypeSymbol { ElementType: var elementType }
+                    } && elementType.Equals(compilation.GetWellKnownType(WellKnownType.System_Type), TypeCompareKind.AllIgnoreOptions);
+                    if (!isValid)
+                    {
+                        diagnostics.Add(ErrorCode.ERR_ClosedBadDerivedTypesProperty, location);
+                    }


I don't think systemType being an error type is worrisome here. The Binder.GetWellKnownType is supposed to report an error in that case, which is good enough.

I'll fix the code to avoid redundant lookup of the systemType though.

Copilot

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.

RikkiGibson · 2026-07-01T22:46:31Z

+                var systemType = compilation.GetWellKnownType(WellKnownType.System_Type);
+                var derivedTypesProperty = compilation.GetWellKnownTypeMember(WellKnownMember.System_Runtime_CompilerServices_IsClosedTypeAttribute__DerivedTypes);
+
+                ImmutableArray<KeyValuePair<WellKnownMember, TypedConstant>> namedArguments;
+                if (derivedTypesProperty != null)


I don't agree with the comment, we will report a use site error in this case and I think it doesn't matter if we fail to synthesize the attribute.

I'll take a moment to actually add the test case though.

RikkiGibson · 2026-07-01T22:49:25Z

+                    var allDerivedTypesSymbols = isClosedTypeAttributeCtor.ContainingType.GetMembers("DerivedTypes");
+                    foreach (var derivedTypesSymbol in allDerivedTypesSymbols)
+                    {
+                        if (!derivedTypesSymbol.Equals(wellKnownDerivedTypesProperty, TypeCompareKind.AllIgnoreOptions))
+                        {
+                            diagnostics.Add(ErrorCode.ERR_ClosedBadDerivedTypesProperty, location);
+                        }
+                    }


I don't think this matters.

wip

f9605a7

build-analysis Bot mentioned this pull request Jul 1, 2026

[Known Build Error] "VsixUtil.exe" exited with code 57005 #83177

Open

RikkiGibson added 2 commits June 30, 2026 21:17

tests

8c8a962

IL test

06256f3

RikkiGibson marked this pull request as ready for review July 1, 2026 04:43

RikkiGibson requested a review from a team as a code owner July 1, 2026 04:43

Copilot AI review requested due to automatic review settings July 1, 2026 04:43

Copilot started reviewing on behalf of RikkiGibson July 1, 2026 04:43 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

Defend against possibility of a use site error

812d96c

jjonescz reviewed Jul 1, 2026

View reviewed changes

AlekseyTs reviewed Jul 1, 2026

View reviewed changes

RikkiGibson added 2 commits July 1, 2026 13:14

report attribute data at runtime

39938a2

Require good DerivedTypes property. Address other feedback.

d40e4b9

Copilot AI review requested due to automatic review settings July 1, 2026 21:39

Copilot started reviewing on behalf of RikkiGibson July 1, 2026 21:39 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

RikkiGibson added 2 commits July 1, 2026 15:35

fix some null safety issues

0b086f2

cleanup trailing spaces

97ebab4

Copilot AI review requested due to automatic review settings July 1, 2026 22:38

verify DerivedTypesMetadata tests which emit

abdad57

Copilot started reviewing on behalf of RikkiGibson July 1, 2026 22:39 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

Simplify

d8c6b4f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Set IsClosedTypeAttribute.DerivedTypes#84350

Set IsClosedTypeAttribute.DerivedTypes#84350
RikkiGibson wants to merge 10 commits into
dotnet:mainfrom
RikkiGibson:cc-metadata

RikkiGibson commented Jul 1, 2026 •

edited

Loading

RikkiGibson commented Jul 1, 2026

Copilot AI left a comment

RikkiGibson Jul 1, 2026

RikkiGibson Jul 1, 2026

AlekseyTs Jul 1, 2026

jjonescz Jul 1, 2026

jjonescz Jul 1, 2026

AlekseyTs Jul 1, 2026 •

edited

Loading

RikkiGibson Jul 1, 2026

AlekseyTs Jul 1, 2026

RikkiGibson Jul 1, 2026

AlekseyTs commented Jul 1, 2026

AlekseyTs Jul 1, 2026

RikkiGibson Jul 1, 2026

RikkiGibson Jul 1, 2026

AlekseyTs Jul 1, 2026

AlekseyTs Jul 1, 2026

AlekseyTs Jul 1, 2026

RikkiGibson Jul 1, 2026

AlekseyTs Jul 1, 2026

RikkiGibson Jul 1, 2026

AlekseyTs commented Jul 1, 2026

Copilot AI left a comment

RikkiGibson Jul 1, 2026

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

RikkiGibson Jul 1, 2026

RikkiGibson Jul 1, 2026

Labels

4 participants

Uh oh!

Conversation

RikkiGibson commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

RikkiGibson commented Jul 1, 2026

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jul 1, 2026

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jul 1, 2026

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

4 participants

RikkiGibson commented Jul 1, 2026 •

edited

Loading

AlekseyTs Jul 1, 2026 •

edited

Loading