Roslyn-analyzers: CA1062 firing when the validation is in another method even with ValidatedNotNull attribute

[JuanZamudioGBM]

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

Example: v2.9.3 (Latest)

Diagnostic ID

CA1062

Repro steps

I have a public method where I validate the parameter in another method, In the old FXCop If you added an ValidatedNotNull attribute to your method the warning will not fire.
Upgrading to 2.9.3 fires the warning

Validation method

public static T ThrowIfNull<T>([ValidatedNotNull]this T value, string name)
{
    if (EqualityComparer<T>.Default.Equals(value, default))
    {
        throw new ArgumentNullException(name);
    }

    return value;
}

Method

public static object GetValueOrNull(this IDataRecord dataRecord, int index)
{
    dataRecord.ThrowIfNull(nameof(dataRecord));
    return !dataRecord.IsDBNull(index) ? dataRecord.GetValue(index) : null;  // warning fires here
}

Expected behavior

No error

Actual behavior

CA1062 warning

[mavasani]

Upgrading to 2.9.3 fires the warning again

@JuanZamudioGBM Are you suggesting the issue does not occur on 2.9.2 release but repros on 2.9.3?

[JuanZamudioGBM]

@mavasani yes, 2.9.2 does not fire the warning, in 2.9.3 does, I edited the post to delete again because i mean that in FXCop happened, with the analyzers this is the first time that the warning fires for me.

[smaillet]

Same here, I've reverted to 2.9.2 because of this regression in 2.9.3.

[mavasani]

https://dotnet.myget.org/feed/roslyn-analyzers/package/nuget/Microsoft.CodeAnalysis.FxCopAnalyzers/2.9.4-beta1.19317.5+45537f7f should have the fix

[RobSiklos]

@mavasani I tried it with https://dotnet.myget.org/feed/roslyn-analyzers/package/nuget/Microsoft.CodeAnalysis.FxCopAnalyzers/2.9.4-beta1.19318.2+bc3ca30e, but am still seeing some weirdness. Please take a look at the following example:

[AttributeUsage(AttributeTargets.Parameter)]
internal sealed class ValidatedNotNullAttribute : Attribute { }

internal static class Param
{
    [SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters", Justification = "Blah blah"), SuppressMessage("Style", "IDE0060:Remove unused parameter")]
    public static void RequireNotNull([ValidatedNotNull] object value)
    {
        Param.RequireNotNull2(value);
    }

    public static void RequireNotNull2([ValidatedNotNull] object value)
    {
        if (value == null)
        {
            throw new ArgumentNullException(nameof(value));
        }
    }
}

public class DataThing
{
    public IList<object> Items { get; }
}

public static class Issue2578Test
{
    public static void DoSomething(DataThing input)
    {
        Param.RequireNotNull(input);

        // This line still generates a CA1062 error.
        Bar(input);
    }

    private static void Bar(DataThing input)
    {
        input.Items.Any();
    }
}

The line Bar(input); still generates a CA1062. However, it shouldn't do this, for 2 reasons:
1) The RequireNotNull method has its param decorated with [ValidatedNotNull], so no matter what is (or is not) happening inside that method, CA shouldn't complain about nulls
2) RequireNotNull calls RequireNotNull2, which does indeed do the null check. This should prevent the CA1062 even if [ValidatedNotNull] were not present.

[mavasani]

@RobSiklos - thanks, I am able to repro the latest issue. Investigating..