Exhaustability for switch expressions on enums should be less strict

[YairHalberstadt]

Currently switch expressions emit a warning if they are not exhaustive.

The following will warn:

enum Reply
{
    Yes,
    No,
}
...
Reply reply = GetReply();
reply switch
{
     Reply.Yes => true,
     Reply.No => false,
};

The reason for this is that this enum can be any value of type int, not just its declared values.

This is only a warning, so the compiler can still compile the code. This is what the compiled code looks like:

reply switch
{
     Reply.Yes => true,
     Reply.No => false,
     _ => throw new InvalidOperationException(),
};

It throws in the unexpected case.

I think it can safely be assumed that the aim of this warning is to reduce bugs. There are two choices on how strict to be in exhaustiveness checking:

a) Pragmatic: A switch expression is considered exhaustive if all declared values for the enum are matched. This will lead to exceptions being thrown at runtime if the enum turns out to be some other value.

b) Strict: A switch expression is considered exhaustive if all possible values for the enum are matched. In practice the means adding a _ => arm to the switch, since for anything other than a byte based enum this is impossible. This will lead to developers not being warned if a new value is added to the enum which they do not account for.

We need some data to help drive this decision. I went through every single example in Roslyn where a switch expression on an enum handles every single declared value of the enum. They all currently have a _ => arm to remove the warning.

Here are all the methods where they are:

NullableAnnotationExtensions.MergeNullableAnnotations
NullableAnnotationExtensions.ToPublicAnnotation
NullableAnnotationExtensions.ToInternalAnnotation
NullableFlowStateExtensions.ToPublicFlowState
NullableFlowStateExtensions.ToInternalFlowState
SeparatedSyntaxListCodeActionComputer.GetNestedCodeActionTitle
CSharpTriviaResult.AnnotationResolver
CSharpTriviaResult.TriviaResolver
CSharpUseRangeOperatorCodeFixProvider.CreateRangeExpression
ExecutionConditionUtil.Architecture
ProjectExternalErrorReporter.ReportError2
VSTelemetryLogger.CreateAndStartScope
VSTelemetryLogger.LogBlockEnd
BaseDiagnosticItem.BrowseObject.MapDiagnosticSeverityToText
BaseDiagnosticItem.BrowseObject.MapReportDiagnosticToText
CSharpProjectShim.OptionsProcessor.SetOutputFileType
FindAllDeclarationsTests.GetSolution
CodeStyleOption<T>.FromXElement
Extensions.ToReportDiagnostic
AbstractTriviaFormatter.GetRuleSpacesOrIndentation
AbstractTriviaFormatter.GetRuleSpacesOrIndentation //second switch on a different type in the same method
SerializableBytes.Seek
CsharpSyntaxGenerator.GetParameterModifiers
CsharpSyntaxGenerator.GetTokenKind

That is 24 locations.
In all but 3, the _ => arm threw an exception. This means the exhaustiveness checking is gaining the developer nothing since an exception would be thrown even without that arm.

In FindAllDeclarationsTests.GetSolution null is returned by the _ => arm. This is a private method that is used 16 times. In every single one of those 16 usages, if null had been returned, a NRE or an ANE would have been thrown later on. In other words, the default behavior of throwing an exception if not all cases are accounted for would have been better than the solution the developer chose to do instead, since it would prevent cascading errors.

VSTelemetryLogger.CreateAndStartScope and VSTelemetryLogger.LogBlockEnd are more interesting. As loggers they are coded extremely defensively, and throwing an exception for them is useless, since it wont be reported anywhere. Instead on the invalid arm they report the exception rather than throwing it:

            try
            {
                // guard us from exception thrown by telemetry
                var kind = kvLogMessage.Kind;
                switch (kind)
                {
                    case LogType.Trace:
                        EndScope<OperationEvent>(functionId, blockId, kvLogMessage, cancellationToken);
                        return;
                    case LogType.UserAction:
                        EndScope<UserTaskEvent>(functionId, blockId, kvLogMessage, cancellationToken);
                        return;
                    default:
                        FatalError.Report(new Exception($"unknown type: {kind}"));
                        break;
                }
            }
            catch
            {
            }

In conclusion, in 21 out of 24 cases the strict exhaustiveness checking caused the same code to be emitted as it would have been under the pragmatic exhaustiveness checking. In 1 case it led to worse code being emitted, as it lead to cascading errors, rather than errors at the point where they should have been reported.

