Implement exceptions for effect handlers using `Callback.register_exception` by kayceesrk · Pull Request #11423 · ocaml/ocaml

kayceesrk · 2022-07-11T13:09:07Z

This PR removes the pre-defined exceptions Unhandled and Continuation_already_taken used by effect handlers. Instead, we now declare these effects in the Effect module, and register them using Callback.register_exception to enable the runtime to raise these exceptions.

This PR also replaces the exception Unhandled with Unhandled_effect : 'a Effect.t -> exn, which carries the unhandled effect.

The PR supersedes #11419 and fixes #11185.

dbuenzli · 2022-07-11T13:15:34Z

Personally I find Effect.Unhandled_effect a bit long winded. I would go for Effect.Unhandled.

gasche · 2022-07-11T13:25:03Z

runtime/interp.c

+      (code_t)raise_unhandled_effect_code;
+    Closinfo_val(raise_unhandled_effect_closure) = Make_closinfo(0, 2);
+    raise_unhandled_effect = raise_unhandled_effect_closure;
+    caml_register_generational_global_root(&raise_unhandled_effect);


Can you explain to a newcomer why this black magic around raise_unhandled_effect is necessary? (The explanation could be added as a comment in the code.)

I'm not sure what parts of the bytecode are allowed to directly raise exceptions (apparently caml_raise_continuation_already_taken() below is fine, and there are existing examples with caml_raise_stack_overflow for example), and which are not / why. Here what's going on seems very complex compared to the semantics we want, or even to the handling of unhandled exceptions in

ocaml/runtime/interp.c

Lines 972 to 977 in 37df934

if (Stack_parent(domain_state->current_stack) == NULL) {

domain_state->external_raise = initial_external_raise;

domain_state->trap_sp_off = initial_trap_sp_off;

domain_state->current_stack->sp =

Stack_high(domain_state->current_stack) - initial_stack_words ;

return Make_exception_result(accu);

Essentially, raise_unhandled_effect is necessary when we do not find a matching effect handler in our stack of handlers. The search had proceeded by checking one effect handler on the stack at a time. When we reach the bottom of the stack of effect handlers and find that there is no parent stack, we have to get the control back to the point of perform (do_resume) and then raise the exception. raise_unhandled_effect is the closure that raises the exception immediately after the control returns to the point of perform.

I'll write a clarifying comment in the code.

dra27 · 2022-07-11T13:43:32Z

I'm not sure that an example can actually be concocted which would trigger this, but it reads slightly strangely that the Effect module initialises a runtime structure (in this case the two exceptions in the named values table). In other words, is it possible to have a program which could lead to caml_raise_unhandled_effect where the exception has not been registered, thinking of full stdlib replacements, etc., or linking without -linkall. Is the registration something which ought to be in CamlinternalEffect and explicitly called as part of the startup code on the OCaml side?

favonia · 2022-07-11T14:00:33Z

Typo in the Git commit message of 37df934: "Undefined"

xavierleroy · 2022-07-11T17:18:42Z

I won't review the implementation until we all agree on the interface and the documentation. +1 for @dbuenzli's suggestions, especially the suggestion to shorten Effect.Unhandled_effect to Effect.Unhandled.

xavierleroy · 2022-07-11T17:20:12Z

testsuite/tests/effects/reperform.reference

 !4]
 !5]
-Unhandled
+Stdlib.Effect.Unhandled_effect(_)


This (_) is unfortunate and reduces the usefulness of including the effect in the exception. It would be 100% better to also print the name of the effect ! For this we'll need some ad-hoc code in the exception printer(s), I'm afraid.

Good point. I see that there is similar code in printexc.ml.

favonia · 2022-07-11T17:39:07Z

As the person who originally requested the renaming of Unhandled (unscoped) to Unhandled_effect, I am satisfied with Effect.Unhandled. 😃

kayceesrk · 2022-07-12T01:28:57Z

Is the registration something which ought to be in CamlinternalEffect and explicitly called as part of the startup code on the OCaml side?

It would be odd to not find the exception when using caml_named_value. Currently, there is an assertion to check that the exception exists.

ocaml/runtime/fiber.c

Lines 669 to 674 in 37df934

    
           exn = atomic_load(&caml_continuation_already_taken_exn); 
        
           if (exn == NULL) { 
        
             exn = caml_named_value("Effect.Continuation_already_taken"); 
        
             CAMLassert (exn); 
        
             atomic_store(&caml_continuation_already_taken_exn, exn); 
        
           }

