[Proposal]: NotNullWhenMember attribute

[Youssef1313]

NotNullWhenMember attribute

• Proposed
• Prototype: Not Started
• Implementation: Not Started
• Specification: Not Started

Summary

A nullable attribute specifies that a method return value is not null if some field/property is true/false.

Motivation

There is no way to properly track null states for the following (example is simplified from roslyn repo):

#nullable enable

var x = new AOrB(new A());
if (x.IsA) _ = x.AsA().ToString(); // No warning with NotNullWhenMember attribute

public class A { }
public class B { }

public class AOrB
{
    private object _value;
    private bool _isA;

    public AOrB(A a)
    {
        _value = a;
        _isA = true;
    }

    public AOrB(B b) => _value = b;

    public bool IsA => _isA;

    //[return: NotNullWhenMember("IsA", true)]
    //[return: NotNullWhenMember("IsB", false)]
    public A? AsA()
    {
        return _value as A;
    }

    //[return: NotNullWhenMember("IsA", false)]
    //[return: NotNullWhenMember("IsB", true)]
    public B? AsB()
    {
        return _value as B;
    }
}

Detailed design

A new attribute will be added to the runtime:

namespace System.Diagnostics.CodeAnalysis
{
    [AttributeUsage(AttributeTargets.ReturnValue, Inherited = false, AllowMultiple = true)]
    public sealed class NotNullWhenMember : Attribute
    {
        public MemberNotNullWhenAttribute(string member, bool returnValue)
        {
            Member = member;
            ReturnValue = returnValue;
        }

        public string Member { get; }
        public bool ReturnValue { get; }
    }
}

Then the compiler should track the nullability states properly

Drawbacks

Alternatives

Allow the existing MemberNotNullWhenAttribute to work with methods, ie, the following would work:

#nullable enable

using System.Diagnostics.CodeAnalysis;

var x = new AOrB(new A());
if (x.IsA) _ = x.AsA().ToString(); // No warning with MemberNotNullWhen attribute

public class A { }
public class B { }

public class AOrB
{
    private object _value;
    private bool _isA;

    public AOrB(A a)
    {
        _value = a;
        _isA = true;
    }

    public AOrB(B b) => _value = b;

    [MemberNotNullWhen(true, "AsA()")]
    public bool IsA => _isA;

    public A? AsA()
    {
        return _value as A;
    }

    public B? AsB()
    {
        return _value as B;
    }
}

Unresolved questions

Design meetings

[YairHalberstadt]

The existing MemberNotNullWhen attribute can be used when a method returns a Boolean which determines if a field or property is null.

The suggested NotNullWhenMember attribute can be used when a field or property returns a Boolean which determines if a method returns null.

That still leaves a gap where a method returns Boolean which determines if another method returns null.

For that reason extending MemberNotNullWhen seems like a better option as it will actually cover all use cases, albeit at the first of having to sort out overload resolution.

[Youssef1313]

I'm okay with any of the approaches. The case where a method return determines another method return nullability wasn't much of a concern to me, but is definitely something to consider for this proposal.

[RikkiGibson]

Could you please share the original scenario from roslyn?

[Youssef1313]

@RikkiGibson It's the IsNode and AsNode() from SyntaxNodeOrToken

[CyrusNajmabadi]

@RikkiGibson FWIW, i would very much like this to be supported. This pattern, while not that common in Roslyn, is a super PITA when it does arise, leading to annoying superfluous suppressions. Thanks!

[RikkiGibson]

It looks like the reason that extending MemberNotNullWhen is listed as an alternative and not the primary proposal is because it feels imprecise/awkward to use [MemberNotNullWhen(true, "AsA()")] bool IsA;. Is that correct? Is there anything else I missed?

[CyrusNajmabadi]

@RikkiGibson what i would prefer it some way of saying, for a method: if you call this, you will always get something non-null back (regardless of parametrs) as long as this other call returned true/false. e.g. AsNode(...) is always non-null if IsNode returned true.

[333fred]

Some thoughts:

1. Backwards analysis like this proposes isn't feasible. We need to know when IsA is called to update the info for AsA().
2. Forwards analysis required us to solve our old friend infoof: https://docs.microsoft.com/en-us/archive/blogs/ericlippert/in-foof-we-trust-a-dialogue.

