Add attributes to indicate that a method result implies nullability of a member.

[DrJohnMelville]

Please consider the following code:

#nullable enable
class Foo
{
    public string? Name { get; set; }
    public bool HasName => Name != null;
    public void NameToUpperCase()
    {
        if (HasName)
        {
            Name = Name.ToUpper();
        }
    }
}

On the Name=Name.ToUpper() I get a warning that Name is a possible null reference, which is clearly incorrect. I can cure this warning by inlining HasName so the condition is if (Name != null).

Inlining may be suboptimal for several reasons. HasName might actually test a lot more things, and I might want to use it in several places, or it might be a public part of the API surface. There are many reasons to want to factor the null check into it's own method, but doing so seems to break the nullable reference checker.

I propose the addition of an optional parameter to the NotNullWhen, MaybeNullWhen, and NotNulIIfNotNull attributes to give the name of a field or property on the containing class which should receive the not-null constraint. Thus the code above could be written

#nullable enable
class Foo
{
    public string? Name { get; set; }
    [NotNullWhen(true, "Name")]
    public bool HasName => Name != null;
    public void NameToUpperCase()
    {
        if (HasName)
        {
            Name = Name.ToUpper();
        }
    }
}

The C# compiler should then understand that the true return result from HasName implies that Name is not null and the code should compile without warnings.

I further propose an optional parameter on the NonNull attribute to indicate a field or property that is not null if the annotated member returns at all. This would allow the following code:

#nullable enable
class Foo
{
    public string? Name { get; set; }
    [NotNull("Name")]
    public void CheckHasName() 
    {
       if (Name == null) throw new Exception();
    }
    public void NameToUpperCase()
    {
          CheckHasName();
          Name = Name.ToUpper();
    }
}

[V0ldek]

As for the optional parameter on NotNull - we already have DoesNotReturn and DoesNotReturnIf(bool) as attributes. I'd rather have a DoesNotReturnIfNull(string), to make the intent clearer and more consistent with existing attributes.

[pinkfloydx33]

While I like the idea as I've run into a similar issue, in the case above you could end up with a race condition if another thread changes the Name property between the test of HasName and the block. How would you propose something like that is handled/guaranteed? In this particularly trivial example you could elide the check itself and replace the entire body with Name = Name?.ToUpper();

[V0ldek]

The compiler is already lax in regards to property access race conditions. For example this code:

if (x.Name != null)
{
    x.Name = x.Name.ToUpper();
}

doesn't produce a warning, even though theoretically the Name property could be set to null between time-of-check and time-of-use. I'd say that this attribute would have the same semantics as an explicit null check.

[canton7]

Note that ?. doesn't care about race conditions either. SharpLab.

[pinkfloydx33]

If it's a property and not a field the value gets stored in a temporary first. I'd argue thats a bit different w/r/t race conditions. Sharplab

[canton7]

If it's a reference type, it gets stored in a temporary too. ?. guarantees that the LHS is evaluated exactly once.

My point was that ?. was not designed to be thread-safe. NRTs have, so far, not been designed to be thread-safe. Suddenly making avoiding race conditions a factor would go against the established precedent. You should use locks or other synchronization mechanisms to avoid race conditions - don't try and rely on NRTs to help you avoid a very narrow class of race conditions.

[V0ldek]

I believe the reasoning behind being lax was that being strict would result in far too many false positives. I base that upon this Mads Torgersen's article (look under the "Avoiding dereferencing of nulls" section). I'd argue this proposal addresses the exact same case - decrease the number of false positives generated for users of your API in terms of NRTs.

[DrJohnMelville]

I believe that the discussion of multithreaded issues is tangential to this proposal. This proposal addreses the difference between

#nullable enable
class Foo
{
    public string? Name { get; set; }
    public bool HasName => Name != null;
    public void NameToUpperCase()
    {
        if (HasName)
        {
            Name = Name.ToUpper();
        }
    }
}

And this:

#nullable enable
class Foo
{
    public string? Name { get; set; }
    public void NameToUpperCase()
    {
        if (Name != null)
        {
            Name = Name.ToUpper();
        }
    }
}

Both of these code samples have identical vulnerability to null dereference errors if they are accessed from multiple threads simultaneously. The first code segment gives a warning and the second does not. That is the distinction this proposal wishes to address.

[ardalis]

Why is this still not implemented in the language/framework?

[333fred]

Duplicate of #3297. This is part of C# 9, the MemberNotNull and MemberNotNullWhen attributes.