Nim: Exception should not be Defect

[yglukhov]

Summary:

• Rename Exception to RootError or something even more ugly to prevent unintentional use.
• Rename CatchableError to Exception, keep CatchableError as an alias to Exception for backward compatibility. Optionally deprecate CatchableError.
• Change except: semantics to except Exception: so that Defects are not caught by default.

Reasoning:

• Defects are usually not intended to be caught. Catching those should be explicit programmer's decision.
• Exception is a commonly known name, mostly used in "catch all" scenarios. Still Defects are not intended to be caught in such scenarios.
• CatchableError is an ambiguous name (because Defects are also catchable), which also takes some time to get used to, unlike Exception.

What will break:

• Code that used catch: and catch Exception: branches to handle (or rather ignore) everything including Defects. Arguably such code is already broken by doing so, because it's execution depends on compilation flags.

How to migrate:

• To all except: and except Exception: branches add except Defect: branch if that was the real intention. Which is unlikely.

Known cases of breaking valid code:

• Unittesting libraries should likely catch all possible errors.

[zah]

I've noticed that people often inherit from Exception because educating everybody about the distinction between Errors and Defects is practically a lost cause.

For this reason, I think the root exception type should indeed have a very unusual name such as StackUnwindingEvent. This is both an accurate description and a name that nobody in their right mind would inherit from.

[zah]

For the root "catchable error" type, I would vote for naming it just Error. Here are some hand-wavy arguments for this:

• It's short and simple - we distinguish between errors and defects.
• There is no Exception type, so people coming from other languages will stop for a moment wondering why, they'll read our documentation and they'll hopefully learn about the distinction between errors and defects. Otherwise, we may see people inheriting from Exception to signal just about everything.

[arnetheduck]

Defects are usually not intended to be caught

if a defect is used to signal "undefined behaviour", it means that it is by definition unsound to catch it, ever.

If they're still catchable, or catchable sometimes, there's a significant cost associated - both in terms of API clarity of what gets designed (we add options and nuances of undefined behaviour - how do you choose between them with "usually" as guidance?), of exception unwinding analysis ("this cannot raise therefore..") meaning that programs are more polluted with stack unwinding and other bloat than they need be.

[Araq]

Defects shouldn't be raise'd, they should quit(1) or similar. Tests should use process isolation like testament and other tools do it so that you can test for the quit. And then except: automatically works as it should.

No new language feature or further exception hierarchy changes or renames are required IMO.

[awr1]

Defects seem to me like they should be relatively analogous with program-stopping asserts in C (and it already seems like they are, considering AssertionError is descendent from Defect).

[zah]

Defects should fail fast, but stack unwinding is a slightly better default way to handle this.

Not all program resources are controlled by the OS supervisor. If you quit immediately, you program may leave behind temporary files, started distributed transactions, distributed locked files and so on. If quitting immediately was always reasonable, the term "graceful shutdown" woudn't exist.

Contrary to popular belief, even Rust handles panics with stack unwinding. Besides being a safer default choice, allowing the Defects to be treated as exceptions has other useful applications as well.

Let's say you are working with a third party package for handling some compression algorithm. Your software occasionally uses this compression algorithm and if any error is encountered along the way, there is an easy way to display an error message to the user. If there is a defect in this third party package, you may find it reasonable to treat the defect as a recoverable error. This is the case, because your software doesn't handle critical data and the state of the third party component is quite isolated (the chances of memory corruption are low). If the language has a too rigid policy about error handling, you are limiting the types of software the can be created. In my error handling proposal for Status, I've also listed some other situations and techniques where it might be reasonable to handle a defect.

With that said, it's sometimes useful to indeed quit immediately on a defect. A configurable thread-local hook can give you the power to do this. We should try to make things as simple as possible, but not simplistic.

And also, the fact is that right now Nim is broken, because an unqualified except: ... clause will happily catch all defects. This proposal offers a solution for this.

[bluenote10]

Defects shouldn't be raise'd, they should quit(1) or similar.

That makes Nim quite unsuited for SLA critical tasks. In this domain examples like the one @zah gave are ubiquitous. Graceful shutdown including fine granularity of recoverability is a key feature. If any function call can basically quit at any time, it's basically necessary to spawn processes all the time to be safe.