If we think that there is a possibility of these exceptions not being registered (with stdlib replacements), then it may be easiest to replace this assertion with a check that exn is not null and fail with a caml_fatal_error if it is null.

Octachron · 2022-07-12T10:10:48Z

@dra27: If I am not mistaken, the exceptions can only be raised by OCaml code that links Effect, thus it seems fine to do the initialization only when linking the Effect module. If a stdlib replacement exposes the runtime effect primitive functions by itself without linking the Effect module, it should also take care to properly register the exceptions itself.

lthls · 2022-07-12T12:24:17Z

@dra27: If I am not mistaken, the exceptions can only be raised by OCaml code that links Effect, thus it seems fine to do the initialization only when linking the Effect module.

That's unfortunately not the case. Defining a new effect only needs to reference the interface, not the implementation, and the same applies for performing an effect (Effect.perform is an external declaration). So you can have code performing effects that has does not link with the Effect module.
Writing effect handlers currently needs to go through the implementation of Effect, but even that might change if we introduce support in the language itself.

Overall, I consider that if there is any amount of initialisation code in a module, it should be compiled with -linkall. This is annoying for Stdlib modules as that means it's almost impossible to not link them even if they're not used, so I'd advise one of the two following alternatives:

Switch back to builtin exceptions (you can still rebind a builtin exception in Effect, using type exn += Unhandled = Unhandled_effect for instance)
Make the Effect module an independent library

kayceesrk · 2022-07-12T12:44:23Z

Defining a new effect only needs to reference the interface, not the implementation, and the same applies for performing an effect (Effect.perform is an external declaration). So you can have code performing effects that has does not link with the Effect module.

This seems like an adversarial case. Is it possible to write programs which will perform effects without linking to the Effect module? In other words, is this problem encountered by programmers who are not actively trying to use effects without linking to the effect module. We should certainly address this issue if the programmer is not trying to do something clever but encounters this issue.

so I'd advise one of the two following alternatives:

Switch back to builtin exceptions (you can still rebind a builtin exception in Effect, using type exn += Unhandled = Unhandled_effect for instance)

Make the Effect module an independent library

What do you think about the following options?

We raise Failure "Effect module not linked" if the runtime is not able to find the exceptions.
Separating the initialisation code to camlinternalEffect?

xavierleroy · 2022-07-12T12:53:59Z

Again we're discussing implementation before we've agreed on the API...

We raise Invalid_argument "Effect module not linked" if the runtime is not able to find the exceptions.

That's what otherlibs/unix does for the Unix_error exception. A fatal error is also possible, see

ocaml/runtime/fail_nat.c

Lines 204 to 218 in ef376dd

    
           void caml_array_bound_error(void) 
        
           { 
        
             static atomic_uintnat exn_cache = ATOMIC_UINTNAT_INIT(0); 
        
             const value* exn = (const value*)atomic_load_acq(&exn_cache); 
        
             if (!exn) { 
        
               exn = caml_named_value("Pervasives.array_bound_error"); 
        
               if (!exn) { 
        
                 fprintf(stderr, "Fatal error: exception " 
        
                   "Invalid_argument(\"index out of bounds\")\n"); 
        
                 exit(2); 
        
               } 
        
               atomic_store_rel(&exn_cache, (uintnat)exn); 
        
             } 
        
             caml_raise(*exn); 
        
           }

Please forget about the implementation and discuss the API (incl. documentation comments).

kayceesrk · 2022-07-13T05:56:43Z

Again we're discussing implementation before we've agreed on the API...

Do we agree that changing the exception Unhandled to Effect.Unhandled : 'a Effect.t -> exn is a good idea? Are there other API-related questions that we are missing?

There are a few (potentially non-controversial) tasks left for improving the PR:

Rename Effect.Unhandled_effect to Effect.Unhandled.
Implement ad-hoc printer for effect values.
Improve API docs based on these comments: Exception for unhandled effect now carries the effect #11419 (review)

Octachron · 2022-07-19T08:51:33Z

I think that renaming the exception to Effect.Unhandled is definitively a good idea.

Thinking a bit about the Effect module, I was wondering about the long term expectation for this module. As far as I understand, we expect to add support in the language itself for most of the primitives exposed by the module. What will happen to this module at this point? Will it contain only some utility functions? Or might it be completely removed at some point?

