Unify runtime error handling #611

AltGr · 2024-04-26T16:41:37Z

Clearly distinguish Exceptions from Errors. The only catchable exception
available in our AST is EmptyError, so the corresponding nodes are made less
generic, and a node FatalError is added
Runtime errors are defined as a specific type in the OCaml runtime, with a
carrier exception and printing functions. These are used throughout, and
consistently by the interpreter. They always carry a position, that can be
converted to be printed with the fancy compiler location printer, or in a
simpler way from the backends.
All operators that might be subject to an error take a position as argument,
in order to print an informative message without relying on backtraces from
the backend

BONUS: this PR also includes a tweak to have message adapt to the terminal
width, and fixes to the cheat-sheets.

It's not an error! It happens in the normal control flow :) This is to distinguish from the other runtime exceptions which are actually fatal errors.

- Clearly distinguish Exceptions from Errors. The only catchable exception available in our AST is `EmptyError`, so the corresponding nodes are made less generic, and a node `FatalError` is added - Runtime errors are defined as a specific type in the OCaml runtime, with a carrier exception and printing functions. These are used throughout, and consistently by the interpreter. They always carry a position, that can be converted to be printed with the fancy compiler location printer, or in a simpler way from the backends. - All operators that might be subject to an error take a position as argument, in order to print an informative message without relying on backtraces from the backend

in order to match the exceptions in OCaml

This puts runtime exception info on par with what we had in the interpreter, and repairs the regression on the interpreter which no longer had them.

when no scope was specified

Positions within the Default terms are specially important since they can come from separate definitions in the source (before this, we would be falling back to the single declaration).

AltGr · 2024-04-30T16:20:23Z

Added 4 patches dedicated to actually having better positions, before we need to report them

the first may make the diff quite bigger, but it's just an addition of position marks to the operators in the EAppOp AST node
some positions were otherwise lost in translation to lcalc, which took a bit to debug (I first suspected optimisations) ; all tests that used explicit interpret instead of test-scope because of this are now fixed and positions in error reporting are now consistent with the different backends (this is verified by make testsuite)

denismerigoux

Thanks you @AltGr, this is a great improvement ! Hopefully it paves the way to enabling tracing in OCaml-compiled modules so that we can have a full trace across compiled modules in the interpreter.

denismerigoux · 2024-05-02T07:19:16Z

compiler/catala_utils/file.ml

@@ -180,6 +180,42 @@ let process_out ?check_exit cmd args =
    assert false
  with End_of_file -> Buffer.contents buf

