Add missing DynamicallyAccessMembers attributes to System.Data.Common/ref

[ViktorHofer]

The existing APICompat tooling doesn't indicate a compatibility error when comparing the contract assembly against the implementation assembly and the [return: ...] attributes aren't in sync. The new tooling that is being bootstrapped in dotnet/runtime via #73263 does flag them.

In this case a few type members were missing the DynamicallyAccessedMembers return attribute in the contract.

cc @smasher164, @ericstj

[dotnet-issue-labeler]

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, to 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]

Tagging subscribers to this area: @roji, @ajcvickers
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The existing APICompat tooling doesn't indicate a compatibility error when comparing the contract assembly against the implementation assembly and the [return: ...] attributes aren't in sync. The new tooling that is being bootstrapped in dotnet/runtime via #73263 does flag them.

In this case a few type members were missing the DynamicallyAccessedMembers return attribute in the contract.

cc @smasher164, @ericstj

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	area-System.Data
Milestone:	-

[ericstj]

Seems a reasonable change. Do we have confirmation with folks like @joperezr or @eerhardt that the DynamicallyAccessedMembers attribute is one that's important to represent in the reference assemblies? I think it is - for linking a shared framework application, but I wanted to confirm this attribute is important and talk about the scenario where it's important.

Probably it would be a good idea to start capturing the list of attributes we think of as public surface area and why.

[eerhardt]

Do we have confirmation with folks like @joperezr or @eerhardt that the DynamicallyAccessedMembers attribute is one that's important to represent in the reference assemblies?

It is important because we have a Roslyn analyzer that runs when you set PublishTrimmed=true or PublishAot=true in your app (or EnableTrimAnalyzer=true in your library). So you get build-time analysis of your reflection usages to know if it is trim-compatible or not. See https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming#enable-project-specific-trimming

[ViktorHofer]

Probably it would be a good idea to start capturing the list of attributes we think of as public surface area and why.

AFAIK in dotnet/runtime we agreed on that every attribute exposed in the implementation is public surface, except for the ones that we explicitly exclude in https://github.com/dotnet/runtime/blob/main/eng/DefaultGenApiDocIds.txt. cc @stephentoub

Seems a reasonable change. Do we have confirmation with folks like @joperezr or @eerhardt that the DynamicallyAccessedMembers attribute is one that's important to represent in the reference assemblies? I think it is - for linking a shared framework application, but I wanted to confirm this attribute is important and talk about the scenario where it's important.

Presumably, as we already expose that attribute in some of our contract assemblies, we at some point made the decision that it's valuable to have it encoded there. In example, Microsoft.Extensions.Options already has it:

runtime/src/libraries/Microsoft.Extensions.Options/ref/Microsoft.Extensions.Options.cs

Lines 16 to 17 in 7aae884

	public static Microsoft.Extensions.DependencyInjection.IServiceCollection ConfigureOptions(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] System.Type configureType) { throw null; }
	public static Microsoft.Extensions.DependencyInjection.IServiceCollection ConfigureOptions<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] TConfigureOptions>(this Microsoft.Extensions.DependencyInjection.IServiceCollection services) where TConfigureOptions : class { throw null; }

[ViktorHofer]

It is important

/backport to release/7.0

[ViktorHofer]

/backport to release/7.0

[github-actions]

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3099956637

[ericstj]

I still think it's a good idea to call out which attributes are public and why. We do similar things to describe what API is public and what is a breaking change.

[stephentoub]

I think @ViktorHofer's point was we currently "call out" which attributes aren't public, via that txt file. Documenting which aren't public more explicitly seems like a good thing to do, but having it be a list of "what's public" vs a list of "what's not public" seems challenging, given there's a never-ending list of attributes, and the number that aren't considered public are the minority.

[ericstj]

I agree with @ViktorHofer that it's not currently part of the process. I'm not suggesting any change here but we've actually talked in the past about how we treat attributes as Public API and how we don't have very good structure around that.

As a user, what am I supposed to think when I'm told that my library is not compatible with my previous version because attribute System.Foo is different? I'd want to know "why is that a problem?" and "how will my consumer break?". We describe such things in our breaking change documentation around API changes. There are also plenty of attributes that have special rules about what types of changes are compatible and not (expansion of flags might not be breaking, changing a value while not removing the attribute might not be breaking, etc).

Testing for equality for all attributes and excluding some was never a careful intentional decision. It was a cheap heuristic we put in place in 3.0 where we could afford to suppress the noise.

number that aren't considered public are the minority

I was actually holding the opposite opinion. We started out .NETCore by stripping most attributes from our API, then only selectively added those back which mattered. When someone creates an attribute and applies it to API, I would assert that it's not typically meaningful to other libraries consuming the API, since the compiler doesn't know about that new attribute to observe it in a meaningful way.

Wouldn't the community benefit from some sort of documentation that described what an attribute was used for and how it impacts compatibility? I'm not looking for us to go off and spend weeks building up documentation, but I do think it makes sense to collect the set of attributes that are important to public API and describing what is a breaking change for them and why, somewhere.

[stephentoub]

There are an unbounded number of attributes, only some of which are in our control. More are added every release. We need to take an explicit action for an attribute to be stripped, which we rarely do, and so everything naturally works when the list is those things we explicitly remove; the public message is that all attributes are included unless it's on the short list of those explicitly removed.

When someone creates an attribute and applies it to API, I would assert that it's not typically meaningful to other libraries consuming the API, since the compiler doesn't know about that new attribute to observe it in a meaningful way.

I don't believe that's true. Many new compiler features add new attributes, many injected by the compiler, that are critical to the public surface area. For example, there are multiple attributes related to the new scoping features, only one of which is actually exposed from the core libraries. But it might be worth auditing to see what exactly we're dealing with.

Wouldn't the community benefit from some sort of documentation that described what an attribute was used for and how it impacts compatibility?

Sure, we could document the compatibility meaning and impact of every attribute, including those that are implementation detail of how the language encodes various features. I'm not sure if that's a good idea or not, but it's also separate from documenting which attributes we strip because we don't consider them part of our public contract.