[Youssef1313]

@333fred 1 sounds reasonable, but for 2, if I understand correctly, it's a point the following (copied from the blog you linked):

Eric: Let’s start with a simple one. Suppose you have infoof(Bar) where Bar is an overloaded method. Suppose there is a specific Bar that you want to get info for. This is an overload resolution problem. How do you tell the overload resolution algorithm which one to pick? The existing overload resolution algorithm requires either argument expressions or, at the very least, argument types.

The member name passed to the attribute won't just be the method name, but also parameter types. There can be multiple ways to specify which overload is referred to, but aligning with SupressMessage's Target could be a good option (I think it's handled by the IDE - but the compiler could use similar logic):

https://github.com/dotnet/roslyn/blob/ea7cfe2a8aed27e018d602d70ba60458d4514d07/src/EditorFeatures/CSharpTest/Diagnostics/Suppression/SuppressionTest_FixAllTests.cs#L382

If overload resolution wasn't your concern for 2, would you be able to elaborate more?

I'll update the proposal and either swap the alternative with the core proposal, or only keep the alternative as the core proposal.

Edit: I see the generic case in the infoof article, I commented too fast 😄

But it seems it's also doable:

https://github.com/dotnet/roslyn/blob/ea7cfe2a8aed27e018d602d70ba60458d4514d07/src/EditorFeatures/CSharpTest/Diagnostics/Suppression/SuppressionTests.cs#L1405

[RikkiGibson]

Backwards analysis like this proposes isn't feasible. We need to know when IsA is called to update the info for AsA().

It feels like the attribute can be placed on either one of the two symbols involved. We control the internal symbol model, so we can always set things up appropriately so that when the flow analysis visits 'IsA', it knows it needs to do something about 'AsA()'.

[333fred]

It feels like the attribute can be placed on either one of the two symbols involved. We control the internal symbol model, so we can always set things up appropriately so that when the flow analysis visits 'IsA', it knows it needs to do something about 'AsA()'.

I don't believe this is true, particularly once you start considering inheritance. We've also talked about versions of MemberNotNull where you'd be able to change the nullability of nested members, and if we wanted to add that here as well it would become truly untenable.

[RikkiGibson]

MemberNotNull doesn't work across inheritance. I don't think there's a need to support inheritance as a part of solving this issue.

Regarding changing states of nested members: that's interesting. I'd be curious to know more about the scenarios where people wished they had it. It seems relevant for when the user doesn't control the definition of the type of their field but would like to check/ensure/assert on the state within the nested fields. In that case it feels like it could be good enough to just support MemberNotNull on parameters without trying to devise a "member path" encoding that could be used to refer to nested members in a MemberNotNull.

[RikkiGibson]

All that said, I'm not a strong supporter of introducing NotNullWhenMember, at least until I better understand what it would take to make MemberNotNull able to refer to methods unambiguously (need to re-read and make sure I understand the info-of discussion).

[timcassell]

Wouldn't this only be correct on immutable objects? The current MemberNotNullWhen doesn't have to worry about mutability, because both values are "returned" at the same time. Is that a concern for analysis, or should the programmer be expected to know that it could potentially break?

[Eli-Black-Work]

Just ran into this today.

Our use case:

private class ResourceAndErrors
{
    public object? Resource { get; init; }
    public IList<string>? Errors { get; init; }

    public bool IsValid => Resource != null || Errors != null;
}

var thing = new ResourceAndErrors();

if(!thing.IsValid)
    return null;

// Warning: `thing.Resource` might be null
thing.Resource.ToString();

[RikkiGibson]

That looks like a case where MemberNotNullWhen (already existing) would work.

[YIShikunov]

I would suggest like to suggest expanding existing NotNullWhen to the Enum value

namespace System.Diagnostics.CodeAnalysis
{
    [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = true)]
    internal sealed class NotNullWhenAttribute<TEnum> : Attribute
    where TEnum: enum
    {
        public NotNullWhenAttribute(TEnum returnValue);

        public TEnum ReturnValue { get; }
    }
}

[CyrusNajmabadi]

@YIShikunov I agree strongly. But can you open another discussion on this. Having good examples would also be valuable. To me, cases like Json.net would really benefit here.

[YIShikunov]

@CyrusNajmabadi #6170 We can try to continue here