That Shouldn't Happen – UnreachableException in .NET 7

https://ab.bot/blog/that-shouldnt-happen-unreachableexception-in-net-7

by icey • 4 years ago

84 58 4 years ago

insaider• 4 years ago

Just wanted to highlight a relevant tool i recently discovered: Sentry.

It's already become invaluable in my .net apps for tracking down tricky errors, especially in the new MAUI apps.

Caught one just yesterday where the app was trying to call an api with decimals serialized with a comma because it was on a dutch user's phone! I imagine it would've been quite the headache catching that without sentry. Integrates with most tech-stacks too, not just .net btw

philliphaydon• 4 years ago

Had to google Sentry, I've been using Raygun for ~10 years. These sort of tools are awesome. The weird errors that users seem to be able to produce but getting a full stack trace and being able to resolve them.

My previous job we used to send exceptions to email until we blew out the gmail account for the day (exceeded the receive amount) and boss switched over to raygun (~10 years ago), because of the global error handling we ended up fixing tons of errors we didn't even know our customers were experiencing. Product became more stable over time with the fixes.

rbanffy2• 4 years ago

In one company, the app error page would also be a "Submit Ticket" page where the poor human would be able to quickly explain what they were trying to do while I could gather all sorts of user-side information.

magicalhippo2• 4 years ago

with decimals serialized with a comma because it was on a dutch user's phone

If you're doing any kind of non-binary serialization/deserialization or file IO, it's always a good exercise to change the user and system regional settings when running tests.

Semaphor• 4 years ago

Server runs de-DE, my dev machine is en-US. So many errors found with formatting/parsing because of it :D

solarkraft2• 4 years ago

As far as I'm aware these kinds of tools usually come with proprietary cloud services attached to them. Are there good local/FOSS versions?

viraptor• 4 years ago

Sentry is open https://github.com/getsentry/sentry

keithnz• 4 years ago

seems to me they are using unreachable to mean unsupported. Also they should put a message in for the first example which should highlight the int value as that is the very first thing you are going to want to know.

Mindless2112• 4 years ago

The state machine example is a good one. It's a textbook case of unreachable code.

The null-checking example is not good. If the external API returns a null where you don't expect one, UnreachableException will be thrown. It doesn't matter if the documentation says it will never be null; clearly the code isn't unreachable. All you need to reach it is bad data! (InvalidDataException would be a better choice, though arguably not an ideal one.)

If you can write a unit test that makes your code throw UnreachableException, you almost certainly should be throwing a different exception.

WirelessGigabit2• 4 years ago

Invalid date means you got a value that’s not a valid date which is different than falling into an impossible state.

AlfeG• 4 years ago

Invalid Data, not date

ad133• 4 years ago

This was my initial thought as well, but from the text I gather there is a flow like this:

[Input Data, maybe null] -> Validate field is not null -> Call this method with the assertion.

This is a small bug-bear for me with nullable types and I wish there was a better way to do it, but many languages allow you to smart-cast away nulls, but only within the local scope. If you want to pass a struct-type around which has nullable fields, but you have already checked for non-null (like this one) you need to convert to a different struct-type, which doesn't have the nullability on its fields. I can't think of a good way round this - as you say with the unit test remark, there is nothing to stop another piece of code calling this method with nulls.

piaste3• 4 years ago

If you want to pass a struct-type around which has nullable fields, but you have already checked for non-null (like this one) you need to convert to a different struct-type, which doesn't have the nullability on its fields.

Which is exactly what IMO the author should have done. It's actually a reasonable use-case for inheritance:

    #nullable enable

    record SlackEvent
        ( int EventId
        , string Content
        , string? TeamId
        );

    record TeamSlackEvent
        ( int EventId
        , string Content
        , string TeamId
        ) 
        : SlackEvent
            ( EventId : EventId
            , Content : Content
            , TeamId : TeamId
            );

gwbas1c• 4 years ago

If you can write a unit test that makes your code throw UnreachableException, you almost certainly should be throwing a different exception.

ArgumentNullException, InvalidOperationException... There's plenty that have been around since the beginning.

What's more important, that I didn't see in the first two examples, are useful error messages.

aloisdg• 4 years ago

`ArgumentNullException` was my first idea too. I would and did use it for this kind of purpose.

WirelessGigabit2• 4 years ago

If TeamId is a non-nullable string then the construction of that object should fail when TeamId is null, not at the consumption site.