If the later option is a possibility, it might make sense to expose the effect module as a library. This has the advantage of making it clearer that the module is an experimental API that does not come with the backward-compatibility guarantee of the standard library. Moreover, integrating a library in the standard library is both simpler and faster than removing a module from the standard library.

dbuenzli · 2022-07-19T09:14:51Z

This has the advantage of making it clearer that the module is an experimental API that does not come with the backward-compatibility guarantee of the standard library. Moreover, integrating a library in the standard library is both simpler and faster than removing a module from the standard library.

It also entails quite a bit of bureaucracy. In general the whole process around the stdlib remains a bit unrealistic in my opinion, especially for something like the 5.0 transition.

You can think as much as you want about design, design errors still occur and most of the time you only get to know about them once it's in the wild and the stuff gets used in anger.

The current process entails that if a design error gets made it is cast in stone for the next 20 years which is not so great. I think the stdlib should have a notion of @experimental when new APIs or even functions are introduced which would allow for corrections for as long as the tag applies. As an end user, you have been warned.

Personally I would even flag all of the entry points introduced specifically for 5.0 as such (e.g. for the little I have looked into I'm already not really happy about the Domain interface).

xavierleroy

Let's try to agree on the API, shall we?

stdlib/effect.mli

manual/src/refman/extensions/effects.etex

kayceesrk · 2022-07-21T08:02:29Z

Thanks @jonludlam for the help. With the latest commits, the outstanding tasks mentioned in #11423 (comment) are complete.

favonia · 2022-07-21T12:50:28Z

It seems there are a few additional details of the API that were implicitly agreed upon: (Or, at very least, I did not see any objections.)

It should be possible to use effects without the standard library. (Suggested by @dra27 but not documented.)
There should not be any top-level aliases of the exception Effect.Unhandled.

The reason I brought up (2) is that it seems to be the only reason to favor exception registration over built-in exceptions. As for (1), if the current PR is tracking this API design, perhaps Continuation_already_taken and the registration of Unhandled and Continuation_already_taken should all be moved to the module CamlinternalEffect?

Personally, I also like @dbuenzli's suggestion to mark new things experimental. Is this part of the API discussion? If so, I support marking both Effect and Domain (especially Domain) experimental in the sense that backward compatibility is not guaranteed.

xavierleroy · 2022-07-21T14:01:59Z

It should be possible to use effects without the standard library. (Suggested by @dra27 but not documented.)

Just to clarify: I do not consider this necessary at all. First, anyone can build on top of the standard library, there is seldom a need to replace it entirely. Second, a stdlib replacement can register exceptions itself, like the stdlib would do.

Built-in exceptions and Camlinternal* modules are best avoided if we can: built-in exceptions because they pollute the scope and complicate the bootstrap, and Camlinternal* modules because they complicate the structure of the stdlib. Here, we can avoid both. Let's do that.

xavierleroy

I read stdlib/effect.mli again, and the API and doc strings look just fine to me. Thanks! @favonia @dbuenzli @fpottier and others: if you have reservations about the current API and documentation, now is the time to tell.

I started looking at the implementation and I have a number of suggestions to make, probably tomorrow.

dbuenzli

Docs and names look good. Thank @kayceesrk.

runtime/caml/fiber.h

stdlib/effect.mli

favonia · 2022-07-21T17:32:31Z

stdlib/printexc.ml

    | [] -> None in
  conv (Atomic.get printers)

+let walk_sum_type x =


(This is about the implementation.) I personally prefer the name extensible_variant over sum_type because the printer here does not work for all variant types (and in OCaml documentation the term "sum types" is not used). This comment also applies to the function string_of_sum_type a few lines below.

stdlib/printexc.ml

favonia

I noticed that in effects.etex, Unhandled is qualified (as Effect.Unhandled) but Continuation_already_taken is not, even though both are defined at the same place now. Otherwise, everything else looks great. @kayceesrk Thank you. 🙂

kayceesrk · 2022-07-22T07:40:46Z

@xavierleroy

Camlinternal* modules because they complicate the structure of the stdlib

I needed to introduce the CamlInternalEffect module to break the cyclic dependency between the Printexc and Effect modules. Agree that it would be good to avoid the introduction of a new CamlInternal* module if there is way around this.

(Apologies for the slow response on this thread. It is the first week of teaching in the new semester for me.)

This bootstrap removes the pre-defined exceptions Undefined and Continuation_already_taken exceptions used by effect handlers.

It seemed sensible not to duplicate the logic to turn sum types into strings that is currently in use for exceptions. However, since it's not exposed in Printexc, and the exception being matched is in effect.ml, which depends on Printexc, I've opted for matching the string "Stdlib.Effect.Unhandled" and assuming the single argument is the effect.

In this case, produce a somewhat clean fatal error if these exceptions are raised.

The documentation was already using "resumed" rather than taken which is a good sign that "Continuation_already_resumed" is clearer.

Octachron · 2022-09-21T12:45:59Z

I have updated and rebased the PR and added @xavierleroy's commit and mine to the branch.
I have also renamed the Continuation_already_taken exception to Continuation_already_resumed to align the exception name with the documentation.

@favonia: After this PR neither the Continuation_already_taken exception nor Unhandled exception are built-in exceptions. It thus seems reasonable to me to keep them printed as regular exceptions.

favonia · 2022-09-21T13:17:36Z

After this PR neither the Continuation_already_taken exception nor Unhandled exception are built-in exceptions. It thus seems reasonable to me to keep them printed as regular exceptions.

I think the difference between "a built-in exception" and "an exception defined in the standard library" is very small. It's only meaningful to people who build compilers or alternative standard libraries. Therefore, I still feel it is better to include all (public) exceptions defined in the standard library. That said, I do not have a strong opinion because the output will be clear either way. I see that there are some unaddressed comments about the documentation. Other than those, I am very satisfied with the current PR and hope it can be shipped with the next alpha or the beta! 😃

@raise

Use "@raise Unhandled" rather than "@raise Unhanded [e]"

kayceesrk · 2022-09-24T11:45:32Z

The PR as it stands looks good to me. The only point of contention is the choice between the strings "Unhandled effect: %s" and "Stdlib.Effect.Unhandled(%s)". This can be changed in a future version without breaking code.

I realised I can't approve the PR since I'd opened it! @Octachron feel free to merge when you are happy.

xavierleroy

Approving on behalf of @kayceesrk and @Octachron , who both wrote the code and reviewed the other's code.

xavierleroy · 2022-09-25T10:01:07Z

There's a bootstrap in the middle, so I'd rather let @Octachron handle the final merge.

Octachron · 2022-09-26T13:08:35Z

Merged and cherry-picked by hand to 5.0 .

Octachron · 2022-09-26T13:53:34Z

The reperform test appears to be failing on Windows-mingw64, I will investigate the issue.

Windows fix for #11423

Windows fix for #11423 (cherry picked from commit 9ed098d)

kayceesrk mentioned this pull request Jul 11, 2022

Exception for unhandled effect now carries the effect #11419

Closed

3 tasks

gasche reviewed Jul 11, 2022

View reviewed changes

xavierleroy reviewed Jul 11, 2022

View reviewed changes

favonia mentioned this pull request Jul 19, 2022

OCaml 5.0 & unhandled exceptions #11074

Open

xavierleroy reviewed Jul 19, 2022

View reviewed changes

stdlib/effect.mli Outdated Show resolved Hide resolved

manual/src/refman/extensions/effects.etex Outdated Show resolved Hide resolved

xavierleroy reviewed Jul 21, 2022

View reviewed changes

dbuenzli reviewed Jul 21, 2022

View reviewed changes

runtime/caml/fiber.h Outdated Show resolved Hide resolved

favonia reviewed Jul 21, 2022

View reviewed changes

stdlib/effect.mli Outdated Show resolved Hide resolved

favonia reviewed Jul 21, 2022

View reviewed changes

stdlib/printexc.ml Outdated Show resolved Hide resolved

favonia reviewed Jul 21, 2022

View reviewed changes

stdlib/printexc.ml Show resolved Hide resolved

favonia reviewed Jul 21, 2022

View reviewed changes

kayceesrk mentioned this pull request Jul 22, 2022

Replace Continuation_already_taken exception with a predefined Invalid_argument exception #11422

Closed

kayceesrk and others added 11 commits September 21, 2022 10:34

Use Callback.register_exception for exceptions in Effect module

4b80d3b

Remove pre-defined exceptions for effect handlers

66b2aa9

Bootstap.

4a949b6

This bootstrap removes the pre-defined exceptions Undefined and Continuation_already_taken exceptions used by effect handlers.

Rename Effect.Unhandled_effect -> Effect.Unhandled

03e0a06

Documentation updates for Effects

ed3c6f2

Avoid string comparison for identifying Unhandled exception in Printexc

7a41e32

Guard against missing registration for exceptions declared in Effect

0cb2318

In this case, produce a somewhat clean fatal error if these exceptions are raised.

Move Unhandled_effect printers to Effect

87a5ce5

Rename Continuation_already_taken to Continuation_already_resumed

a83abcf

The documentation was already using "resumed" rather than taken which is a good sign that "Continuation_already_resumed" is clearer.

test: perform an effect without linking Effect

b65695a

Octachron force-pushed the effect_exceptions branch from 2f22a81 to b65695a Compare September 21, 2022 12:42

typo

51bf6c9

Fix flambda test

5a562f8

xavierleroy added this to the 5.0 milestone Sep 21, 2022

Tweak the Effect module documentation

dde0856

Use "@raise Unhandled" rather than "@raise Unhanded [e]"

xavierleroy approved these changes Sep 25, 2022

View reviewed changes

Octachron merged commit 96df8e8 into ocaml:trunk Sep 26, 2022

favonia mentioned this pull request Sep 26, 2022

Rename the exception Unhandled to Unhandled_effect #11262

Closed

dra27 added a commit that referenced this pull request Sep 26, 2022

Merge pull request #11566 from dra27/unhandled-effect-tweak

9ed098d

Windows fix for #11423

dra27 added a commit that referenced this pull request Sep 26, 2022

Merge pull request #11566 from dra27/unhandled-effect-tweak

3c65a29

Windows fix for #11423 (cherry picked from commit 9ed098d)

anmonteiro mentioned this pull request Sep 27, 2022

Qualify Effect.Unhandled ocaml-multicore/eio#329

Closed

favonia mentioned this pull request Sep 29, 2022

Make it compile after ocaml/ocaml#11423 ocaml-multicore/eio#332

Closed

sim642 mentioned this pull request Oct 24, 2022

Fix compilation on OCaml 5.0.0~beta1 ocaml-batteries-team/batteries-included#1094

Merged

	if (Stack_parent(domain_state->current_stack) == NULL) {
	domain_state->external_raise = initial_external_raise;
	domain_state->trap_sp_off = initial_trap_sp_off;
	domain_state->current_stack->sp =
	Stack_high(domain_state->current_stack) - initial_stack_words ;
	return Make_exception_result(accu);

Conversation

kayceesrk commented Jul 11, 2022

Uh oh!

dbuenzli commented Jul 11, 2022

Uh oh!

gasche Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

kayceesrk Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

dra27 commented Jul 11, 2022

Uh oh!

favonia commented Jul 11, 2022

Uh oh!

xavierleroy commented Jul 11, 2022

Uh oh!

xavierleroy Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

kayceesrk Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

favonia commented Jul 11, 2022

Uh oh!

kayceesrk commented Jul 12, 2022

Uh oh!

Octachron commented Jul 12, 2022

Uh oh!

lthls commented Jul 12, 2022

Uh oh!

kayceesrk commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xavierleroy commented Jul 12, 2022

Uh oh!

kayceesrk commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Octachron commented Jul 19, 2022

Uh oh!

dbuenzli commented Jul 19, 2022

Uh oh!

xavierleroy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kayceesrk commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

favonia commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xavierleroy commented Jul 21, 2022

Uh oh!

xavierleroy left a comment

Choose a reason for hiding this comment

Uh oh!

dbuenzli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

favonia Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

favonia left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kayceesrk commented Jul 22, 2022

Uh oh!

Octachron commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

favonia commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

kayceesrk commented Jul 12, 2022 •

edited

Loading

kayceesrk commented Jul 13, 2022 •

edited

Loading

kayceesrk commented Jul 21, 2022 •

edited

Loading

favonia commented Jul 21, 2022 •

edited

Loading

favonia Jul 21, 2022 •

edited

Loading

favonia left a comment •

edited

Loading

Octachron commented Sep 21, 2022 •

edited

Loading

favonia commented Sep 21, 2022 •

edited

Loading

kayceesrk commented Sep 24, 2022 •

edited

Loading