Invalid NotNullIfNotNull contract is not reported

[alrz]

Version Used: ab680e7

Steps to Reproduce:

using System.Diagnostics.CodeAnalysis;
#nullable enable
public class C {
    public void Main()
    {
        M1(new()).ToString();
        M2(new()).ToString();
        
        M1(null).ToString();
        M2(null).ToString();
    }
    
    [return: NotNullIfNotNull("p")]
    public object? M1(object? p) {
        if (p is null) return null;
        return Nullable(); // no warnings (one expected)
    }
    
    [return: NotNullIfNotNull("p")]
    public object? M2(object? p) {
        if (p is null) return null;
        return M1(p); // no warnings (correct)
    }
    
    object? Nullable() {return null;}
}

(sharplab repro)

[RikkiGibson]

It sounds like when we visit a return, we need to check the flow state of any parameters referenced by NotNullIfNotNull, and if the parameter has not-null flow state, we need to require not-null state on the returned expression.

[alrz]

It sounds like when we visit a return, we need to check the flow state of any parameters referenced by NotNullIfNotNull, and if the parameter has not-null flow state, we need to require not-null state on the returned expression.

That would not cover one hundred percent of cases, consider this:

[return: NotNullIfNotNull("p")]
object? Method(object? p) => null; // still no warning based on above

Method(new()).ToString(); // NRE but no warning

Sounds like we should complain if there is no definitive state on the parameter on exit point, and if there is, then we can decide what is correct in which case the stacked scenario becomes interesting.

[return: NotNullIfNotNull("p")]
object? Method01(object? p) => Method02(p);  // technically ok, but we probably don't track `p` that far?

[return: NotNullIfNotNull("p")]
object? Method02(object? p) { .. }

Even for the case the argument itself is being returned, we might have other NotNullIfNotNull parameters to worry about.

[RikkiGibson]

we should complain if there is no definitive state on the parameter on exit point

I'm not sure what this means. In our analysis, the state of such a parameter at any point is either maybe-null or not-null. I don't think there's anything we can do about a scenario like this:

[return: NotNullIfNotNull("p")]
object? Method(object? p) => null; // still no warning based on above

Method(new()).ToString(); // NRE but no warning

Our analysis is not able to distinguish the above scenario from one like the following:

[return: NotNullIfNotNull("p")]
object? Method(object? p)
{
    if (p is null)
        return null;
    return ...;
}

Method(new()).ToString(); // NRE but no warning

That is, we don't know if the parameter has a maybe-null state because the argument is sometimes null or sometimes non-null, or if it has a maybe-null state because we explicitly checked for null and it's just impossible for it to contain anything else in the given code path.

We also have a precedent with attributes like [NotNullWhen(bool)] to warn only if "we see you're definitely doing something wrong", and not warn if "we don't see you're definitely doing something right" 😉 #44080

Tagging @jcouv @cston to see if you have any ideas for how more NotNullIfNotNull bugs could be caught in flow analysis.

[alrz]

we should complain if there is no definitive state on the parameter on exit point

I'm not sure what this means. In our analysis, the state of such a parameter at any point is either maybe-null or not-null.

I think what I mean by that is to ensure we branched on p state because the contract itself implies that we're checking p for null or not-null at some point. (however this will be an exception in stacked scenarios and where we return the arg itself).

Not quite sure if that's feasible to do but that first example is pretty much "doing something wrong" if not all of them.

[RikkiGibson]

Fixed in #47649