Embed errors in the AST instead of raising

[panglesd]

Currently, when ppx_expect encounters an error, it uses the raise_errorf function to raise a located error.

The exception is caught by ppxlib, which in this case:

• Catch the error,
• stops the rewriting process
• add the error (as a [%%%ocaml.error ...] extension node) to the last valid ast
• Use the resulting AST

The interruption of the rewriting is quite bad for the user experience! The implication for the users are:

• Since ppx_expect runs at the "context-free" phase, the "last valid AST" is before the context-free phase. So, no other derivers/extenders get run, which generates a lot of noise in the errors (such as "uninterpreted extensions" or "unbound identifiers")
• Only one (meaningful) error from your PPX is reported at a time.
  Example

For instance:

let%expect_test "invalid1" =
  Printf.printf "%d" (1 + 2);
  [%expect {| multi
  line without empty line |}]

let%expect_test "invalid2" =
  Printf.printf "%d" (1 + 2);
  [%expect {| multi
  line without empty line |}]

let%expect_test "valid" =
  Printf.printf "%d" (1 + 2);
  [%expect {| 3 |}]

would report several errors:

• Multi-line expectations must start with an empty line for invalid1: the right error
• Uninterpreted extension 'expect_test' for invalid1, invalid2 and valid

The "uninterpreted extensions" errors are noise, and the "multi-line expectations [...]" error is not shown for invalid2.

You can find more information about error reporting in PPXs in this section of the ppxlib manual.

:question: Would you be willing to accept contributions to this issue? I'm considering assigning its resolution as part of an outreachy internship: see more information here.

[panglesd]

Note that we decided that we will change the ppxlib behaviour regarding the handling of exceptions, to match the current use of raise_errorf in PPXs. Catching an exception will no longer stop the rewriting process. So, the example I gave in the original issue is not relevant any more.

However, embedding errors can still have advantages: It allows reporting multiple errors, while still outputting valid AST for the part that were successful. In the case of this PPX, the example could be rewritten as:

let%expect_test "invalid" =
  Printf.printf "%d" (1 + 2);
  [%expect {| multi
  line without empty line |}];
  Printf.printf "%d" (1 + 2);
  [%expect {| multi
  line without empty line |}]

which, when raising instead of embedding errors, would only report a single error, instead of all.

[Annosha]

@panglesd I would like to work on this issue for Outreachy. Can it be assigned to me. Thank you!

[panglesd]

Let it be assigned to you!

[Annosha]

@panglesd I'm not able to spot the expect_error and expect function in src folder to modify and implement changes to embed errors in AST. Do I have to write both functions from scratch and add it to expect_extention.ml file? Also, I was wondering if embedding errors into the AST, will also require modifying the code that runs the tests to handle the errors in the AST.

[panglesd]

I suggest you start with fixing the example given in https://github.com/janestreet/ppx_expect/issues/45#issuecomment-1463612911, which instead of reporting three errors, would only report one: the one mentioned in the first post: Multi-line expectations must start with an empty line. In order to find where this error is raised in the code, you can search for occurrence of this message (do not restrict yourself with the src folder!) I don't think you will need to modify the tests, since I don't think any test test the "multiline should start with an empty line" constraint. Maybe adding one would be nice!

[Annosha]

@panglesd I have made the following changes in ppx_expect_payload.ml file:

1. rest_must_be_emptyfunction is modified to return a tuple containing the location of the error and an error message instead of a simple string.
2. check_expectation_body function is modified to return the result type, which can either beOk or Errorcontaining the location and message of the error.
3. the check_expectation_bodyfunction calls the modified rest_must_be_empty function, and the error location and message are passed as arguments to the Error variant of the result type.

However, when I run dune build I'm getting multiple errors. (Not sure if this library is supposed to run as a dune build)

[Annosha]

@panglesd please review the modified changes in this PR