Replace use of DAMT.All in ComponentModel with a more restricted set

[MichalStrehovsky]

Draft for measurements. I assume this will need API review. This is technically a breaking change. Would fix #106366.

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-componentmodel-dataannotations
See info in area-owners.md if you want to be subscribed.

[MichalStrehovsky]

@steveharter @eerhardt any opinion about re-annotating ComponentModel with a more restricted annotation?

We used .All in multiple places of the framework whenever we needed to access private members in base type (e.g. DynamicallyAccessedMemberTypes.NonPublicMethods matches what BindingFlags.NonPublic will do and therefore doesn't look at nonpublics in the base - we had spots in ComponentModel where we walked base hierarchies looking for privates and .All was the only annotation that allowed this).

At the time ComponentModel was annotated we didn't have .AllMethods and friends.

The difference between .All and the replacement here is that .All means "all members, public and private, in bases and in implemented interfaces, and all implemented interfaces". The new annotation is "all members, public and private, in bases, and all implemented interfaces". I.e. we skip making members of interfaces available for reflection.

So if someone previously did annotatedType.GetInterfaces()[0].GetMethod("GetEnumerator") it would work. It will not work now.

I reviewed uses of GetInterface/GetInterfaces here and doesn't look like we're affected.

The only potential break here is that someone else could have taken a dependency on this being annotated .All.

A mitigating factor right now is that annotatedType.GetInterfaces()[0].GetMethod("GetEnumerator") currently generates trimming warning even if annotatedType is annotated as .All (we preserve the members, but we didn't teach dataflow analysis that this is safe). I'm fixing that in #114149 so it will become more likely someone could take advantage of this in the future.

[sbomer]

The only public APIs where I see the new annotation flowing out of ComponentModel are:

• MetadataTypeAttribute.MetadataClassType
• RangeAttribute.OperandType
• DbConnectionStringBuilder.GetType()

I don't see any reason those APIs would need to support getting interfaces, so I'm in favor of taking the breaking change (especially with considering the mitigating factor you mentioned).

[MichalStrehovsky]

@steveharter @eerhardt any opinion about re-annotating ComponentModel with a more restricted annotation?

@steveharter do you have any opinion about this?

[MichalStrehovsky]

@steveharter @eerhardt any opinion about re-annotating ComponentModel with a more restricted annotation?

@steveharter do you have any opinion about this?

We're running out of time to make this change. If we don't do this for .NET10, we'll lose the mitigating factor I mentioned above (I'll repeat it below). The breaking change would be even more breaking. The measurements over at MichalStrehovsky/rt-sz#118 (comment) suggest this could be a 6.3% size savings for for our benchmark app TodosApi when using the options validator (see #106366 for context).

A mitigating factor right now is that annotatedType.GetInterfaces()[0].GetMethod("GetEnumerator") currently generates trimming warning even if annotatedType is annotated as .All (we preserve the members, but we didn't teach dataflow analysis that this is safe). I'm fixing that in #114149 so it will become more likely someone could take advantage of this in the future.