Embed errors in the AST instead of raising

[panglesd]

Currently, when ppx_let 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_let 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 _ =
  let%map rec invalid1 = 5 in
  x

let _ =
  let%map rec invalid2 = 5 in
  x

let _ =
  let%map valid = 5 in
  x

would report several errors:

• let%map may not be recursive for invalid1: the right error
• uninterpreted extension map for invalid2: the wrong error, it should be as for invalid1
• uninterpreted extension map for valid: an error when there should not be one

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

❓ 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. However, in the case of this PPX, I don't think there is a case where reporting several errors is needed, or outputting a "partially rewritten" AST would make sense.

I'll close this issue, please reopen it if there is a case where embedding errors would improve the user experience!

[Burnleydev1]

Hello @panglesd, I wish to ask if this is related to this other issue

[panglesd]

Hello @Burnleydev1. This issue has been closed, it is not relevant anymore! You can focus on other issues.