Warn if a value is never or always null

[alrz]

Version Used: NullableReferenceTypes branch

Steps to Reproduce:

    public void M(string? o) {
        if (o == null) return;
        var x = o?.Length; //!
    }

Expected Behavior: warning: o is never null

Actual Behavior: no warnings

Relates to #25592

[alrz]

Ideally I'd expect three warnings:

• Possible null
• Always null
• Never null

This is on-par with the other proposed analysis (possibly on the ide side) for "expression is always true/false" to detect more unreachable code. e.g.

ExpressionSyntax expr = null;
// ...
// no expr assignments
// ...
if (expr == null) // expression is always true
{} else {/* unreachable */}

PS: the two analyses do seem to belong to the same bucket, that is, the compiler already knows if expr is or is not null at that point, so it could warn that the comparison is redundant.

[jcouv]

Tagging @cston

[jcouv]

I suspect we won't want to do that because in many cases, the nullable analysis will be imperfect (a trade-off between safety and flooding with warnings). For example, consider if (a.b.c == null) return; M(a.b); var x = a.b.c?.Length;.
The compiler may take the lenient view that a.b.c is non-null, but M could have changed that.

[alrz]

@jcouv So in your example it's not proven that c is never null i.e it's "possibly null' (which may or may not produce warning depending on how much we want to be optimistic to avoid flooding). This analysis, on the other hand, would be very local and only triggered on definitive cases.

[alrz]

I want to add, a "possible null dereference" warning for a definitive null dereference is a lot less useful when it comes to offering a fix - they demand different fixes outright: you add a null check for the former, but the latter is most likely a bug in the logic, or at best a leftover from a recent change which might be misleading for the reader later on. All in all, I think it could be done along with "always true/false" warnings since they both are similar in terms of use cases.

[bkoelman]

I see great value in the distinction between "possibly" and "certainly" warnings. For example, const field access and locals should be "certainly" while property access should be "possibly".

The distinction enables teams suppress the "possibly" warnings but require fixing the "certainly" cases.

[gafter]

I also see value in the "certainly" analysis. But it isn't something we're doing or have any plans to do at this time. In code like this

    public Result SomePublicApi(Argument a)
    {
        if (a == null)
            throw new ArgumentNullException("A");

we should not provide a diagnostic. It is normal and expected for public APIs to check incoming parameters to ensure they are not null at runtime, even though the parameter was declared not to be null. If we did it, a more precise analysis would permit us to give these diagnostics when we are "certain".

[alrz]

The "always/never null" state would only produced by (local) nullable analysis, Argument a is at best "maybe not null" even though it's not annotate as nullable, because we're not sure if any nullable analysis is done on the client code (or if so, it's possible that it's been suppressed)

[CyrusNajmabadi]

Agree with @alrz. This could/should only be done for when we can guarantee non-nullness (basically local/constant analysis only). Anything that flowed in from anywhere else would have to be assumed to be potentially null.

[alrz]

Oh, and a "never null" warning would flag x!! usages. which seems to be sensible.

[jcouv]

Queued those questions on LDM backlog, which means we'll probably discuss in January.

[gafter]

At LDM on 2019-02-20 we confirmed that we are not including these diagnostics as part of the nullable reference types feature (though we might introduce them later)