+(* SIDE EFFECT AT MODULE LOAD: sets up a signal handler on SIGWINCH (window


This is very fancy

Well I had the code lying around, so... 🤷🏿

denismerigoux · 2024-05-02T07:24:04Z

compiler/desugared/from_surface.ml

@@ -366,7 +375,7 @@ let rec translate_expr
          (try
             Runtime.date_of_numbers date.literal_date_year
               date.literal_date_month date.literal_date_day
-           with Runtime.ImpossibleDate ->
+           with Dates_calc.Dates.InvalidDate ->


In my mind the Catala compiler would never directly depend on Dates_calc, to ensure some sort of abstraction of how things like dates are actually implemented. Which is why the runtime re-exported this error and why we used it here. Do you have a good rationale for tweaking this?

Ok, no indeed you have a good point, I'll ensure it's properly shadowed

denismerigoux · 2024-05-02T07:27:33Z

compiler/scalc/ast.ml

  | EExternal of { modname : VarName.t Mark.pos; name : string Mark.pos }

 type stmt =
  | SInnerFuncDef of { name : VarName.t Mark.pos; func : func }
  | SLocalDecl of { name : VarName.t Mark.pos; typ : typ }
  | SLocalInit of { name : VarName.t Mark.pos; typ : typ; expr : expr }
  | SLocalDef of { name : VarName.t Mark.pos; expr : expr; typ : typ }
-  | STryExcept of { try_block : block; except : except; with_block : block }
-  | SRaise of except
+  | STryWEmpty of { try_block : block; with_block : block }


denismerigoux · 2024-05-02T07:36:12Z

compiler/shared_ast/interpreter.ml

+      Message.warning "Assertion failed:@ %a"
        (Print.UserFacing.expr lang)
        (partially_evaluate_expr_for_assertion_failure_message ctx lang
-           (Expr.skip_wrappers e'))
+           (Expr.skip_wrappers e'));


You raise a warning and then you raise a runtime error ?

Ah yes, this one is a bit tricky: here we indeed want to benefit from the greater powers we have in the interpreter to print a more detailed message, and arbitration was needed between that and the unification of error handling between runtime and interpreter.

The choice I made was to print the more detailed message as a side-effect, then raise the exception to be handled in the common path ; but indeed that will make the console show [WARNING] with the detail and then [ERROR] with the position and the message. The alternatives, either specific error handling of this exception at this point, or somehow embedding the added information into the runtime exception, didn't seem worth it.

Maybe this would be better if we add a non-raising variant for Message.error and use that here, though ?

denismerigoux · 2024-05-02T07:36:44Z

compiler/shared_ast/interpreter.ml

+  with Runtime.Error (err, rpos) ->
+    Message.error
+      ~extra_pos:(List.map (fun rp -> "", Expr.runtime_to_pos rp) rpos)
+      "Error during evaluation: %a." Format.pp_print_text
+      (Runtime.error_message err)


OK this is the global handling.

denismerigoux · 2024-05-02T07:45:27Z

tests/arithmetic/bad/division_by_zero.catala_en

@@ -32,90 +32,39 @@ scope Money:


 ```catala-test-inline
-$ catala Interpret -s Dec
-[ERROR] division by zero at runtime
+$ catala test-scope Dec


Could you do a PR in https://gitlab.adullact.net/dgfip/ir-catala to make sure we use test-scope everywhere and no longer Interpret -s anymore? Can you remind me the difference between the two again?

Yes!

Basically the purpose is to be able to run the test with various combination of interpreter options (dcalc/lcalc, exceptions, etc.), so the special syntax is purposefully less explicit than the real catala command interpret.

See the make testuite target; quoting clerk test --help:

--test-flags=FLAGS [...] Flags to pass to the catala interpreter on catala test-scope tests. Comma-separated list. A subset may also be applied to the compilation of modules, as needed. [...] NOTE: if this is set, all inline tests that are not `catala test-scope` are skipped to avoid redundant testing.

denismerigoux · 2024-05-02T07:46:45Z

tests/arithmetic/bad/division_by_zero.catala_en

The error message is slightly less expressive now. Maybe DivisionByZero could carry two positions instead of one in the accompanying position list, the second pointing specifically to the null denominator?

Ah indeed — my first version only carried a single position for runtime error, but I had to extend it for acceptable Conflict exceptions; I didn't re-add the positions for operators though, at the moment it seemed superfluous.

Note, however, that we are now able to point precisely at the operator that failed, while before we had to point to the whole expression, which could for example include several divisions. But indeed, priority of operators could lead to things like 1 / 0 + 1 where underlining the / is not as explicit as underlining the 0.

An issue is that runtime exceptions can have multiple positions, but no associated messages at the moment.

EDIT: after discussion, pointing only to the denominator seems to actualy be the best compromise.

denismerigoux · 2024-05-02T07:48:00Z

tests/date/bad/uncomparable_duration.catala_en

-   └┬ `UncomparableDurations` exception management
-    └─ `>=` operator


So the runtime exceptions do not carry a law heading breadcrumb anymore. Maybe we should add law heading breadcrumbs to Runtime.position?

They do, they do, it's already there.

The diff is misleading here, the change is that we only have the position of the operator instead of 2 positions for the 2 operands

tests/default/bad/empty.catala_en

tests/func/bad/bad_func.catala_en

The dependency should only go through the `Runtime` module

mostly reverting to the ones the interpreter was printing ; for the case of divisions, we choose to point to the denominator instead of the operator as it's where the only possible error (division by zero) comes from.

AltGr · 2024-05-02T14:31:53Z

All should be addressed with the 2 added patches

AltGr added 11 commits April 25, 2024 14:20

Cheat-sheets fixes and improvements

fe2a562

Rename EmptyError to Empty

97d007f

It's not an error! It happens in the normal control flow :) This is to distinguish from the other runtime exceptions which are actually fatal errors.

Messages: adjust to terminal width

791ae32

Adjust exception handling in the Python backend

4f6769c

in order to match the exceptions in OCaml

Remove obsolete "except" type from the interpreter

959bcb9

Pass exception positions to the HandleDefault operators

50d686f

This puts runtime exception info on par with what we had in the interpreter, and repairs the regression on the interpreter which no longer had them.

More precise positions for operators throughout

cee8e57

QoL: guess the scope and make suggestions on 'interpret' command

59740bd

when no scope was specified

Fix exception positions being lost during translation/optimisations

c01d4ac

Positions within the Default terms are specially important since they can come from separate definitions in the source (before this, we would be falling back to the single declaration).

Simplify unthunking function ; cleanup warnings in 'make testuite'

a8635f0

denismerigoux approved these changes May 2, 2024

View reviewed changes

AltGr added 2 commits May 2, 2024 16:30

Remove direct dependency towards dates_calc from the compiler

4d41202

The dependency should only go through the `Runtime` module

Make refactored runtime error messages clearer

facce68

mostly reverting to the ones the interpreter was printing ; for the case of divisions, we choose to point to the denominator instead of the operator as it's where the only possible error (division by zero) comes from.

JSOO runtime fix

8288754

denismerigoux merged commit 0f425dc into CatalaLang:master May 3, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify runtime error handling #611

Unify runtime error handling #611

AltGr commented Apr 26, 2024

AltGr commented Apr 30, 2024

denismerigoux left a comment

denismerigoux May 2, 2024

AltGr May 2, 2024

denismerigoux May 2, 2024

AltGr May 2, 2024

denismerigoux May 2, 2024

denismerigoux May 2, 2024

AltGr May 2, 2024

denismerigoux May 2, 2024

denismerigoux May 2, 2024

AltGr May 2, 2024

denismerigoux May 2, 2024

AltGr May 2, 2024

denismerigoux May 2, 2024

AltGr May 2, 2024 •

edited

Loading

AltGr commented May 2, 2024

		└┬ `UncomparableDurations` exception management
		└─ `>=` operator

Unify runtime error handling #611

Unify runtime error handling #611

Conversation

AltGr commented Apr 26, 2024

AltGr commented Apr 30, 2024

denismerigoux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AltGr May 2, 2024 • edited Loading

Choose a reason for hiding this comment

AltGr commented May 2, 2024

AltGr May 2, 2024 •

edited

Loading