[Araq]

The inspiration for this rule doesn't come from Rust, it comes from the clang sanitizers. There are plenty of things that can be detected at runtime but usually aren't, like data races or use-after-free bugs. In the current paradigm these should then be mapped to Defect or a subclass thereof and can be handled "gracefully", but I think that's silly and it also means detecting these conditions at runtime becomes even more expensive. And worse, your program could start to depend on these checks, "what do you mean, I cannot catch IndexError? It works!"

[Araq]

Let's say you are working with a third party package for handling some compression algorithm. [...] I've also listed some other situations and techniques where it might be reasonable to handle a defect.

Yes, I am aware of these arguments and I don't even disagree with them, but in reality you already have a system that runs on physical hardware, it can fail and you're better off with an addQuitProc in order to try to shutdown gracefully.

I know, I know, Nim is broken, it should somehow map try to addQuitProc.But at some point you have to admit that ever more sophistication in a language's runtime has the opposite effect: It doesn't become more stable nor more reliable, but less so.

[zah]

You hardly addressed most of my points. Graceful shutdown through stack unwinding happens because you have stack objects with destructors. Not with addQuitProc that can't even capture state (its param is not a closure).

Sure, physical hardware can fail and that's why temp files can be cleaned from time to time and distributed resources have timeouts. But there will be systems hurt from the delays coming from such timeouts and in the end of the day it will be us to blame for this.

The proposal here is quite simple and reasonable. I don't see how you can describe it as "too much sophistication". You are asking us to ignore the real problem that we are having right now and to wait for this future where Defects will be treated in a different (arguably worse) way.

[arnetheduck]

Unwinding to handle defects in third parties seems pretty fragile as an approach - should probably be made hard. Unwinding to clean up seems more reasonable, but one thing to remember is that once you gain destructors, the vast majority of unwinding will be memory-related/local, freeing seq's and whatnot which is completely unimportant - wonder if it's possible to differentiate the two somehow, so that only stuff that has global side effects is unwound.

Re addQuitProc, that's one of those things that ideally would be cut from std lib and added to some libc library - atexit is full of traps and gotchas (no recursive calls, limited to 32 callbacks potentially, doesn't always run, threading, etc etc)

[arnetheduck]

Another way to view this is that unwinding in preparation of quitting and unwinding in preparation of trying again / progressing are not necessarily the same beasts.

[Araq]

Ok, look, this proposal is fine and doesn't cause much harm in itself and I agree it improves things. However, I sense that after this proposal there will be another one and already the spec and the implementation diverge...

[arnetheduck]

how would the following work for defects: unwinding code runs, but at the end of catch it continues unwinding in the next frame instead of exiting the catch, as if the catch ended with a naked reraise

[Araq]

Re addQuitProc, that's one of those things that ideally would be cut from std lib and added to some libc library - atexit is full of traps and gotchas (no recursive calls, limited to 32 callbacks potentially, doesn't always run, threading, etc etc)

True, but we can easily write our own implementation of the same idea.

[Araq]

how would the following work for defects: unwinding code runs, but at the end of catch it continues unwinding in the next frame instead of exiting the catch, as if the catch ended with a naked reraise

And here it is, the proposal to make the runtime more sophisticated. ;-)

[Araq]

the vast majority of unwinding will be memory-related/local, freeing seq's and whatnot which is completely unimportant - wonder if it's possible to differentiate the two somehow, so that only stuff that has global side effects is unwound.

Yup, non-memory destructors could be called =close and then you always know whether the destructor is "critical" or not.

[zah]

How much slower do we expect crashing to be when you also do the unnecessary work of freeing memory? This is indeed closer to the "too much sophistication" category, but I'm happy that at least some consensus is forming around the core points discussed here:

1) The ability to run destructors on Defects is valuable.

2) The current state of affairs where except: ... catches Defects and people inherit from Exception is wrong.

Think about what you would need to do in order to solve 2). Whatever scheme you choose, some changes will be necessary and the sooner we do them, the better IMO.

[Araq]

I tried to implement this change and it would break quite some code:

proc checkException(ex: ref Exception) =
  doAssert(ex.name == cstring"ValueError")
  doAssert(ex.msg == "SecondException")
  doAssert(ex.parent != nil)
  doAssert(ex.parent.name == cstring"KeyError")
  doAssert(ex.parent.msg == "FirstException")
  echo "Check passed"

var e: ref Exception
try:
  try:
    raise newException(KeyError, "FirstException")
  except:
    raise newException(ValueError, "SecondException", getCurrentException())
except:
  e = getCurrentException()

getCurrentException() still needs to return RootError and so Nim programmers need to be aware of its existence. I don't see enough benefits for this breaking change.

See https://github.com/nim-lang/Nim/pull/12069 for what it takes...

[arnetheduck]

getCurrentException I find slightly problematic - it's essentially a context-sensitive thread-local global not too different from errno that makes it harder to implement exception handling correctly, and doesn't account for nested exceptions, caught type etc - the exception is already available from except Xxx ex giving us two ways to do the same thing, one of which is less expressive - how about deprecating it?

[Araq]

Sure but what about:

type
  FutureBase* = ref object of RootObj ## Untyped future.
    callbacks: CallbackList

    finished: bool
    error*: ref Exception ## Is that the new RootError then? Or CatchableError?

And who knows where else Exception was actually properly used. It feels like we're punishing people who read the docs and used Exception vs CatchableError appropriately and we're rewarding the people who don't read the docs.

"Look, you inherited from Exception and it was wrong and all of a sudden it's correct now..."

"Look, you used Exception and it was correct and now it's wrong..."

[Araq]

I think the compiler should give a warning about FooError = object of Exception outside of system.nim. The warning should read like: "Inherit from ValueError, IOError or OSError", which is what everybody should do, IMO.

[arnetheduck]

Well, this is in part why it's a little unrealistic to use the same mechanism (raise/try/except) for both expected and unexpected errors, leads to these tricky situations..

assuming Defects are not meant to be handled "usually", the right thing for Future is to not work with Defect because the claim is that all it should do is unwind from the raise point - but then someone comes along with a use case where some library author was unnecessarily obnoxious with the Defects, or some thread/async use case and in it goes - this whole idea with defects asymptotically tends towards making Defect and Error the same..

[Araq]

Let me phrase it differently because I think I have been misunderstood: In the runtime we have sysFatal. Every sysFatal raises some exception that inherits from Defect. I want to be able to map sysFatal to quit / terminate / trap / etc and the spec says that it is allowed. It is very bad style to inherit from Defect and use a raise Defect(...) in your library with the intention of being catchable, but the compiler would not suddenly touch these raise statements and transform them to quit.

[arnetheduck]

And you think users will not consider it a breaking change when suddenly you switch sysFatal to a completely different, albeit in the manual theoretically allowed, semantic? this is exactly the same argument as -d:release vs -d:debug vs -d:danger - global flags make for a poor experience when you have libraries.

if you make the default sysFatal to abort, at least the majority of code out there will be written to assume it's not catchable, but you lose unwinding which sort of was the motivation for this exercise to begin with. also, it seems that defect can be raised without calling sysfatal, what about those?

[Araq]

Well yes, there will be compiler switches for transition. And yeah I will consider it a breaking change, albeit one that only punishes bad code like try: a[i] except IndexError:.

if you make the default sysFatal to abort, at least the majority of code out there will be written to assume it's not catchable, but you lose unwinding which sort of was the motivation for this exercise to begin with.

Excuse me but the original motivation seemed to be something like "people inherit from Exception directly and it's bad style, here is how to change the language so that the code becomes correct".

also, it seems that defect can be raised without calling sysfatal, what about those?

As I said, these are left untouched.

[disruptek]

If the language cannot change to meet the ideal because it breaks too much code, perhaps nim is already at v1.0 and you just don't know it yet.

[yglukhov]

getCurrentException() still needs to return RootError and so Nim programmers need to be aware of its existence. I don't see enough benefits for this breaking change.

If we're talking backward compat, getCurrentException() can return ref Exception with return value being nil when the error is not Exception. Such behavior should satisfy all the existing code that relied on getCurrentException historically back when as syntax was not in place. As @arnetheduck suggest we might also deprecate getCurrentException (I guess we all agree that getCurrentException is a historical function, and now there are better ways in most cases) and introduce a proc getCurrentError(): pointer which by its signature suggests fragility/dependency over runtime internals and thus requires more attention and should generally be avoided.

