Roslyn-analyzers: CA1062 False Positive with own Guard

[zplan]

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

v2.9.3

Diagnostic ID

Example: CA1062: Validate arguments of public methods

Repro steps

I have a self implemented Guard in another assembly but the rule ignores the Guard check.

 public static ILogger Provide(this IProvideLoggers loggerProvider)
 {
    Guard.ArgumentIsNotNull(loggerProvider, nameof(loggerProvider));

    return loggerProvider.Provide(GetLoggerName(type));  <--- rule violation here !!
 }

Guard implementation:

public static void ArgumentIsNotNull<T>([ValidatedNotNull] T argumentValue, string argumentName)
    where T : class
{
    if (argumentValue == null)
    {
        throw new ArgumentNullException(argumentName);
    }
}

Expected behavior

No CA1062 rule violation should occure like in version 2.9.2

Actual behavior

CA1062 rule violation occures even with a null check.

[dkalbertson]

@zplan this seems to be a duplicate to #2215. See @mavasani's comment. It's due to your guard being defined in another assembly and is by design. If the guard was defined in the same assembly, it would not detect a violation.

[LordBenjamin]

This design reduces the value of CA1062 IMHO. It is a backward step from the previous managed binary analysis implementation which could detect guard code across assemblies.

The semantics have also changed - under MBA, a CA1062 result meant "there is no null check before using this variable". It now means "there might not be a null check before using this variable".

We have guard code implemented in a common assembly which is shared across many applications - is the current recommendation to duplicate this into every assembly?

Changing all our code to comply with the new CA1062 would be a lot of work and would also clutter up our codebase with repeated copies of the same guard code. What is the best way to avoid this?

[mavasani]

This design reduces the value of CA1062 IMHO. It is a backward step from the previous managed binary analysis implementation which could detect guard code across assemblies.

This is not true. I just verified the behavior of binary FxCop with the following 2 source files across different projects:

File1.cs in Project1.csproj:

using System;

public class C
{
}

public static class Helper
{
    public static void ValidateNotNull(C c)
    {
        if (c == null)
        {
            throw new ArgumentNullException(nameof(c));
        }
    }

    public static void DoNothing(C c) { }
}

File2.cs in Project2.csproj, where Project2 has a P2P reference to Project1

using System;

public class B
{
    public void No_CA1062(C c)
    {
        Helper.ValidateNotNull(c);
        var x = c.ToString();
    }

    public void CA1062(C c)
    {
        Helper.DoNothing(c);
        var x = c.ToString();
    }
}

Binary FxCop flags CA1062 for both the above c.ToString() invocations, the first one is false positive, but it does not do analysis across assemblies:

Severity    Code    Description Project File    Line    Suppression State
Warning CA1062  In externally visible method 'B.No_CA1062(C)', validate parameter 'c' before using it.  ClassLibrary9   C:\Users\mavasani\Source\Repos\ClassLibrary9\ClassLibrary9\Class1.cs    8   Active
Warning CA1062  In externally visible method 'B.CA1062(C)', validate parameter 'c' before using it. ClassLibrary9   C:\Users\mavasani\Source\Repos\ClassLibrary9\ClassLibrary9\Class1.cs    14  Active

Having said the above, the ability to add custom editorconfig based rule configuration in FxCop analyzers can help alleviate the core issue here - we can add a new editorconfig option that allows users to specify fully qualified names of validator methods from metadata. The analysis can then treat invocations to these methods specially, and avoid the false positives.

[dkalbertson]

@mavasani I believe what he's referring to is that the old Binary FxCop special cased ValidatedNotNullAttribute and would not specify a violation in that case.

[mavasani]

@dkalbertson Ah thanks - I got confused between this issue and #2215

I will fix both the issues - handle the special attributes that binary FxCop used to detect + allow specifying validation helper signatures via editorconfig options.

[LordBenjamin]

we can add a new editorconfig option that allows users to specify fully qualified names of validator methods from metadata. The analysis can then treat invocations to these methods specially, and avoid the false positives.

That sounds like a fair solution to me. Thanks!

[dkalbertson]

I will fix both the issues - handle the special attributes that binary FxCop used to detect + allow specifying validation helper signatures via editorconfig options.

@mavasani IMHO I'm not a huge fan of the special attribute. As I understand it, the implementation from the old binary FxCop was that it would respect any custom attribute with the name"ValidatedNotNull" in it, which makes the analysis "stringly-typed" rather than strongly typed. However, I understand the desire for backwards compatibility.

[drewnoakes]

Worth noting here that the nullable reference type work specified some compiler services attributes for marking this kind of validation.

https://github.com/dotnet/csharplang/wiki/Nullable-Reference-Types-Preview#annotation-attributes

It'd be worth reusing those, or at least supporting them too.

[mavasani]

https://dotnet.myget.org/feed/roslyn-analyzers/package/nuget/Microsoft.CodeAnalysis.FxCopAnalyzers/2.9.4-beta1.19317.5+45537f7f should have the fix as well as editorconfig option to configure null check validation methods

[mavasani]

@drewnoakes - thanks for that information. Can you please file a separate issue for those attributes? I can handle them with a separate PR.