In 2 cases it lead the developer to write better code for their use case. But this was in a class that was anyway very defensively written, and not your typical use case.

In all 24 cases there will be no warning if another enum value is added which the switch does not account for, which there would have been under pragmatic exhaustiveness checking in 22 of those cases.

It seems clear to me that overwhelmingly, more bugs will be caught if csharp uses pragmatic exhaustiveness checking rather than strict.

Notes

C# 8.1

It is very close to C# 8.0 release - possibly too close to make this change. It is not an error to remove a warning later, so this could be changed in C# 8.1 if necessary.

InvalidOperation is not a very good exception

It may be pointed out that I've been unfair here. By default the compiler will emit an InvalidOperationException with no information as to what caused the exception. A developer can choose to throw a more useful exception. A good example might be the InvalidEnumArgumentException which stores the name of the enum, the type of the enum, and the value of the enum.

The compiler might not be able to emit these instead of InvalidOperationException due to the security risk of revealing the names of locals. Also it would have to be a new exception called InvalidEnumSwitchException or something similiar, since the switch might not be on an argument. However if that is the concern, it might be possible for the developer to opt in to which exception should be thrown using attibutes, or some other method. Perhaps an assembly attribute could be overriden by a class attribute could be overriden by a member attribute, etc.

[spydacarnage]

I could get behind a solution where the warning doesn't show if all known enum values are catered for, but if the _ => arm isn't specified, the compiler adds it with the new InvalidEnumSwitchException being thrown. With regards to the risk of exposing locals, the exception could just have the enum value as a property, relying on the developer to use the stack trace to determine where the actual error occurred (which will, in turn, determine the name and type of the enum).

…