[Araq]

The point remains though: It's yet-another-change with no clear benefits. Instead the compiler should produce warnings if your exception types inherit from Exception directly. It always was very bad style, the stdlib mostly inherits from ValueError or IOError or OSError.

[yglukhov]

I don't think warnings should be used for such cases. The priority should be to prevent unintentional bad style, by making it not obvious. And to make the most obvious ways the good style. It was not obvious to me that I should inherit from ValueError or IOError or OSError. CatchableError is even worse. Besides warnings sometimes get unnoticed, so I suggest we use warnings only when other natural ways of prevention can't be used. The API should be designed in a way which is impossible/hard to misuse.

[sinkingsugar]

Bump.

This is a necessary change, specially because any async/await, co-routines or possibly multi-threaded code will swallow Defect.

See this discussion as well when applied to nim-chronos. Obviously it can be fixed library side but the fact the language is not strict about this issue is a big concern and subject of pointless discussion.
https://github.com/status-im/nim-chronos/issues/81
https://github.com/status-im/nim-chronos/issues/81#issuecomment-593759745

[Araq]

Bumping this is fine, but it remains unclear what to do. I actually implemented the suggested changes and it breaks code in the worst possible way: Users who read the documentation need to patch code, users who "simply used Exception" (incorrectly!) have to do nothing.

[yglukhov]

Users who read the documentation need to patch code

But that's just a matter of deprecating CatchableError, no? We can keep it "undeprecated" for now :)

[sinkingsugar]

I see your concern @Araq
maybe how about adding (yet another) compiler flag?
as in like --defects:abort and --defects:except and leave except as default.
But maybe you already had this idea anyway I guess.

[Araq]

--exceptions:goto actually does implement "on defect quit``. Of course there are the voices who want to continue after bugs because "omg it's a server and crashing it means all connections are lost, not just the one which triggered the bug".

[disruptek]

If you think it's hard to change the language now, do you think it's going to be any easier next year? Three years from now?

[Araq]

Silly question, we're trying to find a good solution first and then figuring out a transition period. And a good solution remains to be "The compiler warns if you directly inherit from Exception outside of system.nim". Why not teach people about what we have instead of replacing it by something different that in the best case breaks code and in the wost case has new unexpected gotchas?

[disruptek]

The point is, breakage is comparatively minimal now. So much so that it bears no consideration, IMO. So the best solution for the future is the one I'd support today.

[sinkingsugar]

The compiler warns if you directly inherit from Exception outside of system.nim

Afraid that this won't solve issues with async/coro/threads and swallowing tho. Cos those happen even with a simple assert

--exceptions:goto, I've never tried!

"omg it's a server and crashing it means all connections are lost, not just the one which triggered the bug"

I'd like to know who makes those packages so I can stay away as far as possible 😄
Ignoring Defect or trying to "handle" them is asking for troubles.

[disruptek]

"omg it's a server and crashing it means all connections are lost, not just the one which triggered the bug"

No one wants to continue after bugs. But since you brought it up, yeah, it sucks big time that our http server crashes whenever an async client raises. And, worse, that there is no interest in enabling a solution short of reimplementing the server loop from scratch.

This sort of stance makes it very hard to take these discussions seriously, because the stdlib is insufficient for any serious application. So why worry about breaking it? Obviously, no serious uses exist.

[Araq]

No one wants to continue after bugs.

People really want to do that, maybe you don't want to do it, others certainly do. How do I know? They told me.

But since you brought it up, yeah, it sucks big time that our http server crashes whenever an async client raises. And, worse, that there is no interest in enabling a solution short of reimplementing the server loop from scratch.

Strawman, nobody minds improvements to asynchttpserver.

This sort of stance makes it very hard to take these discussions seriously, because the stdlib is insufficient for any serious application. So why worry about breaking it? Obviously, no serious uses exist.

The stdlib is better than you think but even assuming it's worse than you think, this is a concrete proposal about restructuring the exception hierarchy. Which we did in the past several times already.

