Microsoft.CodeAnalysis appears to report false FlowState, when a switch handles null

[hugener]

Version Used:

Microsoft Visual Studio Enterprise 2019 Version 16.11.3
Microsoft.CodeAnalysis Version 3.11.0

Steps to Reproduce:

Solution that shows the problem in VS:
https://github.com/dotnet/roslyn/files/7303368/DUTester.zip

Test that reproduces it when running the analyzer:
https://github.com/sundews/Sundew.DiscriminatedUnions/blob/main/Source/Sundew.DiscriminatedUnions.Test/GenericSwitchExpressionAnalyzerTests.cs
Given_SwitchExpressionInEnabledNullableContext_When_ValueIsNotNullAndAllCasesAndNullCaseAreHandled_Then_HasUnreachableNullCaseIsReported2

In the following file at line 273: TypeInfo.Nullability.FlowState reports MaybeNull. Removing the null case in the test above changes the FlowState to NotNull.
https://github.com/sundews/Sundew.DiscriminatedUnions/blob/main/Source/Sundew.DiscriminatedUnions.Analyzer/DiscriminatedUnionSwitchAnalyzer.cs

Expected Behavior:
DUTester solution: A diagnostic reported on line 18, that the null case should not be handled.
Test: TypeInfo.Nullability.FlowState reported as NotNull

Actual Behavior:
DUTester solution: No diagnostic.
Test: TypeInfo.Nullability.FlowState reported as MaybeNull

[jcouv]

@hugener Could you provide a more contained repro, for example a code snippet on sharplab or some such?
Also, could you clarify whether the problem is with compiler APIs (which give you a TypeInfo) or the nullability analysis itself (ie. the expression that give a MaybeNull flow state would create an incorrect warning in code if it were dereferenced)? My guess is the latter, and so it should be possible to illustrate the problem with a snippet of code with an incorrect nullability warning.

[hugener]

@jcouv Since the source code above has changed, the line numbers I posted no longer match up. I created a branch so the code will not change as I make other changes and extracted the test here: https://github.com/sundews/Sundew.DiscriminatedUnions/blob/feature/RoslynNullabilityProblem/Source/Sundew.DiscriminatedUnions.Test/ContradictingTests.cs#L69-L125

I'm using the compiler API semanticModel.GetTypeInfo here:
https://github.com/sundews/Sundew.DiscriminatedUnions/blob/feature/RoslynNullabilityProblem/Source/Sundew.DiscriminatedUnions.Analyzer/DiscriminatedUnionSwitchAnalyzer.cs#L175

Here is a screenshot of that:

Maybe I'm using it wrong?

I use the same code for various other cases, but in this particular test, an instance is returned by the method "Compute" and the return type is a non-nullable Option (nullable is enabled).

I think this is somehow significant because I have another test here, where there instance is passed in as a parameter. And that test works: https://github.com/sundews/Sundew.DiscriminatedUnions/blob/feature/RoslynNullabilityProblem/Source/Sundew.DiscriminatedUnions.Test/ContradictingTests.cs#L19-L67

The only difference in the two tests is that the value is coming from the Compute method vs being passed in as a parameter.

Not sure if it's related, but I noticed something similar in VS when the value comes from a method. As I hover the "result" here, it's reporting it as nullable. Again, nullable is enabled and the return value of Compute is non-nullable:

[huggecmi]

I have put a snippet on sharplab here.

It's the same two examples I use in my contradicting tests above.
Hovering the "option" shows the same result that option is nullable as in VS:

That's not the case with the option passed in as a parameter

[jcouv]

Thanks for the additional information. I think I see what's going on.
option is declared as Option<int> which is marked as non-nullable. But the analysis says that it may be null here.
This is because the switch includes a null case. This is considered a "pure null test" and that tells the compiler that the user thinks the value may be null despite the compiler's own analysis.
The reason for this behavior is that we want to allow a public API to declare a non-nullable parameter, yet still defensively check (see example below).

The part that may be surprising is that arguably the effect of the pure null test should be after the switch. Will need to take a look at that to confirm whether that's by-design.

public void M(string parameter)
{
    if (parameter is null) // pure null check is taken seriously
        parameter.ToString(); // warn about null dereference
}

[hugener]

Thanks for confirming what's going on. That also explains why it conflicts with my intention as I'm writing an analyzer that should report an error about unreachable null cases.

Come to think about it, I guess pure null tests are necessary because of null!
Even with NNRTs someone could pass null with "null!". Which raises two questions.
Is there a setting where null! is an error (In the worst case, I could write an analyzer)?
And would it generally be considered bad practice to report against pure null tests?

[jcouv]

Thanks for confirming.
There is no setting to disallow null!. There are some initialization scenarios where those are quite useful.
Pure null tests are not bad practice in general. Public APIs with defensive coding are an example where they are useful.

[hugener]

Thanks for the additional information. I think I see what's going on. option is declared as Option<int> which is marked as non-nullable. But the analysis says that it may be null here. This is because the switch includes a null case. This is considered a "pure null test" and that tells the compiler that the user thinks the value may be null despite the compiler's own analysis. The reason for this behavior is that we want to allow a public API to declare a non-nullable parameter, yet still defensively check (see example below).

The part that may be surprising is that arguably the effect of the pure null test should be after the switch. Will need to take a look at that to confirm whether that's by-design.

public void M(string parameter)
{
    if (parameter is null) // pure null check is taken seriously
        parameter.ToString(); // warn about null dereference
}

Any updates on this?

[jcouv]

@hugener We've placed this into to Backlog and I do not expect we'll get to it given the priority of other issues. The impact of this issue is relatively limited: the compilation produces correct diagnostics and codegen, but the information reported by the semantic model to the IDE has incorrect nullability.