On hook routines raising exceptions

[zerbina]

Recently, the language specification was changed to disallow exceptions being raised from hook procedures (=copy, =sink, =destroy, =trace, =deepCopy).

Before the change, what happened when a hook routine raised an exception was undefined behaviour. In many cases, actually raising from a hook procedure could result in crashes or other unexpected behaviour (e.g., #1012).

The aforementioned change made the behaviour well specified, but the significant change in behaviour was brought up to be, perhaps, too restrictive.

What are hook routines?

Hook routines are much like normal procedures/functions, with the difference that the compiler can place invocations thereof itself. They're used to "hook"/override what some compiler-decided action does at run-time.

A procedure becomes a hook routine if it uses a special name and signature.

What actions can be hooked?

• copying a value into a location (=copy). Invoked on assignment, or when copying into a sink parameter
• sinking a value into a location (=sink). Invoked on assignment, or when moving into a sink parameter
• destroying/cleaning up a location (=destroy). Invoked when a location (which includes compiler-inserted temporaries), is destroyed, usually because it goes out of scope
• tracing a location (=trace). Potentially invoked as part of cycle collection
• deep-copying a value (=deepCopy). Invoked when calling deepCopy

If for an action no hook is provided by user code, the compiler usually synthesizes one.

What needs to be discussed?

The questions that need to be answered, for each hook, whether them raising exceptions should be allowed, and, if so under, what circumstances.

Put in a more concrete way:

• should hook routine x be allowed to raise? What happens if it does raise at run-time?
• what are the use-cases for hook routine x raising? Can it be solved differently?

For answering these questions, it could help to look at what exception-related rules exist in other languages with similar facilities (destructors in C++, Rust, etc.). How do libraries handle the restrictions (if any) there?

Due to both applying to assignments, =copy and =sink can be treated as one hook, in the context of the discussion.

[mrgaturus]

on Rust they implemented copy and destroy hooks as Clone<T>, Drop<T> traits respectively, however such traits don't return Result<T> or Option<T> so there is not a way to handle exceptions.
a concern would be for example how they can handle file closing?, looking at std::fs::File they ignore file closing errors on Drop trait implementation and provides a way to ensure file is done returning a Result<T> before closing and ignore errors safetly on Drop.

for this case, on Nimskull we can keep close() which raises an IOError and on =destroy hook we just call close() if is not closed manually then ignore errors.

i'm more in favor forbid raising on hooks because allowing it can led more complexity on control flow and implicit raising cases

[alaviss]

FWIW you can raise in Rust's Drop via panics, which is Rust exception mechanism. Once that happens that particular Drop is considered finished and Rust will continue cleaning up the rest until the program either terminates or some code catched the panic.

Trouble is when you panic twice, which will terminate the program immediately.

[mrgaturus]

AFAIK panic! terminates program and output error, you can recover from panic! through another thread, on Nimskull you can still use try catch expressions inside =destroy if you want replicate panic! behavior when an exception is catched

a common pattern on Rust is use unwrap() on a Result<T> or Option<T> which panics program when there is an error :p

[saem]

Initial thoughts on hooks raising, with a focus on managing other resources:

=copy/sink

I can't really imagine a use case where raise makes sense, outside of hook abuse/poor API design. Yes, I know one could create resources that are unique to the new copy, but again that seems like an API failure.

=destroy

From a programmer perspective it sounds handy to be able to raise, but I have no idea how the programmer reasons about it, let alone how we might implement it. At runtime any raise would screw up any subsequent =destroy calls for the same scope exit. The goal from a programmer perspective would be pretty simple, they want to be able to cleanly close files, join threads, etc.

Assuming we can't raise the only options I see for the programmer today:

• handle, or suppress, the raised exception locally, within the =destroy body
  • for cases where the object being destroyed lacks context, they wrap them with extra data to handle the issue
• if they're writing a library have their library users use defer or wrap as above
• we provide them API where action and error fetching are separate, things like doCleanUp and getCleanUpErrors

At most I can see us, allowing raising a single exception that we save, then run the remaining hooks, and then finally re-raise the exception that was raised from the hook. If another raise happens in the meantime that's a terminate/defect?

=trace

This is for cycle breaking, I don't think raise should be allowed here.

=deepCopy

I don't think this should be able to raise, again hard to imagine a use case which isn't a failure in API.

---

Although, I'm sure in certain situations having hooks able to raise would be 'nice', I think overall it's broken by design. The issue at present is that we really don't have an alternative solution at the moment.

[zerbina]

From a programmer perspective it sounds handy to be able to raise, but I have no idea how the programmer reasons about it, let alone how we might implement it. At runtime any raise would screw up any subsequent =destroy calls for the same scope exit

A possible solution here is to model each =destroy call as happening within its own finally:

block:
  var a = Object()
  var b = Object()
  discard "some code"

# would become, in terms of semantics:
block:
  var a = Object()
  try:
    var b = Object()
    try:
      discard "some code"
    finally:
      `=destroy`(b)
  finally:
    `=destroy`(a) 

Despite being in the same scope, if the =destroy hook for b raises, then the destroy hook for a is still invoked. The behaviour of raising in a =destroy hook when an exception is already active would be the same as what happens currently when raising from within a finally entered through an exception: the previously active exception is aborted/discarded.

That's not to say that this is a good solution from the perspective of language-design and expectations, of course.

[saem]

That would be one way, with the caveat that programmers have to ensure they deinitialize things in the right order (intra destroy steps).

I'm still not convinced that hooks raising makes sense, at least not yet. In the ideal world we could do a cps transform on each hook, where an exception can raise. So when we do raise, we run whatever handling, then we resume at the point of the raise, continuing to destroy, after destroying we resume after the raise handler. This would allow us to handle all exceptions raised in hooks.

[Varriount]

So here are my thoughts, coming from high-level/managed languages

=copy/sink

Since the compiler is free to insert copy/sink operations in a multitude of
places, I don't see how a user would be able to accurately handle exceptions
from those functions without placing try/except statements everywhere. If one
is at that point, I think they need to step back and use a different error
handling mechanism.

=destroy

Same opinion as copy/sink. If one has an object that might raise an exception
on cleanup, then that cleanup should be handled in a more explicit manner.

An example of this is Python - one can perform cleanup in an object's
destructor, however most/all Python interpreters ignore exceptions raised
from destructors. Instead, best practice is to use other context management
mechanisms(the with statement) that make managing an object's lifetime
explicit and allow more direct handling of errors.

=trace

I'm not familiar enough with the hook to give an opinion.

=deepCopy

Unless I'm mistaken, this hook is unlike the others in that it's actually
invoked somewhat explicitly (via deepCopy()). That being the case, I don't
see why this shouldn't be able to raise exceptions. If it's invoked
implicitly anywhere, that's a different matter.

Overall

For most of these hooks, actually catching any exceptions raised from them
is troublesome due to their implicit insertion by the compiler. The scenarios
I can see where exceptions could be feasibly raised would be better served by
a more explicit means of initialization/cleanup management.

That being said, I feel that there should be some sort of last-ditch
mechanism for handling exceptions raised in these hooks, if only so their
presence can be detected by logging or monitoring frameworks.

[zerbina]

That being said, I feel that there should be some sort of last-ditch
mechanism for handling exceptions raised in these hooks

If an exception escapes a hook, it is treated as an unhandled exception, which means that the unhandledExceptionHook is invoked, at least when using the C backend (it not being called with the other backends is a bug).

[Varriount]

Something should probably be written at some point about what state (or lack thereof) an unhandledExceptionHook can expect to find the program in.

[zerbina]

I've thought through and gathered some of the consequences that the hooks being allowed to raise would have.

=copy/=sink

Consider:

var x = Object()
x = nonMoveable

Since x already stores a value, and the source of the assignment is not moveable, a copy needs to be performed, and thus the =copy hook is invoked. If the hook raises, the =destroy hook for x would be called, but due to the =copy hook not running to completion, x might be in an inconsistent or invalid state.

A similar problem exists for =sink, with the additional problem that a partially moved value needs to be handled somehow.

Interaction with .raises tracking

If the exception effects of the two hooks need to be considered by .raises tracking, which I think they should, otherwise .raises tracking cannot really be relied on, then that's a problem, because =copy/=sink hook calls are inserted after optimization (move analysis), which takes place after .raises tracking/inference.

A solution could be to pessimistically have .raises tracking assume that each assignment could have the union of exception effects from the =copy and =sink hooks. The same applies for arguments to sink parameters.

=destroy

The =destroy hook is invoked:

1. through calls directly inserted by the compiler
2. via an indirect call, in the case of polymorphic ref types
3. by the cycle collector, during cleanup of cyclic references

Invocation case number 1 is the least problematic. If the exception effects should be visible to .raises tracking, the tracking needs to pessimistically assume that all locals and (potential) temporaries require destruction.

Number 2 means that copying, sinking, and destroying non-final polymorphic refs needs to be treated as raising unknown exceptions, as each of these actions might invoke a =destroy hook that can raise.

Finally, number 3 means that running the cycle collector has to be treated as raising unknown effects, and thus copying, sinking, and destroying a cyclic ref type needs to be too, as each of these actions might trigger the cycle collector.

=trace

The =trace hook is called by the cycle collector for discovery of graph edges. If it raises, and the cycle collector ignores the exception, reference counts of cells and the cycle collector itself would be in an inconsistent state, and program execution can effectively not continue safely.

A =trace hook call not succeeding means that cycle collection cannot continue (there might be unknown graph edges), so the only thing the cycle collector can do in the case of an exception raised from a =trace, apart from panicking, is to stop collection, undo all temporary changes done to reference counters, and keep the root list as-is.

In other words, a =trace hook raising would abort cycle collection, with the exception being discarded. It would be possible to re-raise the exception after restoring a valid state, but that would mean that running the cycle collector (both explicitly and implicitly) now raises unknown exceptions.

=deepCopy

If the =deepCopy hook is allowed to raise, then the deepCopy procedure would need to be treated as raising unknown exceptions, making it unusable in .raises: [] procedures.

Edit: deepCopy is a generic procedure, so only the deepCopy instantiation for polymorphic refs need to be pessimized.

[saem]

Seems like a categorically bad idea.