False positive CS8604 in case of indirect usage of return value inside conditional expression

[rbeurskens]

Version Used:
4.4.0-6.22608.27 (af1e46a)

Steps to Reproduce:

1. Have a method of the form

 bool TryGetXX([MaybeNullWhen(false)] out MyRefType output)

3. Call it and indirectly use the return value in a conditional expression.
4. Try to use the output if the method in the conditional block (the CS8604 does not show if the method call is directly used as the conditional expression)

#nullable enable
public bool TryGetValue([MaybeNullWhen(false)] out MyType obj); // implementation not relevant
public void DoSomething(MyType obj); // implementation not relevant

public void Sample1()
{
    bool result = TryGetValue(out var output);
    if (result)  // or: if (result = TryGetValue(out var output))
        DoSomething(output);  // <=== CS8604 here (unless 'output' is suffixed with '!')
}
public void Sample2()
{
    if (TryGetValue(out var output))
        DoSomething(output);  // <=== No CS8604 here
}

Diagnostic Id:

CS8604

Expected Behavior:

No warning in Sample1() / same behavior as in Sample2()

Actual Behavior:

warning / false positive in Sample1(), unless null-tolerance operator ! is added

[RikkiGibson]

Thank you for taking the time to file this issue. The behavior you are seeing is by-design. Once we "exit" the expression where nullability depends on whether the result is true or false, i.e. TryGetValue(out var output), the conditional nullability information is lost. #27011

#52717 (comment) details a bit of why this is a somewhat difficult problem in general.

[rbeurskens]

Thank you for taking the time to file this issue. The behavior you are seeing is by-design. Once we "exit" the expression where nullability depends on whether the result is true or false, i.e. TryGetValue(out var output), the conditional nullability information is lost. #27011

#52717 (comment) details a bit of why this is a somewhat difficult problem in general.

Thank you for your reply and explanation.
I can imagine when it comes to named local variables, they might be accessible outside in a wider scope making it possible to modify it in another thread. Even if it is a method level variable. But I do not completely understand where/when the state could be altered. I imagined async code capturing local variables would cause 'unguarded' access, but my code attempt to reproduce that did not generate a warning, which I do not understand. Also given that there is a warning even if a constant is added to the conditional expression in Sample2(). I guess because the unnamed temporary variable holding the value for the conditional is not accessible from code, it can not be captured or altered by it, or am I missing something?

Could you help me understand? Thank you in advance.

public void Sample1()
{
        MyType? output;
        Task.WaitAll(
            Task.Delay(5).ContinueWith(_ =>
            {
                output = null;
            }),
            Task.Delay(5).ContinueWith(_ =>
            {
                if (TryGetValue(out output))
                    // ===========   <== no lock, access from other thread might happen here
                    DoSomething(output); // No warning here. Why? output could be modified by parallel task beween evaluation if the conditional and using it, isn't it?  (?)
            })
        );
}

public void Sample2()
{
    if (TryGetValue(out var output) & true)
        DoSomething(output);  // <=== CS8604, second & operand is always evaluated even if the first returns false
}
public void Sample3()
{
    if (TryGetValue(out var output) && true)
        DoSomething(output);  // <=== No CS8604, second && operand is only evaluated if the first returns true
}

[RikkiGibson]

The compiler's analysis assumes that there is no possibility of other threads writing to a variable while the current thread is trying to read it. Our static analysis tends to make many simplifying assumptions like this, for example that aliasing is not occurring, and that method calls do not modify the state of variables unless specifically indicated by the method's signature.

The issue here is that for performance reasons, the compiler tries very hard to perform path-independent analysis. That means that we want to determine a single worst-case nullable state before visiting an expression, and to visit that expression just once with that state, if we can. (This description ignores other complications which arise from backwards-branching constructs like loops and gotos.)

If we were to associate additional nullable state with a boolean variable b1, it means that for each subsequent expression we would need to visit twice: once for when b1 is false and once when b1 is true, giving two resulting states for the expression. If the subsequent expression also performs null tests and stores its value to a variable b2 then we need to associate additional state with that too--now 4 states instead of 2, for all the combinations of (b1: true, b2: true), (b1: false, b2: true), (b1: true, b2: false), (b1: false, b2: false). This results in combinatorial explosion pretty quickly.

In addition to all of this, we need users to be able to reason about why the compiler gives certain warnings or not--and as much as it may be confusing to have information get lost in a single indirection like this, it could result in some incredibly confusing action at a distance if we tried to track an indefinite number of indirections.

I'm not aware if there are techniques to reduce the computational complexity here while still giving useful answers that users can reason about.

Regarding &, it's actually a special case of path-independence requiring us to drop information. More detail at https://stackoverflow.com/questions/61076184/why-do-i-get-dereference-of-a-possibly-null-reference-warning-when-using-non-sho/68824025#68824025.

[rbeurskens]

Thank you for a more in-depth explanation. I did not realize how complicated flow analysis could become without drawing a line for which scenarios should be considered. Of course it should not put that much load in the system that it would cause performance issues on a developer's system.