This is exactly the issue what Rust is solving. A struct construction completes or fails. There is no weird in between state.

MobiusHorizons• 4 years ago

That’s all well and good when you write both the consumer and producer. But how do you detect consumers which depend on some data integrity aspect you need to change for whatever reason?

masklinn• 4 years ago

The types or constructors ought change to match?

If TeamId goes from non-nullable to nullable, it goes from TeamId to Option<TeamId>, and consumers get a type error.

piaste3• 4 years ago

They could easily have declared TeamId as non-nullable in modern C# too.

The issue they had is:

(a) TeamId is legitimately nullable at creation, there are valid SlackEvents without a TeamId

(b) They wanted to assert "TeamId won't be null" from a certain point in the code path

(c) They didn't want to create a different data structure (TeamSlackEvent or something) to hold the definitely-has-a-TeamId events.

I strongly suspect (c) was a mistake and they were just too lazy to define a new (sub)-class or record.

DeathArrow• 4 years ago

It's not like you can't define your own exceptions in C#, so you always could do what the article is saying.

Even if exceptions might be handy, I wouldn't use exceptions for error handling for permance reasons. Instead I have another recommendation: never have void methods and use a Result type that packages the actual value along with an IsValid and Error property. So you will always return Result<T>.

That way, error handling is easy and doesn't cause much performance overhead.

jve2• 4 years ago

I don't think you'd be hitting UnreachableException that much for that to be of performance concern. You actually halt the execution.

And what you are proposing, it's just a different way to handle errors. Maybe appropriate in critical path. However you still have exceptions from .NET framework. OutOfMemoryException, various IOExceptions etc.

branko_d• 4 years ago

doesn't cause much performance overhead

Exceptions are slow when thrown.

If an error happens rarely, the corresponding exception is thrown rarely, so the performance impact is minimal.

OTOH, if an error can happen frequently, it is no longer "exceptional", and so is probably better handled without exceptions anyway.

VoidWhisperer2• 4 years ago

Off-topic slightly: does anything similar to what they are doing with that require extension method exist in Kotlin?

oaiey• 4 years ago

The expression part or the extension method part? (Obviously: No clue about Kotlin, just interested)

VoidWhisperer2• 4 years ago

The extension part mainly, like the ability to add an extension method that can apply to 'everything' to an extent like the Require() here does

nomercy400• 4 years ago

Is this like java.lang.UnsupportedOperationException?

orthoxerox2• 4 years ago

No, that's System.NotSupportedException in .NET.

jug• 4 years ago

Which in turn is not for unreachable or even unexpected cases but expected cases you’re fully aware of, maybe even documented, but that you don’t support.

_old_dude_• 4 years ago

In Java, usually an Error like AssertionError is used for those cases.

By example, for a switch on an enum, the compiler inserts a "throw new ...Error()" automatically when the default case is not specified.

fabian2k• 4 years ago

For null checking I'd use the relatively new ArgumentNullException.ThrowIfNull, that automatically adds the parameter name in the exception and is a very compact and obvious way to check for null in your method parameters.

For enums I use ArgumentExceptions, something called this code with an invalid enum argument. I think there are some cases where Unreachable is a better error, but I'd probably be more inclined to use that for logic errors, where the code itself should not be reachable according to your understanding and reaching it would mean the code is wrong. The enums and null are cases where the arguments are unexpected, which to me is different.

kagaw• 4 years ago

Is it more helpful if you will create a custom exception instead? E.g. class UnsupportedStateException : Exception

mattmanser• 4 years ago

Generally, no. Only you will use it in your team.

piaste3• 4 years ago

UnreachableException has no special behaviour at all [0]. It's just a standardized 'tag' around regular Exception.

The only advantage of using a standard tag is that it will be recognized by other programmers, and maybe some supporting tooling might choose to give it special treatment.

E.g. a code analyzer could warn you if you raise a NotImplementedException (forgot to finish a feature?), but accept an UnreachableException as valid.

If you want to add a custom name (for easier grepping maybe?) or stuff some extra data in the exception, you can still inherit from UnreachableException.

[0] https://github.com/dotnet/runtime/pull/63922/files#diff-588c...

Someone• 4 years ago

I don’t think that’s a good name for this. Clearly, that code can be reached. From the name, I would expect ”throw new UnreachableException” to be something the compiler inserts to make it more robust in the sense that, if the compiler has some bug in its control flow analysis, it throws, rather than executes the wrong code.

