Add analyzer that ensures checks for null on public non-nullable members

[dotMorten]

When building class libraries, nullable annotations doesn't actually guarantee that null isn't passed in. Especially when you're getting called from code that doesn't have nullability enabled, or is incorrectly using null-forgiveness. Generally that's not so much a problem with internal code, as you can ensure you always use it, and don't break the contract, but class library developers can't guarantee what other users do, nor do they control which compiler that user is using.

This means you'd always need to add a null-check on your public API surface, regardless of whether a parameter is non-nullable or not. Example:

public class MyAPI
{
   public string Method1(object nevernull)
   {
      return nevernull.ToString(); //Potential null-reference exception. This should generate a warning
   }
   public string Method2(object nevernull)
   {
      // Seemingly unnecessary null-check, but is often necessary after-all
      return nevernull?.ToString() ?? throw new ArgumentNullException(nameof(nevernull)); 
   }
   private string Method3(object nevernull)
   {
      return nevernull.ToString(); //Not a potential for null-reference exceptions if I don't have nullability-warnings in my build
   }
}

Normally I'd get a warning if I call these methods with a null, but if I'm using an older compiler or didn't enable nullability analysis, there's still a good chance null will be parsed into the public members. The problem with nullability here is that it actually increases the potential for null-reference exceptions, because you're falsely led into thinking you don't need to perform as many null-checks as you used to do.

I'd really like a compiler that'll help me as a library author to better support all users regardless of whether they have nullability warnings on or not.

[mavasani]

This is already implemented as CA1062

[dotMorten]

@mavasani odd I'm not getting that warning. Also it looks like this analyzer was created before nullability annotations was even a thing?

[dotMorten]

CA1062 works like a charm. Didn't use the FXCop analyzers until now.

[dotMorten]

I take that back. It doesn't understand null annotations at all. Example:

public void MyMethod(object? instance)
{
      PrivateMethod(instance); // Warning generated here despite that method says it accepts nulls
}
private void PrivateMethod(object? instance)
{
}

In a different example where we use a helper to validate the object

public void MyMethod(object instance)
{
      ValidateObject(instance); // Warning generated here although that method is meant to null-check
}
private void ValidateObject([NotNull]object? instance)
{
   if(instance is null) throw new ArgumentNullException(nameof(instance));
}

[mavasani]

Yes, there is a pending work item for DFA framework to understand nullable annotations. I can’t find it, so let’s use this issue to track the work.

[mavasani]

@dotMorten I tried the below sources, and neither of them generate CA1062

#nullable enable

public class C
{
    public void MyMethod(object? instance)
    {
        PrivateMethod(instance); // Warning generated here despite that method says it accepts nulls
    }
    private void PrivateMethod(object? instance)
    {
    }
}

#nullable enable

using System;
using System.Diagnostics.CodeAnalysis;

public class C
{
    public void MyMethod(object instance)
    {
        ValidateObject(instance); // Warning generated here although that method is meant to null-check
    }
    private void ValidateObject([NotNull] object? instance)
    {
        if (instance is null) throw new ArgumentNullException(nameof(instance));

    }
}

Can you provide an example where you see a false positive?

[dotMorten]

Hmm odd perhaps I oversimplified my real code. Let me get back to you...

[Evangelink]

@dotMorten Have you been able to find a way to reproduce the FP?