Annotate unsupported APIs in System.Reflection

[buyaa-n]

As part of #47228 i am running an analyzer to detect APIs throwing PNSE but not annotated with Obsolete/SupportedOSPlatform/UnsupportedOSPlatform attributes, we need to annotate them so that developers get warnings when they use them unexpectedly

For now, I have results only cross-platform and .Net 5.0 or higher builds, analysis of targeted builds or lower env are coming soon where more APIs could be detected

API	Comment	Suggestion	Location
'Assembly.ReflectionOnlyLoad(byte[])'	unconditionally throws on all platforms	add [Obsolete]	libraries\System.Private.CoreLib\src\System\Reflection\Assembly.cs(376,73)
'Assembly.ReflectionOnlyLoad(string)'	unconditionally throws on all platforms	add [Obsolete]	libraries\System.Private.CoreLib\src\System\Reflection\Assembly.cs(378,76)
'Assembly.ReflectionOnlyLoadFrom(string)'	unconditionally throws on all platforms	add [Obsolete]	libraries\System.Private.CoreLib\src\System\Reflection\Assembly.cs(380,78)
'StrongNameKeyPair.StrongNameKeyPair(string)'	unconditionally throws on all platforms	add [Obsolete]	libraries\System.Private.CoreLib\src\System\Reflection\StrongNameKeyPair.cs(33,13)
'StrongNameKeyPair.PublicKey.get'	unconditionally throws on all platforms	add [Obsolete]	libraries\System.Private.CoreLib\src\System\Reflection\StrongNameKeyPair.cs(36,13)
'Type.ReflectionOnlyGetType(string, bool, bool)'	unconditionally throws on all platforms	add [Obsolete]	libraries\System.Private.CoreLib\src\System\Type.cs(544,110)
'Activator.CreateInstance(Type, BindingFlags, Binder?, object?[]?, CultureInfo?, object?[]?)'	throws with non platform related condition	consider changing the exception to NotSupportedException	libraries\System.Private.CoreLib\src\System\Activator.RuntimeType.cs(33,17)

Note: We are suggesting to add [Obsolete] with the corresponding message for APIs supported in .Net framework but not supported in .NetCore, if it is only supported on a specific platform(s) consider adding [SupportedOSPlatform("platformName")] or if it is unsupported on a specific platform(s) [UnsupportedOSPlatform("platformName")] attribute instead

Suggestions for changing the exception is optional

cc @jeffhandley @terrajobst @GrabYourPitchforks

[buyaa-n]

Notes need to consider:

If the API has an issue associated with future support we are not suggesting to add [Obsolete], we need to make sure that we are not gonna support the API ever and related issue should be closed, then we can add [Obsolete]
If the API is internal/private the annotation is not necessary, but they could point you to the public API needs annotation

[buyaa-n]

While annotating APIs in StrongNameKeyPair noticed that all APIs in that type is not supported in .NetCore and existing APIs only throwing PNSE or no-op. I am planning to update the no-op constructor overloads to throw PNSE and [Obsolete] the entire type. Which also causing to Obsolete AssemblyName.KeyPair property. This type and API doesn't have real usage in runtime repo but from https://source.dot.net/ we see it is referenced a lot from MSBuild repo (most likely not used for .NetCore build).

Please let me know if you have any issue/comment with Obsoleting StrongNameKeyPair and AssemblyName.KeyPair property @terrajobst @stephentoub @bartonjs

[bartonjs]

I am planning to update the no-op constructor overloads to throw PNSE and [Obsolete] the entire type.

Have we been obsoleting types, as opposed to just their ctors (and specific methods, when needed)? I thought we generally didn't like putting the marker on the type itself.

[jeffhandley]

We did obsolete some types in .NET 5, particularly in CAS.
https://docs.microsoft.com/en-us/dotnet/core/compatibility/syslib-warnings/syslib0003

[GrabYourPitchforks]

StrongNameKeyPair and AssemblyName.KeyPair are a bit of a weird special-case. Buyaa identified that nobody aside from msbuild seems to be using this property, and once the ctors are changed to be always-throwing the only thing they can actually pass via the property setter is nullptr. And if you can't ever instantiate a class, that seems like it'd be a good candidate to obsolete the entirety of the type.

