Metaquot: Embed errors in the AST instead of raising

[panglesd]

Currently, when ppxlib.metaquot 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 ppxlib.metaquotl 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 = [%expr [%e? _]]

let invalid2 = function [%expr [%e _]] -> 1 | _ -> 2

let invalid3 = function [%expr [%e? _ when true]] -> 1 | _ -> 2

would report several errors:

• expression expected for invalid1 (the right error, but not super explicit!)
• 'Uninterpreted extension 'expr'forinvalid1, invalid2andinvalid3`.

The right error for invalid2 ("pattern expected", again it could be more precise) is not shown, and similarly the error for invalid3 ("guard not expected here") is not displayed.
Moreover, the "uninterpreted extension" errors add a lot of noise!

Since ppxlib.metaquot can (in my opinion) be confusing with antiquote, it would be nice if the error reporting were user-friendly and precise!

This issue is 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 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 invalid = [%expr [%e? _] [%e? _]]

which, when raising instead of embedding errors, would not list both errors (both [%e? _] raise "Expression expected").

[Burnleydev1]

Hello @panglesd, I would love to work on this issue but I am still new to ocaml language and would like to please request for your guidance in working on this task.

[panglesd]

Hello @Burnleydev1 !

In order to work on this issue, you will have to:

• Understand well the issue, and be able to reproduce it. One way to do this is to setup a small dune project using metaquot, and find examples of errors (I wrote one in the example issue!
• Understand the code associated with it (in the https://github.com/ocaml-ppx/ppxlib/blob/main/metaquot/ppxlib_metaquot.ml file. It uses some syntax of OCaml (functors and objects, that it would help if you already know!)
• Try to change the raising errors with error embedding.

In order to understand metaquot itself, in addition to the resources that you should have read in the outreachy issue #389, you can read the chapter of ppxlib on metaquot: https://ocaml.org/p/ppxlib/latest/doc/generating-code.html#metaquot and https://ocaml.org/p/ppxlib/latest/doc/matching-code.html#metaquot

Finally, to turn raising into embeddings, you can look at ppxlib's example: here is the version raising: https://github.com/ocaml-ppx/ppxlib/blob/427f96e126d306538eb541ac591f71b2c68e5dd4/examples/simple-extension-rewriter/ppx_get_env.ml and the same file where the raising has been turned into error embedding: https://github.com/ocaml-ppx/ppxlib/blob/main/examples/simple-extension-rewriter/ppx_get_env.ml

[Burnleydev1]

Hi @panglesd, thanks for the pointers. I’ll get on it right away.

[Burnleydev1]

Hello @panglesd,
I did went through the resources and did some more research,
and I would love to ask if changing the code

  let cast ext =
    match snd ext with
    | PPat (p, None) -> p
    | PPat (_, Some e) ->
        Location.raise_errorf ~loc:e.pexp_loc "guard not expected here"
    | _ -> Location.raise_errorf ~loc:(loc_of_extension ext) "pattern expected"
end)

in ppx_metaquot file

  let loc = loc_of_extension ext in
  match snd ext with
  | PPat (p, None) -> p
  | PPat (_, Some e) ->
      Location.Error.createf ~loc:e.pexp_loc "guard not expected here"
      |> Location.Error.to_extension
      |> Extension_constructor.create "Error" "guard_not_expected_here" []
  | _ ->
      Location.Error.createf ~loc "pattern expected"
      |> Location.Error.to_extension
      |> Extension_constructor.create "Error" "pattern_expected" []

is a good start to solving the issue,
I learned that using result type is also another method but I'm not sure if these will actually work yet,
and also I wish to ask if I need to change the Raise_errorf function in `Location too.
thanks

[Burnleydev1]

Hello @panglesd, I think I understood this steps:
Search your code for all uses of [raise_errorf](https://ocaml.org/p/ppxlib/latest/doc/Ppxlib/Location/index.html#val-raise_errorf), using grep, for instance. For each of them, turn them into functions returning a (_, extension) result type, using [error_extensionf](https://ocaml.org/p/ppxlib/latest/doc/Ppxlib/Location/index.html#val-error_extensionf) to generate the Error.
but I have difficulties understanding the next steps about propagating the results (using maps and bind), will this be done in another file or it will be done in each file containing raise_errorf

[panglesd]

That is a very good start!

The result type is when you cannot embed the error directly in a part of AST. In the present case, you can directly return an AST node, as you've tried to do it in the code sample you sent me, so you don't need to use the result type.

There are still errors in the code you sent me (the Extension_constructor module does not exists).
You have three steps in your code:

1. Create an error (Location.Error.createf)
2. Turn it into an "extension" parsetree node (https://ocaml.org/p/ppxlib/latest/doc/Astlib/Ast_500/Parsetree/index.html#type-extension)
3. The call to the unbound module Extension_constructor.create, I'm not sure what you meant here.

You need to replace 3. with:
3. Turn the extension node into an AST node of the right type (using functions from the Ast_builder module, with the _extension suffix`.)

