API Proposal: UnreachableException

[carlreinke]

Motivation

This exception is expected to never be thrown. Code coverage tools might choose to ignore non-coverage of the statement or branch that throws this exception. In fact, coverage of such a statement or branch is undesirable, as it would indicate that the code is making an invalid assumption.

Proposal

#nullable enable
using System.Runtime.Serialization;

namespace System
{
    public sealed class UnreachableException : SystemException
    {
        public UnreachableException()
            : base("The program executed an instruction that was thought to be unreachable.")
        {
        }

        public UnreachableException(string? message)
            : base(message)
        {
        }

        public UnreachableException(string? message, Exception? innerException)
            : base(message, innerException)
        {
        }

        private UnreachableException(SerializationInfo info, StreamingContext context)
            : base(info, context)
        {
        }
    }
}

See Also

• ExceptionUtilities.Unreachable in runtime
• ExceptionUtilities.Unreachable in Roslyn

[carlreinke]

@bartonjs @joperezr Any concerns or can this be ready-for-review?

[GrabYourPitchforks]

How would this interact with the proposed "never" return type (dotnet/csharplang#538)? Is this API a point-in-time patch until that feature comes online?

Presumably code coverage analyzers could already look for methods marked with [DoesNotReturn] and exclude any code after such call sites from being included in the final metrics.

It would be helpful to clarify this exception's place in the ecosystem in a world where both of these capabilities exist.

[carlreinke]

[DoesNotReturn] and "never" do not apply in a lot of cases where UnreachableException would be used.

Consider a random example taken from Roslyn:
https://github.com/dotnet/roslyn/blob/e704ca635bd6de70a0250e34c4567c7a28fa9f6d/src/Compilers/CSharp/Portable/Binder/BlockBinder.cs#L64-L72

In this case, the method does sometimes return (in fact, if the unreachability assumption is correct, it always returns), so neither [DoesNotReturn] nor "never" are applicable to that method, yet there is a code path that is expected to be unreachable.

Applying [DoesNotReturn] or "never" to an "Unreachable" helper method and calling that instead doesn't solve the problem because that helper method still has to throw some exception to indicate that the presumably unreachable code path was executed (or it could FailFast, but that's not what you want in a lot of cases).

Also, code coverage should expect not to cover the throw new UnreachableException() statement itself, not code following it. Any code following it would already be considered unreachable by the compiler's reachability analysis by nature of an exception being thrown.

