Missing warning for non-exhaustive switch-expression

[bernd5]

Version Used: c31d8af

Steps to Reproduce:
Compile:

namespace Tests
{
    public class Prog
    {
        static int EvalPoint((int, int)? point) => point switch
        {
            (0, 0) => 1,
            var (_, _) => 2,
        };

        static void Main()
        {
	     EvalPoint(null);
        }
    }
}

Expected Behavior:
Warning: warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern 'null' is not covered.

or
warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern 'null' is not covered.

Actual Behavior:
No warning

[bernd5]

The warning is emitted in NullableWalker, but lost because of:

roslyn/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

Line 1038 in 11d6292

Analyze(compilation, method, node, new DiagnosticBag(), useConstructorExitWarnings: false, initialNullableState: null, getFinalNullableState: false, out _, requiresAnalysis: false);

Here a new DiagnosticBag is created and thrown away - all diagnostics are lost...
Why not simply use the diagnostics parameter?

With #nullable enable the diagnostics are not lost in AnalyzeIfNeeded.

[333fred]

@gafter any chance you could comment here? When the switch expression is #nullable enabled we do actually see a non-exhaustive warning, but I would not have expected that to change the behavior given that this is a nullable struct type, and the only thing I would have expected to see is making warnings go away when operating on non-nullable reference types in a #nullable enable context.

[bernd5]

To fix this, we should not throw diagnostics away and remove the following lines from "nullable warnings" in "ErrorFacts":

nullableWarnings.Add(GetId(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNull));
nullableWarnings.Add(GetId(ErrorCode.WRN_SwitchExpressionNotExhaustiveForNullWithWhen));

see: #52715

[RikkiGibson]

#nullable enable is expected to control whether warnings are issued for both nullable reference types and nullable value types. Handling null input in patterns is only required when the input's flow state is maybe-null/maybe-default. For example, the following scenario does not warn:

#nullable enable
using System.Diagnostics.CodeAnalysis;

int M([DisallowNull] int? i) => i switch { int j => j }; // ok

I feel like there are two "weird" things you could potentially do if you wanted to change this:

1. Always give the null-exhaustiveness warning when nullable is disabled. Now, disabling nullable in existing code can actually introduce new nullable warnings.
2. Always give the null-exhaustiveness warning, even when nullable is enabled and the input has not-null flow state. Now I may start to get warnings on my existing code where I already was testing for null and handling it outside the pattern.

Given that, it may be best to leave the existing behavior as-is.

[333fred]

Always give the null-exhaustiveness warning when nullable is disabled. Now, disabling nullable in existing code can actually introduce new nullable warnings.

Yes, this is what I would expect. I expect nullable flow state to help the compiler understand that a null check is unnecessary. The fact that we're not doing this is severely breaking my brain right now.

[bernd5]

The situation is at least inconsistent. If you write the same with a switch statement:

namespace Tests
{
    public class Prog
    {
        static int EvalPoint((int, int)? point){
            switch(point)
            {
                case (0, 0): 
                    return 1;
                case var (_, _): 
                    return 2;
            }
        }
        
        static void Main()
        {
	     EvalPoint(null);
        }
    }
}

You get the error message:

error CS0161: 'Prog.EvalPoint((int, int)?)': not all code paths return a value

regardless of which nullable settings you have set.

I would expect something similar for switch expressions.

[RikkiGibson]

Yes, this is what I would expect. I expect nullable flow state to help the compiler understand that a null check is unnecessary. The fact that we're not doing this is severely breaking my brain right now.

There isn't a precedent for #nullable disable introducing a null-safety warning. Before proceeding on this, it might be reasonable to start an internal thread with the language design or compiler team and confirm exactly what we want to do about this issue.

If we do proceed on this, then it seems like the new warning should be under a warning wave, and I would prefer that both nullable reference types and nullable value types are handled, since both can be null, and the behavior if you don't handle null is the same.

[RikkiGibson]

It appears that the LDM decision is that the nullable context controls whether an exhaustiveness warning is issued when null is the only unhandled value. Perhaps this can be revisited on the csharplang side or internally if someone on the compiler/language team feels strongly about it, but this roslyn issue is considered to be "by design" at this time.

https://github.com/dotnet/csharplang/blob/3c8559f186d4c5df5d1299b0eaa4e139ae130ab6/meetings/2018/LDM-2018-10-10.md#nullable-reference-types-vs-switch-analysis

[alrz]

It appears that the LDM decision is that the nullable context controls whether an exhaustiveness warning is issued when null is the only unhandled value.

That decision is strictly about reference types? This does look like a bug to me.

[RikkiGibson]

I find the notes slightly ambiguous because the section Nullable reference types vs switch analysis
states:

Alternatively, we can phrase exhaustiveness as exclusive of nullable reference types and let the null analysis handle switch exhaustiveness for null specifically. Nullable value types would be handled as part of traditional exhaustiveness.

But the subheading Conclusion states:

Let's explore the separation of exhaustiveness between null and non-null, st. all exhaustiveness warnings do not consider null, and warnings related to null are delayed until nullable analysis.

This does appear to be written after the decision was made to track nullable value types akin to nullable reference types: dotnet/csharplang#1865.

It isn't clear to me why it was indicated that nullable value types should be handled separately. But going by all this you may be right @alrz.

[RikkiGibson]

I think the next step is probably to confirm with LDT whether the shipped behavior shown in the issue description is what they intended or not.

[333fred]

I don't think there's a meaningful distinction between reference types and value types here.

[gafter]

If I recall correctly, this is by design. Examples that are not exhaustive due to not checking for some null are only intended to get a warning in a nullable enabled context.

[jcouv]

Yes, this matches my recollection as well (this is by-design). The exhaustiveness analysis with regards to nulls is part of nullability analysis and requires a nullable-enabled context.
Here are the notes I could find ("Let's explore the separation of exhaustiveness between null and non-null, st. all exhaustiveness warnings do not consider null, and warnings related to null are delayed until nullable analysis."): https://github.com/dotnet/csharplang/blob/main/meetings/2018/LDM-2018-10-10.md#nullable-reference-types-vs-switch-analysis