CS8602: Incorrect nullable flow analysis for indirect null check

[tom-englert]

Microsoft Visual Studio Professional 2019
Version 16.8.2

Steps to Reproduce:

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

[Youssef1313]

Duplicate of #48354, which was closed as by-design.

[tom-englert]

@Youssef1313 no, this one is different. Compiler can know that a has been checked already in the first line.
It was working fine with https://github.com/microsoft/CodeContracts and R# nullability annotations, but stopped after migrating to Roslyn.

[Youssef1313]

Then adding @jcouv to have a look.

[RikkiGibson]

This is by design. When you null check a variable, we don't attempt to infer what other variables could have their flow state modified based on that null check.

[tom-englert]

But there IS a null check on a in the first line: a?.ToString(). It's just the flow analysis that goes wrong.
See these variants:

1. This works: (same as original sample, just not using ?. but explicit if/else.

    string? b;
    if (a == null || (b = a.ToString()) == null) return;

2. This works:

    if ((a?.ToString()) == null) return;

3. This does NOT work, despite it's merely the same as 2.:

    string? b;
    if ((b = a?.ToString()) == null) return;

Also as already stated above, any other analyzer claiming to be state of the art and industrial standard (CodeContracts, ReSharper, SonarCube) can do this right.

[jaredpar]

@RikkiGibson and @Youssef1313 are correct that this is behaving as designed. The flow checking engine used by the compiler does not handle cases where the result of a null check is stored in a bool and that bool is used later on in a conditional construct. There have been discussions about changing that in future versions of the language but for now it's not included in the flow analysis.

This works: (same as original sample, just not using ?. but explicit if/else.

This works because it includes a direct check of a in the if and hence is included in our flow analysis checking.

This works:

Same as above.

This does NOT work, despite it's merely the same as 2.:

This is not the same as it involves extra constructs, notably the assignment, that the compiler must include in its flow analysis. Sure it's a case we could add into the analysis but at the moment it's not part of the included set.

[tom-englert]

@jaredpar I raised this issue because this is not part of the included set.
If we argue like this we can close any issue, just because any issue is a known limitation.
Shouldn't this rather be turned into a feature request?

[jaredpar]

The roslyn repository holds the compiler code base which is the implementation of the language design. At this point the compiler is correctly implementing the language design which does not include this level of requests.

I agree this could be a feature request but it should be a feature request on the language in dotnet/csharplang.

[tom-englert]

I see, this was the missing information. I'll create a request in that repo.

[tom-englert]

@jaredpar dotnet/csharplang is only about the language spec - but this is not about a new spec.

[jaredpar]

dotnet/csharplang is about the language spec, features and design alterations to existing features.

[tom-englert]

Are you talking about this? https://github.com/dotnet/csharplang/blob/29df547564c4ffc51b1dedf8369dc91e8f0ba854/proposals/csharp-8.0/nullable-reference-types-specification.md?

[jaredpar]

That is the C# 8 version of the spec, probably want to refer to the 9.0 one

https://github.com/dotnet/csharplang/blob/master/proposals/csharp-9.0/nullable-reference-types-specification.md

[tom-englert]

OK, only found this related to the problem:

https://github.com/dotnet/csharplang/blob/master/proposals/csharp-9.0/nullable-reference-types-specification.md#member-access

var person = new Person();

// The receiver is a tracked expression hence the member_access of the property 
// is tracked as well 
if (person.FirstName is not null)
{
    Use(person.FirstName);
}

So doesn't this mean it should work?

[tom-englert]

Or where in this document is written that the flow analysis should not work in that case?

[jaredpar]

Or where in this document is written that the flow analysis should not work in that case?

Specs of this nature are generally focused on discussing the places in which the behavior takes place. Specifically cases in which we will track the null state. The set of cases where we don't are generally not listed because that list is basically infinite. There are a few cases where we have called it out, like array element access, because it's a common misconception by users.

[tom-englert]

@jaredpar I've created the proposal in issues, but it has been moved to discussions: dotnet/csharplang#4208