It would be good if you can try the code before sending it for help! Could you successfully setup and compile the project? In which case, were you able to make your modifications, get an error from the compiler, and understand the error message?

[Burnleydev1]

You need to replace 3. with:
3. Turn the extension node into an AST node of the right type (using functions from the Ast_builder module, with the _extension suffix`.)

@panglesd thanks a lot, I will get on in right away

It would be good if you can try the code before sending it for help! Could you successfully setup and compile the project? In which case, were you able to make your modifications, get an error from the compiler, and understand the error message?

I actually created a new project using dune init project <project name>
and started working from there to test the code but I was thinking I will need to create a rewrite I think or edit the ppxlib_metaquot file in opam/mirage/lib/ppxlib/metaquot/ppxlib_metaquot.ml
that is where I am confused on how to implement the changes since I am scared of modifying a file that can break my current ocaml compiler.
Also, I would like to say that the code in ppxlib_metaquot.ml gives lots of errors when I open it in vs code, please Are there some dependencies I will have to install to have it function normally?

[Burnleydev1]

I will try to apply the corrections you suggested.

[Burnleydev1]

In which case, were you able to make your modifications, get an error from the compiler, and understand the error message?

@panglesd Please I wish to ask if you mean the error message I get when I run dune build?

[panglesd]

I think I have understood how to do this

I assume you were able to clone the ppxlib repo (this one), install the dependencies if needed, and run dune build, before making modifications to the code. (If not, that's the first step!)

Once you have successfully run dune build (without your modifications), VS Code should not show any error! And you can start trying to modify the code.

The file in opam/mirage/lib/ppxlib/metaquot/ppxlib_metaquot.ml has been downloaded when installing some other package, you should not modify it directly! The file you should modify is the one in this repository.

[Burnleydev1]

I think I have understood how to do this

I assume you were able to clone the ppxlib repo (this one), install the dependencies if needed, and run dune build, before making modifications to the code. (If not, that's the first step!)

Once you have successfully run dune build (without your modifications), VS Code should not show any error! And you can start trying to modify the code.

The file in opam/mirage/lib/ppxlib/metaquot/ppxlib_metaquot.ml has been downloaded when installing some other package, you should not modify it directly! The file you should modify is the one in this repository.

Hello @panglesd, thank you for this information.
I tried using the ast_builder.default.extension but I got the error Unbound value Ast_builder.Default.extension
but there is already open Ast_builder.default so wish to ask for help solving this issue,
I did research and tried adding let loc = loc_of_extension in but i get the error Syntax error after unclosed struct, expecting end'` as well but the ohter error above is no longer displayed

      |> Ast_builder.Default.extension
          (Location.mknoloc (Longident.Lident "Error.pattern_expected")) []

[Burnleydev1]

[panglesd]

You should try as much as you can to read the error message, they are already informing you what is wrong!

• Syntax error about the let... in: "toplevel" let bindings (let that are not inside other lets) don't have a in at the end: let ... in ... are for local definitions.
• "Unbound value ..." means that you are using some values that have not been defined. You can see the list of defined values in the API: for instance, here is the API of Ast_builder.Default, and there is no mention of an extension value.

[Burnleydev1]

Hello @panglesd, I have been trying the various values from the Ast_builder.Default API and saw the
extension_condtructor but I don't know how exactly to call it as I am still getting the Unbound module Ast_builder.Default.Extension_constructor, and I tried calling it using the method |> Ast_builder.Default.Extension_constructor.create "Error" "pattern_expected" []
please I need help grabbing how to use the values in the Ast_builder.Default API

[Burnleydev1]

Hello @panglesd, I created a Pull request to this issue here, I decide to use the Ppat_extension since i kept getting the error This expression should not be a constructor, the expected type is Ppxlib.pattern when I used other functions.

[panglesd]

I think you were misled by the extension constructor thing. Extension constructors are for the "extensive sum types": see here and here.

Even though you don't need extension_constructor, be careful that the API says that it is a value. You are using it as a module, that's why OCaml complains that it is unbound.

ppat_extension is the right function to use: it builds a pattern from an extension node.

[Burnleydev1]

I now understand why extension_constructor was not working.

[panglesd]

Fixed in #397.