Repository navigation
API Proposal: UnreachableException #35324
Description
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
Reacted by Aaron R Robinson, Egor Bogatov, Fraser Ashyggton, Miha Zupan, Huo Yaoyuan, GSPP, ProphetLamb, Tau Gärtli, Amr, Petr Onderka and 15 more
Reacted by theolivenbaum
Reacted by Markus Schlotbohm and Ashley Stanton-Nurse
Activity
carlreinke commentedon Aug 18, 2020
@bartonjs @joperezr Any concerns or can this be ready-for-review?
GrabYourPitchforks commentedon Aug 19, 2020
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 commentedon Aug 19, 2020
[DoesNotReturn]and "never" do not apply in a lot of cases whereUnreachableExceptionwould 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 couldFailFast, 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 commentedon Nov 17, 2020
This seems very useful to me. Sometimes, a code location is to be considered unreachable. Having this exception type confers the following benefits:
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 commentedon Feb 18, 2021
@GrabYourPitchforks Any other concerns or can this be put into the review queue?
GrabYourPitchforks commentedon Apr 21, 2021
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 commentedon Apr 21, 2021
I like this and it'd be also useful for tools like IL linker which have to now uses
NotSupportedExceptionwith a string explanation that the body is unreachable.bartonjs commentedon Apr 21, 2021
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.
24 remaining items
bartonjs commentedon Jan 18, 2022
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 commentedon Jan 18, 2022
Great 👍🏼!
Is this a small change (i.e. just implement the API + tests) or are there other changes that need to be made?
bartonjs commentedon Jan 18, 2022
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: Exceptioninstead of: SystemException.deeprobin commentedon Jan 18, 2022
Since this is only a small change, I'm happy to work on that issue.
stephentoub commentedon Jan 18, 2022
There's already an in-flight PR for it.
jstaro commentedon Jan 18, 2022
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
NotImplementedExceptionwhich lives in theSystemnamespace.