CS8509 The switch expression does not handle all possible inputs when it does

[miloush]

Version Used: VisualStudio.16.Preview/16.3.0-pre.3.0+29230.61

Steps to Reproduce:

class Program
{
    enum E { a, b, c }

    static int Main()
    {
        E e = E.a;

        if (e == E.a)
            return e switch
            {
                E.a => 0
            };

        switch (e)
        {
            case E.a:
            case E.b:
                return e switch
                {
                    E.a => 0,
                    E.b => 1
                };
        }

        return e switch
        {
            E.a => 0
        };

        // unreachable
        return 0;
    }
}

Expected Behavior: no warnings (or at least not on the first two)

Actual Behavior: CS8509 for all switch expression

At minimum, "the switch expression does not handle all possible inputs" is simply not true.

[huoyaoyuan]

For the first switch, the compiler doesn't perform dependent-type like check yet. Currently only bools and nullability has such a check.
For the second switch, even the dependent-type analysis is performed, the switch expression still does not handle all possible inputs. The possible values for an enum is all values of corresponding numeric type, not declared values only.

[miloush]

@huoyaoyuan Sorry, the code is supposed to show 3 separate scenarios, obviously everything after the first return is unreachable. The second switch expression is only executed when e is E.a or E.b, it's basically the same case as the first one but with switch instead of if.

I do expect to be told this kind of analysis is not being performed, and clearly the conditions can be arbitrarily complicated, though I did hit the second case in actual code and felt unhappy about being forced into writing unreachable code to satisfy the compiler.

At minimum, "the switch expression does not handle all possible inputs" is simply not true.

That was supposed to say that in the code above, all possible inputs to the switch expression are in fact handled. "the switch expression does not handle all enum values" appears more appropriate.

[jaredpar]

I did hit the second case in actual code and felt unhappy about being forced into writing unreachable code to satisfy the compiler

What unreachable code did you feel you had to add here? The switch expressions you listed are not exhaustive hence the compiler is correctly emitting a warning for the scenario.

[gafter]

@miloush We could instead say "The switch expression does not handle all possible values of its input type." to be more precise (and pedantic). Would that address your concern?

[miloush]

@jaredpar

public int ToInt32(MyEnum e)
{
    switch (e)
    {
          case MyEnum.One:
                return 1;
          case MyEnum.Two:
          case MyEnum.Three:
          case MyEnum.Four:
                return e switch
                {
                    MyEnum.Two => 2,
                    MyEnum.Three => 3,
                    MyEnum.Four => 4,
                    _ => // unreachable
                };
        case MyEnum.Five:
            return 5;
        default:
            throw new NotSupportedException();
    }
}

I don't see reason why I should have to add the _ case to the switch expression as it can never be reached, or am I missing something? What should I do in that case? Throw unreachable-code-reached exception?

@gafter If this is by design then yes, that wording sounds better - you cannot argue with that and makes it clear how to fix the warning.

[jaredpar]

@miloush

I don't see reason why I should have to add the _ case to the switch expression as it can never be reached, or am I missing something?

This can absolutely be reached. Consider the following invocation: ToInt32((MyEnum)42). That is legal, verifiable .NET code that will hit the _ case in the switch expression.

[miloush]

@jaredpar I am aware of that feature, but the whole branch of the switch statement that contains the switch expression will not be executed in that case...

[jaredpar]

I am aware of that feature, but the whole branch of the switch statement that contains the switch expression will not be executed in that case...

Okay I see. You're saying the exhaustiveness code for the e switch value should consider the domain of MyEnum.Two, MyEnum.Three and MyEnum.Four as the expression has already been limited to that. If so that is not the design of the feature. It considers only the domain of the type in the expression, not using flow analysis to limit it to a smaller domain of values.

[miloush]

Indeed, that is what I tried to describe in the issue. Hence I have to write code that cannot be executed to silence a warning. I guess my suggestion here is, would it be worth considering some simple flow analysis for this feature?

[jaredpar]

@miloush that type of flow analysis is expensive. Our work on nullable reference types really drove hom the cost of doing this correctly. At the time I don't think it's worth the necessary language design and compiler implementation cost. Closing but keeping on the backlog of future possible improvements if enough scenarios / asks came up.

[gafter]

Reopening to clarify the diagnostic as described in #38571 (comment)

[ihavenonickname]

It considers only the domain of the type in the expression, not using flow analysis to limit it to a smaller domain of values.

It is incredibly counter-intuitive that listing all enum values is not enough to make the switch expression exhaustive. Other major languages with similar feature do not require this dummy catch-all case.

In fact, there is a first-class citizen language in the .net ecosystem that handles this scenario very well. The following is valid F# code and does not generate any warning or error.

type E = A | B | C

let f = function
    | A -> "a"
    | B -> "b"
    | C -> "c"

[<EntryPoint>]
let main argv =
    printfn "%s" (f A)
    0

Outside of the .net land, Haskell is another good example.

data E = A | B | C

