Reference source files aren't representing the public api contract that customers bind on

[ViktorHofer]

Reference source files under "src/libraries/*/ref" are generated and updated on demand via the GenAPI tooling. GenAPI inspects the API surface area of a library's intermediate assembly and based on different inputs, emits the reference source file. One of the inputs to GenAPI is an attribute exclusion list to make it possible to avoid certain attributes from the reference source file. This option is used by our libraries and configured via this file: https://github.com/dotnet/runtime/blob/main/eng/DefaultGenApiDocIds.txt.

This means that the libraries' reference assemblies do not contain any of these attributes (unless someone manually updates the reference source file) and by that diverge from their counterpart (implementation assembly).

Admittedly, this makes implementing #58163 much harder but there's a more important present issue:
Since .NET 6, reference assemblies aren't part of our out-of-band packages anymore. That means that our customers when consuming our packages, bind/compile against the package's implementation assembly and not its reference assembly. Therefore the API surface area that we carefully maintain via the reference source files isn't the actual API surface area that our customers target.

As an example, the ObsoleteAttribute and EditorBrowsableAttribute types were until just recently not in sync between the reference and the implementation assembly and some of the recent obsoletion changes potentially didn't reach customers.

Note that Roslyn's RefOut/RefOnly feature doesn't omit attributes and the public api surface area of their emitted reference assemblies match their implementation assembly as closely as possible.

I suggest that we stop omitting attributes from the reference source so that we have an honest representation of the public api surface area that our customers bind on. FWIW as soon as we start leveraging Roslyn to generate our reference assemblies we won't be able to omit attributes anymore.

@eerhardt @ericstj

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

Issue Details

---

Reference source files under "src/libraries/*/ref" are generated and updated on demand via the GenAPI tooling. GenAPI inspects the API surface area of a library's intermediate assembly and based on different inputs, emits the reference source file. One of the inputs to GenAPI is an attribute exclusion list to make it possible to avoid certain attributes from the reference source file. This option is used by our libraries and configured via this file: https://github.com/dotnet/runtime/blob/main/eng/DefaultGenApiDocIds.txt.

This means that the libraries' reference assemblies do not contain any of these attributes (unless someone manually updates the reference source file) and by that diverge from their counterpart (implementation assembly).

Admittedly, this makes implementing #58163 much harder but there's a more important present issue:
Since .NET 6, reference assemblies aren't part of our out-of-band packages anymore. That means that our customers when consuming our packages, bind/compile against the package's implementation assembly and not its reference assembly. Therefore the API surface area that we carefully maintain via the reference source files isn't the actual API surface area that our customers target.

As an example, the ObsoleteAttribute and EditorBrowsableAttribute types were until just recently not in sync between the reference and the implementation assembly.

Note that Roslyn's RefOut/RefOnly feature doesn't omit attributes and the public api surface area of their emitted reference assemblies match their implementation assembly as closely as possible.

I suggest that we stop omitting attributes from the reference source so that we have an honest representation of the public api surface area that our customers bind on. FWIW as soon as we start leveraging Roslyn to generate our reference assemblies we won't be able to omit attributes anymore.

@eerhardt @ericstj

Author:	ViktorHofer
Assignees:	-
Labels:	Design Discussion, area-Infrastructure-libraries
Milestone:	7.0.0

[stephentoub]

I suggest that we stop omitting attributes from the reference source

No 😄

Reference source is for the public surface area. For better or worse, there are many attributes that are about implementation detail and not about public surface area. Such attributes must not be in the refs. It's effectively then guaranteeing implementation publicly, causing unnecessary churn in the public refs when private details change and all the fallout that yields, etc.

[ViktorHofer]

Did you see the part where I'm explaining that the implementation assembly is the contract when a reference assembly doesn't exist? In example all the out of band packages that we ship.

AspNetCore's reference assemblies are generated by roslyn which doesn't omit such attributes. Customers who ship their library via nuget packages don't include reference assemblies and even if they did, these attributes would be present. Are you saying that our partners and customers are all incorrectly authoring their contract and leaking implementation details?

