Mark Obsoletions for .NET 5

[jeffhandley]

This contributes the remaining obsoletions from dotnet/designs#139.

• SYSLIB0003 - Code Access Security is not supported or honored by the runtime.
• SYSLIB0004 - The Constrained Execution Region (CER) feature is not supported.
• SYSLIB0005 - The Global Assembly Cache is not supported.
• SYSLIB0006 - Thread.Abort is not supported and throws PlatformNotSupportedException.
• SYSLIB0007 - The default implementation of this cryptography algorithm is not supported.
• SYSLIB0008 - The CreatePdbGenerator API is not supported and throws PlatformNotSupportedException.
• SYSLIB0009 - The AuthenticationManager Authenticate and PreAuthenticate methods are not supported and throw PlatformNotSupportedException.
• SYSLIB0010 - This Remoting API is not supported and throws PlatformNotSupportedException.

Existing obsoletions were updated to reflect the diagnostic id prefix of SYSLIB. BinaryFormatter was moved to SYSLIB0011.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[jeffhandley]

It looks like there are at least a few legitimate test failures from this where compensating changes need to be made for the obsoleted APIs. I'll look at those later this morning.

[jkotas]

Can this be applied unconditionally? (Same as my other comment.)

[jeffhandley]

I think this one needs to remain conditional because we build this for netcoreapp3.0 and netstandard2.0 too.

@steveharter - Can you confirm that I need this #ifdef to apply [Obsolete] to RoAssembly.GlobalAssemblyCache only in net5.0+ only?

[steveharter]

I see no reason to have the condition. MetadataLoadContext doesn't provide a way to "resolve" an assembly from the GAC (even if running under .NET Framework) so this will always be false.

[jeffhandley]

Thanks. I ended up finding it does still need to be conditional, since the DiagnosticId and UrlFormat properties are new in net5.0 and this compiles against lower versions. I'm going to change the logic to be #if GAC_OBSOLETIONS though, so it connotes the reasoning more clearly.

[jeffhandley]

If the types and members are defined in netstandard then forwarded in netframework then a netframework app would never see the attributes nor warnings. A netstandard library would warn that library author appropriately.

Thanks -- I didn't know that. The suggestion makes more sense to me now.

This is intriguing and worth further consideration, but it was called out as a non-goal in the better obsoletion doc. Would it be OK with you if we moved this PR forward but then I submit an issue for us to consider down-leveling the obsoletions to netstandard2.0 in a separate issue, @ericstj?

@GrabYourPitchforks - I've addressed all of your feedback -- thanks for checking so many details!

[jkotas]

Would it be OK with you if we moved this PR forward

Yes, this should be done as follow up - if we decide that it is the right thing to do.

[ericstj]

Yeah I’m ok with that. Folks need to consider how that non-goal plays out in practice to understand what makes the most sense.

[jeffhandley]

@GrabYourPitchforks - I found a regression where we had code calling into RuntimeHelpers.ExecuteCodeWithGuaranteedCleanup, where I'd removed that call but removing that call changed the behavior of the code. I pushed a commit that replicated the behavior that method provided and it fixed the regression, but I'm now second-guessing whether or not we should obsolete that helper method.  I don't actually see any CER logic in it.

        [Obsolete(Obsoletions.ConstrainedExecutionRegionMessage, DiagnosticId = Obsoletions.ConstrainedExecutionRegionDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
        public static void ExecuteCodeWithGuaranteedCleanup(TryCode code, CleanupCode backoutCode, object? userData)
        {
            if (code == null)
                throw new ArgumentNullException(nameof(code));
            if (backoutCode == null)
                throw new ArgumentNullException(nameof(backoutCode));
            bool exceptionThrown = true;
            try
            {
                code(userData);
                exceptionThrown = false;
            }
            finally
            {
                backoutCode(userData, exceptionThrown);
            }
        }

I'm considering removing the obsolete attribute from it and reverting the change where we removed the call to it. What do you think?

[GrabYourPitchforks]

ExecuteCodeWithGuaranteedCleanup call sites shouldn't be removed, since the code has a side effect. Instead, the call sites should be changed to use normal try / finally semantics instead of using this helper routine.

In .NET Framework, the implementation of this method made guarantees about how this code could execute within CERs. The implementation was, roughly:

PrepareDelegate(tryMethod);
PrepareDelegate(finallyMethod);

PrepareConstrainedRegions();
try
{
    tryMethod(userData);
}
finally
{
    finallyMethod(userData);
}

In .NET Core, since CER guarantees cannot be made, it's still a good idea to mark the method as obsolete in the sense of "this method doesn't do what you think it does."

[jeffhandley]

The only check failing is because of the following, which is unrelated to these changes and has been failing on other PRs as well.

Assertion failed '!foundDiff' in 'OpenSsl:GetSslError(Microsoft.Win32.SafeHandles.SafeSslHandle,int,byref):int' during 'Linear scan register alloc' (IL size 99)

[jeffhandley]

@ericstj I filed https://github.com/dotnet/designs/issues/145 for consider the downlevel obsoletion for OOB package APIs.

[jeffhandley]

Well shoot -- it appears I somehow missed the "Classes that derive from CodeAccessSecurityAttribute" category. I'll submit a follow-up PR for those.

[jeffhandley]

dotnet/docs#20894 submitted for the breaking change document.