On Fri, 19 Jul 2019 at 09:05, Yair Halberstadt ***@***.***> wrote: Currently switch expressions emit a warning if they are not exhaustive. The following will warn: enum Reply { Yes, No, } ...Reply reply = GetReply();reply switch { Reply.Yes => true, Reply.No => false, }; The reason for this is that this enum can be any value of type int, not just its declared values. This is only a warning, so the compiler can still compile the code. This is what the compiled code looks like: reply switch { Reply.Yes => true, Reply.No => false, _ => throw new InvalidOperationException(), }; It throws in the unexpected case. I think it can safely be assumed that the aim of this warning is to reduce bugs. There are two choices on how strict to be in exhaustiveness checking: a) *Pragmatic:* A switch expression is considered exhaustive if all declared values for the enum are matched. This will lead to exceptions being thrown at runtime if the enum turns out to be some other value. b) *Strict:* A switch expression is considered exhaustive if all possible values for the enum are matched. In practice the means adding a _ => arm to the switch, since for anything other than a byte based enum this is impossible. This will lead to developers not being warned if a new value is added to the enum which they do not account for. We need some data to help drive this decision. I went through every single example in Roslyn where a switch expression on an enum handles every single declared value of the enum. They all currently have a _ => arm to remove the warning. Here are all the methods where they are: NullableAnnotationExtensions.MergeNullableAnnotations NullableAnnotationExtensions.ToPublicAnnotation NullableAnnotationExtensions.ToInternalAnnotation NullableFlowStateExtensions.ToPublicFlowState NullableFlowStateExtensions.ToInternalFlowState SeparatedSyntaxListCodeActionComputer.GetNestedCodeActionTitle CSharpTriviaResult.AnnotationResolver CSharpTriviaResult.TriviaResolver CSharpUseRangeOperatorCodeFixProvider.CreateRangeExpression ExecutionConditionUtil.Architecture ProjectExternalErrorReporter.ReportError2 VSTelemetryLogger.CreateAndStartScope VSTelemetryLogger.LogBlockEnd BaseDiagnosticItem.BrowseObject.MapDiagnosticSeverityToText BaseDiagnosticItem.BrowseObject.MapReportDiagnosticToText CSharpProjectShim.OptionsProcessor.SetOutputFileType FindAllDeclarationsTests.GetSolution CodeStyleOption<T>.FromXElement Extensions.ToReportDiagnostic AbstractTriviaFormatter.GetRuleSpacesOrIndentation AbstractTriviaFormatter.GetRuleSpacesOrIndentation //second switch on a different type in the same method SerializableBytes.Seek CsharpSyntaxGenerator.GetParameterModifiers CsharpSyntaxGenerator.GetTokenKind That is 24 locations. In all but 3, the _ => arm threw an exception. This means the exhaustiveness checking is gaining the developer nothing since an exception would be thrown even without that arm. In FindAllDeclarationsTests.GetSolution null is returned by the _ => arm. This is a private method that is used 16 times. In every single one of those 16 usages, if null had been returned, a NRE or an ANE would have been thrown later on. In other words, the default behavior of throwing an exception if not all cases are accounted for would have been better than the solution the developer chose to do instead, since it would prevent cascading errors. VSTelemetryLogger.CreateAndStartScope and VSTelemetryLogger.LogBlockEnd are more interesting. As loggers they are coded extremely defensively, and throwing an exception for them is useless, since it wont be reported anywhere. Instead on the invalid arm they report the exception rather than throwing it: try { // guard us from exception thrown by telemetry var kind = kvLogMessage.Kind; switch (kind) { case LogType.Trace: EndScope<OperationEvent>(functionId, blockId, kvLogMessage, cancellationToken); return; case LogType.UserAction: EndScope<UserTaskEvent>(functionId, blockId, kvLogMessage, cancellationToken); return; default: FatalError.Report(new Exception($"unknown type: {kind}")); break; } } catch { } In conclusion, in 21 out of 24 cases the strict exhaustiveness checking caused the same code to be emitted as it would have been under the pragmatic exhaustiveness checking. In 1 case it led to worse code being emitted, as it lead to cascading errors, rather than errors at the point where they should have been reported. In 2 cases it lead the developer to write better code for their use case. But this was in a class that was anyway very defensively written, and not your typical use case. In all 24 cases there will be no warning if another enum value is added which the switch does not account for, which there would have been under pragmatic exhaustiveness checking in 22 of those cases. It seems clear to me that overwhelmingly, more bugs will be caught if csharp uses pragmatic exhaustiveness checking rather than strict. Notes C# 8.1 It is very close to C# 8.0 release - possibly too close to make this change. It is not an error to remove a warning later, so this could be changed in C# 8.1 if necessary. InvalidOperation is not a very good exception It may be pointed out that I've been unfair here. By default the compiler will emit an InvalidOperationException with no information as to what caused the exception. A developer can choose to throw a more useful exception. A good example might be the InvalidEnumArgumentException <https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.invalidenumargumentexception?view=netframework-4.8> which stores the name of the enum, the type of the enum, and the value of the enum. The compiler might not be able to emit these instead of InvalidOperationException due to the security risk of revealing the names of locals. Also it would have to be a new exception called InvalidEnumSwitchException or something similiar, since the switch might not be on an argument. However if that is the concern, it might be possible for the developer to opt in to which exception should be thrown using attibutes, or some other method. Perhaps an assembly attribute could be overriden by a class attribute could be overriden by a member attribute, etc. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <https://github.com/dotnet/csharplang/issues/2671?email_source=notifications&email_token=ADIEDQLP7PTFGGPBLQQQC6DQAFYWBA5CNFSM4IFDPE52YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HAGS2VA>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADIEDQJKPFGZITEZYOPQABDQAFYWBANCNFSM4IFDPE5Q> .

[CyrusNajmabadi]

I'd prefer the compiler be loose here. And literally ship with analyzers in-the-box that allowed people to just state that they wanted more strictness.

Specifically i would prefer the following:

1. compiler ships with no warnings about exhaustiveness for switch expressions.
2. we have a roslyn analyzer (which should ship in-box in VS as well) with three options. "No switch exhaustiveness warnings", "warn if any declared switch value is missing" "warn if any possible switch value is missing".

users then opt into the level of checking they want.

[CyrusNajmabadi]

Tagging @jcouv .

[YairHalberstadt]

A further point is that in about 80% of all cases the _ => arm just threw, even if not all declared enums were accounted for, so I think theres a very strong case to be made that no exhaustiveness checking is a good default.

[spydacarnage]

@CyrusNajmabadi - that works. One of the things I liked about the switch expression was the F#-Match-like exhaustiveness checking, so as long as it is still there in analyzer form, I'd be happy.

[canton7]

we have a roslyn analyzer (which should ship in-box in VS as well) with three options. "No switch exhaustiveness warnings", "warn if any declared switch value is missing" "warn if any possible switch value is missing".

So long as this runs as part of the build process, and not just when the offending file is open. The value here is in being able to change an enum and see all switches which are affected, wherever in the solution they might be.