[bartonjs]

So long as we're not doing something new, seems fine with me. Someone who has a utility method for working with one that wants it to stick around in both netstandard and net(core) compiles (to avoid surface area reduction problems) can just suppress it. They'd have to anyways, if all the members are also Obsolete.

In my head, the "nicest" thing is to obsolete the ctors (after making them throw) (and any property/method that returns one), and leave the type and methods as unannotated. You'll never get one in Core, so it doesn't matter that the member would throw, but it avoids cross-compile noise. But then it's annoying to us because the tool needs to understand that pattern, or except types on a one-off basis. Since it's not (to my knowledge) a type with any significant usage, the noise to the handful using it is probably fine.

[buyaa-n]

In my head, the "nicest" thing is to obsolete the ctors (after making them throw) (and any property/method that returns one), and leave the type and methods as unannotated. You'll never get one in Core, so it doesn't matter that the member would throw, but it avoids cross-compile noise. But then it's annoying to us because the tool needs to understand that pattern, or except types on a one-off basis. Since it's not (to my knowledge) a type with any significant usage, the noise to the handful using it is probably fine.

Yeah, that could be less breaking change, then probably leave AssemblyName.KeyPair as is without obsoleting

[GrabYourPitchforks]

Spoke with Buyaa offline. We may need to revert the change to the internal AssemblyName ctor since it has ripple effects throughout the native reflection stack. But that's doesn't impact the larger goal of getting these annotations visible to our users, and we can always do this cleanup later when we have some free cycles.

[terrajobst]

@bartonjs

Have we been obsoleting types, as opposed to just their ctors (and specific methods, when needed)? I thought we generally didn't like putting the marker on the type itself.

We didn't used to because types can be very noisy. Since we can now provide diagnostic IDs, I don't see an issue with obsoleting types. In fact, I much prefer that over obsoleting .ctors because it expresses the intent more clearly.

[buyaa-n]

We didn't used to because types can be very noisy. Since we can now provide diagnostic IDs, I don't see an issue with obsoleting types. In fact, I much prefer that over obsoleting .ctors because it expresses the intent more clearly.

@terrajobst I might be misunderstood, but didn't we checked in the last meeting that Obsoleting with diagnostic ID was also a warning at the same level? Which makes me think it still will be very noisy. Anyway, in case we Obsolete the type with ID what diagnostic ID i should use?

[jeffhandley]

what diagnostic ID i should use?

The PR for obsoleting the API should include adding the API to the table in the list of obsoletions document. And you can just claim the next SYSLIB id value.

[buyaa-n]

Thanks, Jeff, so noise will be reduced not by the warning count/level, but because it can be suppressed by the custom diagnostic ID without affecting other Obsoletions.

I have got another question, should we also suggest adding [EditorBrowsable(EditorBrowsableState.Never)] for all of these obsoletions?

[terrajobst]

@terrajobst I might be misunderstood, but didn't we checked in the last meeting that Obsoleting with diagnostic ID was also a warning at the same level? Which makes me think it still will be very noisy. Anyway, in case we Obsolete the type with ID what diagnostic ID i should use?

Not sure I understand. Obsoleting a type can cause multiple warnings because it basically warns whenever the type is used, even if passthrough. Before, the user could either #pragma suppress each occurrence (not practical) or disable obsoletion in its entirety (which we don't want). Now we can give the type (or family of types) as single diagnostic ID which makes a single, targeted suppression possible.

[buyaa-n]

If we not obsolete the entire type I will not need to touch the references in the AssemblyName type, it doesn't make sense to disable warnings for all those references as they are not really used. If we want to do a clean up of unused StrongNameKeyPair field/property and ctor parameter from AssemblyName as @GrabYourPitchforks pointed out that requires do clean up throughout the native reflection stack, which i am not sure how safe to clean up. But i will give it try if we want to do the clean up with the PR, else i will just obsolete the ctors and method instead of the entire type.