CA1416 should Ignore diagnostics on throw statements

[buyaa-n]

Describe the improvement

When supported attributes added by SDK for the targeted platform we are seeing warnings in the statements just throwing PNSE, the warnings for the resource string used for the exception message which we couldn't separate for specific build.

Related to dotnet/runtime#44231.

Describe suggestions on how to achieve the rule

We could ignore diagnostics in throw statements as they are not platform specific operation at all

Additional context

For example in net5.0-Browser targeted build SupportedOSPlatform("Browser") attribute will be added to the project's AssemblyInfo.cs by MSBuld, which makes everything included in the assembly are Browser only including resource strings

[System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]
public abstract partial class Aes : System.Security.Cryptography.SymmetricAlgorithm
{
    protected Aes() { throw new System.PlatformNotSupportedException(System.SR.SystemSecurityCryptographyAlgorithms_PlatformNotSupported);  }
    public static new System.Security.Cryptography.Aes Create() { throw new System.PlatformNotSupportedException(System.SR.SystemSecurityCryptographyAlgorithms_PlatformNotSupported);  }
    public static new System.Security.Cryptography.Aes? Create(string algorithmName) { throw new System.PlatformNotSupportedException(System.SR.SystemSecurityCryptographyAlgorithms_PlatformNotSupported);  }
}

So for the above code we will see warnings:

\runtime\artifacts\obj\System.Security.Cryptography.Algorithms\net6.0-Browser-Debug\System.Security.Cryptography.Algorithms.notsupported.cs(13,118):
 error CA1416: This call site is unreachable on: 'Browser'. 'SR.SystemSecurityCryptographyAlgorithms_PlatformNotSupported' is only supported on: 'Browser'.
\runtime\artifacts\obj\System.Security.Cryptography.Algorithms\net6.0-Browser-Debug\System.Security.Cryptography.Algorithms.notsupported.cs(12,74):
 error CA1416: This call site is unreachable on: 'Browser'. 'SR.SystemSecurityCryptographyAlgorithms_PlatformNotSupported' is only supported on: 'Browser'. 
\runtime\artifacts\obj\System.Security.Cryptography.Algorithms\net6.0-Browser-Debug\System.Security.Cryptography.Algorithms.notsupported.cs(14,139):
 error CA1416: This call site is unreachable on: 'Browser'. 'SR.SystemSecurityCryptographyAlgorithms_PlatformNotSupported' is only supported on: 'Browser'. 

cc @jeffhandley @terrajobst

[mavasani]

@buyaa-n FYI: We already have a helper to detect this, which is used by many existing analyzers:

roslyn-analyzers/src/Utilities/Compiler/Extensions/OperationBlockAnalysisContextExtension.cs

Line 14 in 690be17

public static bool IsMethodNotImplementedOrSupported(this OperationBlockStartAnalysisContext context)

Hopefully, this should be a one line fix.

[buyaa-n]

Great, thanks!

[mavasani]

Actually, you might have to extend that helper to detect the platform not supported exception type:

roslyn-analyzers/src/Utilities/Compiler/Extensions/OperationBlockAnalysisContextExtension.cs

Lines 56 to 60 in 690be17

	if (Equals(context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemNotImplementedException), createdExceptionType.OriginalDefinition)
	|| Equals(context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemNotSupportedException), createdExceptionType.OriginalDefinition))
	{
	return true;
	}

So maybe a 2 line fix :-)

[buyaa-n]

I was actually thinking to ignore for any type of exceptions, anyway for now it is fine to ignore for only those exceptions

[buyaa-n]

The helper seems for throw only methods without any other statements, and it covers the issue mentioned, but i think we don't need to flag for exceptions thrown with some other statements too, probably add check for string message as constructor argument, what do you think @jeffhandley @terrajobst? I can add another extension method if we want to handle exception throwing differently.

[assembly: SupportedOSPlatform("Browser")]

public static class SR
{
    public static string Message {get; set;}
    public static void M1() { }
}

[UnsupportedOSPlatform("browser")]
public class Test
{
    void OnlyThrows() // Works well, not warning
    {
        throw new PlatformNotSupportedException(SR.Message);
    }
    void ThrowsWithOtherStatements()
    {
        [|SR.M1()|]; // should only warn here
        throw new PlatformNotSupportedException(SR.Message); // Fail, warning here too
    }
}

[buyaa-n]

Actually, you might have to extend that helper to detect the platform not supported exception type:

roslyn-analyzers/src/Utilities/Compiler/Extensions/OperationBlockAnalysisContextExtension.cs

Lines 56 to 60 in 690be17

	if (Equals(context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemNotImplementedException), createdExceptionType.OriginalDefinition)
	|| Equals(context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemNotSupportedException), createdExceptionType.OriginalDefinition))
	{
	return true;
	}

Thanks @mavasani, I am little worried that adding another exception the condition might break/malfunction other analyzers using that method. Run all tests in the repo no any failures though, maybe its OK.

[mavasani]

@buyaa-n - you can add a flag to the existing extension method to decide if it should flag only throw with specific exception types (not implemented and not supported) or all exception types.

[buyaa-n]

@buyaa-n - you can add a flag to the existing extension method to decide if it should flag only throw with specific exception types (not implemented and not supported) or all exception types.

Sure, i can do that or create separate extension method, just meant about updating the existing method, will update depending on how we want to disable warnings

[jeffhandley]

It's OK with me to ignore throws of PlatformNotSupportedException and NotImplementedException, also ignoring the arguments passed in. Other exception types, especially custom application exceptions, might accept or grab device-specific data though, so we'd want the analyzer to still flag unsupported exceptions/arguments generally.

[buyaa-n]

It's OK with me to ignore throws of PlatformNotSupportedException and NotImplementedException, also ignoring the arguments passed in. Other exception types, especially custom application exceptions, might accept or grab device-specific data though, so we'd want the analyzer to still flag unsupported exceptions/arguments generally.

I was thinking ignore any string argument passed for any exceptions, but i am OK to restrict to only PNSE and NIE as they are the main scenario

[buyaa-n]

Fixed with #4577