Change virtuals in abstract classes to abstract.

[brentschmaltz]

We added methods to an abstract class in a minor release and needed to mark them virtual and throw NotImplementedException. We should take the breaking compile change in the 8.x major release. TokenHandler https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/55cc10ea53503b129afd55734ad9e9dd8203b339/src/Microsoft.IdentityModel.Tokens/TokenHandler.cs#L66 https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/55cc10ea53503b129afd55734ad9e9dd8203b339/src/Microsoft.IdentityModel.Tokens/TokenHandler.cs#L76 https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/55cc10ea53503b129afd55734ad9e9dd8203b339/src/Microsoft.IdentityModel.Tokens/TokenHandler.cs#L85 SignatureProvider https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/55cc10ea53503b129afd55734ad9e9dd8203b339/src/Microsoft.IdentityModel.Tokens/SignatureProvider.cs#L105 https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/55cc10ea53503b129afd55734ad9e9dd8203b339/src/Microsoft.IdentityModel.Tokens/SignatureProvider.cs#L118 https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/55cc10ea53503b129afd55734ad9e9dd8203b339/src/Microsoft.IdentityModel.Tokens/SignatureProvider.cs#L150

[halter73]

I'm not sure this is a good idea. I understand wanting to make breaking changes in major release, but this is more than a compile-time breaking change. This will likely cause runtime TypeLoadExceptions in scenarios where you would not have gotten a NotImplementedException previously.

Take SignatureProvider for example. There could be code out there in nuget packages or other prebuilt assemblies that only implement byte[] Sign(byte[] input), bool Verify(byte[] input, byte[] signature) and void Dispose(). It'd be nice if it also implemented the new overloads that support offsets and spans, but the existing implementations that don't are still usable as long as no one actually calls the not implemented methods.

If we made the new overloads abstract instead of a virtual that throws a NotImplementedException, apps that upgrade to 8.x would be unable to make use of any the methods that are implemented because will no longer be able to instantiate an instance of a derived type compiled against an earlier version of Microsoft.IdentityModel.Tokens which did not override the now-abstract methods. You'd get an error like the following when trying to construct it:

Unhandled exception. System.TypeLoadException: Method 'Sign' in type 'DerivedSignatureProvider' from assembly 'SomeNugetPackageOrDll, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.

This is why types like Stream didn't add any abstract methods to support things like ReadOnlySpan<byte>. Instead, the virtual method copies the span into an array and calls the Write method which has always been abstract. I think SignatureProvider and TokenHandler should do something similar. If the virtual method absolutely cannot be written in terms of other methods which have always been abstract, we should keep the virtual that throws a NotImplementedException.

In order to achieve a compile-time only break, we could add an analyzer that ensure that derived types override these virtual methods. This would be good even if the virtual method does something like copy a span into an array, because overriding will likely make things faster. If the virtual always throws, the analyzer warning could be more severe. ProvideStreamMemoryBasedAsyncOverrides is a Roslyn analyzer for Stream that does this.