Proposal: MatchFailureException

[gafter]

Rationale

In order to facilitate the language feature at dotnet/csharplang#45, which has been approved by the LDM and mostly implemented, we should add the exception MatchFailureException to the framework. This exception is to be thrown when no branch of a pattern-matching switch expression matches the input at runtime.

Proposal

namespace System
{
    /// <summary>
    /// Indicates that a switch expression that was non-exhaustive failed to match its input
    /// at runtime, e.g. in the C# 8 expression <code>3 switch { 4 => 5 }</code>.
    /// The exception optionally contains an object representing the unmatched value.
    /// </summary>
    [System.Runtime.InteropServices.ComVisible(true)]
    [Serializable]
    public class MatchFailureException : InvalidOperationException
    {
        public MatchFailureException();
        public MatchFailureException(object unmatchedValue);
        public object UnmatchedValue { get; }
        [System.Security.SecurityCritical]
        public override void GetObjectData(SerializationInfo info, StreamingContext context);
    }
}

See also dotnet/roslyn#27747
/cc @jcouv @jaredpar

[jcouv]

It may be useful to provide more context with an example of "when no branch of a pattern-matching switch expression matches the input at runtime."

[svick]

Should the exception include a constructor that takes a string message? Even if the compiler didn't use it initially, that constructor could be useful in the future to provide a more detailed message.

[gafter]

@svick Like with NullReferenceException, I can't imagine circumstances in which the compiler could use the message parameter.

[gafter]

I've added an (optional) object unmatchedValue parameter.

[gafter]

I changed the base type to InvalidOperationException.

[stephentoub]

It should also override GetObjectData (and maybe ToString).

[gafter]

@stephentoub I was proposing the API, not the implementation.

[stephentoub]

