Proposal: sealed enums (completeness checking in switch statements)

[orthoxerox]

Since full-blown discriminated unions are a long way away (they'll come after records and records will come in C#8.0 or later), I propose to implement a much smaller feature that will allow us to avoid incorrect defaults or skipped clauses.

1. A new enum-targeted attribute should be introduced, System.SealedEnumerationAttribute. It is a compile-time error when a single type has both System.SealedEnumerationAttribute and System.FlagsAttribute.
2. Enumeration declarations should accept sealed modifier. This is equivalent to decorating the type with System.SealedEnumerationAttribute.
3. Switch statement binder should check the type of its expression. If it's a sealed enumeration, then the list of switch labels must not include pattern labels or default lables and must include every single enumerated value.
4. To prevent enumeration values added in another assembly after compilation from silently skipping the switch statement, a default case must be added by the binder to all such switch statements. The default case should throw.

[svick]

If it's a sealed enumeration, then the list of switch labels must not include default, must not include pattern labels and must include every single enumerated value.

This sounds too restrictive to me. I think you want to prevent mistakes in the common case, where every enum value has a separate case. But I don't see why you should also prohibit more advanced cases, like default or patterns, especially considering that it's the enum author who chooses whether it's sealed or not, not the user.

---

How much protection will there be from creating a "wrong" value? I assume the compiler would not allow MySealedEnum e = (MySealedEnum)42;. But would the CLR prohibit MySealedEnum e = (MySealedEnum)Enum.ToObject(typeof(MySealedEnum), 42); or MySealedEnum e = (MySealedEnum)(object)42;?

[alrz]

If it's a sealed enumeration, then the list of switch labels must not include default, must not include pattern labels and must include every single enumerated value.

(1) I think this should only affect completeness of a switch statement, e.g.

[Complete]
enum E { A, B }
int M(E e) {
    switch (e) {
      case E.A: return 0;
      case E.B: return 1;
    }
} // no error (CS0161)

(2) The default case never contribute to subsumption checking, for example, you can have a default case after case var. This is because default is evaluated regardless of where it appears. However a discard i.e. case _ should be flagged as unreachable if the enum is marked as complete.

(3) I agree that this should emit the default case anyways but perhaps we should add this functionally to struct ADTs so that we don't have to worry about CLR enums accepting any possible value of the underlying type.

[Pzixel]

How it will work with serialization? For example:

static void Main()
{
	var colors = new List<E> { E.A };
	var result = Serialize(colors);
	Console.WriteLine(result);
}

public static string Serialize<T>(T obj)
{
	var serializer = new NetDataContractSerializer();
	var sb = new StringBuilder();
	using (var writer = XmlWriter.Create(sb))
	{
		serializer.WriteObject(writer, obj);
		writer.Flush();
		return sb.ToString();
	}
}

[Complete]
enum E { A = 1, B }

Now it fails at runtime because of internals of NetDataContractSerializer. But it's just because it checks if enum is actually is in valid range. However, your enum have to check it at compile time. And here you see what happens: we just can encounter a situation where abstractions leak and we can't even add this enum to the list.

[YotaXP]

I would prefer something more along the lines of D's final switch statement. An alternative version of switch that fails to compile if any of the enumeration values are not used.

Makes more sense to me for this sort of restriction to be requested by the consumer, rather than specified on the type itself.

[Pzixel]

@YotaXP and what if this final switch encounter the situation when value doesn't have a corresponding branch? Assume we have only branches for true and false boolean:

using System;
using System.Runtime.InteropServices;
 
public class Test
{
	public static void Main()
	{
		bool x = LolBool.Value;
 
		switch (x)
		{
			case true:
			case false:
				break;
			default:
				Console.WriteLine("lol");
				break;
		}
	}
 
	public static class LolBool
	{
		public static readonly bool Value = new LolBoolUnion(0x80).BoolValue;
 
		[StructLayout(LayoutKind.Explicit)]
		private struct LolBoolUnion
		{
			[FieldOffset(0)]
			private readonly byte byteValue;
			[FieldOffset(0)]
			public readonly bool BoolValue;
 
			public LolBoolUnion(byte byteValue) : this()
			{
				this.byteValue = byteValue;
			}
		}
	}
}

[orthoxerox]

@Pzixel was your comment addressed to svick or to me? Because I don't see any switches in your example code. I do not propose to prevent instantiation of invalid enum values in this issue.

[jnm2]

@Pzixel Copying my code is absolutely fine, but converting it to use tabs... tsk tsk. 🤔

[Pzixel]

@orthoxerox I'm just going your way and show that even today we have some problems with invalid enum values and cannot just remove them from the language or add a new feature that works this different. And I was talking with @alrz .

[sirgru]

I also believe the control whether the whole enum should be used is the consumer's responsibility. In my view, such hypothetical final switch would not allow default, but would have a 'hidden' default clause that throws an exception if ever reached. This way it could combine reasonable compile-time safety with runtime 'safety' (doing what is expected by the user).

[alrz]

Ha, this is the reversed proposal for rust-lang/rfcs#2008 😄

[abjennings]

I would still love to see algebraic data types and real enums in C#, but in the meantime, I created a compiler extension that helps me make sure I'm getting all the cases when I want to:

https://github.com/abjennings/EnumAnalyzer

It looks for places where I throw an "EnumNotExhaustedException", then checks the surrounding code block to make sure all the defined enum values are mentioned at least once. If not, it gives me a diagnostic error.

There are definitely corner cases it won't handle, but for me, it seems a good 90/10 solution (90% of the benefit for 10% of the cost). Also, it's my first compiler extension, so any feedback is appreciated...

[colejohnson66]

This is still a feature I'd like to have. Currently, with switch expressions, the compiler still complains about non-completeness, and wants a default arm:

public enum MyEnum
{
    A,
    B,
}

// ...

public int GetValue(MyEnum value) =>
    value switch
    {
        MyEnum.A => 0,
        MyEnum.B => 1,
        // one of the following, depending on desired contract
        _        => throw new ArgumentOutOfRangeException(nameof(value), value, $"Unknown enum value of type {nameof(MyEnum)}."),
        _        => throw new UnreachableException(),
    };

It's not that big of a deal, but it can be annoying sometimes.

[brantburnett]

I would argue that requiring the default arm on switch expressions for enums, as it stands today, is actually detrimental in some cases. Specifically, I always have "what if" concerns when you have two different teams or developers working on two different parts of the code, whether internal to a large project or consumption of a library or any other reason. Specifically, imagine this scenario from above:

public enum MyEnum
{
    A,
    B,
}

public int GetValue(MyEnum value) =>
    value switch
    {
        MyEnum.A => 0,
        MyEnum.B => 1,
        _ => throw new ArgumentOutOfRangeException(nameof(value), value, $"Unknown enum value of type {nameof(MyEnum)}.")
    };

However, let's assume that the enumeration and the consuming method are maintained by different developers in different projects. Perhaps the enumeration is part of the .NET runtime. When an upgrade occurs, a new enumeration value is added.

public enum MyEnum
{
    A,
    B,
    C,
}

public int GetValue(MyEnum value) =>
    value switch
    {
        MyEnum.A => 0,
        MyEnum.B => 1,
        _ => throw new ArgumentOutOfRangeException(nameof(value), value, $"Unknown enum value of type {nameof(MyEnum)}.")
    };

In this case, because the default arm is present, there is no compiler warning that you have an unhandled case. It would be easy for the developer to move on, thinking everything is fine, but then get unexpected behaviors in production at runtime when the new enumeration value reaches the method.

I understand the need for the default arm, because the enumeration is an integer and could be any value, even one not defined on the enumeration. However, it would be nice to support some method to (optionally) still require all enum values to be handled at compile time. Perhaps something like below would give a warning or error:

public int GetValue(MyEnum value) =>
    value switch
    {
        MyEnum.A => 0,
        MyEnum.B => 1,
        checked _ => throw new ArgumentOutOfRangeException(nameof(value), value, $"Unknown enum value of type {nameof(MyEnum)}.")
    };

[brantburnett]

True, but this means that in order to get the warning about not covering all enum values I must leave off the default arm, meaning that I then lose runtime coverage for the case where the enum was somehow mapped to an invalid value.

[333fred]

meaning that I then lose runtime coverage for the case where the enum was somehow mapped to an invalid value.

No, you get an exception in that case already. https://sharplab.io/#v2:C4LgTgrgdgPg4gU2ANQIYBsIIBTYLICeAolBALYCUAjAAwUDcAsAFAsCWUwABIihlvmKkyXAG78EFLgF4AfCy6KxErgGcA7m2ABjABYKlAbwNKlhEuQB0AQRmyuNADQnTXc8MsAhO1yrPmpgC+TKzMAAIAzFwIwm5C5CzGAUrW/qae/oEsQA

[brantburnett]

But not an exception under the developer's control, unless you wrap in a try/catch block and throw a different exception, which is a lot more ceremony. In my experience, the odds of getting my team to remember to do this are near 0. A simple keyword, such as my example of checked, at least has a shot.

public int GetValue(MyEnum value)
{
    try
    {
        #pragma warning disable CS8524
        return value switch
        {
            MyEnum.A => 0,
            MyEnum.B => 1,
        };
        #pragma warning restore CS8524
    }
    catch (SwitchExpressionException)
    {
        throw new ArgumentOutOfRangeException(nameof(value), value, $"Unknown enum value of type {nameof(MyEnum)}.");
    }
}

[CyrusNajmabadi]

In my experience, the odds of getting my team to remember to do this are near 0.

You can always write an analyzer catered to your teams' needs here.

[brantburnett]

You can always write an analyzer catered to your teams' needs here.

Yes, I could. But that would be very complex and difficult, at least to those of us that don't live in the Roslyn world. I'd estimate it'd take me a couple weeks to get it working right with decent performance, especially since I've historically found little to no documentation on how to write analyzers well (please point me to it if I'm just missing it).

I'm just trying to propose something I feel would be a nice-to-have feature in the compiler to prompt discussion, not just for me but for many users. I was thinking of it much like the static keyword on lambda methods. That's basically a compile-time only feature that doesn't affect generated code (to my knowledge) and that was relatively low impact on the language because it reused an existing keyword. But it makes it much easier to prevent developers from making unexpected mistakes. It seems to me that reusing the checked keyword here to reenable the CS8509 warning even if there's a default arm is similar, but maybe it's more complicated than I think. Or maybe there could be a new built-in analyzer that's off by default that uses a different warning value that could be turned on to cover this case?