Now, for a better name? Maybe ShouldNeverGetHereException, but that may be seen as too whimsical.

zamalek• 4 years ago

It's anagolous to `unreachable` in Rust. I think the author is defining it too broadly. It's for scenarios where it would be possible to statically guarantee that something is unreachable, but is not implemented in control flow analysis (e.g. it could be too computationally expensive, or it could be a structure invariant). It's useful for getting the compiler to shut up about a branch that you know can't be reached, e.g. you might not return a value from the `default` case in an exhaustive switch.

It doesn't seem like .Net is implementing it correctly either. It is supposed to be optimized away in release code, resulting in UB if the developer was wrong about the condition.

cesarb• 4 years ago

It's anagolous to `unreachable` in Rust. [...] It doesn't seem like .Net is implementing it correctly either. It is supposed to be optimized away in release code, resulting in UB if the developer was wrong about the condition.

Rust actually has two things called "unreachable". There's the unreachable!() macro (https://doc.rust-lang.org/core/macro.unreachable.html), which is a short-hand for panic!(), and therefore is never UB even if somehow it's reached; and there's the unsafe unreachable_unchecked() compiler hint (https://doc.rust-lang.org/core/hint/fn.unreachable_unchecked...), which is optimized away in release code, and is always UB if somehow it's reached. Most of the time, you should prefer the safer unreachable()! macro; the unsafe hint is for when it makes a performance difference (and as with every use of unsafe in Rust, you really should carefully review the code to make sure it's really unreachable).

afiori• 4 years ago

Maybe UnexpectedCodePathException

rbanffy2• 4 years ago

There is code I wrote that has `raise ImpossibleError("X should never happen")` or, simply, a `NotImplementedError("You shouldn't be here")`

magicalhippo2• 4 years ago

Maybe ShouldNeverGetHereException, but that may be seen as too whimsical.

In Delphi we have the EProgrammerNotFound exception[1]. I use it for code paths which should never be executed.

[1] https://docwiki.embarcadero.com/Libraries/Alexandria/en/Syst...

whoisthemachine• 4 years ago

I agree, `UnreachableException` is really poorly used here, and most of these are argument exceptions, and clearly are reachable. However, for other "unexpected behaviors", you should really throw an `InvalidOperationException` - you did something that is invalid. And even better if you sub-class Invalid with a custom exception that more clearly describes the unexpected behavior (also easier to search for that exception type in your log analysis tool).

[0] https://learn.microsoft.com/en-us/dotnet/api/system.invalido...

bob1029• 4 years ago

I think I still prefer NotImplementedException. It also covers things that are logically reachable but not ready for use yet. The nuance is the string argument to the ctor.

kevingadd• 4 years ago

It's useful to have a separate type since things like exception messages can get optimized out for size in some cases.

hardware2win2• 4 years ago

Different intention.

Deukhoofd2• 4 years ago

I do let my IDE register NotImplementedExceptions as TODOs, which is not really desired for things that should not be reachable.

dustedcodes• 4 years ago

Fortunately, .NET 7 adds a new type to help us out here: UnreachableException.

There was an UnreachableException before .NET 7 as well.

Look:

public class UnreachableException : Exception {}

codeflo2• 4 years ago

Underrated comment. I’m all for precisely modeling domain and I/O errors. But for “this is a bug” type of exceptions, what good do all those tiny variations (and the resulting philosophical debates about when to use which one) do, if all that changes in practice is the text of log message? You’ll never catch that one specifically or treat it any differently than an ArgumentNullException — or any other of the zillion already existing ones.

jve2• 4 years ago

When opening article, I was thinking: hey, maybe they introduced some compile-time checks for this special exception type? Like switching non-exhaustive list of enums and default:ing with UnreachableException... Errm, maybe not.

fkarg• 4 years ago

it helps significantly for debugging

hardware2win2• 4 years ago

C# needs closed enums

Deukhoofd2• 4 years ago

https://github.com/dotnet/csharplang/issues/3179

thom• 4 years ago

Ignoring the subject matter entirely, it's always fun to see some snippets of C# du jour and marvel at the number of new syntactical features added since my last encounter.

pif• 4 years ago

Yeah, it started like C++ for wannabee programmers, but during the last years it has being becoming a more and more useful tool for professional developers.

That Shouldn't Happen – UnreachableException in .NET 7

Related Stories