[Proposal]: Flow analysis should handle indirect null check

[tom-englert]

Flow analysis should detect chained null checks

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

Summary

Flow analysis should handle chained null checks and propagate the nullable state properly to all involved variables.
According to @jaredpar in dotnet/roslyn#49653 this is not properly specified in the language design.

Motivation

All other analyzers (e.g. R#, Sonar) handle this properly and complain about extra null checks that Roslyn nullability analyzer requires.

Detailed design

public void Method(object? a) 
{
    var b = a?.ToString();

    if (b == null)
        return;

    // If b is not null, a can't be null here, but a CS8602 is shown:
    var c = a.ToString();
}

Expected Behavior:
No warning in var c = a.ToString();

Actual Behavior:
CS8602

Drawbacks

Alternatives

Unresolved questions

Design meetings

[huoyaoyuan]

Indirect flow analysis causes halting problem. There must be a limited pattern list that can be recognized.

[alrz]

it is currently implemented as a pattern-based approach. but I think resharper does provide more precise results as it tracks true/false/null values more deeply.

[jaredpar]

According to @jaredpar in dotnet/roslyn#49653 this is not properly specified in the language design.

That's not an accurate description here. My statement is essentially that:

1. This behavior is presently by design as this is not one of the constructs supported by our nullable flow state analysis.
2. The language spec focuses on calling out the constructs that contribute to flow state analysis. It does not call out every construct which does not. There are a few cases where we do call out constructs which are not supported because they cause considerable customer confusion. Best example is that we explicitly call out that array elements don't get tracked in the nullable flow state. This is the exception though not the rule.

As such the document does properly specify the behavior of this pattern because it's not explicitly listed as one of the supported constructs. At the same time I do agree this has been a source of confusion and it's something I intend to add in next time I update the spec.

Flow analysis should handle chained null checks and propagate the nullable state properly to all involved variables.

The proposal needs to go deeper here though. It needs to get concrete on specifically:

• What type of flow state does a boolean capture: not-null, maybe-null, maybe-default or all of them?
• When does the state stored in a boolean get invalidated: when all or part of the contributing values are mutated?
• Does boolean logic also need to combine the flow state of the booleans? For example can bool c = a && b create a variable c which has the null flow state that is joined from a and b?
• etc ...

These are the trickier parts of this feature that would need to be worked out and costed.

[alrz]

@jaredpar

The proposal needs to go deeper here though. It needs to get concrete on specifically:

Not sure if this is feasible, but I think if we lower states to a bunch of horn clauses for each basic block, at each point we can query whether or not a variable can be null.

null(a) :- null(b).
null(a)? // false

null(a) :- null(b).
null(b).
null(a)? // true

Then the spec would only need to specify the lowering strategy, not the solver.

Of course, that's assuming we have a solver available in the compiler.