[stephentoub]

Did you see the part where I'm explaining that the implementation assembly is the contract when a reference assembly doesn't exist?

Yes

Are you saying that our partners and customers are all incorrectly authoring their contract and leaking implementation details?

I'm saying that we have a much higher bar than most, and if we're going to use reference assemblies at all, we should do it right. Including implementation details in reference assemblies is not doing it right. And, yes, anyone including attributes like MemberNotNull(_privateField) in their ref assemblies is leaking implementation details; that's not an opinion, that's a fact.

[ViktorHofer]

That means that all the libraries that don't ship a reference assembly (all out of band libraries) must not use attributes which are leaking implementation details like the one discussed.

Also as the issue title suggests, we should decide if we want our reference source file to represent the public API surface area that consumers bind on or a theoretical optimized contract which isn't used anywhere.

Don't get me wrong, I absolutely agree with your statement that a contract shouldn't leak implementation details but it also sounds like the entire ecosystem isn't honoring these principals which is irritating.

In addition to that it sounds like Roslyn isn't authoring contract assemblies correctly as they leak implementation details.

[stephentoub]

That means that all the libraries that don't ship a reference assembly (all out of band libraries) must not use attributes which are leaking implementation details like the one discussed.

No, it doesn't mean that; it means by not shipping a reference assembly we're apparently choosing to leak implementation details. It doesn't mean we should start leaking implementation details everywhere else.

[ViktorHofer]

Thanks for the responses so far.

@stephentoub @eerhardt @bartonjs @ericstj is anyone actually concerned that the reference source doesn't represent the libraries' public API surface area when they ship without a reference assembly (ie packages)?

Asking because if not, I will just close this issue and not think about it any further. I believed that we want the checked in reference source to be the actual contract but after this discussion I'm not so sure about that anymore.

[stephentoub]

I believed that we want the checked in reference source to be the actual contract

The purpose of checked in reference source is to be the actual contract. That's its raison d'etre. Which is why you see me pushing back on including things that aren't part of the contract.

is anyone actually concerned that the reference source doesn't represent the libraries' public API surface area when they ship without a reference assembly

In an ideal world, I think we should have reference assemblies for everything we ship, and they should contain the contract, the whole contract, and nothing but the contract. We've skimped on that in the case of some of our assemblies. We could choose to say that for cost reasons we're not going to fix that, or for concerns about download size we're not going to fix that, or whatever, and that may be fine. If we stay on that path, however, we shouldn't use that as an argument to then sacrifice the correctness of the contract in places where we do ship one.

[bartonjs]

For the packages that ship without being rid-split, I don't have any real issue with using the normal "the lib is the ref" model. It's what most packages that most people ship will do, and is fine. (I'd actually prefer that we used intentional ref DLLs in those packages, too, but I can buy both the "it's not normal" and "it's not free" (increases the size of the nupkg) arguments)

Once there's a RID-split then it's important to understand what's "the contract" vs the implementation (not to mention that we need something in the AnyOS slot anyways). Anywhere we have those in our OOB packages (System.Security.Cryptography.Pkcs springs to mind) we need the ref. And many of our inbox assemblies also are "RID" split, and so having the separate contract assembly provides for the one true view for the build.

is anyone actually concerned that the reference source doesn't represent the libraries' public API surface area when they ship without a reference assembly

Yes, and no.

The best way to explain my position here is "the ref.cs file represents anything that has made it through the API review process". Looking at its history for any particular assembly shows the customer-visible changes over time... we added this, we obsoleted that... etc. [MemberNotNull] has no bearing on that unless it's for protected/public fields... and we don't have those (and our guidance to others is to not have them, either).

So the ref.cs should absolutely have anything in it that might make an impact on our docs (obsoletion, the member existing, etc), and should represent the idealized customer view. We don't include private members in it, because they don't matter. Any attributes that were feeding data to source generators wouldn't matter. Things that don't matter wouldn't go to API Review, and therefore shouldn't be in the file.

In a perfect world, the things that don't matter also wouldn't be in the reference assembly that the customer used, either.