[YairHalberstadt]

Some other cases that ought to be considered:

a) The following warns, since there might be IVTs. But in 99% of cases that warning is unhelpful:

using System;
public class P {
    public string M() 
    {
        A a = GetA();
        return a switch {
                B b => "b",
                C c => "c",
                null => "null",
        };
    }

    private A GetA()
    {
        throw new NotImplementedException();
    }
}

internal abstract class A{}
internal class B : A {}
internal class C : A {}

b) Flags are likely to be treated differently to other enums, as they are very difficult to treat exhaustively.

c) Someone might want to have a catch all where he throws a custom exception, but still warn if he doesn't explicitly handle all enums.

I think all of this is pointing to the fact that exhaustability checking for switch expressions is not a one size fits all matter. I think it makes most sense to leave it out the language entirely, but ship analysers with roslyn by default, which can be configured to your preferences. They also could pick up on attributes/comments to adjust the settings for a specific switch.

[CyrusNajmabadi]

So long as this runs as part of the build process, and not just when the offending file is open.

Yes. That's how analyzers work. :)

[CyrusNajmabadi]

@gafter @jcouv @MadsTorgersen as i know there is very little time, but i do think this warrants attention for 8.0.

[HaloFour]

The impetus for this discussion I believe was #2667 where the OP wanted a feature that would allow them to associate a value of an enum with a result and have the compiler enforce that every value of the enum would be matched, particularly in the case where a new value is added to the enum after the fact. They had a specific feature in mind based on one from Delphi but the conversation turned to switch expressions.

The problem is that with the current exhaustiveness check you have two options:

1. Leave out the catch-all, in which case the compiler will always warn and that isn't helpful at all.
2. Have the catch-all, in which case the compiler will never warn as the catch-all will handle missed enum values, and that isn't helpful at all.

You could go with 2 and have analyzers do a second pass on exhaustiveness checking, but at that point what's the benefit of having the compiler do the work? You'd still be required to supply the catch-all to silence the compiler, and odds are the only thing you could do in that case is throw anyway.

[canton7]

Yes. That's how analyzers work. :)

It's not how IDE0010 works, so wanted to make sure!

[CyrusNajmabadi]

It's not how IDE0010 works, so wanted to make sure!

Understood :)

[gafter]

there is very little time, but i do think this warrants attention for 8.0.

C# 8 is done. Only bug fixes for serious issues like crashes or bad code generation can possibly be included.

[RemmondSoftTechInc]

I'm the original OP of #2667. I wasn't aware of the exhaustiveness check of the switch statement, which is interesting, but it's not what I was looking for in #2667. I'm not sure how I feel about my original idea of using enums as array indexes evolving into enhancing the switch statement. I'm concerned the switch behavior will be changed, risking backward compatibility, and I'm no further ahead in my original requirement. I was looking for a clean/simple way to define lookup arrays indexed on an enum (simple enums starting at 0) that would get the compiler to report an error if there's arrays that are missing values (similar to what it does now if you have a set sized array, see original post for more info).

[HaloFour]

@RemmondSoftTechInc

I'm not sure how I feel about my original idea of using enums as array indexes evolving into enhancing the switch statement.

The switch expression was already being added to the language, along with exhaustiveness checking. The question was whether or not that exhaustiveness checking as it applied to matching enums would satisfy the desire to know if you've matched all possible values of that enum. Unfortunately it sounds like the answer is "no", at least for the C# 8.0 timeframe.

Your proposal would be evaluated separately from switch expressions or pattern matching in general.

[Eirenarch]

The problem is we'll also need an analyzer to remove warnings

[yaakov-h]

How so?

[Eirenarch]

Because I don't want to have to write a default case to suppress a warning. Now technically I can suppress the warning in the config but then I am suppressing it for values like ints where I want it. In addition it feels super strange that the case for 99% of users would be to suppress a warning and then add an analyzer.

[mqudsi]

I‘ll disagree with @Eirenarch here, my reasoning is simply that there is much more merit in Roslyn warning about an explicitly extant enum that isn’t covered in a switch than there is in it always warning it the default branch isn’t covered. I.E. if anything I feel one should “write an analyzer” to force explicit handling of the _ branch rather than the other way around.

Whether the goal is to maximize utility or to maximize safety, I feel the developer’s interests are best served by prioritizing the dual logic/safety error of “explicit enum not covered” over “default behavior not overridden” (because it is implicitly covered by the codegen with the throwing behavior).

