False positives in c# 8 nullable reference suggestions

I'm seeing a lot of C# 8 Nullable Reference Type issues.

Issue 1

These issues remind me to turn on nullable context, which is already enabled project wide in the csproj.

Issue 2

See Example 1 for a constructor issue, declaring a property as nullable in an API indicates you intend for the consumer to pass nulls. In this case we don't (and throw exceptions).

Issue 3

See example 2, There's no way to declare a var as nullable and the target of the foreach is non-null.

Issue 4

See example 3, There's no way to declare a var as nullable.

Code snippets are from OAT which triggers many of the same types of issues: https://github.com/microsoft/OAT

For context, I do have nullable reference types enabled project wide.

Example 1

This code triggers "Enable Nullable Context and declare constructor parameters as nullable. I explicitly don't want nulls passed in, but am already handling nulls. Setting these parameters to be nullable would indicate to consumers that they may expect reasonable behavior if they pass in nulls. This is explicitly an API you may not pass nulls to.

public Violation(string description, Rule rule, Clause[] clauses)
{
    this.description = description ?? throw new ArgumentNullException(nameof(description), $"{nameof(description)} may not be null");
    this.rule = rule ?? throw new ArgumentNullException(nameof(rule), $"{nameof(rule)} may not be null");
    this.clauses = clauses ?? throw new ArgumentNullException(nameof(clauses), $"{nameof(clauses)} may not be null");
}

Example 2

This is a false positive. The target is both not null and there's no way to declare a "var" as nullable. NullableWarning

Example 3

This is another false positive. There's no way to declare a var as nullable. It's fine that this is may be null here.

public bool Applies(Rule rule, object? state1 = null, object? state2 = null)
{
    if (rule != null)
    {
        var sample = state1 is null ? state2 : state1;

NullableWarning2

@gfs thank you so much for putting effort into assembling such a detailed list! Shame on me, all of the reported issues are well known and documented already, more than a year ago :-( If you take a look at the EnableNullableContextAndDeclareIdentifierAsNullableAnalyzer.cs (dated June 18th, 2019) you will see these comments inside:

// TODO: What to do when a variable is declared by using the var keyword?
//       var? does not work at the moment.
//       At the moment we will simply pretend that the feature is there.
//       It is planned and will be implemented one day.

bool IdentifierIsAlreadyNullable()
{
    // This is VS2017 implementation.
    // If the identifier is value type and is
    // surely nullable it can only by Nullable<T> (T?).
    // So, it is already nullable if it is a
    // value type.
    // TODO-IG: Solve how to approach to support for VS2017 and VS2019
    //          at the same time and implement properly the VS2019
    //          version of this logic.
    //          Also, add the negative-case smoke tests to the
    //          CSharp80.VS2019.csproj.
    //          In VS2019 (means C# 8.0) we have nullable reference types ;-)
    return typeSymbol.IsValueType;
}

bool NullableContextIsAlreadyEnabled()
{
    // TODO-IG: This is the implementation for VS2017.
    //          Solve how to approach to support for VS2017 and VS2019
    //          at the same time and implement properly the VS2019
    //          version of this method.
    //          Also, add the negative-case smoke tests to the
    //          CSharp80.VS2019.csproj.
    return false;
}

// TODO-IG: Just ignoring structs so far. That's why the IsReferenceType checks below.
//          Structs are rarely used compared to classes, so not that much of an issue at the moment.
//          Still, see what to do with structs.

// TODO: Ignore the case where the comparison with null is actually a null guard.
//       E.g.: if (identifier == null) throw ArgumentNullException(...);
//       E.g.: identifier ?? throw ArgumentNullException(...);

At the time I was implementing that first version nullable reference types were still a work in progress, as some like some other C# features. My focus was on getting the feature ready to the level where it clearly shows how many places we have in our real-life code that will benefit from the features. When I say ready, it means ready for the conferences and talks I was giving. I didn't expect that time that there will be such a huge gap in the Sharpen's development :-(

For the record, writing the proper VS2019 implementation for IdentifierIsAlreadyNullable(), NullableContextIsAlreadyEnabled(), and similar requires the solution for this issue. Coming to a solution here also took me some time but I have it now (not documented in the README at the moment.)

Thanks once again a lot for pointing the issues. Your examples clearly show that now, a year after, the TODOs left in code pose serious issues. The fix will be part of the next release.

sharpenrocks / Sharpen