Roslyn-analyzers: CA1062: Validate arguments of public methods - when using custom extension method

[kiwiant]

Visual Studio 2019 (16.1.6) with Microsoft.CodeAnalysis.FxCopAnalyzers 2.9.3.

We use an extension method that helps keep our methods clean and simple but this results in a "CA1062: Validate arguments of public methods" warning. This isn't ideal as we still want the warning if we don't call our extension method (CheckArgumentNull).

Extension method:

        public static void CheckArgumentNull(this object value, string paramName)
        {
            if (value == null)
                throw new ArgumentNullException(paramName);
        }

Allows us to check parameters like this:
paramName.CheckArgumentNull(nameof(paramName));

...rather than the more wordy:

            if (paramName == null)
                throw new ArgumentNullException(nameof(paramName));

Some thoughts on this:

1. Perhaps the code analysis can look into extension method for a check.
2. Perhaps we can decorate the extension method with something like AttributeUsage to indicate the CA1062 is handled and can be ignored.

[CyrusNajmabadi]

Perhaps we can decorate the extension method with something like AttributeUsage to indicate the CA1062 is handled and can be ignored.

Woudln't that just be equivalent to adding a suppression around the extension method?

[kiwiant]

Adding the following attribute to the extension method would prevent the warning within the extension method but it doesn't affect the caller (it will complain about CA1062 even though the arguments are checked with the extension method).

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1062:ValidateArgumentsOfPublicMethods")]

[mavasani]

Can you try the latest NuGet package: https://www.nuget.org/packages/Microsoft.CodeAnalysis.FxCopAnalyzers/2.9.4-beta1.final

[kiwiant]

Same issue using Microsoft.CodeAnalysis.FxCopAnalyzers (2.9.4-beta1.final).

[mavasani]

You have couple of ways to workaround this:

1. Use the editorconfig based configuration described here to define your null check validation methods. See this for enabling editorconfig based configuration for the rule.
2. Apply a ValidatedNotNullAttribute to the parameters which are validated to be non-null by external methods.

[kiwiant]

ValidatedNotNullAttribute is not an elegant approach as this would riddle the code with many attributes so .editorconfig is our preferred approach and this is now working but without the full signature.

Signature of validation method in static class XXX.Common.ValidationExtensions:
public static void ThrowIfArgumentNull(this object value, string paramName)

.editorconfig that works (partial):

[*.cs]
dotnet_code_quality.null_check_validation_methods = ThrowIfArgumentNull

These signatures in .editorconfig (and Rebuilding) seem to be ignored:

XXX.Common.ValidationExtensions.ThrowIfArgumentNull
XXX.Common.ValidationExtensions.ThrowIfArgumentNull(object,string)
XXX.Common.ValidationExtensions.ThrowIfArgumentNull(string)
M:XXX.Common.ValidationExtensions.ThrowIfArgumentNull
M:XXX.Common.ValidationExtensions.ThrowIfArgumentNull(object,string)
M:XXX.Common.ValidationExtensions.ThrowIfArgumentNull(string)

[mavasani]

Thanks @kiwiant

We do have unit tests here for the doc comment ID based configuration: https://github.com/dotnet/roslyn-analyzers/blob/8a5f87b066b79a0f8f9e54eb34089ddbc3d0561a/src/Microsoft.CodeQuality.Analyzers/UnitTests/QualityGuidelines/ValidateArgumentsOfPublicMethodsTests.cs#L2031-L2036

I will investigate why your scenario is failing. Maybe something to do with these being extension methods?

[CoolDadTx]

I would go so far as to argue that the this parameter on an extension method should not be validated for null at all.

The guidelines around the behavior for extension methods recommend that the method behave just like an instance method. While there is still debate in the community as to the best practice for this the general consensus is that in order to behave like an instance method an extension method should throw NullReferenceException if source is null. However this is a system exception and should not be thrown so you're left with ArgumentNullException. But if you throw that exception then the behavior is now different than a normal instance method. Hence, unless it is specifically important to the extension method, you shouldn't check for null on the source parameter and just let it crash normally. But that will trigger the CA1062 warning.

The only workaround is to suppress the warning on all your extension methods but that is unrealistic for any reasonable size app. The alternative is to disable the rule altogether but then you lose the benefit on all the non-extension methods.

[mavasani]

The only workaround is to suppress the warning on all your extension methods but that is unrealistic for any reasonable size app. The alternative is to disable the rule altogether but then you lose the benefit on all the non-extension methods

Another option is to define a configuration option that the analyzer uses to decide whether extension method's this parameter should be flagged or not.

[andez2000]

Is this actually fixed? I am going round in circles.

I have an extension method based on an interface in a different project. Based on Ardalis Guard:
https://github.com/ardalis/GuardClauses/blob/master/src/GuardClauses/GuardClauseExtensions.cs

Here is my Guard clause in namespace NRTGuards:

public class Guard : IGuardClause
{
    public static IGuardClause Against { get; } = new Guard();

    private Guard() { }
}
public static class GuardClauseExtensions
{
    public static void Null([NotNull] this IGuardClause guardClause,
        [NotNull] object? input, [NotNull] string parameterName)
    {
        if (null == input)
        {
            throw new ArgumentNullException(parameterName);
        }
    }
}

My .editorconfig in its entirety is:

[*.cs]
dotnet_diagnostic.CA1062.severity = warning
dotnet_code_quality.CA1062.exclude_extension_method_this_parameter = true

dotnet_code_quality.CA1062.null_check_validation_method = NRTGuards.Guard.Against.Null`

This does not work. If I move the Guard classes into the project that calls them, then I dont get the warning.

[Vitegor]

I have two .editorconfig files in root folder, for all projects, and own .editorconfig in project in solution.
In my case rule null_check_validation_method work only if them in .editorconfig that in concrete project.

[mavasani]

@Vitegor Your scenario should work with the latest 3.3.x packages from https://dotnet.myget.org/feed/roslyn-analyzers/package/nuget/Microsoft.CodeAnalysis.FxCopAnalyzers. This was fixed pretty recently.

[Vitegor]

@mavasani, when this version will be available in Nuget ? I can use only nuget.org packages source on security policy of my company

[jberezanski-mdg]

Regarding issues with full signatures, my testing (VS 16.7.2, FxCopAnalyzers 3.3.0) resulted in two findings:
1) the symbol resolver is sensitive to formatting (spaces):

dotnet_code_quality.null_check_validation_methods = Acme.ArgumentAssert.IsNotNull(System.Object, System.String) // does not work
dotnet_code_quality.null_check_validation_methods = Acme.ArgumentAssert.IsNotNull(System.Object,System.String) // works, BUT...

2) symbol resolution does not work across assemblies (I'd guess that the mechanism used internally to resolve the validation methods works only in the context of the current compilation).

Proper formatting of signatures can be easily discovered by enabling generation of xml documentation and inspecting the generated file.
Unfortunately, the cross assembly restriction makes full signatures useless in contexts such as #2215 - which is kind of ironic, considering that this configuration setting was created specifically to work around the lack of cross assembly analysis in Roslyn (https://github.com/dotnet/roslyn/issues/35104).