[Eirenarch]

@mqudsi I don't see how you disagree with me. I've been saying the same thing all through this thread.

[mqudsi]

Sorry, I meant about wanting to remove warnings. I don’t have a problem writing extra code to please the compiler (in the presence of a theoretical extra analyzer to check for unused enums I would not be sufficiently irked by the extra branch being required to want to strip that warning out). But we absolutely agree on what the ideal behavior should look like, I think.

[Eirenarch]

The problem is that not only is this code bloat (a reminder that the compiler will indeed add the exception case for you) but that if you really want to handle some cases via the default clause you can't because it will result in a warning

[gafter]

Please see #3179

[stephan-tolksdorf]

How do you distinguish: "they accidentally left this out", and "they purposefully left it out"?

Generate two different warnings for the case where a switch doesn't handle a named enum value and for the case where the switch doesn't handle a value for which no enum constant exists. Then I can selectively suppress the latter warning and still get a compiler warning when somebody forgets updating a switch after adding a new enum constant.

[CyrusNajmabadi]

This feels like something an analyzer could really simply do. It would even be a good example used to teach people how to write analyzers (esp. IOp analyzers).

[gafter]

Once dotnet/roslyn#44702 is integrated, there will be different warnings when a named enum value is not handled vs when an unnamed enum value is not handled. In the former case the compiler will tell you the name of an enum constant that is not handled. In the latter case it will say something like For example '(EnumType)23' is not handled.

[binki]

@gafter

Once dotnet/roslyn#44702 is integrated, there will be different warnings when a named enum value is not handled vs when an unnamed enum value is not handled. In the former case the compiler will tell you the name of an enum constant that is not handled. In the latter case it will say something like For example '(EnumType)23' is not handled.

I was hoping that by “different warnings” you meant different diagnostic codes. Then I could blacklist the the named warning with WarningsAsErrors and then silence the other one (since I would rather catch bad data at the edge/load/parse-time and am fine with a runtime explosion if it somehow gets in because that should be impossible). However, both warnings use the same code of CS8509, so I can’t distinguish them without parsing post-translation/human-readable text…

C:\Users\ohnob\repos\Main 33\Code\«redacted».cs(1207,25,1207,31): error CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '(System.Data.DbType)24' is not covered.

If we had different warnings for the undefined enum case and the defined case, that would solve this for me.

[binki]

Looks like this happened in dotnet/roslyn#47066 . Yay, thanks @gafter and @dextercd!

[jonkuhn]

I agree with @binki I would really like to be able to have a way to write a pattern matching switch statement such that the compiler would error if a new value was added to the enum that was not handled. If it were possible to write the switch statement without the default case and then ignore one warning (for not handling every possible runtime value), but configure another warning as an error (for not handling a named enum value) that would solve this use case.

[nmoreaud]

I think the integer value warning should be off by default, this is cryptic: For example '(EnumType)23' is not handled.

[binki]

That error is pretty clear to me. You do understand that the following is valid code, right? How else would the error be written to be clearer?

using System;

UseMyEnum(MyEnum.A);
UseMyEnum((MyEnum)4);

void UseMyEnum(MyEnum myEnum) {
    Console.WriteLine($"{myEnum}={(int)myEnum}");
}

enum MyEnum {
    A,
    B,
}

Output:

A=0
4=4

[nmoreaud]

Sure this is valid, but it is not the main use case.
If I modify my code to get rid of the warning, I will forget to add case entries for new enum values

[HaloFour]

The compiler intentionally emits two different warnings, CS8524 if you don't cover "invalid" enum values and CS8509 if you didn't cover all of the values declared in the enum. You can tell the compiler to ignore one and not the other.

[jnm2]

InvalidEnumArgumentException should only be thrown if the thing being switched on is a method argument.

[binki]

What’re you replying to? ;-)

[jnm2]

I'm replying to the post.

InvalidOperation is not a very good exception

It may be pointed out that I've been unfair here. By default the compiler will emit an InvalidOperationException with no information as to what caused the exception. A developer can choose to throw a more useful exception. A good example might be the InvalidEnumArgumentException which stores the name of the enum, the type of the enum, and the value of the enum.

[jnm2]

Oh, I missed this sentence which clears it up:

Also it would have to be a new exception called InvalidEnumSwitchException or something similiar, since the switch might not be on an argument.