Inconsistency when using multiple NotNullIfNotNull attributes

[joakimriedel]

I'm currently migrating to .NET 6 and found some inconsistency in the use of multiple NotNullIfNotNull attribute on a method. I've tried to read all available sources online to understand if it's a bug or by design, but resort to posting here for clarification.

Version Used: main (2021-11-09)

Steps to Reproduce:

    public static void Main()
    {
        string result = ReturnFirstOrSecondOrNullIfBothNull("first", "second"); // no warning (correct)
        Console.WriteLine(result); // "first"

        result = ReturnFirstOrSecondOrNullIfBothNull("first", null); // no warning (correct)
        Console.WriteLine(result); // "first"

        result = ReturnFirstOrSecondOrNullIfBothNull(null, "second"); // no warning (correct)
        Console.WriteLine(result); // "second"

        result = ReturnFirstOrSecondOrNullIfBothNull(null, null); // warning (correct)
        Console.WriteLine(result); // null

        TestMethod("first", "second");
    }
    
    [return: NotNullIfNotNull("first")]
    [return: NotNullIfNotNull("second")]
    public static string? ReturnFirstOrSecondOrNullIfBothNull(string? first, string? second)
    {
        return first ?? second;
    }

    public static void TestMethod(string? first, string? second)
    {
        string result;
        if (first is not null || second is not null)
        {
            result = ReturnFirstOrSecondOrNullIfBothNull(first, second); // ** WARNING (not expected)
            Console.WriteLine(result); // "first"
        }

        if (first is not null && second is not null)
        {
            result = ReturnFirstOrSecondOrNullIfBothNull(first, second); // no warning (correct)
            Console.WriteLine(result); // "first"
        }
        
        if (first is null && second is null)
        {
            result = ReturnFirstOrSecondOrNullIfBothNull(first, second); // warning (correct)
            Console.WriteLine(result); // never executed
        }

        if (!(first is null && second is null))
        {
            result = ReturnFirstOrSecondOrNullIfBothNull(first, second); // ** WARNING (not expected)
            Console.WriteLine(result); // "first"
        }
        
        if (first is null && second is null)
        {
            throw new ApplicationException("foo");
        }
        
        result = ReturnFirstOrSecondOrNullIfBothNull(first, second); // ** WARNING (not expected)
        Console.WriteLine(result); // "first"
    }

See this link for full repro

Expected Behavior:

No warnings if null values are eliminated through conditions.

Actual Behavior:

I get warnings, the lines marked with "** WARNING" are not expected.

[RikkiGibson]

Thanks for reporting this issue. It is correct that NotNullIfNotNull has "or" semantics when used multiple times, i.e. the output decorated with the attribute will get not-null flow state after the call if any of the referenced inputs are not-null before the call.

Because the compiler performs a path-independent flow analysis, a check like if (first is not null || second is not null) { ... } actually causes the compiler to learn nothing at all about the state of first or second in this scenario. It's not capable of tracking, for example, "I know that at least one of them is not null, and depending on the result of a test on one of them, I could learn something about the state of the other one." To remove nullable warnings in scenarios like this, it might be necessary to introduce Debug.Assert calls to assert that certain variables are not null.

This is discussed in more detail in #36927 (comment).

[alrz]

Linking to #43906 for "and" semantics.

[joakimriedel]

Thanks for the clarification and link to background @RikkiGibson

It would have made for some really pretty code, so I hope this will be revisited if/when path-dependent flow analysis is supported by the compiler.

I don't see how I could solve this with Debug.Assert, would you mind to elaborate a bit on that?

    public static void TestMethod(string? first, string? second)
    {
        string result;
        if (first is not null || second is not null)
        {
            // this won't help
            Debug.Assert(first is not null || second is not null);
            
            // neither would this
            if (first is null) {
                Debug.Assert(second is not null);
            }
            else if (second is null) {
                Debug.Assert(first is not null);
            }

            result = ReturnFirstOrSecondOrNullIfBothNull(first, second); // still warning
            Console.WriteLine(result); // "first"
        }
    }

instead I need to do something similar to the following, which is the pattern that I've resorted to use in my real code

    public static void TestMethod(string? first, string? second)
    {
        string result;
        if (first is not null || second is not null)
        {
            // no warning            
            result = ReturnFirstOrSecondOrNullIfBothNull(first, second) ??
                throw new ArgumentException("this would never throw since one of the arguments is certain to be not null");
            Console.WriteLine(result); // "first"
        }
    }