Only enforce Member/NotNullWhen on constants

[jcouv]

My previous PR (#46201) to relax enforcement of those conditional attribute on non-constants returns was incorrect. It relied on the state being conditional or not, but (1) we split the state for non-constant values (!expr and method invocations involving attributes), and (2) we don't split the state for constant values.
This PR corrects that, by directly checking for the return having a constant value.

Update: this PR now only enforced those warnings if the target of the attribute (ie. parameter or member) has a different state in the two branches of analysis of the return statement. This allows to warn on statements that we can analyze like return !TryGetValue(..., out p); or return p != null;, while avoiding annoying warnings on return M(); or return TryGetValue(..., out _);.

Fixes #48126
Original issue: #44080

[RikkiGibson]

we split the state for non-constant values (!expr and method invocations involving attributes)

It feels like !expr should only have a split state if we entered a split state after visiting expr.

we don't split the state for constant values

It feels like if we don't maintain the split state for constant-valued bool expressions in general, there will be other bugs (granted, probably related to uninteresting scenarios such as !true or true || false).

Regardless of the above comments, I agree with the strategy to check for a constant-valued bool expression when deciding whether to give the warning. I do wonder if there is additional tuning that should be done in order to check methods which return in a conditional state: SharpLab

using System;
using System.Diagnostics.CodeAnalysis;
#nullable enable

public class C {
    public string? s;
    

    [MemberNotNullWhen(true, "s")]
    public bool M1() {
        s = "a";
        return true;
    }
    
    [MemberNotNullWhen(false, "s")]
    public bool M2() {
        s = "a";
        return false;
    }
    
    [MemberNotNullWhen(true, "s")]
    public bool M3() {
        return M1(); // ok
    }
    
    [MemberNotNullWhen(true, "s")]
    public bool M4() {
        return M2(); // bad
    }
    
    [MemberNotNullWhen(true, "s")]
    public bool M5() {
        return !M2(); // ok
    }
    
    [MemberNotNullWhen(true, "s")]
    public bool M6() {
        // if we only warn on constant bools, then this case will warn while `M4` does not.
        if (M2())
            return true;
        else
            return false;
    }
    
    [MemberNotNullWhen(true, "s")]
    public bool M7() {
        return s == null; // bad -- it would be unfortunate to lose this warning IMO.
    }
    
    [MemberNotNullWhen(true, "s")]
    public bool M8() {
        return s != null; // ok
    }
}

#Resolved

[jcouv]

It feels like !expr should only have a split state if we entered a split state after visiting expr.
It feels like if we don't maintain the split state for constant-valued bool expressions in general, there will be other bugs.

I toyed with that both ideas, but ended pulling them out of this change since not necessary to fix and not observable. I'm happy to add back and we can discuss.

I do wonder if there is additional tuning that should be done in order to check methods which return in a conditional state

Yes, we're losing some useful warnings for sure. I tried splitting the problem different ways to do something better, but I don't have a solution... #Resolved

[RikkiGibson]

It feels like in order to catch return s == null; or return !TryInitializeThing();, before giving a diagnostic you'd have to check that you're in a split state and that the state for the referenced output (e.g. out parameter, member) is actually different between the StateWhenFalse and StateWhenTrue (as a proxy to answer the question: was the return expr performing a test or conditional assignment to this variable?). I think we would have to reason very carefully through some scenarios to decide if we're happy with all the consequences of something like that.

I'm slightly sad to see some of the tests where we dropped warnings here, but I defer to your judgment on whether this is the best trade-off (considering that present unexpected warnings are perhaps more annoying than missing expected warnings here?) #Resolved

[RikkiGibson]

It feels like some of the scenarios where we warn or not can feel a little arbitrary in practice, such as in M4 vs M6 in the above example. People may end up filing bugs for it. But I suppose that is the nature of doing a "best-effort"/pragmatic analysis. #Resolved

[jcouv]

was the return expr performing a test or conditional assignment to this variable?

I like that. Let me take a stab. #Resolved

[RikkiGibson]

(doesn't need to be addressed in this PR) I have been thinking that it doesn't make sense for a parameter to ever not have a slot--it will never be constant, for example. Perhaps we should use a helper for getting parameter slots that throws if we fail to get a slot for it. #WontFix

[RikkiGibson]

LGTM!

[333fred]

Shouldn't we only suppress when both states are unreachable? We could have just done a test and thrown in the other branch, but accidentally messed up the order of the branches and returned incorrectly. #ByDesign

[333fred]

Same for the other tests of this condition.

---

In reply to: 503602424 [](ancestors = 503602424)

[jcouv]

The only way to have one state being unreachable is using a constant condition (which is case above).

---

In reply to: 503602558 [](ancestors = 503602558,503602424)

[333fred]

node.ConstantValue.BooleanValue [](start = 20, length = 31)

Consider extracting this in the pattern like the other additions in this file. #Closed

[333fred]

Unused? #Resolved

[333fred]

Why would this not have warnings? There should be different state here between the branches? Or is it because it's nested inside a ||? #ByDesign

[jcouv]

The when-true and when-false states for the return are identical (every variable may be null), so the expression as a whole is not considered to inform the nullability of y or z.

---

In reply to: 503605598 [](ancestors = 503605598)

[333fred]

The fact that these work but putting them inside a || does not is really confusing for me. I don't think this will be obvious at all for users. #ByDesign

[jcouv]

Agreed. Still, in terms of maximizing useful warnings and minimizing bad ones, this design seems the best option so far...

---

In reply to: 503605817 [](ancestors = 503605817)

[333fred]

Please add a test for something like this scenario:

using System.Diagnostics.CodeAnalysis;

static bool M([NotNullWhen(true)] object? o)
{
    return o == null ? true : throw null!;
}

#Resolved

[333fred]

Done review pass (commit 6)

[333fred]

I super don't like this behavior. It's obviously incorrect code.