Improvements around external modules and error handling #605
Conversation
This would deserve a further look into it, but for now it's impeding development without providing any meaningful security.
This was a pending TODO: now the Catala program compiled into OCaml should return better messages and a little more information about uncaught exceptions. Note that this also concerns, at the moment, compiled modules called from the Catala interpreter: in this case, it's already better than nothing, but what we need is proper interoperation between the runtime exceptions and the interpreter handling (`EmptyError` should already be handled properly since it is critical to the computation flow, but "error" exceptions are left uncaught and will kill the interpreter). This may be part of a bigger task on unifying the output of the runtime and toplevel, which also concerns computation traces. Note 2: All runtime exceptions don't have a position available, which is quite unfortunate when your program hits an error. With `OCAMLRUNPARAM=b` and if compiled with `-g` (which should normally be the case), you can get an OCaml backtrace but that's not very friendly. Ideas for improvement: - The runtime could force-enable backtrace recording (`Printexc.record_backtrace true`) to supersede the need for `OCAMLRUNPARAM`. We can also record our own handler to print the file position and/or backtrace in the way we see fit - The printer of OCaml code in Catala could insert line directives so that the positions in the backtrace actually trace automatically back to the Catala code - If we don't want to leverage any OCaml machinery in this way, the compiler should add position information to any operator that might fail (e.g. divisions, date comparisons, etc.). Note that running in trace mode might already help pinpoint the location of the error ?
Module names must be capitalised (start with a capital letter), and the name of the file on disk must match ; however, matching up to capitalisation is allowed, i.e. the file on disk can start with a lowercase letter. A mismatch between Clerk assuming generated module artifacts would match the capitalised module name, and `catala depends` matching the names of files on disk (because it would otherwise mean treating dependencies differently depending on if they originate from modules or not) was causing "file not found" errors later on in the compilation chain. This patch enforces that the capitalisation of the original file name on disk (which is always known) takes precedence in Clerk, matching the behaviour of `catala depends` and fixing the issue. It's also actually a small simplification in Clerk code.
There was a problem hiding this comment.
Thanks very much @AltGr! Nonetheless I still don't understand something, see my comments.
| Dynlink.allow_only | ||
| (List.filter (( <> ) "Driver__Plugin") (Dynlink.all_units ())); | ||
| (* From here on, no plugin registration is allowed. However, the interpreter | ||
| may yet use Dynlink to load external modules. - TODO: This used to allow | ||
| only "Runtime_ocaml__Runtime", but forbidding external Catala modules to | ||
| use the OCaml Stdlib was a bit much. We should examine how to re-add some | ||
| more filtering here without being too restrictive. *) |
There was a problem hiding this comment.
This policy sounds more reasonable.
| | exception e -> | ||
| Format.ksprintf | ||
| (fun s -> raise (CatalaException (Crash s, pos))) | ||
| "@[<hv 2>This call to code from a module failed with:@ %s@]" | ||
| (Printexc.to_string e) |
There was a problem hiding this comment.
So here I don't understand why you're not more precise and match each of the exceptions that the OCaml runtime can raise :
catala/runtimes/ocaml/runtime.mli
Lines 72 to 78 in ce7bed1
Something like :
| Runtime_ocaml.NoValueProvided pos ->
Message.error ~pos:(runtime_position_to_pos pos) "No value provided here"There was a problem hiding this comment.
Well, we call Printexc.to_string which will resort to the NoValueProvided printer registered here , so the position will be available in the message and we don't gain much with special catching here.
|
|
||
| (* TODO: register exception printers for the above | ||
| (Printexc.register_printer) *) | ||
| (* Register exceptions printers *) |
There was a problem hiding this comment.
OK so you're going through OCaml's exception printing mechanism, which is more general. Why go through that very general mechanism that forces you to print source code positions in a crude way when you could catch in interpreter.ml the exceptions of Runtime_ocaml properly and access to their positions in a structured way (and that you can print beautifully)?
There was a problem hiding this comment.
At the moment, the position types in the runtime are different from the ones in the compiler, so we would need specific printing functions ; the interpreter doesn't seem like the right place to put those ?
Ideally, like for backtraces, we'd like unified printing for interpreter and runtime errors ; it would probably be best to start with unification of the underlying types. That's ongoing work but I wanted a quick fix: having a message with a usable position was the priority here.
| @@ -76,6 +76,7 @@ exception UncomparableDurations | |||
| exception IndivisibleDurations | |||
| exception ImpossibleDate | |||
| exception NoValueProvided of source_position | |||
| exception Division_by_zero (* Shadows the stdlib definition *) | |||
This allows external modules to access all modules (see first patch), protects
the interpreter for better reporting of exceptions raised from modules (third
patch), and register exception printers in the OCaml runtime.
Quoting here the message from the second patch because it mentions some
important points, that may warrant discussion:
OCaml runtime: register fallback exception printers
This was a pending TODO: now the Catala program compiled into OCaml should
return better messages and a little more information about uncaught
exceptions.
Note that this also concerns, at the moment, compiled modules called from
the Catala interpreter: in this case, it's already better than nothing, but
what we need is proper interoperation between the runtime exceptions and
the interpreter handling (
EmptyErrorshould already be handled properlysince it is critical to the computation flow, but "error" exceptions are
left uncaught and will kill the interpreter).
This may be part of a bigger task on unifying the output of the runtime and
toplevel, which also concerns computation traces.
Note 2: All runtime exceptions don't have a position available, which is
quite unfortunate when your program hits an error. With
OCAMLRUNPARAM=band if compiled with
-g(which should normally be the case), you can getan OCaml backtrace but that's not very friendly. Ideas for improvement:
The runtime could force-enable backtrace recording (
Printexc.record_backtrace true) to supersede the need forOCAMLRUNPARAM. We can also record our ownhandler to print the file position and/or backtrace in the way we see fit
The printer of OCaml code in Catala could insert line directives so that the
positions in the backtrace actually trace automatically back to the Catala
code
If we don't want to leverage any OCaml machinery in this way, the compiler
should add position information to any operator that might fail (e.g.
divisions, date comparisons, etc.).
Note that running in trace mode might already help pinpoint the location of the
error ?