Improve some of our recent obsoletions

[jeffhandley]

I took a pass over all changes since release/net5.0 snapped to find places where we didn't fully align to the new guidance for obsoleting APIs and using SYSLIB0### diagnostic id values; This PR updates those obsoletions to align to the guidance and gets us to a state where all new 6.0 obsoletions are using SYSLIB0### diagnostic ids.

Here are the PRs for which the obsoletions are revised:

• Update IgnoreNullValues obsoletion message #41141: System.Text.Json -- JsonSerializerOptions.IgnoreNullValues (/cc @layomia)
• Push Evidence down to System.Security.AccessControl #49411: System.Security.AccessControl (this refactored existing obsoletions; the diagnostic id is not needed) (/cc @ericstj)
• Annotate Interop APIs that always throw PNSE as Obsolete #50666: System.Runtime.Interop -- RuntimeEnvironment (/cc @AaronRobinsonMSFT)

I was also able to replace a NET50_OBSOLETIONS build constant with the NET5_0_OR_GREATER constant that the SDK now provides.

To verify that this PR didn't result in any new or removed [Obsolete] attributes, I ran ApiCompat over all libraries comparing the main build to this branch's build. I verified that there were no occurrences of "CannotRemoveAttribute : Attribute 'System.ObsoleteAttribute'" in either comparison direction (after confirming that accidental additions or removals of obsolete attributes would in fact produce a result of that search).

This change will need to be treated as a preview-to-preview breaking change since the obsoletions from #41141 and #50666 would previously require suppressing through the general CS0618 ID and now they have their own diagnostic ids.

[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.

[AaronRobinsonMSFT]

LGTM - Thanks!

/cc @jkoritzinsky @elinor-fung

[jeffhandley]

Note that there's another open PR that adds SYSLIB0017 and SYSLIB0018, which is why I skipped those.

[jeffhandley]

Preview-to-preview breaking change document issue: dotnet/docs#23891

[jeffhandley]

The dotnet/docs#23856 issue is aggregating all breaking change obsoletion documentation.

[jeffhandley]

@ericstj / @layomia -- if by chance you have any post-merge feedback, let me know and I'll be happy to address it.

[ericstj]

Should a condition be applied here, or instead place one

runtime/src/libraries/Directory.Build.targets

Line 247 in 466deef

<When Condition="'$(IncludeInternalObsoleteAttribute)' == 'true' and ($(TargetFramework.StartsWith('netstandard')) or $(TargetFramework.StartsWith('netcoreapp')) or '$(TargetFrameworkIdentifier)' == '.NETFramework')">

so that the attribute isn't included when the framework carries a sufficient copy?

[jeffhandley]

As best as I can tell, the condition there in runtime/src/libraries/Directory.Build.targets should already be accomplishing that by only applying for netstandard, netcoreapp, or .NETFramework. I think that would exclude net5.0 and newer.

Is there a condition that I'm missing where you think it could be including the attribute when that's unnecessary? And/or do you think the property should be named such that the condition is connoted?

[ericstj]

I suppose you're right. I was thinking of net5.0 as netcoreapp since it's still treated that way behind the scenes, but the way the condition is written it will not apply when folks use net5.0 and we don't use netcoreapp5.0 or netcoreapp6.0 in our repo.

[jeffhandley]

FWIW I had to go double-check that we used net5.0 and net6.0 (not netcoreapp5.0 / netcoreapp6.0) 😉. We could potentially simplify this down to checking to see if the target framework version is less than 5.0.

[jeffhandley]

That approach was used just a few lines above actually. I do think it's more concise.

<When Condition="'$(IncludeInternalObsoleteAttribute)' == 'true' and ('$(TargetFrameworkIdentifier)' != '.NETCoreApp' or $([MSBuild]::VersionLessThan($(TargetFrameworkVersion), '5.0')))">

This makes me somewhat yearn for the implicitly defined constants of NET5_0_OR_GREATER to also be available as properties. 🤔 But I'll queue up a follow-up PR to make this adjustment. I'll wait a bit though to see if you or @layomia have any other feedback.

Pending branch/PR: https://github.com/dotnet/runtime/compare/main...jeffhandley:jeffhandley/obsoletions-cleanup-followup?expand=1

[ericstj]

This makes me somewhat yearn for the implicitly defined constants of NET5_0_OR_GREATER to also be available as properties

Funny: the reason for those constants is that we don't have a complex preprocessor in C#. MSBuild gives you much more flexibility, but there's less prescriptive patterns to follow.

I'm fine so long as we don't unnecessarily duplicate the attribute definition, which was the case already. I like the simplified condition in your diff.

[jeffhandley]

Thanks. I'll leave this as I have it in that pending PR. We'll keep on eye on this to determine if there are enough scenarios to justify MSBuild properties such as NET5_0_OR_GREATER and NET6_0_OR_GREATER.