@gafter, it actually is API (at least from the perspective of what's in a ref assembly). The type isn't sealed, so if those aren't exposed in the API but they're there in the implementation, a derived type doing "base." to invoke them may end up skipping the implementation and going to the next ancestor in the hierarchy.

[Neme12]

With the current proposal, if UnmatchedValue is null, there's no way to know whether the value was actually null or unmatchedValue wasn't provided. Also, why is unmatchedValue optional? When would the compiler omit it?

[stephentoub]

When would the compiler omit it?

e.g. if it's something that can't be boxed, like a ref struct

[Neme12]

If only we had Optional<T>... Adding bool HasUnmatchedValue is ugly but I don't see any other option.

[gafter]

Updated to add GetObjectData

[terrajobst]

We had a quick look today. It looks we should have a chat with the C# folks to better understand the scenario.

Why do we need a new exception? Can't we just throw an existing one, such as:

• NotImplementedException
• InvalidOperationException
• NotSupportedException

One of the reasons to not require a new exception type is that can (trivially) support this feature on all existing .NET platforms without any problems.

It seems (at least to me) that he unmatchedValue isn't a good idea if it can't be used in all cases, such as ref structs (because it means developers have to read the message and check the source code anyway).

[stephentoub]

It seems (at least to me) that he unmatchedValue isn't a good idea if it can't be used in all cases, such as ref structs (because it means developers have to read the message and check the source code anyway).

FWIW, I pushed for this to be included because it can only help with debugging, and in most cases it should be able to include the value (it won't in corner cases like matching on a ref struct). The value's ToString would be output as part of Exception's ToString, similar to how it is for ArgumentOutOfRangeException.

[Neme12]

I think in most cases developers would not have to check the source code, as long as there's a property called HasUnmatchedValue or an equivalent way of knowing that the unmatched value is present (and therefore it's actually null, not just a missing piece of information).

[gafter]

Why do we need a new exception?

There is no existing exception whose meaning is close enough for our purposes:

• NotImplemenetedException is for when a requested method or operation is not implemented. That is not the case here.
• InvalidOperationException is for when a method call is invalid for the object's current state. That is not the case here.
• NotSupportedException is for when an invoked method is not supported. That is not the case here.

Most other systems and languages that have pattern-matching use a designated exception for this situation.

Our current plan does not have a problem running on older platforms, but at the loss of a meaningful exception on such platforms - you get the not quite appropriate InvalidOperationException.

[jcouv]

@gafter I assume that once this type is added for Core 3, you'll want it ported to mono as well.
If so, please file an issue there (similar to mono/mono#12019) and track it in the umbrella issue for patterns feature.

[terrajobst]

Our current plan does not have a problem running on older platforms, but at the loss of a meaningful exception on such platforms - you get the not quite appropriate InvalidOperationException.

That seems like a bad idea as that's a source breaking change:

1. I compile for .NET Core 2.x. I get InvalidOperationException.
2. I retarget to .NET Core 3.x. I now get MatchFailureException.

Are we sure we want behavior mismatches like this? FWIW, I think either of the existing exception APIs is good enough. Personally, I think NotImplementedException is the closest match as (1) this bug is virtually unrecoverable (2) the author didn't implement all the cases. However, I do buy the argument that having a dedicated exception type would be slightly more desirable, especially because it can provide better diagnostics. The proposed type shape looks good to me.

My only concern is that this leads to source breaking changes.

[gafter]

@terrajobst What do you mean by source breaking changes?

If your program crashes on .NET Core 2.x, it will crash on .NET Core 3.x.

If you catch the excepetion (using the type InvalidOperationException) on .NET Core 2.x, your code will also catch the exception on .NET Core 3.x, as MatchFailureException is derived from InvalidOperationException.

What is the source breaking change you're concerned about?

[danmoseley]

Would it be confusing to have an exception named MatchFailureException that isn't used by Regexes? I'm wondering if the name could be more specific.

[terrajobst]

If you catch the excepetion (using the type InvalidOperationException) on .NET Core 2.x, your code will also catch the exception on .NET Core 3.x, as MatchFailureException is derived from InvalidOperationException.

Ah, I missed the inheritance relationship. That addresses my concern.

[terrajobst]

Would it be confusing to have an exception named MatchFailureException that isn't used by Regexes? I'm wondering if the name could be more specific.

What about PatternMatchException?

[Neme12]

Why would "match" imply regex? Is that a regex-specific word?

[danmoseley]

Why would "match" imply regex? Is that a regex-specific word?

No, but it's a common use of it. For example our public type named Match is part of regex.

What about PatternMatchException?

I think that's better (although Pattern is also a term used in regex API, PatternMatch is not)

[terrajobst]

Video

• There is a possibility for a breaking change: library author ships a library for 3.0 and then later downgrades to 2.0, which is is expected to be transparent to consumer. Now a different exception exception is thrown. Seems remote though.
• We don't expect any user code to specifically handle this exception (outside of implicit catch-all style handlers). We should move this to System.Runtime.CompilerServices.
• We should have a less generic name, for instance SwitchExpressionException.

namespace System.Runtime.CompilerServices
{
    /// <summary>
    /// Indicates that a switch expression that was non-exhaustive failed to match its input
    /// at runtime, e.g. in the C# 8 expression <code>3 switch { 4 => 5 }</code>.
    /// The exception optionally contains an object representing the unmatched value.
    /// </summary>
    [System.Runtime.InteropServices.ComVisible(true)]
    [Serializable]
    public sealed class SwitchExpressionException : InvalidOperationException
    {
        public SwitchExpressionException();
        public SwitchExpressionException(object unmatchedValue);
        public object UnmatchedValue { get; }
        [System.Security.SecurityCritical]
        public override void GetObjectData(SerializationInfo info, StreamingContext context);
    }
}

[Neme12]

So how do we know whether UnmatchedValue is present or not? That's impossible to tell with the current API 😕

[gafter]

The current shape of the API does not provide a way for someone catching it to know if an unmatched value is present. However, it is not intended for programmatic use.

[Neme12]

But it's important for debugging. If I see this exception and see that UnmatchedValue is null, I know nothing. I have no idea whether it was actually null or if this information is just missing. (This is quite sad because I'm guessing null will be the most common unmatched value.)

[Neme12]

Since this is not intended to ever be caught and accessed by user code, could UnmatchedValue just throw if the value is unknown, with an additional HasUnmatchedValue property?

[gafter]

@terrajobst The name of this exception is now tied to the name of the construct in C#. If we add a similar construct in VB, it probably won't be called a switch expression, and reusing this exception would seem like a stretch.

[danmoseley]

@maryamariyan can you please add this new exception next week? Looks like it's super easy, API is approved, basically no code (like eg InvalidDataException) except for this object getter, basic tests to new it up and get/set.

@gafter @terrajobst are [System.Security.SecurityCritical] and [ComVisible(true)] both required? SecurityCritical has no effect in .NET Core and no other exceptions have it. No other exceptions have ComVisible(true) either.

[danmoseley]

@terrajobst can you confirm whether it's SwitchExpressionEXception as reviewed or MatchFailureException as @gafter requests just above.

[gafter]

It is SwitchExpressionException as reviewed https://github.com/dotnet/corefx/issues/33284#issuecomment-448332714

[danmoseley]

@gafter OK thanks, I was confused by your subsequent comment suggesting you disagreed. https://github.com/dotnet/corefx/issues/33284#issuecomment-448385260

[danmoseley]

@gafter this is in please feel free to wire it up!

[danmoseley]

@jaredpar @jcouv just making sure you saw this is now available and you aren't waiting on us.

[gafter]

The compiler is all wired up in the latest VS2019 preview, though I haven't tried it with the latest Core.