[Araq]

1. I tried to implement precisely that and I wasn't happy with the outcome.
2. The problem is that afterwards Foo = object of Exception is still wrong, in practice you are supposed to inherit from:

• ValueError
• OSError
• IOError

And even if not, you need to invest some thinking into what to inherit from.

1. "Nobody uses X, so let's break X so that it continues to be the case that X cannot be used in production" is what we did for over a decade with Nim. More often than not, we only shifted problems around.

[arnetheduck]

And, worse, that there is no interest in enabling a solution short of reimplementing the server loop from scratch.

this is nim-chronos, and why it's often easier to do radical changes outside of the std lib.

fwiw, we've started tackling this in a structured way there, and a small step is to start annotating functions with what they raise ({.raises.}), limitiinog exceptions from propagating too far between abstraction boundaries, and limiting the things that can raise at all to avoid the problem completely.

One thing that stands out is https://github.com/nim-lang/Nim/issues/12862 - it prevents us for example from having compiler guarantees about exceptions - a classic example is the callback: you don't want asynchronous callbacks to raise anything except defects possibly because then the exception messes up the event loop. A small step towards solving the things in here would be clearing up that issue.

in terms of isolating our code from these issues, it is indeed often easier to turn to other solutions than to make sense of the logic here - ie the lack of clarity, the inconsistencies in the std lib and the fundamental issues described make it difficult to use the exceptions at all, causing confusion and having multiple standards for doing the same thing, both of with hurt productivity.

[disruptek]

Strawman, nobody minds improvements to asynchttpserver.

12325 suggests otherwise.

we've started tackling this in a structured way

I gave up on Nim exceptions a long time ago, mostly due to the async behavior but also because without quirky, they don't tend to buy me anything for the grief they bring to my users. .raises. is either broken or wrong, depending on who you ask, and completely dysfunctional in async regardless of speaker. So my exceptions have a real cost to users of my library. Yeah, no thanks.

But now we're championing exceptions:goto and exceptions as a usable tool are back in Araq's favor. That means that we are encouraged to use libraries that may make use of exceptions, and their behavior (or misbehavior) may no longer be chalked up to poor programming. "Use exceptions! We'll support them, now."

Except #11081, which no longer even compiles. I guess that's an improvement. Unless you want to use C++ libraries, I mean.

To me, sysFatal nails it. Nim should take the stand that Defects will only be handled one way, and in doing so, correct at least one flaw in the abstraction.

Will .raises. be fixed to bubble possible Defect or sysFatal taint? I honestly don't even care anymore. Please, just let the bugs break. It's only the first step in making it possible to use other people's code.

Kudos to Status for solving the problem, but developing one's own stdlib is not a solution available to all Nim programmers.

[Araq]

12325 suggests otherwise.

Bummer, I stand corrected.

[arnetheduck]

But now we're championing exceptions:goto and exceptions as a usable tool are back in Araq's favor.

Our issues with exceptions are not of the implementation/performance kind, but rather with the logical soundness of how things fit together: there's confusion what a Defect is, why things should be Defects, why Defects are not consistent with exception tracking, why Exception (still) exists, what to inherit from, whether to have a hierarchy of exceptions, whether to translate exceptions with nesting (as is popular in java) on API boundaries etc etc.

In addition, it's very hard to write exception safe code in Nim - almost all code in our project has issues with this, as does the standard library: resources are leaked or not closed on exception. This is tied to there being few features in nim supporting the safe use of exception such as constructors and destructors - simple destructors not enough since they're not deterministic with ref, copy constructors or the deletion thereof to avoid multiple destructions of the same resource, move support to "disarm" the destructor, constructor support for guaranteed initialization of the object etc). These issues are exacerbated when ref is involved, which is often (exceptions themselves are ref!). In our project, we've had very many time-consuming bugs to hunt down because of this - smaller projects will not notice it because they are usually not long-running processes - a leak or crash won't matter as much. If it were an isolated case you could try to make the argument that it's the developers that are sloppy, but these issues are spread all across the code-base, nim std lib included.

We opened this ticket after long discussions of a logical structure of exceptions that would help clear up at least some of these inconsistencies and confusions.