Keep Obsolete and EditorBrowsable attributes consistent between ref and src and DefineConst clean-up

[ViktorHofer]

Contributes to #58163

Commit 1: NET5_0_OR_GREATER, NET6_0_OR_GREATER -> NETCOREAPP
The minimum supported .NETCoreApp version in the repository is 6.0,
hence defines that check on older versions and above can be replaced
with the versionless NETCOREAPP define.
There is no need to version APIs below the minimum supported .NETCoreApp
version.

No manual change, only replace operations:

• NETCOREAPP3_1_OR_GREATER -> NETCOREAPP
• NET5_0_OR_GREATER -> NETCOREAPP
• NET6_0_OR_GREATER -> NETCOREAPP

Commit 2: Sync ObsoleteAttribute between ref and src
ApiCompat wasn't enabled to check if the ObsoleteAttribute is in sync
between the ref and the src assembly. Enabling it showed numerous
mismatches which this commit fixes.

Also making sure that the ApiCompat run that compares the live build
against the previous version of .NETCoreApp and .NETStandard2.x doesn't
complain about ObsoleteAttribute API changes as those are intentional.

Commit 3: Sync EditorBrowsableAttribute between ref and src
The EditorBrowsableAttribute attribute wasn't enabled to be checked by
ApiCompat and in many cases there were mismatches. The most prominent
were InteropServices.

@eerhardt I hope it's ok that I ask you to review this PR.

[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: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Contributes to #58163

Commit 1: NET5_0_OR_GREATER, NET6_0_OR_GREATER -> NETCOREAPP
The minimum supported .NETCoreApp version in the repository is 6.0,
hence defines that check on older versions and above can be replaced
with the versionless NETCOREAPP define.
There is no need to version APIs below the minimum supported .NETCoreApp
version.

No manual change, only replace operations:

• NETCOREAPP3_1_OR_GREATER -> NETCOREAPP
• NET5_0_OR_GREATER -> NETCOREAPP
• NET6_0_OR_GREATER -> NETCOREAPP

Commit 2: Sync ObsoleteAttribute between ref and src
ApiCompat wasn't enabled to check if the ObsoleteAttribute is in sync
between the ref and the src assembly. Enabling it showed numerous
mismatches which this commit fixes.

Also making sure that the ApiCompat run that compares the live build
against the previous version of .NETCoreApp and .NETStandard2.x doesn't
complain about ObsoleteAttribute API changes as those are intentional.

Commit 3: Sync EditorBrowsableAttribute between ref and src
The EditorBrowsableAttribute attribute wasn't enabled to be checked by
ApiCompat and in many cases there were mismatches. The most prominent
were InteropServices.

@eerhardt I hope it's ok that I ask you to review this PR.

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	area-Meta
Milestone:	-

[ViktorHofer]

The builds are now passing (quite complex to build locally for all configurations targeting the different versions of System.Private.CoreLibs). It would be great if we could get the remaining discussions solved until EOD :)

[eerhardt]

This one seems surprising. Are we sure GetSchema() should be EBS.Never?

cc @dotnet/area-system-xml

[ViktorHofer]

ping @dotnet/area-system-xml

[krwq]

cc @StephenMolloy @HongGit

[ViktorHofer]

This is the last open discussion. Would love to reach conclusion today.

[eerhardt]

I think this falls into the

For the changes that are just updating src to match the ref, I'd be fine with opening a follow-up issue for each.

bucket. So I guess I wouldn't let this one discussion block you.

[ViktorHofer]

Fair enough as System.Private.Xml is an internal assembly anyway so customers never bind on it.

[bartonjs]

IApplicationResourceStreamResolver can't change from Obsolete-as-Error to Obsolete-as-Warning.

Until we can figure out a tooling approach that works for that, this will just have to be a ref vs source mismatch.

[ViktorHofer]

IApplicationResourceStreamResolver can't change from Obsolete-as-Error to Obsolete-as-Warning.
Until we can figure out a tooling approach that works for that, this will just have to be a ref vs source mismatch.

@bartonjs Updated the PR. For the sake of this PR I kept the mismatch as is and added a config to make ApiCompat ignore the mismatch (without a big hammer approach as before).

I still think we should find a way to avoid this mismatch and I'm not yet certain if the fault lies in tooling. Based on @ericstj's earlier investigation on this matter, the compiler emits the insuppressible CS error for typeof(Type) statements when the passed-in Type is obsolete. To build upon @bartonjs's previous suggestions, here's a complete set of possibilities:

1. Ask the roslyn team to make it possible to suppress that error so that typeof() statements on Obsolete types are possible.
2. Emit the typeforwards in IL instead of in C#
3. Don't typeforward to this type and live with MissingMethodExcepetion thrown (not a sustainable solution as we will have more Obsolete types in the future).
4. Ask the roslyn team to make it possible to change attributes between the produced reference assembly and the source assembly without requiring an additional csc invocation (just for the reference assembly via the ProduceOnlyReferenceAssembly switch).