f :: E -> String
f A = "a"
f B = "b"
f C = "c"

main :: IO ()
main = print $ (f A)

In the JVM land, Scala is yet another example.

sealed trait E
case object A extends E
case object B extends E
case object C extends E

def f(s: E) = s match {
    case A => "a"
    case B => "b"
    case C => "c"
}

println(f(A))

And many others.

All snippets above compile without warnings because the compiler understands that the pattern matching expressions are exhaustive. Also, a warning pops up if one of the cases is removed, as expected.

(In Haskell, the warning requires the compiler flag -Wincomplete-patterns)

The fact C# behaves differently is surprising and annoying.

As of dotnet version 3.1.300, the following C# code generates a warning.

enum E
{
    A, B, C
}

class Program
{
    static string f(E x) => x switch
    {
        E.A => "a",
        E.B => "b",
        E.C => "c",
    };

    static void Main(string[] args)
    {
        Console.WriteLine(f(E.A));
    }
}

[gafter]

All great reasons to regret C#'s C heritage. There is a proposal to permit declaring enums that don't have unnamed values. See dotnet/csharplang#3179

[dextercd]

To me enums are a kind of safety feature that limit the amount of values you can use, you can technically circumvent such limitation but you generally shouldn't. (In my opinion)

Let's say I write this code:

public enum Operation
{
    Plus,
    Minus,
    Multiply
}

public static string OperationToString(Operation op)
{
    return op switch
    {
        Operation.Plus => "+",
        Operation.Minus => "-",
    };
}

Now I get this warning:

CS8509 The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern 'Program.Operation.Multiply' is not covered.

This is a very nice warning telling me precisely that I'm not handling Operation.Multiply. This warning has prevented a bug, great!

But when I add the code to handle this case the warning doesn't go away, instead it only changes the example from Program.Operation.Multiply to a cast like (Program.Operation)3.

I almost never do casts like this and honestly don't mind a SwitchExpressionException when someone takes off the safety feature and it goes wrong.. But I don't like warnings so I'll add a pattern that handles cases like this:

_ => throw new ArgumentOutOfRangeException(nameof(op)),

Great, the warning is gone, but _ handles every possible value.. so now when I add a new operation (i.e. Operation.Divide) I don't get a new warning! but of course I'd really like to get such a warning!, since this is now a real bug.

I don't think I'm the only person that uses enums as if they can only contain values defined in the enum declaration. The closed enum types proposal is great, but I don't think it'll get added to C# anytime soon, and there's already a lot of existing code that uses enums this way.

If the enum cast vs. defined enum value missing pattern warnings got separate warning codes people could decide for themselves how they'd like to handle cases like this by enabling/disabling the appropriate warnings.

[miloush]

I don't mind the openness of the enum, it's just a different feature and needs to be used accordingly.

You might be interested in this discussion, as I was having the same requirement: dotnet/csharplang#2472

[dextercd]

I'm not really bothered by the unnamed values of the enum. I do think it's unfortunate that the switch expression warning takes it into account. It forces you to add a catch-all pattern, but after you add a pattern that catches everything (_ => throw ..) it'll never warn again even when you add new named values to an enum that is not handled separately in the switch expression.

[CyrusNajmabadi]

Great, the warning is gone, but _ handles every possible value.. so now when I add a new operation (i.e. Operation.Divide) I don't get a new warning! but of course I'd really like to get such a warning!,

@dextercd You can accomplish this with an analyzer. Then you can have whatever rules you want here. For example, you could run that rule for all enums, only your own enums, only your own enums taht have a certain attribute on them, etc. etc.

A good rule of thumb: if you want the language to be more restrictive in what it allows, but otherwise it's all the same lang syntax and features, then just go write an analyzer. That's their primary use case :)

[dextercd]

@CyrusNajmabadi Thanks for the analyzer suggestion, I definitely should to take a look at them. The reason I'm commenting here is because I truly think this warning has the potential to prevent a lot of bugs, but that its effectiveness is severely lessened due to the fact that it encourages people to add a catch all pattern, thereby guaranteeing the warning will never again trigger for that code.

A function call that fails when you pass it a named enum value is fundamentally different to me than a function call that fails due to passing an unnamed enum value. That's why I think these cases should be reflected by different warnings.

But I might be the only one with this opinion.. maybe one day I'll write an analyzer and convince the whole world to use it 😅 .

[CyrusNajmabadi]

That's why I think these cases should be reflected by different warnings

Sure. We made a call on which to align with. With the understanding that the otehr side could always supplant the problem with an analyzer. i.e. even if we made the language not warn if you missed the _ case, tehre would be some who would want that. So it was either have them write the analyzer, or you. We chose you :)

[gafter]

The compiler could be modified to issue a different warning when a switch expression is only incomplete because of the possibility of an unnamed enum value being part of the input. That would be a reasonable feature request. I suggest you file that request as a separate issue.

[dextercd]

Thank you for the encouragement! I've opened a new issue: #47066.