Embed errors in the AST instead of raising

[panglesd]

Currently, when ppx_rapper 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_rapper 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 invalid1 = [%rapper invalid {sql| |sql}]

let invalid2 = [%rapper invalid {sql| |sql}]

let valid = [%rapper get_opt {sql| |sql}]

would report several errors:

• Error in ppx_rapper: Supported actions are [...] for invalid1 (the right error)
• 'Uninterpreted extension 'rapper'forinvalid1,invalid2andvalid`.

The right error for invalid2 is not shown, and the "uninterpreted extension" errors add a lot of noise!

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.

[anmonteiro]

IMO this makes sense to have, and I can offer myself to review the change in case @roddyyaga isn't available.

[roddyyaga]

Hey, that sounds great, happy to review.

[panglesd]

Thanks a lot for the reviewing offer!

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 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, an example where embedding errors is better could written as:

let many_arg_execute =
  [%rapper
    execute
      {sql|
      UPDATE users
      SET (username, email, bio) = (%invalid1{username}, %invalid2{email}, %string{bio})
      |sql}]

which, when raising instead of embedding errors, would not list both errors (both invalid1 and invalid2 raise "Unknown type specification). Moreover, it could still be able to generate a function of the right arity! (username:_ -> email:_ -> bio:_ -> _ -> _)

[marrious11]

Hi senior! To work on an issue, do we have to apply for it so that the issue is assigned to us? If yes, then let this issue be assigned to us please :(

[Annosha]

I would like to work on this issue along with @marrious11 please.

[panglesd]

You can of course work together on this issue if you want! Bear in mind that at the end of the Outreachy contribution period, I will be only able to accept one applicant, and it might be harder for me to distinguish between people who have worked together.

(If @marrious11 prefers to work alone, I would say the issue is assigned to them as they were first to ask for an assignment!)

[marrious11]

Hi sir @panglesd, happy new month. Please, I have a couple of issues: Firstly, when i rum the command dune build, receive this error:-

Finally, according to the README.md: How do I add these line (libraries ppx_rapper_lwt) (preprocess (pps ppx_rapper)) to the relevant stanzas and the libraries base ppx_rapper_lwt caqti-type-calendar?

[Burnleydev1]

Hi @marrious11, I think you should run opam install ppx_rapper_lwt And opam install ppx_rapper

[panglesd]

Hello @marrious11,

Firstly, when i rum the command dune build, receive this error:-

The error is telling you that the caqti-type-calendar library is not found. This is because some of the dependencies have not been installed.

In order to install the dependencies of a project you want to work on, the best is to run:

opam install . --deps-only --with-test

Running opam install ppx_rapper, as suggested by @Burnleydev1, is also possible, but it will not only install the dependencies of ppx_rapper, but also ppx_rapper itself, from the opam repository.

Since you want to work on ppx_rapper, you don't need to install the opam version. You might want to install your modified version if you want to test it, but that's not necessary right now.

Finally, according to the README.md: How do I add these line (libraries ppx_rapper_lwt) (preprocess (pps ppx_rapper)) to the relevant stanzas and the libraries base ppx_rapper_lwt caqti-type-calendar?

The README mention how to use ppx_rapper as a PPX in another project project. It is also explained here. If you want to try ppx_rapper in a different project (which would be a good thing to do), then it is required. But it is not required in ppx_rapper itself.

[marrious11]

Thank you sir @panglesd, it worked. No errors generated again.