Did I miss something? We should come up with a strategy that works for other types as well as we will add more ObsoleteAttributes in the future.

@jaredpar @agocke @ericstj @eerhardt

[ghost]

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

[ViktorHofer]

Unsure if I should wait a few more days for people to respond to the remaining questions or just open follow-up issues and get this in? Opinions @bartonjs @eerhardt? As I'm not active on corporate mail or Teams I can't annoy people offline ;)

[eerhardt]

wait a few more days for people to respond to the remaining questions or just open follow-up issues and get this in? Opinions @bartonjs @eerhardt? As I'm not active on corporate mail or Teams I can't annoy people offline ;)

I pinged a couple people.

For the changes that are just updating src to match the ref, I'd be fine with opening a follow-up issue for each.

But the changes to the ref assemblies shouldn't be merged without approval IMO.

[ericstj]

I added breaking-change tag to this. For every case where a reference assembly gains an ObsoleteAttribute or that attribute changes severity we need signoff by the area owners and we need to document the break.

In general I'm supportive of this change -- omitting these attributes from reference assemblies is an artifact of the past when treated our reference assemblies as authored contracts with intentional differences -- that's no longer the case and excluding these is more likely to result in bugs than any benefit.

[ViktorHofer]

In general I'm supportive of this change -- omitting these attributes from reference assemblies is an artifact of the past when treated our reference assemblies as authored contracts with intentional differences -- that's no longer the case and excluding these is more likely to result in bugs than any benefit.

Actually we were not just omitting these attributes from reference assemblies but also from source assemblies. It's currently wild west in main and there's no protection for it as ApiCompat doesn't check compatibility of these attributes. What makes this really concerning is that since .NET 6 we aren't shipping the contract anymore for out-of-band projects which means that package consumers bind against the implementation assembly. In the extreme case, for libraries which ship both out-of-band and inbox, you might even get different results based on what is used.
Fixing this inconsistency makes sure that our api surface area doesn't unintentionally diverge and by adding protection makes sure that this won't regress again in the future.

But the changes to the ref assemblies shouldn't be merged without approval IMO.

@eerhardt we should treat source and ref changes equally going forward as package consumers bind against the implementation assembly and not the contract anymore. FWIW if we succeed in collapsing source and ref projects, this will be a no brainer anyway.

[ViktorHofer]

I would need an approval please :) cc @eerhardt

Feedback addressed

[eerhardt]

These changes look good to me.

[ericstj]

But the changes to the ref assemblies shouldn't be merged without approval IMO.

@eerhardt we should treat source and ref changes equally going forward as package consumers bind against the implementation assembly and not the contract anymore. FWIW if we succeed in collapsing source and ref projects, this will be a no brainer anyway.

This is true, however we still ship reference assemblies and you introduced new obsolete attributes to those. Even though you're just resolving an inconsistency it will have impact to customers. We need folks that own that surface area to understand the changes you are making and approve of them. Specifically: all the additions of ObsoleteAttributes to System.Net reference assemblies.

[ViktorHofer]

This is true, however we still ship reference assemblies and you introduced new obsolete attributes to those. Even though you're just resolving an inconsistency it will have impact to customers.

@ericstj I think you misunderstood what I meant. I don't deny that the changes that I made here are breaking changes and that they should be documented. Did I say something that made you assume that I wouldn't treat them as breaking? On the contrary, I'm pointing out that any type of public api change is breaking, irrelevant of if it's made in a reference assembly or in an implementation assembly (except for private assembly like System.Private.Uri which customers never bind on).

We need folks that own that surface area to understand the changes you are making and approve of them. Specifically: all the additions of ObsoleteAttributes to System.Net reference assemblies.

Didn't we already do that as part of the review process of this PR? @eerhardt was so kind to loop in area-owners and @karelz approved the System.Net surface area changes and other area-owners approved theirs.

[ericstj]

Did I say something that made you assume that I wouldn't treat them as breaking?

I read this exchange as @eerhardt asking for approval of ref changes (also something I asked for above), then a reply to him quoting that statement saying we should treat source and ref changes equally going forward followed by an explicit ask for approval again. The part missing for me was where we got approval for the ref changes. I think I wouldn't have misunderstood this if there was an explicit agreement with @eerhardt or if there was some other evidence of the approval on the main thread of the PR (perhaps by restating it).

I see that now, that @karelz's feedback was hidden in the collapsed comments and didn't see any other signoffs or comments on the main thread so it appeared like we were missing that feedback/approval. Glad we had NCL have a look. I think we're on the same page now 👍.

[ericstj]

Doc was created as dotnet/docs#31401