It would actually also be the reverse -- any code on a code path that inevitably leads to throw new UnreachableException() can be considered "unreachable" for code coverage purposes. (Though I'm not suggesting anything would or even should do that analysis.)

[GSPP]

This seems very useful to me. Sometimes, a code location is to be considered unreachable. Having this exception type confers the following benefits:

1. It documents intent.
2. It makes C# flow analysis understand that this point in the code does not continue execution.
3. If this is ever reached at runtime due to a bug, the algorithm aborts. Presumably, control transfers to some generic top-level exception handler that logs the exception and alerts developers.
4. Resharper could be taught to understand what this type means.
5. Having this in the BCL creates an ecosystem-wide standard.

Adding this type would not be a complicated change. It can easily be trimmed. The assembly size increase would be modest.

I have defined this type myself and I'm using it.

[carlreinke]

@GrabYourPitchforks Any other concerns or can this be put into the review queue?

[GrabYourPitchforks]

Marking this as ready for review.

Open question for reviewers: given that the target scenario is for debugging, code coverage, and other dev tools; would System.Diagnostics be a more appropriate namespace than System?

[marek-safar]

I like this and it'd be also useful for tools like IL linker which have to now uses NotSupportedException with a string explanation that the body is unreachable.

[bartonjs]

System.Diagnostics feels more appropriate to me. In all the cases I'd see myself using it I currently have a Debug.Fail, then the exception so the compiler doesn't tell me my execution flow makes no sense.

If we end up doing a public ThrowHelper-type API for ArgumentNullException (#48573) then we'd probably want one here, too.

[FilipToth]

The only case where I'd use this is when I have an unexpected case in a switch statement of something, what I usually do is just throw a regular System.Exception with exception text being something like: "Unexpected .". This API seems very niche to me. Could you provide some direct cases where using UnreachableException over just regular Exception would be useful. Also, I would certainly not go over all my previous code and change the exceptions.

But, for new code, I'd certainly consider using this exception type, as @marek-safar already mentioned, This would be good for some applications which now had to use NotSupportedException, I guess. Although, I do not think this should be a priority.

[GrabYourPitchforks]

Could you provide some direct cases where using UnreachableException over just regular Exception would be useful.

@FilipToth Per earlier discussion, the best use cases seem to be debuggers, flow analyzers, and other design-time and diagnostics tools. I don't think anybody would go back and change their existing code to use this API unless they want the benefits these tools offer.

[bartonjs]

• This exception made us muse for a long time on wishing that we had better language support for showing that control won't come back. (General ThrowHelper problem)
• We didn't add a static Throw method yet, because of the ThrowHelper control flow problem
• We might want to get the runtime to consider it uncatchable, like StackOverflowException (or what @bartonjs remembers ThreadAbortException doing: you can catch it to log it, but it's being propagated unconditionally at the end of the catch block).
• Since the type is just a .NET N type (not netstandard2.0) the serialization ctor and attr aren't needed
• We discussed UnreachableException vs UnreachableCodeException. We were split 50/50, so let the tie break go to the proposer
• We're approving it as deriving from SystemException on the assumption we'll give it special treatment. If it's "just another exception" at the end of the day we should derive directly from Exception.

#nullable enable
using System.Runtime.Serialization;

namespace System
{
    public sealed class UnreachableException : SystemException
    {
        public UnreachableException()
            : base("The program executed an instruction that was thought to be unreachable.")
        {
        }

        public UnreachableException(string? message)
            : base(message)
        {
        }

        public UnreachableException(string? message, Exception? innerException)
            : base(message, innerException)
        {
        }
    }
}

[stephentoub]

This exception made us muse for a long time on wishing that we had better language support for showing that control won't come back.

For reference:
dotnet/csharplang#538

[GrabYourPitchforks]

@bartonjs Shouldn't this be under System.Diagnostics as previously discussed?

[jkotas]

We might want to get the runtime to consider it uncatchable, like StackOverflowException

Stack overflow is hard fail fast. We do not allow any handling of it in managed code. (.NET Framework allowed handling stackoverflow exceptions using SQL hosting interfaces. We do not have those in .NET Core.)

ThreadAbortException: you can catch it to log it, but it's being propagated unconditionally at the end of the catch block

Thread abort was not a simple uncatchable exception. The thread abort switched the thread into a special aborting mode, it was about much more than just rethrowing exception at the end of catch block.

We do not have concept of uncatchable exceptions in the runtime and I do not think we should be introducing it for this one.

The only difference between uncatchable exceptions and Environment.FailFast is that uncatchable exception would allow custom logging. The process is going to be unconditionally terminated in both cases anyway. If we want to make this situation uncatchable, but still allow custom logging for it, I think we may want to rather look at adding logging hook to Environment.FailFast.

[svick]

Personally I think this should not have any kind of uncatchable behavior. Consider a plugin system, like Roslyn analyzers: an UnreachableException thrown from a plugin means that the plugin has a bug, but that doesn't necessarily mean the whole process (Visual Studio, or its child process) should crash. It should be up to the plugin runner to decide what happens.

Also, there's already a somewhat similar type, SwitchExpressionException, that's thrown by a switch expression with unexpected input. Would it make sense to make SwitchExpressionException inherit from UnreachableException? Though this is probably not particularly valuable, since both of these exceptions shouldn't be caught by name.

[GSPP]

https://imgur.com/gallery/C9qm57g

What would happen if a single request in a web app encounters such an exception? Would thread or process be torn down? That's unacceptable. The application must keep running and keep serving traffic.

Bugs happen all the time. Even state corrupting ones. We can't tear down the process when that happens.

Web apps also rarely experience true state corruption because, usually, requests are isolated and the request state is cleanly thrown away at the end of request processing.

[GSPP]

For the same reason, it is not possible to use Debug.Assert in production applications. It either ignores the assert in release builds or pops up a dialog or crashes the process. In a concurrent, multi-user, highly-available application, these things are non-starters.

This is why I suggested an AssertFailedException. The design concept is very much like this UnreachableException. (It is unfortunate that this proposal was closed and locked...)

It's simply an exception that anyone can use to their liking. There is no runtime integration. Nothing in the framework reacts specially to that.

But we can standardize the community on using these two exception types to expect common semantics. I consider that to be valuable.

[terrajobst]

@bartonjs Shouldn't this be under System.Diagnostics as previously discussed?

One suggestion was System.Diagnostics.CodeAnalysis, but maybe that's a bad fit becaust it contains the attributes for helping code analysis, this feels more like a plumbing type. Personally, I think I'm leaning towards System.

[deeprobin]

Why does this need a constructor for Inner Exceptions? Is there any use case to nest Exceptions in UnreachableExceptions?

[terrajobst]

Why does this need a constructor for Inner Exceptions? Is there any use case to nest Exceptions in UnreachableExceptions?

All exceptions have these constructors; it's the standard pattern.

[AraHaan]

Could this type be used for those like me who want to use it at runtime as well (to notify any application that something unexpected happened inside of one of it's dependencies)? Currently I have mine stuck between these:

• ns2.0 only dependency:
  • Throw Exception (or NotSupportedException if it's available)
• net5.0 only dependency:
  • Throw NotSupportedException
• net7.0 only dependency:
  • Throw UnreachableException (I think this is targeting .NET 7)

Although most of my libraries target ns2.0 I do use reflection to probe for the existence of newer features in the runtime and then if they are not found fallback to the ones defined in ns2.0. I could do a throw helper in my ns2.0 libraries with things like:

• using reflection to find UnreachableException, then if the methodinfo (it's ctor) is found (aka the result from reflection is not null) then call it to throw the exception.
• if the UnreachableException is not found using reflection it would then fallback to either NotSupportedException or Exception for throwing exceptions.

[GrabYourPitchforks]

@AraHaan There's nothing terribly unique about this exception type from a runtime perspective. You should be able to use reflection to instantiate it just as you would any other type, cast to the base Exception type, then throw to your heart's content. :)

[deeprobin]

Feel free to assign me 😄

[bartonjs]

The PR observed that there were still some floating points. Marking as review-ready and blocking so we discuss them today to unblock the PR.

[deeprobin]

The PR observed that there were still some floating points. Marking as review-ready and blocking so we discuss them today to unblock the PR.

Great 👍🏼!

Is this a small change (i.e. just implement the API + tests) or are there other changes that need to be made?

[bartonjs]

Video

We seem to have hit the most agreeable (perhaps "least disagreeable") position with System.Diagnostics, on the basis that it's rather like an assert (System.Diagnostics.Debug.Assert); and based on @jkotas feedback, the exception isn't really special... so : Exception instead of : SystemException.

namespace System.Diagnostics
{
    public sealed class UnreachableException : Exception
    {
        public UnreachableException()
            : base("The program executed an instruction that was thought to be unreachable.")
        {
        }

        public UnreachableException(string? message)
            : base(message)
        {
        }

        public UnreachableException(string? message, Exception? innerException)
            : base(message, innerException)
        {
        }
    }
}

[deeprobin]

Since this is only a small change, I'm happy to work on that issue.

[stephentoub]

Since this is only a small change, I'm happy to work on that issue.

There's already an in-flight PR for it.

[jstaro]

I know this is a bit late, but I was surprised the discussion didn't float the idea that this exception is quite similar (at least in my mind) in concept to NotImplementedException which lives in the System namespace.