runtime: API Proposal: UnreachableException

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

About this issue

  • Original URL
  • State: closed
  • Created 4 years ago
  • Reactions: 28
  • Comments: 29 (25 by maintainers)

Commits related to this issue

Most upvoted comments

@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.

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.

How would this interact with the proposed “never” return type (https://github.com/dotnet/csharplang/issues/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.

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)
        {
        }
    }
}

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.

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.

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: https://github.com/dotnet/csharplang/issues/538

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.

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

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

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.

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.

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